Use JSON string array of ids instead of ordered_aggregation linked li…

…st (#5778)

* Proof of concept

* Cache MasterFiles to avoid calling fedora each time

* Rename property and main read/write methods

* Migrate from ordered_aggregation to JSON id array property lazily

* Workaround #5834

* Rework migration callbacks and add some tests

* Fix tests and convert #master_files calls to #sections

* Further test fixes and reworking of saving master_files

* Convert calls to master_files or master_file_ids to sections/section_ids and rename variables where appropriate as well

* Remove #master_files/#master_file_ids from presenter and test #sections/#section_ids

* Changes based upon code review

app/controllers/bookmarks_controller.rb

@@ -141,7 +141,7 @@ def add_to_playlist_action documents
     playlist = Playlist.find(params[:target_playlist_id])
     Array(documents.map(&:id)).each do |id|
       media_object = SpeedyAF::Proxy::MediaObject.find(id)
-      media_object.ordered_master_files.to_a.each do |mf|
+      media_object.sections.each do |mf|
         clip = AvalonClip.create(master_file: mf)
         PlaylistItem.create(clip: clip, playlist: playlist)
       end

app/controllers/master_files_controller.rb

@@ -203,13 +203,10 @@ def destroy
     authorize! :destroy, @master_file, message: "You do not have sufficient privileges to delete files"
     filename = File.basename(@master_file.file_location) if @master_file.file_location.present?
     filename ||= @master_file.id
-    media_object = MediaObject.find(@master_file.media_object_id)
-    media_object.ordered_master_files.delete(@master_file)
-    media_object.master_files.delete(@master_file)
-    media_object.save
+    media_object_id = @master_file.media_object_id
     @master_file.destroy
     flash[:notice] = "#{filename} has been deleted from the system"
-    redirect_to edit_media_object_path(media_object, step: "file-upload")
+    redirect_to edit_media_object_path(media_object_id, step: "file-upload")
   end
 
   def set_frame

app/controllers/media_objects_controller.rb

@@ -119,7 +119,7 @@ def add_to_playlist
     end
     playlistitem_scope = params[:post][:playlistitem_scope] #'section', 'structure'
     # If a single masterfile_id wasn't in the request, then create playlist_items for all masterfiles
-    masterfile_ids = masterfile_id.present? ? [masterfile_id] : @media_object.ordered_master_file_ids
+    masterfile_ids = masterfile_id.present? ? [masterfile_id] : @media_object.section_ids
     masterfile_ids.each do |mf_id|
       mf = SpeedyAF::Proxy::MasterFile.find(mf_id)
       if playlistitem_scope=='structure' && mf.has_structuralMetadata? && mf.structuralMetadata.xpath('//Span').present?
@@ -139,7 +139,7 @@ def add_to_playlist
         end
       else
         #create a single item for the entire masterfile
-        item_title = @media_object.master_file_ids.count>1? mf.embed_title : @media_object.title
+        item_title = @media_object.section_ids.count > 1 ? mf.embed_title : @media_object.title
         clip = AvalonClip.new(title: item_title, master_file: mf)
         playlist.items += [PlaylistItem.new(clip: clip, playlist: playlist)]
       end
@@ -216,7 +216,7 @@ def update_media_object
     if !@media_object.save
       error_messages += ['Failed to create media object:']+@media_object.errors.full_messages
     elsif master_files_params.respond_to?('each')
-      old_ordered_master_files = @media_object.ordered_master_files.to_a.collect(&:id)
+      old_ordered_sections = @media_object.section_ids
       master_files_params.each_with_index do |file_spec, index|
         master_file = MasterFile.new(file_spec.except(:structure, :captions, :captions_type, :files, :other_identifier, :label, :date_digitized))
         # master_file.media_object = @media_object
@@ -230,12 +230,11 @@ def update_media_object
         master_file.date_digitized = DateTime.parse(file_spec[:date_digitized]).to_time.utc.iso8601 if file_spec[:date_digitized].present?
         master_file.identifier += Array(params[:files][index][:other_identifier])
         master_file.comment += Array(params[:files][index][:comment])
-        master_file._media_object = @media_object
+        master_file.media_object = @media_object
         if file_spec[:files].present?
           if master_file.update_derivatives(file_spec[:files], false)
             master_file.update_stills_from_offset!
             WaveformJob.perform_later(master_file.id)
-            @media_object.ordered_master_files += [master_file]
           else
             file_location = file_spec.dig(:file_location) || '<unknown>'
             message = "Problem saving MasterFile for #{file_location}:"
@@ -248,10 +247,14 @@ def update_media_object
 
       if error_messages.empty?
         if api_params[:replace_masterfiles]
-          old_ordered_master_files.each do |mf|
+          old_ordered_sections.each do |mf|
             p = MasterFile.find(mf)
+            # FIXME: Figure out why this next line is necessary
+            # without it the save below will fail with Ldp::Gone when attempting to set master_files in the MediaObject before_save callback
+            # This could be avoided by doing a reload after all of the destroys but I'm afraid that would mess up other changes already staged in-memory.
             @media_object.master_files.delete(p)
-            @media_object.ordered_master_files.delete(p)
+            # Need to manually remove from section_ids in memory to match changes that will persist when the master file is destroyed
+            @media_object.section_ids -= [p.id]
             p.destroy
           end
         end
@@ -284,7 +287,7 @@ def update_media_object
 
   def custom_edit
     if ['preview', 'structure', 'file-upload'].include? @active_step
-      @masterFiles = load_master_files
+      @masterFiles = load_sections
     end
 
     if 'preview' == @active_step
@@ -364,9 +367,9 @@ def show_progress
         [encode.master_file_id, mf_status]
       end
     ]
