fix solr query in MultipleMembershipChecker and refactor to improve readability #5055
Conversation
| # @param collection_ids [Array<String>] a list of collection identifiers | ||
| # @param include_current_members [Boolean] a flag to also scan an object's | ||
| # current collection memberships | ||
| # @param include_current_members [Boolean] if true, include item's existing collections in check; else if false, check passed in collections only |
There was a problem hiding this comment.
can we shorten the line lengths here? i try to set a soft cap at 80 chars especially in consideration of samvera/bixby#48.
since these lines can be wrapped without syntax considerations, it seems best to do so.
There was a problem hiding this comment.
I shortened all long lines.
spec/spec_helper.rb
Outdated
| @@ -108,6 +108,7 @@ def clean_active_fedora_repository | |||
| # The JS is executed in a different thread, so that other thread | |||
| # may think the root path has already been created: | |||
| ActiveFedora.fedora.connection.send(:init_base_path) | |||
| Hyrax.persister.wipe! if Hyrax.config.query_index_from_valkyrie | |||
There was a problem hiding this comment.
i'm not following this. the Hyrax.persister is independent of the index, right?
There was a problem hiding this comment.
You would think that. And perhaps this should be somewhere else, but I'm just using the existing wipe! method defined in wings persister.
hyrax/lib/wings/valkyrie/persister.rb
Lines 60 to 65 in 4efa6b8
There was a problem hiding this comment.
An alternative...
# define in Hyrax::SolrService
def wipe!
Hyrax::SolrService.delete_by_query("*:*")
Hyrax::SolrService.commit
end
# update in Wings::Valkyrie::Persister
def wipe!
Hyrax::SolrService.wipe!
ActiveFedora::Cleaner.clean!
end
There was a problem hiding this comment.
i don't think it's safe to assume that Hyrax.persister is a the wings persister (it's not for many tests).
i think i'm not understanding the use case in general, either. this method is named #clean_active_fedora_repository; when/why does that need to clear the custom, non-AF solr index?
There was a problem hiding this comment.
I added a wipe! method to Hyrax::SolrService and moved that call to the :clean_repo section of spec_helper.rb since that is what triggers the call from the spec. I think the assumption here is that the writer of the test does want all repos (e.g. Fedora, Redis, Solr) cleared to start the tests with a clean slate, including Valkyrie solr if it is in use.
Long term, this should perhaps call Hyrax.persister.wipe! with the theory that if :clean_repo is being called by a spec, then that persister was used. Or we may want to make this more granular (e.g. :wipe_hyrax_persister, :wipe_solr, etc.).
elrayle
force-pushed
the
refactor/multi_mem_checker
branch
from
1f9619e
to
50c2e66
Compare
…eadability The code was complex and difficult to follow. I refactored so that each method has one and only one task and is clearly named for that task. Also, tests were heavily mocked and were hiding errors in the generated solr query. The tests are rewritten to organize them into clear test scenarios with real solr access.
elrayle
force-pushed
the
refactor/multi_mem_checker
branch
from
50c2e66
to
9780d95
Compare
|
FYI... I kicked off the failing tests again. The two that failed on the last round fail locally whether on my branch or main. And they passed for some ruby versions. It looks like they may be random failures. |
|
yeah. the two failing specs are new in the last week or so, and appear to be a test order issue. i'll try to clear it later today. |
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
DO NOT MERGE - pending merge of PR #5055 ---- * deprecates `Hyrax::CollectionBehavior#add_member_objects` * updates all places that call it to `Hyrax::Collections::CollectionMemberService.add_members`
The solr query being generated was causing the multiple membership #check to fail if there were two collections of the same single membership type even if the item was not in both collections. The tests failed to catch this very common scenario.
This led to a refactor of the code which was complex and difficult to follow. Now each method has one and only one task and is clearly named for that task.
Also the reason the tests failed to catch the common scenario is that the access to solr was heavily mocked and returned what was expected to be returned from solr and not what was actually being returned from solr. This was hiding errors in the generated solr query for multiple tests. The tests are rewritten to organize them into clear scenarios based on the options for passed in parameters and the state of membership for the item using real solr access.
@samvera/hyrax-code-reviewers