-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use solr's graph query builder for nested collection queries #5850
Changes from all commits
f23358d
6772fe2
a39da92
78a6098
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,6 @@ def initialize(parent: nil, | |
validates :parent, presence: true | ||
validates :child, presence: true | ||
validate :parent_and_child_can_be_nested | ||
validate :nesting_within_maximum_depth | ||
|
||
def save | ||
return false unless valid? | ||
|
@@ -83,12 +82,11 @@ def available_parent_collections | |
# rerouting to new_dashboard_collection_path to add the new collection as | ||
# a child. Since we don't yet have a child collection, the valid? option can't be used here. | ||
def validate_add | ||
if nestable?(parent) | ||
nesting_within_maximum_depth | ||
else | ||
unless nestable?(parent) | ||
errors.add(:parent, :cannot_have_child_nested) | ||
false | ||
return false | ||
end | ||
true | ||
end | ||
|
||
def remove | ||
|
@@ -104,17 +102,6 @@ def remove | |
|
||
attr_accessor :query_service, :persistence_service, :context, :collection | ||
|
||
# ideally we would love to be able to eliminate collections which exceed the | ||
# maximum nesting depth from the lists of available collections, but the queries | ||
# needed to make the determination are too expensive to do for every possible | ||
# collection, so we only test for this situation prior to saving the new | ||
# relationship. | ||
def nesting_within_maximum_depth | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm trying to write a commit message for the backport of this change---which no-ops this validation if the solr graph query is configured. but i'm a bit stuck and realizing i'm not entirely clear on why we're removing this. can you help me @cjcolvar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that the max depth was only needed to limit the amount of nested reindexing to undertake. Now that the indexing isn't necessary the technical limit seems like solr's graph query parser's efficiency. If there is a functional reason for having a limit (and making it adopter configurable) then I could restore the depth limit enforcement with a new implementation. The configuration of the max depth was also on the Samvera::NestingIndexer so we'd have to make a new config in Hyrax for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks! super helpful |
||
return true if query_service.valid_combined_nesting_depth?(parent: parent, child: child, scope: context) | ||
errors.add(:collection, :exceeds_maximum_nesting_depth) | ||
false | ||
end | ||
|
||
def parent_and_child_can_be_nested | ||
if nestable?(parent) && nestable?(child) | ||
return true if query_service.parent_and_child_can_nest?(parent: parent, child: child, scope: context) | ||
|
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we guarding circular nesting? (e.g. A has member B has member C has member A)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the documentation, the graph query parser is "cyclic aware".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on a current Hyrax, we allow circular nesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is being handled by the
parent_and_child_can_be_nested
validation on the form.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@no-reply Can you give an example of currently allowed/working circular nesting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was able to create this situation trivially in the UI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't able to reproduce this in nurax-dev. Can you please post the exact steps that allowed this for you? I'd like to reproduce this in nurax-dev then test the same steps with this PR to see how the behavior differs.