Use JSON string array of ids instead of ordered_aggregation linked list #5778

cjcolvar · 2024-04-12T16:03:25Z

Create a new property on media objects which store a JSON serialized array of ids: section_list. This property and related methods, sections and section_ids, replace nearly all calls to master_files, ordered_master_files, and indexed_master_files methods along with their *_ids counterparts. The only calls left to these methods are in the migration to the new property and one instance in MediaObjectsController that I wasn't able to figure out how to remove.

In order to migrate a media object it needs to be saved thus a full migration of all media objects might take too long to be done during a maintenance window. For this reason migrating is done in-place and lazily. When a media object is loaded from fedora it is migrated in-memory and made final on save. Subsequent loads from fedora use the already migrated section_list and should not attempt to read from list_source. After some time a portion of the repository should have been migrated naturally and a follow-up background job can be setup to force migration of the remainder of media objects. A future version of Avalon that requires the migration is complete might remove the ordered_aggregation and master_files association entirely.

While implementing this change I wanted to ensure that edits to section_list/sections/section_ids are in-memory only until saving. Previously changes to master_files and ordered_master_files were persisted immediately. In order to avoid that I put the synchronization of section_list and master_files into a before_save callback. This did cause some complications because that synchronization can lead to saves when objects were in odd states in the test suite. This was a reason for doing the switch of all references to master_files to sections throughout the code base. The only place I wasn't able to resolve this was in MediaObjectsController#update_media_object. One thing to be aware of is that because of this synchronization you may need to reload the media object after saving in order to get the updated master_files loaded. (See MasterFile#media_object= for an example of this.)

Review notes:

The main changes occur in app/models/media_object.rb and app/models/master_file.rb
I tried to leave in-line comments in important places to explain why certain things were necessary. Please add questions or suggestions if anything is unclear or could be improved.

It probably makes sense to squash this PR when merging.

Resolves #5749

Future work:

Test performance on avalon-dev on loading/editing many-sectioned items
Test migration on existing data

…ids and rename variables where appropriate as well

…ns/#section_ids

masaball

As far as I can tell this looks good. I do have a couple clarifying questions and some small suggestions.

masaball · 2024-05-31T14:24:22Z

spec/factories/media_objects.rb

@@ -49,6 +49,7 @@
        rights_statement { ['http://rightsstatements.org/vocab/InC-EDU/1.0/'] }
        terms_of_use { [ 'Terms of Use: Be kind. Rewind.' ] }
        series { [Faker::Lorem.word] }
+        master_files { [] }


Should this be sections { [] }?

Good question.

I don't think so? I think it is needed to initialize that association for tests that were depending on it. But I could try switching it and see if the tests still pass.

app/models/master_file.rb

app/presenters/speedy_af/proxy/media_object.rb

masaball · 2024-05-31T19:29:52Z

app/presenters/speedy_af/proxy/media_object.rb

    @attrs[:section_id] = solr_document["section_id_ssim"]
+    @attrs[:section_ids] = solr_document["section_id_ssim"]


Do these need to be separate fields? Seems weird to have a field with a singular name that contains the same data as a field with the pluralized name.

I think they do because adding @attrs[:section_ids] allows this presenter to proxy the new MediaObject#section_ids method. Alternatively this presenter could delegate section_ids to section_id or implement a new method which returns section_id. Do you think either of those would make more sense? I didn't want to remove section_id from the attributes because we had already added it so I know it is used elsewhere although I can't remember exactly where.

spec/models/media_object_spec.rb

masaball · 2024-05-31T19:54:19Z

spec/models/media_object_spec.rb

+      it 'migrates to section_list without interaction' do
+        expect(media_object.section_list).to eq nil
+        mo = MediaObject.find(media_object.id)
+        mo.save
+        mo = MediaObject.find(media_object.id)
+        expect(mo.section_list).not_to eq nil
+        expect(mo.sections).to eq [section, section2]
+        expect(mo.section_ids).to eq [section.id, section2.id]
+      end


I'm not sure I understand what exactly this test case is for? Is it just that section_list fields are persisted?

I think you're right that this test only shows that section_list persists. I added a test to show that when section_list is set a MediaObject won't use ordered_master_files anymore. This new test plus the one above which shows that MediaObject will read from ordered_master_files to populate section_list when it isn't set should probably cover all cases so I removed this test.

