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: Roundtrip of single value attribute become an array after saving #4556

Closed
elrayle opened this issue Oct 21, 2020 · 5 comments
Closed

Comments

@elrayle
Copy link
Contributor

elrayle commented Oct 21, 2020

Descriptive summary

In a valkyrie resource, saving the resource converts single value attributes from String to Array.

Rationale

The roundtrip conversion from resource to AF object and back to resource should not change the value of an attribute.

Expected behavior

resource = MyResource.new(single_value_attr: "one")
resource. single_value_attr           # "one"
updated_resource = Hyrax.persister.save(resource: resource)
updated_resource. single_value_attr   # "one"

NOTE: single_value_attr is "one" before and after the save

Actual behavior

resource = MyResource.new(single_value_attr: "one")
resource. single_value_attr           # "one"
updated_resource = Hyrax.persister.save(resource: resource)
updated_resource. single_value_attr   # ["one"]

NOTE: single_value_attr is "one" before and an Array ["one"] after the save

Steps to reproduce the behavior

TBD - creating a test to demonstrate the problem

@tpendragon
Copy link
Contributor

@elrayle This happens when single_value_attr is defined as a Valkyrie::Types::String?

@no-reply
Copy link
Member

at first glance, i think this is expected Wings behavior.

ActiveFedora/Fedroa still treats these attributes as multi-valued RDF properties, and Wings doesn't try to cast back to a single value, since that risks silent data loss.

@elrayle
Copy link
Contributor Author

elrayle commented Oct 21, 2020

@tpendragon It happens for all single value types for the reason described by @no-reply.

@elrayle
Copy link
Contributor Author

elrayle commented Oct 21, 2020

How Valkyrie handles this [@tpendragon in Slack]...

It’s just if there’s only one value it picks off the first one in the conversion from Fedora->Resource, and then if it’s defined as a Valkyrie::Set the set type conversion takes care of making it an array again.

Valkyrie’s conversion logic for Fedora is mad complicated, but https://github.com/samvera/valkyrie/blob/master/lib/valkyrie/persistence/fedora/persister/orm_converter.rb#L524-L537 is the hunk of code for this.

Postgres (and everything else) uses this: https://github.com/samvera/valkyrie/blob/master/lib/valkyrie/persistence/shared/json_value_mapper.rb#L107-L130

@elrayle elrayle added this to Ready in Hyrax-Valkyrization Oct 29, 2020
@tpendragon tpendragon moved this from Ready to In Progress in Hyrax-Valkyrization Nov 16, 2020
@tpendragon tpendragon self-assigned this Nov 16, 2020
no-reply pushed a commit that referenced this issue Nov 17, 2020
make TransformerValueMapper/OrmConverter treat single value arrays as singular
value objects by default. change the generated fields for ad-hoc valkyrie models
to be Set.of(Anything), by default. don't override existing fields when
generating valkyrie models.

related to #4556
no-reply pushed a commit that referenced this issue Nov 17, 2020
make TransformerValueMapper/OrmConverter treat single value arrays as singular
value objects by default. change the generated fields for ad-hoc valkyrie models
to be Set.of(Anything), by default. don't override existing fields when
generating valkyrie models.

related to #4556

Co-authored-by: Trey Pendragon <tpendragon@princeton.edu>
no-reply pushed a commit that referenced this issue Nov 17, 2020
make TransformerValueMapper/OrmConverter treat single value arrays as singular
value objects by default. change the generated fields for ad-hoc valkyrie models
to be Set.of(Anything), by default. don't override existing fields when
generating valkyrie models.

related to #4556

Co-authored-by: Trey Pendragon <tpendragon@princeton.edu>
no-reply pushed a commit that referenced this issue Nov 17, 2020
make TransformerValueMapper/OrmConverter treat single value arrays as singular
value objects by default. change the generated fields for ad-hoc valkyrie models
to be Set.of(Anything), by default. don't override existing fields when
generating valkyrie models.

related to #4556

Co-authored-by: Trey Pendragon <tpendragon@princeton.edu>
no-reply pushed a commit that referenced this issue Nov 17, 2020
make TransformerValueMapper/OrmConverter treat single value arrays as singular
value objects by default. change the generated fields for ad-hoc valkyrie models
to be Set.of(Anything), by default. don't override existing fields when
generating valkyrie models.

related to #4556

Co-authored-by: Trey Pendragon <tpendragon@princeton.edu>
@tpendragon tpendragon moved this from In Progress to Ready for Review in Hyrax-Valkyrization Nov 17, 2020
@elrayle
Copy link
Contributor Author

elrayle commented Nov 18, 2020

Closing... Addressed by PR #4607 - wings: handle single value fields for valkyrie native models

@elrayle elrayle closed this as completed Nov 18, 2020
@elrayle elrayle moved this from Ready for Review to DONE in Hyrax-Valkyrization Nov 18, 2020
@elrayle elrayle moved this from DONE to Archive in Hyrax-Valkyrization Apr 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Hyrax-Valkyrization
Ready to be Archived
Development

No branches or pull requests

3 participants