-    master_files_count = @media_object.master_files.size
-    if master_files_count > 0
-      overall.each { |k,v| overall[k] = [0,[100,v.to_f/master_files_count.to_f].min].max.floor }
+    sections_count = @media_object.sections.size
+    if sections_count > 0
+      overall.each { |k,v| overall[k] = [0,[100,v.to_f/sections_count.to_f].min].max.floor }
     else
       overall = {success: 0, error: 0}
     end
@@ -452,7 +455,7 @@ def tree
       }
       format.json {
         result = { @media_object.id => {} }
-        @media_object.indexed_master_files.each do |mf|
+        @media_object.sections.each do |mf|
           result[@media_object.id][mf.id] = mf.derivatives.collect(&:id)
         end
         render :json => result
@@ -525,11 +528,11 @@ def load_resource
 
   def master_file_presenters
     # Assume that @media_object is a SpeedyAF::Proxy::MediaObject
-    @media_object.ordered_master_files
+    @media_object.sections
   end
 
-  def load_master_files(mode = :rw)
-    @masterFiles ||= mode == :rw ? @media_object.indexed_master_files.to_a : master_file_presenters
+  def load_sections(mode = :rw)
+    @masterFiles ||= mode == :rw ? @media_object.sections : master_file_presenters
   end
 
   def set_player_token
@@ -554,13 +557,13 @@ def load_player_context
 
     if params[:part]
       index = params[:part].to_i-1
-      if index < 0 or index > @media_object.master_files.size-1
+      if index < 0 or index > @media_object.sections.size - 1
         raise ActiveFedora::ObjectNotFoundError
       end
-      params[:content] = @media_object.indexed_master_file_ids[index]
+      params[:content] = @media_object.section_ids[index]
     end
 
-    load_master_files(mode: :ro)
+    load_sections(mode: :ro)
     load_current_stream
   end
 
@@ -583,7 +586,7 @@ def set_active_file
       end
     end
     if @currentStream.nil?
-      @currentStream = @media_object.indexed_master_files.first
+      @currentStream = @media_object.sections.first
     end
     return @currentStream
   end

app/helpers/media_objects_helper.rb

@@ -195,10 +195,10 @@ def get_duration_from_fragment(start, stop)
         milliseconds_to_formatted_time((stop.to_i - start.to_i) * 1000, false)
       end
 
-      # This method mirrors the one in the MediaObject model but makes use of the master files passed in which can be SpeedyAF Objects
+      # This method mirrors the one in the MediaObject model but makes use of the sections passed in which can be SpeedyAF Objects
       # This would be good to refactor in the future but speeds things up considerably for now
-      def gather_all_comments(media_object, master_files)
-        media_object.comment.sort + master_files.collect do |mf|
+      def gather_all_comments(media_object, sections)
+        media_object.comment.sort + sections.collect do |mf|
           mf.comment.reject(&:blank?).collect do |c|
             mf.display_title.present? ? "[#{mf.display_title}] #{c}" : c
           end.sort

app/helpers/security_helper.rb

@@ -30,20 +30,20 @@ def secure_streams(stream_info, media_object_id)
   # media_object CDL check only happens once
   # session tokens retrieved in batch then passed into add_stream_url
   # Returns Hash[MasterFile.id, stream_info]
-  def secure_stream_infos(master_files, media_objects)
+  def secure_stream_infos(sections, media_objects)
     stream_info_hash = {}
 
     not_checked_out_hash = {}
-    mo_ids = master_files.collect(&:media_object_id)
+    mo_ids = sections.collect(&:media_object_id)
     mo_ids.each { |mo_id| not_checked_out_hash[mo_id] ||= not_checked_out?(mo_id, media_object: media_objects.find {|mo| mo.id == mo_id}) }
-    not_checked_out_master_files = master_files.select { |mf| not_checked_out_hash[mf.media_object_id] }
-    checked_out_master_files = master_files - not_checked_out_master_files
+    not_checked_out_sections = sections.select { |mf| not_checked_out_hash[mf.media_object_id] }
+    checked_out_sections = sections - not_checked_out_sections
 
-    not_checked_out_master_files.each { |mf| stream_info_hash[mf.id] = mf.stream_details }
+    not_checked_out_sections.each { |mf| stream_info_hash[mf.id] = mf.stream_details }
 
-    stream_tokens = StreamToken.get_session_tokens_for(session: session, targets: checked_out_master_files.map(&:id))
+    stream_tokens = StreamToken.get_session_tokens_for(session: session, targets: checked_out_sections.map(&:id))
     stream_token_hash = stream_tokens.pluck(:target, :token).to_h
-    checked_out_master_files.each { |mf| stream_info_hash[mf.id] = secure_stream_info(mf.stream_details, stream_token_hash[mf.id]) }
+    checked_out_sections.each { |mf| stream_info_hash[mf.id] = secure_stream_info(mf.stream_details, stream_token_hash[mf.id]) }
 
     stream_info_hash
   end

app/javascript/components/MediaObjectRamp.jsx

@@ -40,7 +40,7 @@ const ExpandCollapseArrow = () => {
 
 const Ramp = ({
   urls,
-  master_files_count,
+  sections_count,
   has_structure,
   title,
   share,
@@ -153,7 +153,7 @@ const Ramp = ({
               </React.Fragment>
               )
             : (<React.Fragment>
-              {master_files_count > 0 &&
+              {sections_count > 0 &&
                 <React.Fragment>
                   <MediaPlayer enableFileDownload={false} />
                   <div className="ramp--rails-title">
@@ -217,7 +217,7 @@ const Ramp = ({
                           ref={expandCollapseBtnRef}
                         >
                           <ExpandCollapseArrow />
-                          {isClosed ? ' Expand' : ' Close'} {master_files_count > 1 ? `${master_files_count} Sections` : 'Section'}
+                          {isClosed ? ' Expand' : ' Close'} {sections_count > 1 ? `${sections_count} Sections` : 'Section'}
                         </button>
                       </Col>
                     }
@@ -244,13 +244,13 @@ const Ramp = ({
             )
           }
         </Col>
-        <Col sm={(master_files_count == 0) ? 12 : 4} className="ramp--tabs-panel">
+        <Col sm={(sections_count == 0) ? 12 : 4} className="ramp--tabs-panel">
           {cdl.enabled && <div dangerouslySetInnerHTML={{ __html: cdl.destroy }} />}
           <Tabs>
             <Tab eventKey="details" title="Details">
               <MetadataDisplay showHeading={false} displayTitle={false} />
             </Tab>
-            {(cdl.can_stream && master_files_count != 0 && has_transcripts) &&
+            {(cdl.can_stream && sections_count != 0 && has_transcripts) &&
               <Tab eventKey="transcripts" title="Transcripts" className="ramp--transcripts_tab">
                 <Transcript
                   playerID="iiif-media-player"

app/models/avalon/rdf_vocab.rb

@@ -55,6 +55,7 @@ class MediaObject < RDF::StrictVocabulary("http://avalonmediasystem.org/rdf/voca
       property :avalon_publisher,     "rdfs:isDefinedBy" => %(avr-media_object:).freeze, type: "rdfs:Class".freeze
       property :supplementalFiles,    "rdfs:isDefinedBy" => %(avr-media_object:).freeze, type: "rdfs:Class".freeze
       property :avalon_uploader,      "rdfs:isDefinedBy" => %(avr-media_object:).freeze, type: "rdfs:Class".freeze
+      property :section_list,         "rdfs:isDefinedBy" => %(avr-media_object:).freeze, type: "rdfs:Class".freeze
     end
 
     class Collection < RDF::StrictVocabulary("http://avalonmediasystem.org/rdf/vocab/collection#")

app/models/batch_entries.rb

@@ -73,13 +73,13 @@ def encoding_status
     return (@encoding_status = :error) unless media_object
 
     # TODO: match file_locations strings with those in MasterFiles?
-    if media_object.master_files.to_a.count != files.count
+    if media_object.sections.count != files.count
       return (@encoding_status = :error)
     end
 
     # Only return success if all MasterFiles have status 'COMPLETED'
     status = :success
-    media_object.master_files.each do |master_file|
+    media_object.sections.each do |master_file|
       next if master_file.status_code == 'COMPLETED'
       # TODO: explore border cases
       if master_file.status_code == 'FAILED' || master_file.status_code == 'CANCELLED'

app/models/concerns/master_file_behavior.rb

@@ -108,8 +108,7 @@ def display_title
                  structuralMetadata.section_title
                elsif title.present?
                  title
-               # FIXME: The test for media_object.master_file_ids.size is expensive and takes ~0.25 seconds
-               elsif file_location.present? && (media_object.master_file_ids.size > 1)
+               elsif file_location.present? && (media_object.section_ids.size > 1)
                  file_location.split("/").last
                end
     mf_title.blank? ? nil : mf_title

app/models/concerns/media_object_behavior.rb

@@ -14,6 +14,7 @@
 
 # This module contains methods which transform stored values for use either on the MediaObject or the SpeedyAF presenter
 module MediaObjectBehavior
+
   def as_json(options={})
     {
       id: id,
@@ -58,11 +59,11 @@ def access_text
   end
 
   def has_captions
-    master_files.any? { |mf| mf.has_captions? }
+    sections.any? { |mf| mf.has_captions? }
   end
 
   def has_transcripts
-    master_files.any? { |mf| mf.has_transcripts? }
+    sections.any? { |mf| mf.has_transcripts? }
   end
 
   # CDL methods

app/models/concerns/media_object_intercom.rb

@@ -15,7 +15,7 @@
 module MediaObjectIntercom
   def to_ingest_api_hash(include_structure = true, remove_identifiers: false, publish: false)
     {
-      files: ordered_master_files.to_a.collect { |mf| mf.to_ingest_api_hash(include_structure, remove_identifiers: remove_identifiers) },
+      files: sections.collect { |mf| mf.to_ingest_api_hash(include_structure, remove_identifiers: remove_identifiers) },
       fields:
         {
           duration: duration,

app/models/file_upload_step.rb

@@ -36,15 +36,11 @@ def after_step context
   end
 
   def execute context
-    deleted_master_files = update_master_files context
-    context[:notice] = "Several clean up jobs have been sent out. Their statuses can be viewed by your sysadmin at #{ Settings.matterhorn.cleanup_log }" unless deleted_master_files.empty?
+    deleted_sections = update_master_files context
+    context[:notice] = "Several clean up jobs have been sent out. Their statuses can be viewed by your sysadmin at #{ Settings.matterhorn.cleanup_log }" unless deleted_sections.empty?
 
-    # Reloads media_object.master_files, should use .reload when we update hydra-head
     media = MediaObject.find(context[:media_object].id)
-    unless media.master_files.empty?
-      media.save
-      context[:media_object] = media
-    end
+    context[:media_object] = media
 
     context
   end
@@ -56,16 +52,15 @@ def execute context
   # label - Display label in the interface
   # id - Identifier for the masterFile to help with mapping
   def update_master_files(context)
-    media_object = context[:media_object]
     files = context[:master_files] || {}
-    deleted_master_files = []
+    deleted_sections = []
     if not files.blank?
       files.each_pair do |id,master_file|
         selected_master_file = MasterFile.find(id)
 
         if selected_master_file
           if master_file[:remove]
-            deleted_master_files << selected_master_file
+            deleted_sections << selected_master_file
             selected_master_file.destroy
           else
             selected_master_file.title = master_file[:title] unless master_file[:title].nil?
@@ -80,6 +75,6 @@ def update_master_files(context)
         end
       end
     end
-    deleted_master_files
+    deleted_sections
   end
 end

app/models/iiif_manifest_presenter.rb

@@ -20,16 +20,16 @@ class IiifManifestPresenter
   IIIF_ALLOWED_TAGS = ['a', 'b', 'br', 'i', 'img', 'p', 'small', 'span', 'sub', 'sup'].freeze
   IIIF_ALLOWED_ATTRIBUTES = ['href', 'src', 'alt'].freeze
 
-  attr_reader :media_object, :master_files, :lending_enabled
+  attr_reader :media_object, :sections, :lending_enabled
 
   def initialize(media_object:, master_files:, lending_enabled: false)
     @media_object = media_object
-    @master_files = master_files
+    @sections = master_files
     @lending_enabled = lending_enabled
   end
 
   def file_set_presenters
-    master_files
+    sections
   end
 
   def work_presenters
@@ -202,7 +202,7 @@ def iiif_metadata_fields
   end
 
   def thumbnail_url
-    master_file_id = media_object.ordered_master_file_ids.try :first
+    master_file_id = media_object.section_ids.try :first
 
     video_count = media_object.avalon_resource_type.map(&:titleize)&.count { |m| m.start_with?('moving image'.titleize) } || 0
     audio_count = media_object.avalon_resource_type.map(&:titleize)&.count { |m| m.start_with?('sound recording'.titleize) } || 0