-
Notifications
You must be signed in to change notification settings - Fork 124
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
no-reply
force-pushed
the
wings-reflections-handling
branch
from
September 6, 2023 22:49
4bef859
to
46e8233
Compare
orangewolf
previously approved these changes
Sep 6, 2023
no-reply
force-pushed
the
wings-reflections-handling
branch
from
September 6, 2023 23:10
46e8233
to
7db9cfb
Compare
no-reply
commented
Sep 6, 2023
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)
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
no-reply
force-pushed
the
wings-reflections-handling
branch
from
September 6, 2023 23:11
ae94b9a
to
a777af1
Compare
orangewolf
approved these changes
Sep 7, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me and solves the original issue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Work to address #6171
Summary
This work contains two commits, both improving the handling of
Reflections
in ActiveFedora when translating toValkyrie
.Guidance for testing, such as acceptance criteria or new user interface behaviors:
Detailed Description
introduce more targeted handling for
ActiveFedora::Reflection
behavior inWings
.this involved a review of the
AssociationReflection
implementations inActiveFedora
to determine what the appropriate handling for each reflectiontype should be.
HasManyReflection
provides access to an inversebelongs_to
association;e.g.
members
onAdminSet
viaadmin_set
as defined byHyrax::InAdminSet
. it's not necessary to transform or persist this data, whichis 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
andHasAndBelongsToManyReflection
: these reflectionsprovide model accessor methods in terms of a
foreign_key
refelection;e.g.
:original_file
built fromoriginal_file_id
. we only need to translatethe id reference given by
SingularRDFPropertyReflection
andActiveFedora::Reflection::RDFPropertyReflection
, respectively. it's notnecessary to transform or persist the expanded data.
FilterReflection
is excluded because it provides access to data canonicallystored via other associations, via
:extending_from
.ActiveFedora::Reflection::OrdersReflection
currently relies on the underlyingunordered_association
for its canonical representation, with special handlingin
ModelTransformer
to ensuremember_ids
is provided as ordered. this couldprobably be refactored to generalize the handling for other
OrdersReflection
instances in
AttributeTransformer
or somewhere similar.handling for
BasicContainsReflection
andHasSubresourceReflection
had beenskipped 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 previousimplementation, 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_000member objects benchmarks as follows:
we had previously manually populated valkyrie Resource
#member_ids
fromActiveFedora's
#ordered_member_ids
when it was present. the implementation hadfour problems:
#member_ids
on objects for which it was defined as aHasManyReflection
, causing the tranformation to load all the members(!!);#member_ids
if any#ordered_member_ids
were present. in Hydra::Works, it's possible to have both.
OrdersReflections
other than#ordered_member_ids
;#member_ids
, sinceAttributeTransformer
alreadyhandled the unordered mapping via
IndirectlyContainsReflection
.this should solve all three problems by acting on
OrdersReflection
as well thevarious
*ContainsReflection
instances. if anOrdersReflection
is present theresulting ids will contain the relevant values in order, either before
or after the unordered values (which may, most often, be empty).
after this change:
@samvera/hyrax-code-reviewers