From bad84b1f8f97edfae2d341c37f85da1d31072308 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Fri, 14 Jun 2024 20:20:07 +0800 Subject: [PATCH 1/7] Implement LiquidCachedAssigns --- .../concerns/liquid_enabled_resource.rb | 2 +- app/services/liquid_assigns_service.rb | 51 +--------------- app/services/liquid_cached_assigns.rb | 59 +++++++++++++++++++ 3 files changed, 63 insertions(+), 49 deletions(-) create mode 100644 app/services/liquid_cached_assigns.rb diff --git a/app/controllers/concerns/liquid_enabled_resource.rb b/app/controllers/concerns/liquid_enabled_resource.rb index 1dac5829e..04e94046f 100644 --- a/app/controllers/concerns/liquid_enabled_resource.rb +++ b/app/controllers/concerns/liquid_enabled_resource.rb @@ -35,6 +35,6 @@ def project_assigns project = Project.find(params[:project_id]) authorize! :use, project - LiquidAssignsService.new(project: project, text: params[:text]).assigns + LiquidAssignsService.new(project: project).assigns end end diff --git a/app/services/liquid_assigns_service.rb b/app/services/liquid_assigns_service.rb index b781c77b6..0260060f3 100644 --- a/app/services/liquid_assigns_service.rb +++ b/app/services/liquid_assigns_service.rb @@ -1,61 +1,16 @@ class LiquidAssignsService - AVAILABLE_PROJECT_ASSIGNS = %w{ evidences issues nodes notes tags }.freeze + attr_accessor :project - attr_accessor :project, :text - - def initialize(project:, text: nil) + def initialize(project:) @project = project - @text = text end def assigns - result = project_assigns - result.merge!(assigns_pro) if defined?(Dradis::Pro) - result + LiquidCachedAssigns.new(project: project) end private def assigns_pro end - - # This method uses Liquid::VariableLookup to find all liquid variables from - # a given text. We use the list to know which project assign we need. - def assigns_from_content - return AVAILABLE_PROJECT_ASSIGNS if text.nil? - - variable_lookup = Liquid::VariableLookup.parse(text) - return (variable_lookup.lookups & AVAILABLE_PROJECT_ASSIGNS) - end - - def cached_drops(records, record_type) - return [] if records.empty? - - cache_key = "liquid-project-#{project.id}-#{record_type.pluralize}:#{records.maximum(:updated_at).to_i}-#{records.count}" - drop_class = "#{record_type.camelize}Drop".constantize - - Rails.cache.fetch(cache_key) do - records.map { |record| drop_class.new(record) } - end - end - - def project_assigns - project_assigns = { 'project' => ProjectDrop.new(project) } - - assigns_from_content.each do |record_type| - records = - case record_type - when 'evidences' - project.evidence - when 'nodes' - project.nodes.user_nodes - else - project.send(record_type.to_sym) - end - - project_assigns.merge!(record_type => cached_drops(records, record_type.singularize)) - end - - project_assigns - end end diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb new file mode 100644 index 000000000..cba8cbe8e --- /dev/null +++ b/app/services/liquid_cached_assigns.rb @@ -0,0 +1,59 @@ +class LiquidCachedAssigns < Hash + AVAILABLE_PROJECT_ASSIGNS = %w{ evidences issues nodes notes project tags }.freeze + + attr_accessor :assigns, :project + + def initialize(project:) + @assigns = { 'project' => ProjectDrop.new(@project) } + @project = project + end + + def [](record_type) + return unless AVAILABLE_PROJECT_ASSIGNS.include?(record_type) + + if assigns[record_type] + assigns[record_type] + else + assigns[record_type] = cached_drops(record_type) + end + end + + def inspect + assigns.inspect + end + + def key?(msg) + AVAILABLE_PROJECT_ASSIGNS.include?(msg.to_s) + end + + def respond_to?(msg) + return true if key?(msg) + super(msg) + end + + private + + def cached_drops(record_type) + records = project_records(record_type) + + return [] if records.empty? + + cache_key = ActiveSupport::Cache.expand_cache_key([project.id, records], 'liquid') + drop_class = "#{record_type.singularize.camelize}Drop".constantize + + Rails.cache.fetch(cache_key) do + records.map { |record| drop_class.new(record) } + end + end + + def project_records(record_type) + case record_type + when 'evidences' + project.evidence + when 'nodes' + project.nodes.user_nodes + else + project.send(record_type.to_sym) + end + end +end From 8e8dc68ce5180e5d64c5cbc363e8cd8b63508114 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Wed, 19 Jun 2024 18:40:58 +0800 Subject: [PATCH 2/7] Add support for merge and add specs --- .../concerns/liquid_enabled_resource.rb | 2 +- app/services/liquid_assigns_service.rb | 16 ----- app/services/liquid_cached_assigns.rb | 32 +++++----- spec/services/liquid_assigns_service_spec.rb | 59 ------------------ spec/services/liquid_cached_assigns_spec.rb | 61 +++++++++++++++++++ 5 files changed, 77 insertions(+), 93 deletions(-) delete mode 100644 app/services/liquid_assigns_service.rb delete mode 100644 spec/services/liquid_assigns_service_spec.rb create mode 100644 spec/services/liquid_cached_assigns_spec.rb diff --git a/app/controllers/concerns/liquid_enabled_resource.rb b/app/controllers/concerns/liquid_enabled_resource.rb index 04e94046f..aa0661e31 100644 --- a/app/controllers/concerns/liquid_enabled_resource.rb +++ b/app/controllers/concerns/liquid_enabled_resource.rb @@ -35,6 +35,6 @@ def project_assigns project = Project.find(params[:project_id]) authorize! :use, project - LiquidAssignsService.new(project: project).assigns + LiquidCachedAssigns.new(project: project) end end diff --git a/app/services/liquid_assigns_service.rb b/app/services/liquid_assigns_service.rb deleted file mode 100644 index 0260060f3..000000000 --- a/app/services/liquid_assigns_service.rb +++ /dev/null @@ -1,16 +0,0 @@ -class LiquidAssignsService - attr_accessor :project - - def initialize(project:) - @project = project - end - - def assigns - LiquidCachedAssigns.new(project: project) - end - - private - - def assigns_pro - end -end diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index cba8cbe8e..25a49b564 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -4,35 +4,31 @@ class LiquidCachedAssigns < Hash attr_accessor :assigns, :project def initialize(project:) - @assigns = { 'project' => ProjectDrop.new(@project) } + @assigns = { 'project' => ProjectDrop.new(project) } + @assigns.merge!(assigns_pro) + @project = project end def [](record_type) - return unless AVAILABLE_PROJECT_ASSIGNS.include?(record_type) - - if assigns[record_type] - assigns[record_type] - else - assigns[record_type] = cached_drops(record_type) - end + assigns[record_type] ||= cached_drops(record_type) end - def inspect - assigns.inspect + def key?(key) + AVAILABLE_PROJECT_ASSIGNS.include?(key.to_s) || assigns.key?(key) end - def key?(msg) - AVAILABLE_PROJECT_ASSIGNS.include?(msg.to_s) - end - - def respond_to?(msg) - return true if key?(msg) - super(msg) + def merge!(hash) + @assigns.merge!(hash) + self end private + def assigns_pro + {} + end + def cached_drops(record_type) records = project_records(record_type) @@ -47,6 +43,8 @@ def cached_drops(record_type) end def project_records(record_type) + return [] unless AVAILABLE_PROJECT_ASSIGNS.include?(record_type) + case record_type when 'evidences' project.evidence diff --git a/spec/services/liquid_assigns_service_spec.rb b/spec/services/liquid_assigns_service_spec.rb deleted file mode 100644 index 5e1fd0796..000000000 --- a/spec/services/liquid_assigns_service_spec.rb +++ /dev/null @@ -1,59 +0,0 @@ -require 'rails_helper' - -RSpec.describe LiquidAssignsService do - let!(:project) { create(:project) } - - before do - node = create(:node, project: project) - issue = create(:issue, node: project.issue_library) - create(:evidence, issue: issue, node: node) - create(:note, node: node) - create(:tag) - end - - describe '#project_assigns' do - context 'with the :text argument' do - LiquidAssignsService::AVAILABLE_PROJECT_ASSIGNS.each do |assign| - it "adds #{assign} to the project_assigns if present in the text" do - text = "#[Description]#\n {% for #{assign.singularize} in #{assign} %}{% endfor %}\n" - liquid_assigns = described_class.new(project: project, text: text).assigns - - expect(liquid_assigns.keys).to include(assign) - end - end - end - - context 'without the :text argument' do - let(:liquid_assigns) { described_class.new(project: project).assigns } - - it 'builds a hash of liquid assigns' do - expect(liquid_assigns['project'].name).to eq(project.name) - expect(liquid_assigns['issues'].map(&:title)).to eq(project.issues.map(&:title)) - expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title)) - expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label)) - expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title)) - expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name)) - end - end - end - - context 'with pro records', skip: !defined?(Dradis::Pro) do - let(:liquid_assigns) { described_class.new(project: project).assigns } - - let!(:project) { create(:project, :with_team) } - - before do - report_content = project.content_library - report_content.properties = { 'dradis.project' => project.name } - report_content.save - - create(:content_block, project: project) - end - - it 'builds a hash with Dradis::Pro assigns' do - expect(liquid_assigns['document_properties'].available_properties).to eq({ 'dradis.project' => project.name }) - expect(liquid_assigns['team'].name).to eq(project.team.name) - expect(liquid_assigns['content_blocks'].map(&:content)).to eq(project.content_blocks.map(&:content)) - end - end -end diff --git a/spec/services/liquid_cached_assigns_spec.rb b/spec/services/liquid_cached_assigns_spec.rb new file mode 100644 index 000000000..b53ec6c29 --- /dev/null +++ b/spec/services/liquid_cached_assigns_spec.rb @@ -0,0 +1,61 @@ +require 'rails_helper' + +RSpec.describe LiquidCachedAssigns do + let!(:project) { create(:project) } + let(:liquid_assigns) { described_class.new(project: project) } + + before do + node = create(:node, project: project) + issue = create(:issue, node: project.issue_library) + create(:evidence, issue: issue, node: node) + create(:note, node: node) + create(:tag) + end + + context 'fetching an assign from an available collection' do + it 'lazily loads the assigns' do + expect(liquid_assigns.assigns.keys).to_not include( + %w{issues evidences nodes notes tags} + ) + end + + it 'builds a hash of liquid assigns' do + expect(liquid_assigns['project'].name).to eq(project.name) + expect(liquid_assigns['issues'].map(&:title)).to eq(project.issues.map(&:title)) + expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title)) + expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label)) + expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title)) + expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name)) + end + end + + context 'fetching an assign from a unavailable collection' do + it 'returns an empty array' do + expect(liquid_assigns['fake']).to be_empty + end + end + + context 'with pro records', skip: !defined?(Dradis::Pro) do + let!(:project) { create(:project, :with_team) } + + before do + report_content = project.content_library + report_content.properties = { 'dradis.project' => project.name } + report_content.save + + create(:content_block, project: project) + end + + context 'fetching an assign from an available collection' do + it 'lazily loads the assigns' do + expect(liquid_assigns.assigns.keys).to_not include('content_blocks') + end + + it 'builds a hash with Dradis::Pro assigns' do + expect(liquid_assigns['document_properties'].available_properties).to eq({ 'dradis.project' => project.name }) + expect(liquid_assigns['team'].name).to eq(project.team.name) + expect(liquid_assigns['content_blocks'].map(&:content)).to eq(project.content_blocks.map(&:content)) + end + end + end +end From 1a56cde52d4ccc386683345d71cc99e17527ff4f Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Wed, 19 Jun 2024 18:51:32 +0800 Subject: [PATCH 3/7] Reorder initialization --- app/services/liquid_cached_assigns.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index 25a49b564..b1bd61079 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -4,10 +4,10 @@ class LiquidCachedAssigns < Hash attr_accessor :assigns, :project def initialize(project:) + @project = project + @assigns = { 'project' => ProjectDrop.new(project) } @assigns.merge!(assigns_pro) - - @project = project end def [](record_type) From cdaa6620b2d658a9b02df7cce79ce9674b6ffe92 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Fri, 21 Jun 2024 16:34:08 +0800 Subject: [PATCH 4/7] Add comment and implement #merge method --- app/services/liquid_cached_assigns.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index b1bd61079..2d64744d7 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -14,10 +14,18 @@ def [](record_type) assigns[record_type] ||= cached_drops(record_type) end + # SEE: https://github.com/Shopify/liquid/blob/77bc56/lib/liquid/context.rb#L211 + # Liquid is checking if the variable is present in the assigns hash by + # calling the `key?` method. Since we're lazily loading the keys, the variable + # may not yet be present in the assigns hash. def key?(key) AVAILABLE_PROJECT_ASSIGNS.include?(key.to_s) || assigns.key?(key) end + def merge(hash) + LiquidCachedAssigns.new(project: project).merge!(@assigns.merge(hash)) + end + def merge!(hash) @assigns.merge!(hash) self From 1ba771f2840bb2df8ee23fdd33e9b6daf093c2aa Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Mon, 24 Jun 2024 14:28:27 +0800 Subject: [PATCH 5/7] Include only node notes in liquid notes --- app/services/liquid_cached_assigns.rb | 3 +++ spec/services/liquid_cached_assigns_spec.rb | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index 2d64744d7..bf0c03b23 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -58,6 +58,9 @@ def project_records(record_type) project.evidence when 'nodes' project.nodes.user_nodes + when 'notes' + # FIXME - ISSUE/NOTE INHERITANCE + project.notes.includes(:node).where.not(node: { type_id: Node::Types::ISSUELIB }) else project.send(record_type.to_sym) end diff --git a/spec/services/liquid_cached_assigns_spec.rb b/spec/services/liquid_cached_assigns_spec.rb index b53ec6c29..34889a466 100644 --- a/spec/services/liquid_cached_assigns_spec.rb +++ b/spec/services/liquid_cached_assigns_spec.rb @@ -20,11 +20,13 @@ end it 'builds a hash of liquid assigns' do + issues = project.issues.map(&:title) + expect(liquid_assigns['project'].name).to eq(project.name) - expect(liquid_assigns['issues'].map(&:title)).to eq(project.issues.map(&:title)) + expect(liquid_assigns['issues'].map(&:title)).to eq(issues) expect(liquid_assigns['evidences'].map(&:title)).to eq(project.evidence.map(&:title)) expect(liquid_assigns['nodes'].map(&:label)).to eq(project.nodes.user_nodes.map(&:label)) - expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title)) + expect(liquid_assigns['notes'].map(&:title)).to eq(project.notes.map(&:title) - issues) expect(liquid_assigns['tags'].map(&:display_name)).to eq(project.tags.map(&:display_name)) end end From f7ec2af0c016a06ff37b523901cd7644a3b04d9b Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Wed, 26 Jun 2024 20:25:21 +0800 Subject: [PATCH 6/7] Simplify project.notes query --- app/services/liquid_cached_assigns.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index bf0c03b23..9e74dc0f1 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -60,7 +60,7 @@ def project_records(record_type) project.nodes.user_nodes when 'notes' # FIXME - ISSUE/NOTE INHERITANCE - project.notes.includes(:node).where.not(node: { type_id: Node::Types::ISSUELIB }) + project.notes.where.not(node_id: project.issue_library.id) else project.send(record_type.to_sym) end From 604b95b84c129fa5717e46b7697c8b9f88b054c2 Mon Sep 17 00:00:00 2001 From: Aaron Manaloto Date: Wed, 26 Jun 2024 21:55:07 +0800 Subject: [PATCH 7/7] Update merge method --- app/services/liquid_cached_assigns.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/services/liquid_cached_assigns.rb b/app/services/liquid_cached_assigns.rb index 9e74dc0f1..5f1e82562 100644 --- a/app/services/liquid_cached_assigns.rb +++ b/app/services/liquid_cached_assigns.rb @@ -23,7 +23,9 @@ def key?(key) end def merge(hash) - LiquidCachedAssigns.new(project: project).merge!(@assigns.merge(hash)) + lca = LiquidCachedAssigns.new(project: project) + lca.assigns = @assigns.merge(hash) + lca end def merge!(hash)