From 940bbad8841a4849f23ad235e21cd50ed6a2da4b Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Wed, 15 Dec 2021 13:35:54 +0530 Subject: [PATCH 1/6] 0 length path in eager loading module --- lib/active_graph/node/query/query_proxy_eager_loading.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading.rb b/lib/active_graph/node/query/query_proxy_eager_loading.rb index c8b5baa49..8cfe9162b 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading.rb @@ -18,7 +18,8 @@ def perform_query .map do |record, eager_data| record = cache_and_init(record, with_associations_tree) eager_data.zip(with_associations_tree.paths.map(&:last)).each do |eager_records, element| - eager_records.first.zip(eager_records.last).each do |eager_record| + eager_records.each do |eager_record| + next unless eager_record.first && eager_record.first.type.to_s == element.association.relationship_type.to_s add_to_cache(*eager_record, element) end end @@ -68,7 +69,7 @@ def add_to_cache(rel, node, element) def init_associations(node, element) element.each_key { |key| node.association_proxy(key).init_cache } - node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:max] == '' + node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:min] == 0 end def cache_and_init(node, element) @@ -130,13 +131,13 @@ def query_from_association_tree def with_association_query_part(base_query, path, previous_with_vars) optional_match_with_where(base_query, path, previous_with_vars) .with(identity, - "[#{relationship_collection(path)}, collect(#{escape path_name(path)})] "\ + "collect([#{relationship_collection(path)}, #{escape path_name(path)}]) "\ "AS #{escape("#{path_name(path)}_collection")}", *previous_with_vars) end def relationship_collection(path) - path.last.rel_length ? "collect(last(relationships(#{escape("#{path_name(path)}_path")})))" : "collect(#{escape("#{path_name(path)}_rel")})" + path.last.rel_length ? "last(relationships(#{escape("#{path_name(path)}_path")}))" : "#{escape("#{path_name(path)}_rel")}" end def optional_match_with_where(base_query, path, _) From 56e5785b514cfdf357f21f86fb3d04719e0801b3 Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Fri, 17 Dec 2021 17:05:56 +0530 Subject: [PATCH 2/6] parsing relationship --- activegraph.gemspec | 1 + lib/active_graph.rb | 2 ++ .../node/query/query_proxy_eager_loading.rb | 2 +- .../association_tree.rb | 7 +++---- lib/active_graph/string_parsers.rb | 8 ++++++++ .../string_parsers/relation_parser.rb | 18 ++++++++++++++++++ 6 files changed, 33 insertions(+), 5 deletions(-) create mode 100644 lib/active_graph/string_parsers.rb create mode 100644 lib/active_graph/string_parsers/relation_parser.rb diff --git a/activegraph.gemspec b/activegraph.gemspec index 5d161a1f5..384c66fea 100644 --- a/activegraph.gemspec +++ b/activegraph.gemspec @@ -36,6 +36,7 @@ DESCRIPTION s.add_dependency('activesupport', '>= 4.0') s.add_dependency('i18n', '!= 1.8.8') # https://github.com/jruby/jruby/issues/6547 s.add_dependency('orm_adapter', '~> 0.5.0') + s.add_dependency('parslet') s.add_dependency('sorted_set') s.add_dependency("neo4j-java-driver", '>= 4.3.0') s.add_development_dependency('guard') diff --git a/lib/active_graph.rb b/lib/active_graph.rb index df622027e..3b1f5dda2 100644 --- a/lib/active_graph.rb +++ b/lib/active_graph.rb @@ -1,4 +1,5 @@ require 'forwardable' +require 'parslet' require 'active_graph/version' require 'active_graph/core' @@ -8,6 +9,7 @@ require 'active_graph/transactions' require 'active_graph/base' require 'active_graph/model_schema' +require 'active_graph/string_parsers/relation_parser' require 'active_model' require 'active_support/concern' diff --git a/lib/active_graph/node/query/query_proxy_eager_loading.rb b/lib/active_graph/node/query/query_proxy_eager_loading.rb index 8cfe9162b..8f67f2aa1 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading.rb @@ -69,7 +69,7 @@ def add_to_cache(rel, node, element) def init_associations(node, element) element.each_key { |key| node.association_proxy(key).init_cache } - node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:min] == 0 + node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:min] == "0" end def cache_and_init(node, element) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb index 559423538..1a95abf09 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb @@ -56,10 +56,9 @@ def add_nested(key, value, length = nil) end def process_string(str) - head, rest = str.split('.', 2) - k, length = head.split('*', -2) - length = {max: length} if length - add_nested(k.to_sym, rest, length) + map = StringParsers::RelationParser.new.parse(str) + #binding.pry + add_nested(map[:rel_name].to_sym, map[:rest_str], map[:length_part]) end private diff --git a/lib/active_graph/string_parsers.rb b/lib/active_graph/string_parsers.rb new file mode 100644 index 000000000..c7d872a63 --- /dev/null +++ b/lib/active_graph/string_parsers.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +require 'active_graph/strin_parsers/relation_parser' + +module ActiveGraph + module StringParsers + end +end diff --git a/lib/active_graph/string_parsers/relation_parser.rb b/lib/active_graph/string_parsers/relation_parser.rb new file mode 100644 index 000000000..2e3511d35 --- /dev/null +++ b/lib/active_graph/string_parsers/relation_parser.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module ActiveGraph + module StringParsers + # Filtering relationships with length + class RelationParser < ::Parslet::Parser + rule(:asterix) { str('*') } + rule(:digit) { match('[\d]').repeat } + rule(:range) { str('..') } + rule(:dot) { str('.') } + rule(:length) { asterix >> digit.maybe.as(:min) >> range.maybe >> digit.maybe.as(:max) } + rule(:rel) { match('[a-z,_]').repeat.as(:rel_name) } + rule(:key) { rel >> length.maybe.as(:length_part) } + rule(:anything) { match('.+') } + rule(:root) { key >> dot.maybe >> anything.maybe.as(:rest_str) } + end + end +end From 40e9ae6f98fd887f5d2867622946507a6a66424e Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Mon, 3 Jan 2022 13:58:36 +0530 Subject: [PATCH 3/6] allowing only zero length start --- .../query/query_proxy_eager_loading/association_tree.rb | 5 ++--- lib/active_graph/string_parsers/relation_parser.rb | 7 +++++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb index 1a95abf09..f40b363e8 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb @@ -57,14 +57,13 @@ def add_nested(key, value, length = nil) def process_string(str) map = StringParsers::RelationParser.new.parse(str) - #binding.pry - add_nested(map[:rel_name].to_sym, map[:rest_str], map[:length_part]) + add_nested(map[:rel_name].to_sym, map[:rest_str].to_s.presence, map[:length_part]) end private def target_class(model, key) - association = model.associations[key] + association = model.associations[key.to_sym] fail "Invalid association: #{[*path, key].join('.')}" unless association model.associations[key].target_class end diff --git a/lib/active_graph/string_parsers/relation_parser.rb b/lib/active_graph/string_parsers/relation_parser.rb index 2e3511d35..8b91179d1 100644 --- a/lib/active_graph/string_parsers/relation_parser.rb +++ b/lib/active_graph/string_parsers/relation_parser.rb @@ -8,10 +8,13 @@ class RelationParser < ::Parslet::Parser rule(:digit) { match('[\d]').repeat } rule(:range) { str('..') } rule(:dot) { str('.') } - rule(:length) { asterix >> digit.maybe.as(:min) >> range.maybe >> digit.maybe.as(:max) } + rule(:zero) { str('0') } + rule(:length_1) { zero.as(:min) >> range >> digit.maybe.as(:max) } + rule(:length_2) { digit.maybe.as(:max) } + rule(:length) { asterix >> (length_1 | length_2) } rule(:rel) { match('[a-z,_]').repeat.as(:rel_name) } rule(:key) { rel >> length.maybe.as(:length_part) } - rule(:anything) { match('.+') } + rule(:anything) { match('.').repeat } rule(:root) { key >> dot.maybe >> anything.maybe.as(:rest_str) } end end From 83b5d9d715eb23fff8f4d8e387ab083893b4679f Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Tue, 4 Jan 2022 12:25:16 +0530 Subject: [PATCH 4/6] Specs and allowing only one zero length path --- .../node/query/query_proxy_eager_loading.rb | 2 +- .../query_proxy_eager_loading/association_tree.rb | 14 ++++++++++++++ spec/e2e/association_proxy_spec.rb | 13 +++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading.rb b/lib/active_graph/node/query/query_proxy_eager_loading.rb index 8f67f2aa1..0c5c2a33d 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading.rb @@ -30,7 +30,7 @@ def perform_query def with_associations(*spec) new_link.tap do |new_query_proxy| new_query_proxy.with_associations_tree = with_associations_tree.clone - new_query_proxy.with_associations_tree.add_spec(spec) + new_query_proxy.with_associations_tree.add_spec_and_validate(spec) end end diff --git a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb index f40b363e8..886da79ae 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb @@ -17,6 +17,20 @@ def clone super.tap { |copy| copy.each { |key, value| copy[key] = value.clone } } end + def add_spec_and_validate(spec) + add_spec(spec) + validate_for_zero_length_paths + end + + def validate_for_zero_length_paths + fail 'Can not eager load more than one zero length path.' if values.count { |value| value.zero_length_path? } > 1 + end + + def zero_length_path? + rel_length&.fetch(:min, nil)&.to_s == '0' || + values.any? { |value| value.zero_length_path? } + end + def add_spec(spec) fail_spec(spec) unless model diff --git a/spec/e2e/association_proxy_spec.rb b/spec/e2e/association_proxy_spec.rb index 9424e7c88..07d81b947 100644 --- a/spec/e2e/association_proxy_spec.rb +++ b/spec/e2e/association_proxy_spec.rb @@ -356,17 +356,26 @@ def deep_traversal(person) let(:friend1) { Person.create(name: 'f-1', knows: friend2) } let(:friend2) { Person.create(name: 'f-2', knows: friend3) } let(:friend3) { Person.create(name: 'f-3') } - let(:billy_comment) { Comment.create(text: 'test-comment', owner: node) } - let(:comment) { Comment.create(text: 'test-comment', owner: friend1) } + let(:billy_comment) { Comment.create(text: 'billy-comment', owner: node) } + let(:comment) { Comment.create(text: 'f-1-comment', owner: friend1) } before { Post.create(name: 'Post-1', owner: node, comments: [comment, billy_comment]) } + it 'Raises error if attempting to eager load more than one zero length paths' do + expect { Person.all.with_associations(['knows*0..','comments.owner.knows*0..']) }.to raise_error(RuntimeError, /Can not eager load more than one zero length path./) + end + it 'Should allow for string parameter with variable length relationship notation' do expect_queries(1) do Post.comments.with_associations(owner: 'knows*').map(&:owner).each(&method(:deep_traversal)) end end + it 'Should allow for zero length paths' do + expect_queries(1) do + Post.comments.with_associations(owner: 'knows*0..', ).map(&:owner).each(&method(:deep_traversal)) + end + end it 'allows on demand retrieval beyond eagerly fetched associations' do expect(Post.owner.with_associations('knows*2')[0].knows[0].knows[0].knows[0].name).to eq 'f-3' From efb44b0213bdd3c65d50d6b5a09e118c06d83c45 Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Tue, 4 Jan 2022 12:41:01 +0530 Subject: [PATCH 5/6] fix for inti association --- lib/active_graph/node/query/query_proxy_eager_loading.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading.rb b/lib/active_graph/node/query/query_proxy_eager_loading.rb index 0c5c2a33d..62e7b3c1b 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading.rb @@ -69,7 +69,7 @@ def add_to_cache(rel, node, element) def init_associations(node, element) element.each_key { |key| node.association_proxy(key).init_cache } - node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:min] == "0" + node.association_proxy(element.name).init_cache if element.rel_length && element.rel_length[:max] == '' end def cache_and_init(node, element) From 1a77b927701940f4656c9e442ecd2e217d673c5a Mon Sep 17 00:00:00 2001 From: Amit Suryavanshi Date: Tue, 4 Jan 2022 13:44:14 +0530 Subject: [PATCH 6/6] refactor --- lib/active_graph/node/query/query_proxy_eager_loading.rb | 4 ++-- .../node/query/query_proxy_eager_loading/association_tree.rb | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading.rb b/lib/active_graph/node/query/query_proxy_eager_loading.rb index 62e7b3c1b..91a483f20 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading.rb @@ -19,7 +19,7 @@ def perform_query record = cache_and_init(record, with_associations_tree) eager_data.zip(with_associations_tree.paths.map(&:last)).each do |eager_records, element| eager_records.each do |eager_record| - next unless eager_record.first && eager_record.first.type.to_s == element.association.relationship_type.to_s + next unless eager_record.first&.type&.to_s == element.association.relationship_type.to_s add_to_cache(*eager_record, element) end end @@ -137,7 +137,7 @@ def with_association_query_part(base_query, path, previous_with_vars) end def relationship_collection(path) - path.last.rel_length ? "last(relationships(#{escape("#{path_name(path)}_path")}))" : "#{escape("#{path_name(path)}_rel")}" + path.last.rel_length ? "last(relationships(#{escape("#{path_name(path)}_path")}))" : escape("#{path_name(path)}_rel") end def optional_match_with_where(base_query, path, _) diff --git a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb index 886da79ae..c2a883fca 100644 --- a/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb +++ b/lib/active_graph/node/query/query_proxy_eager_loading/association_tree.rb @@ -23,12 +23,11 @@ def add_spec_and_validate(spec) end def validate_for_zero_length_paths - fail 'Can not eager load more than one zero length path.' if values.count { |value| value.zero_length_path? } > 1 + fail 'Can not eager load more than one zero length path.' if values.count(&:zero_length_path?) > 1 end def zero_length_path? - rel_length&.fetch(:min, nil)&.to_s == '0' || - values.any? { |value| value.zero_length_path? } + rel_length&.fetch(:min, nil)&.to_s == '0' || values.any?(&:zero_length_path?) end def add_spec(spec)