From 5fff58cdb4d0e348bbc864ef75f954f71a730db8 Mon Sep 17 00:00:00 2001 From: Jeremy Friesen Date: Thu, 2 Feb 2023 11:22:59 -0500 Subject: [PATCH] Ensuring self-contained memoization of method (#725) Prior to this commit, the guard condition `return self.collection_ids if collections_created?` served as memoization (e.g. caching the results of an expensive operation). However, it was not a self-contained memoization (e.g. the method is fully in charge of managing the memoization). It instead relied on the previous implementation of `#collections_created?`. Downstream implementations may change `#collection_created?` which could create unintended consenquences of assignign collections based on the OAI spec. Also, if we look to other implementations in Bulkrax of `#collections_created?` we see that many simply `return true`. The impact of the previous code is that were someone to make the above change (e.g. always `return true`), we end up never assigning any `setSpec` `collection_ids` and always returning `[]` for `#find_collection_ids`. dependent on external methods. With this commit, we are internalizing the memoization mechanism so that this method is less dependent on external factories. For further discussion of the interplay of the methods regarding collection creation see the [comments found here][1]. References: - https://github.com/scientist-softserv/adventist-dl/pull/247 - https://github.com/scientist-softserv/adventist-dl/issues/240 [1]: https://github.com/scientist-softserv/adventist-dl/issues/240#issuecomment-1412456590 --- app/models/bulkrax/oai_entry.rb | 5 ++++- spec/models/bulkrax/oai_entry_spec.rb | 10 ++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/bulkrax/oai_entry.rb b/app/models/bulkrax/oai_entry.rb index f465f74d..94623859 100644 --- a/app/models/bulkrax/oai_entry.rb +++ b/app/models/bulkrax/oai_entry.rb @@ -91,7 +91,8 @@ def add_thumbnail_url # If OAI-PMH doesn't return setSpec in the headers for GetRecord, use parser.collection_name # in this case, if 'All' is selected, records will not be added to a collection. def find_collection_ids - return self.collection_ids if collections_created? + return self.collection_ids if defined?(@called_find_collection_ids) + if sets.blank? || parser.collection_name != 'all' collection = find_collection(importerexporter.unique_collection_identifier(parser.collection_name)) self.collection_ids << collection.id if collection.present? && !self.collection_ids.include?(collection.id) @@ -101,6 +102,8 @@ def find_collection_ids self.collection_ids << c.id if c.present? && !self.collection_ids.include?(c.id) end end + + @called_find_collection_ids = true return self.collection_ids end end diff --git a/spec/models/bulkrax/oai_entry_spec.rb b/spec/models/bulkrax/oai_entry_spec.rb index dc14e9b2..d89dbdd5 100644 --- a/spec/models/bulkrax/oai_entry_spec.rb +++ b/spec/models/bulkrax/oai_entry_spec.rb @@ -98,6 +98,11 @@ class ::Avacado < Work # rubocop:enable RSpec/VerifiedDoubles it 'derives the factory class before proceeding with adding other metadata' do + # In changing the memoization mechanism, I was encountering a case where the application was + # attempting to call SOLR. Something previous not required due to implementation details. + # As we're not testing collection id related things, this "hack" seems acceptable. + entry.instance_variable_set("@called_find_collection_ids", true) + # TODO: Not a big fan of this method chain antics. Ideally we'd pass in the object at # instantiation time. However I'm cribbing past work and trying to get this fix out into # the wild. @@ -128,6 +133,11 @@ class ::Avacado < Work end it 'adds admin set id to parsed metadata' do + # In changing the memoization mechanism, I was encountering a case where the application was + # attempting to call SOLR. Something previous not required due to implementation details. + # As we're not testing collection id related things, this "hack" seems acceptable. + entry.instance_variable_set("@called_find_collection_ids", true) + allow(entry).to receive_message_chain(:record, :header, :identifier).and_return("some_identifier") allow(entry).to receive_message_chain(:record, :header, :set_spec).and_return([]) allow(entry).to receive_message_chain(:record, :metadata, :children).and_return([])