Wings reflections handling (#6298)

* wings: improve handling for ActiveFedora reflections in Wings

introduce more targeted handling for `ActiveFedora::Reflection` behavior in
`Wings`.

this involved a review of the `AssociationReflection` implementations in
`ActiveFedora` to determine what the appropriate handling for each reflection
type should be.

`HasManyReflection` provides access to an inverse `belongs_to` association;
e.g. `members` on `AdminSet` via `admin_set` as defined by
`Hyrax::InAdminSet`. it's not necessary to transform or persist this data, which
is canonically stored on the object of the inverse relation. __This is the
primary change proposed by this commit__, avoiding the need to serialize
potentially large sets of members that are tracked via an inverse relation__ as
with `AdminSet#members`.

for `BelongsToReflection` and `HasAndBelongsToManyReflection`: these reflections
provide model accessor methods in terms of a `foreign_key` refelection;
e.g. `:original_file` built from `original_file_id`. we only need to translate
the id reference given by `SingularRDFPropertyReflection` and
`ActiveFedora::Reflection::RDFPropertyReflection`, respectively. it's not
necessary to transform or persist the expanded data.

`FilterReflection` is excluded because it provides access to data canonically
stored via other associations, via `:extending_from`.

`ActiveFedora::Reflection::OrdersReflection` currently relies on the underlying
`unordered_association` for its canonical representation, with special handling
in `ModelTransformer` to ensure `member_ids` is provided as ordered. __this could
probably be refactored to generalize the handling__ for other `OrdersReflection`
instances in `AttributeTransformer` or somewhere similar.

handling for `BasicContainsReflection` and `HasSubresourceReflection` had been
skipped previously, but __may need to be better tested in follow up work__.

for the remainder of the reflection types, derive an `attribute_name`
and (`Dry::Types`) `type` for the Resource's attribute. Unlike in the previous
implementation, we crash if we end up with a duplicate
`attribute_name`.
Likewise raise an error if an unexpected Reflection is present. this is the full
list of supported ActiveFedora Reflections. we want to make the adopter aware if
an application has somehow shimmed in their own and it's not a subclass of one
of these.

----

after this change, transforming an `AdminSet` with 1, 10, 100, 1_000, and 10_000
member objects benchmarks as follows:

1        0.084475   0.003275   0.087750 (  0.122035)
10       0.094453   0.002799   0.097252 (  0.138020)
100      0.087165   0.003446   0.090611 (  0.153771)
1_000    0.238141   0.009217   0.247358 (  0.507827)
10_000   1.114510   0.077724   1.192234 (  2.784685)

* wings: use AttributesTransformer to populate ordered reflections

we had previously manually populated valkyrie Resource `#member_ids` from
ActiveFedora's `#ordered_member_ids` when it was present. the implementation had
four problems:

  - it accessed `#member_ids` on objects for which it was defined as a
  `HasManyReflection`, causing the tranformation to load all the members(!!);
  - it failed to retain unordered `#member_ids` if any `#ordered_member_ids`
  were present. in Hydra::Works, it's possible to have both.
  - it ignored `OrdersReflections` other than `#ordered_member_ids`;
  - it repeatedly loaded the `#member_ids`, since `AttributeTransformer` already
  handled the unordered mapping via `IndirectlyContainsReflection`.

this should solve all three problems by acting on `OrdersReflection` as well the
various `*ContainsReflection` instances. if an `OrdersReflection` is present the
resulting ids will contain the relevant values in order, either before
or after the unordered values (which may, most often, be empty).

---

after this change:

1        0.032793   0.006267   0.039060 (  0.047836)
10       0.057116   0.003111   0.060227 (  0.067870)
100      0.044683   0.003256   0.047939 (  0.057653)
1_000    0.102635   0.000026   0.102661 (  0.120755)
10_000

* satisfy rubocop; string interpolation -> #to_s

lib/wings/attribute_transformer.rb

@@ -15,29 +15,36 @@ def self.for(obj)
       end
     end
 
-    def self.run(obj) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength
-      attrs = obj.reflections.each_with_object({}) do |(key, reflection), mem|
+    def self.run(obj) # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity
+      attrs = obj.reflections.each_with_object({}) do |(key, reflection), mem| # rubocop:disable Metrics/BlockLength
         case reflection
-        when ActiveFedora::Reflection::HasManyReflection,
-             ActiveFedora::Reflection::HasAndBelongsToManyReflection,
-             ActiveFedora::Reflection::IndirectlyContainsReflection
-          mem[:"#{key.to_s.singularize}_ids"] =
-            obj.association(key).ids_reader
-        when ActiveFedora::Reflection::DirectlyContainsReflection
-          mem[:"#{key.to_s.singularize}_ids"] =
-            Array(obj.public_send(reflection.name)).map(&:id)
-        when ActiveFedora::Reflection::FilterReflection,
-             ActiveFedora::Reflection::OrdersReflection,
-             ActiveFedora::Reflection::HasSubresourceReflection,
-             ActiveFedora::Reflection::BelongsToReflection,
-             ActiveFedora::Reflection::BasicContainsReflection
+        when ActiveFedora::Reflection::BelongsToReflection, # uses foreign_key SingularRDFPropertyReflection
+             ActiveFedora::Reflection::BasicContainsReflection, # ???
+             ActiveFedora::Reflection::FilterReflection, # rely on :extending_from
+             ActiveFedora::Reflection::HasAndBelongsToManyReflection, # uses foreign_key RDFPropertyReflection
+             ActiveFedora::Reflection::HasManyReflection, # captured by inverse relation
+             ActiveFedora::Reflection::HasSubresourceReflection, # ???
           :noop
+        when ActiveFedora::Reflection::OrdersReflection
+          mem[:"#{reflection.options[:unordered_reflection].name.to_s.singularize}_ids"] ||= []
+          mem[:"#{reflection.options[:unordered_reflection].name.to_s.singularize}_ids"] +=
+            obj.association(reflection.name).target_ids_reader
         when ActiveFedora::Reflection::DirectlyContainsOneReflection
           mem[:"#{key.to_s.singularize}_id"] =
             obj.public_send(reflection.name)&.id
+        when ActiveFedora::Reflection::IndirectlyContainsReflection
+          mem[:"#{key.to_s.singularize}_ids"] ||= []
+          mem[:"#{key.to_s.singularize}_ids"] +=
+            obj.association(key).ids_reader
+        when ActiveFedora::Reflection::DirectlyContainsReflection
+          mem[:"#{key.to_s.singularize}_ids"] ||= []
+          mem[:"#{key.to_s.singularize}_ids"] +=
+            Array(obj.public_send(reflection.name)).map(&:id)
+        when ActiveFedora::Reflection::RDFPropertyReflection
+          mem[reflection.name.to_sym] =
+            obj.public_send(reflection.name.to_sym)
         else
-          mem[reflection.foreign_key.to_sym] =
-            obj.public_send(reflection.foreign_key.to_sym)
+          raise NotImplementedError, "Expected a known ActiveFedora::Reflection, but got #{reflection}"
         end
       end
 

lib/wings/model_transformer.rb

@@ -150,17 +150,9 @@ def additional_attributes
       { :id => pcdm_object.id,
         :created_at => pcdm_object.try(:create_date),
         :updated_at => pcdm_object.try(:modified_date),
-        :member_ids => member_ids,
         ::Valkyrie::Persistence::Attributes::OPTIMISTIC_LOCK => lock_token }
     end
 
-    # Prefer ordered members, but if ordered members don't exist, use non-ordered members.
-    def member_ids
-      ordered_member_ids = pcdm_object.try(:ordered_member_ids)
-      return ordered_member_ids if ordered_member_ids.present?
-      pcdm_object.try(:member_ids)
-    end
-
     def lock_token
       result = []
       result << ::Valkyrie::Persistence::OptimisticLockToken.new(adapter_id: 'wings-fedora-etag', token: pcdm_object.etag) unless pcdm_object.new_record?

lib/wings/orm_converter.rb

@@ -78,29 +78,34 @@ def to_model
         end
 
         # add reflection associations
-        ldp_reflections = (klass.try(:reflections) || []).select do |_key, reflection|
+        (klass.try(:reflections) || []).each do |reflection_key, reflection|
           case reflection
-          when ActiveFedora::Reflection::FilterReflection,
-               ActiveFedora::Reflection::OrdersReflection,
-               ActiveFedora::Reflection::BasicContainsReflection,
-               ActiveFedora::Reflection::HasSubresourceReflection
-            false
-          else
-            true
-          end
-        end
-
-        ldp_reflections.each do |reflection_key, reflection|
-          if reflection.collection?
-            type           = ::Valkyrie::Types::Set.of(::Valkyrie::Types::ID)
+          when ActiveFedora::Reflection::BelongsToReflection, # uses foreign_key SingularRDFPropertyReflection
+               ActiveFedora::Reflection::BasicContainsReflection, # ???
+               ActiveFedora::Reflection::FilterReflection, # rely on :extending_from
+               ActiveFedora::Reflection::HasAndBelongsToManyReflection, # uses foreign_key RDFPropertyReflection
+               ActiveFedora::Reflection::HasManyReflection, # captured by inverse relation
+               ActiveFedora::Reflection::HasSubresourceReflection, # ???
+               ActiveFedora::Reflection::OrdersReflection # map to :unordered_association in Wings::AttributeTransformer (but retain order)
+            next
+          when ActiveFedora::Reflection::DirectlyContainsOneReflection
+            attribute_name = (reflection_key.to_s.singularize + '_id').to_sym
+            type = ::Valkyrie::Types::ID.optional
+          when ActiveFedora::Reflection::DirectlyContainsReflection,
+               ActiveFedora::Reflection::IndirectlyContainsReflection
             attribute_name = (reflection_key.to_s.singularize + '_ids').to_sym
+            type = ::Valkyrie::Types::Set.of(::Valkyrie::Types::ID)
+          when ActiveFedora::Reflection::SingularRDFPropertyReflection
+            attribute_name = reflection.name.to_sym
+            type = ::Valkyrie::Types::ID.optional
+          when ActiveFedora::Reflection::RDFPropertyReflection
+            attribute_name = reflection.name.to_sym
+            type = ::Valkyrie::Types::Set.of(::Valkyrie::Types::ID)
           else
-            type           = ::Valkyrie::Types::ID.optional
-            attribute_name = (reflection_key.to_s.singularize + '_id').to_sym
+            raise NotImplementedError, "Expected a known ActiveFedora::Reflection, but got #{reflection}"
           end
 
-          next if fields.include?(attribute_name)
-          attribute attribute_name, type
+          attribute(attribute_name, type)
         end
 
         def internal_resource

spec/wings/model_transformer_spec.rb

@@ -219,6 +219,13 @@ class Book < Hyrax::Resource
           work.members << child_work2
         end
 
+        it 'includes ordered_members too' do
+          ordered_members = [FactoryBot.create(:work), FactoryBot.create(:work)]
+          ordered_members.each { |w| work.ordered_members << w }
+
+          expect(factory.build.member_ids).to include(*['cw1', 'cw2'] + ordered_members.map(&:id))
+        end
+
         it 'sets member_ids to the ids of the unordered members' do
           expect(subject.build.member_ids).to match_valkyrie_ids_with_active_fedora_ids(['cw1', 'cw2'])
         end
@@ -286,7 +293,7 @@ class Book < Hyrax::Resource
 
     let(:page_class) do
       ActiveFedoraPage = Class.new(ActiveFedora::Base) do
-        belongs_to :active_fedora_book_with_active_fedora_pages, predicate: ActiveFedora::RDF::Fcrepo::RelsExt.isPartOf
+        belongs_to :active_fedora_monograph, predicate: ActiveFedora::RDF::Fcrepo::RelsExt.isPartOf
       end
     end
 
@@ -324,7 +331,19 @@ class Book < Hyrax::Resource
           .to have_attributes title: book.title,
                               contributor: book.contributor,
                               description: book.description
-        expect(subject.build.active_fedora_page_ids).to eq(['pg1', 'pg2'])
+      end
+
+      it "skips has_many reflections" do
+        expect(factory.build).not_to respond_to :active_fedora_page_ids
+      end
+
+      it "populates belongs_to reflections" do
+        monograph = book_class.create(**attributes)
+
+        expect(described_class.new(pcdm_object: page1).build)
+          .to have_attributes active_fedora_monograph_id: monograph.id
+        expect(described_class.new(pcdm_object: page2).build)
+          .to have_attributes active_fedora_monograph_id: monograph.id
       end
     end
   end
@@ -364,6 +383,20 @@ class Book < Hyrax::Resource
     end
   end
 
+  context 'with an admin set' do
+    let(:pcdm_object) { AdminSet.create(title: ['my admin set']) }
+
+    before do
+      5.times do |i|
+        GenericWork.create(title: [i.to_s], admin_set_id: pcdm_object.id)
+      end
+    end
+
+    it 'does not have member_ids' do
+      expect(factory.build).not_to respond_to :member_ids
+    end
+  end
+
   context 'build for access control' do
     let(:af_object) { ActiveFedora::Base.create }
     let(:permission) { { name: "[email protected]", access: :edit.to_s, type: :person } }