masaball · 2024-05-31T19:59:15Z

spec/models/media_object_spec.rb

+        expect(mo.ordered_master_files.to_a).to eq [section, section2]
+        expect(mo.ordered_master_file_ids).to eq [section.id, section2.id]
+        expect(mo.sections).to eq [section, section2]
+        expect(mo.section_ids).to eq [section.id, section2.id]


Instead of explicitly testing for all those specific arrays, would it make sense to condense this to equality checks?

Suggested change

expect(mo.ordered_master_files.to_a).to eq [section, section2]

expect(mo.ordered_master_file_ids).to eq [section.id, section2.id]

expect(mo.sections).to eq [section, section2]

expect(mo.section_ids).to eq [section.id, section2.id]

expect(mo.sections).to eq mo.ordered_master_files.to_a

expect(mo.section_ids).to eq mo.ordered_master_file_ids

I like this change, but I think I'd also like to keep one of the expectations to ensure that mo.ordered_master_files is actually populated the way I'm expecting.

app/models/media_object.rb

cjcolvar force-pushed the ordered_aggregation_fire branch from 34c3f33 to 71f68e7 Compare April 12, 2024 18:43

cjcolvar force-pushed the develop branch from 5471ec1 to 3832fb1 Compare April 26, 2024 20:31

cjcolvar force-pushed the ordered_aggregation_fire branch from e5dcb7c to 5fadc1b Compare April 30, 2024 18:59

cjcolvar added 4 commits May 17, 2024 12:04

Proof of concept

f883b5a

Cache MasterFiles to avoid calling fedora each time

579e717

Rename property and main read/write methods

d2a1ee9

Migrate from ordered_aggregation to JSON id array property lazily

c1768fe

cjcolvar force-pushed the ordered_aggregation_fire branch from 5b23684 to c1768fe Compare May 17, 2024 16:08

cjcolvar added 2 commits May 20, 2024 12:48

Workaround #5834

5fb5b18

Rework migration callbacks and add some tests

dc1bc63

cjcolvar force-pushed the ordered_aggregation_fire branch from f434642 to dc1bc63 Compare May 21, 2024 14:41

cjcolvar added 2 commits May 21, 2024 12:45

Fix tests and convert #master_files calls to #sections

1920e8f

Further test fixes and reworking of saving master_files

9dfe685

cjcolvar force-pushed the ordered_aggregation_fire branch 2 times, most recently from f71b199 to 5828937 Compare May 22, 2024 19:33

Convert calls to master_files or master_file_ids to sections/section_…

efb8b83

…ids and rename variables where appropriate as well

cjcolvar force-pushed the ordered_aggregation_fire branch from 5828937 to efb8b83 Compare May 22, 2024 22:25

cjcolvar marked this pull request as ready for review May 23, 2024 13:02

cjcolvar changed the title ~~[Proof of concept] Use JSON string array of ids instead of ordered_aggregation linked list~~ Use JSON string array of ids instead of ordered_aggregation linked list May 23, 2024

Remove #master_files/#master_file_ids from presenter and test #sectio…

1787dc9

…ns/#section_ids

cjcolvar force-pushed the ordered_aggregation_fire branch from 65e3815 to 1787dc9 Compare May 24, 2024 15:59

masaball reviewed May 31, 2024

View reviewed changes

Changes based upon code review

3ef0e6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use JSON string array of ids instead of ordered_aggregation linked list #5778

Use JSON string array of ids instead of ordered_aggregation linked list #5778

cjcolvar commented Apr 12, 2024 •

edited

masaball left a comment

masaball May 31, 2024

cjcolvar May 31, 2024

masaball May 31, 2024

cjcolvar May 31, 2024

masaball May 31, 2024

cjcolvar May 31, 2024

masaball May 31, 2024

cjcolvar May 31, 2024

		@attrs[:section_id] = solr_document["section_id_ssim"]
		@attrs[:section_ids] = solr_document["section_id_ssim"]

Use JSON string array of ids instead of ordered_aggregation linked list #5778

Are you sure you want to change the base?

Use JSON string array of ids instead of ordered_aggregation linked list #5778

Conversation

cjcolvar commented Apr 12, 2024 • edited

masaball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcolvar commented Apr 12, 2024 •

edited