Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wings reflections handling #6298

Merged
merged 3 commits into from
Sep 7, 2023
Merged

Wings reflections handling #6298

merged 3 commits into from
Sep 7, 2023

Commits on Sep 6, 2023

  1. 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)
    tamsin johnson committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    4ef57d8 View commit details
    Browse the repository at this point in the history
  2. 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
    tamsin johnson committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    601b857 View commit details
    Browse the repository at this point in the history
  3. satisfy rubocop; string interpolation -> #to_s

    tamsin johnson committed Sep 6, 2023
    Configuration menu
    Copy the full SHA
    a777af1 View commit details
    Browse the repository at this point in the history