Create `FileSetDescription` to find characterization for FileSets #4215

no-reply · 2020-01-25T01:34:09Z

For discussion as possible input for #4213

Valkyrie-based FileSet objects don't have direct access to characterization
information. In ActiveFedora, we used a #characterization_proxy to provide
that information. Here, we provide a small service that can find the "primary"
file (FileMetadata) for a given FileSet, and ask it about its description.

Closes #4226

@samvera/hyrax-code-reviewers

app/services/hyrax/characterization/file_set_description.rb

no-reply

Holding this pending rebase on #4225

Use the test adapter in specs by doing: `context 'some context', valkyrie_adapter: :test_adapter do`

Valkyrie-based FileSet objects don't have direct access to characterization information. In ActiveFedora, we used a `#characterization_proxy` to provide that information. Here, we provide a small service that can find the "primary" file (`FileMetadata`) for a given FileSet, and ask it about its description.

The concept of "use" is more properly localized to `FileMetadata` than `FileSet`, since `FileMetadata` holds the actual use (`#type`) attribute.

Rather than using some branching logic in `FileMetadata.for`, take advantage of Valkyrie attributes' default values. When initializing a `FileMetadata`, assume it's an `ORIGINAL_FILE` unless a different use is passed.

no-reply · 2020-01-28T20:01:01Z

app/actors/hyrax/actors/file_actor.rb

-          return :thumbnail_file if relation.to_s.casecmp(Hyrax::FileSet::THUMBNAIL_USE.to_s)
-          :original_file
+          case relation
+          when Hyrax::FileMetadata::Use::ORIGINAL_FILE


Suggested change

when Hyrax::FileMetadata::Use::ORIGINAL_FILE

no-reply · 2020-01-28T20:01:13Z

app/actors/hyrax/actors/file_actor.rb

-          :original_file
+          case relation
+          when Hyrax::FileMetadata::Use::ORIGINAL_FILE
+            :original_file


Suggested change

:original_file

no-reply · 2020-01-28T20:02:19Z

app/actors/hyrax/actors/file_actor.rb

-          return :thumbnail_file if relation.to_s.casecmp(Hyrax::FileSet::THUMBNAIL_USE.to_s)
-          :original_file
+          case relation
+          when Hyrax::FileMetadata::Use::ORIGINAL_FILE


as noted by suggestions, this branch could be dropped and fall through to the else instead. i think the existing way is more clear, but only slightly.

lib/wings/hydra/works/services/add_file_to_file_set.rb

This query was added, but never registered. It needs to be added to the `.queries` method for the query handler in order to be picked up by most adapters.

This method is supplanted by a new custom query. Use that query instead whereever `used_for?` was called.

Since `#type` is located in `FileMetadata` and there's a convenient custom query, these methods mostly function to provide `.first`. My feeling is that this is a bit obfuscating; why not encourage queriers to handle the edge case where there are multiple matches in context appropriate ways instead? In some cases that might be `.first`, but not always. This also has the benefit of removing some business logic from the model.

elrayle

I have a few questions and notes. But the part I'm not following is how FileSetDescription is going to be used. It is defined in this PR, but I don't see it used anywhere. So it is hard to have the context for why it is needed.

app/services/hyrax/characterization/file_set_description.rb

lib/wings/hydra/works/services/add_file_to_file_set.rb

app/models/hyrax/file_metadata.rb

elrayle · 2020-01-28T21:43:13Z

lib/wings/services/file_metadata_builder.rb

+      def current_original_file(file_set)
+        Hyrax.query_service
+          .custom_queries
+          .find_many_file_metadata_by_use(resource: file_set, use: Hyrax::FileMetadata::Use::ORIGINAL_FILE)


Part of what I liked about the #original_file method was that it was simple and had the knowledge that there should be only one, so it was ok to grab the first one returned. I'd hate to see this code block repeated in multiple places. If it doesn't belong in FileSet (for single responsibility reasons), is there somewhere else it can live (to keep it DRY)? And if not, should it be a service. Getting the original file is something that happens a lot in Hyrax.

~~I think the idea of a service is a good one. Maybe there should be a navigator?~~
There already is one. I'll look to make sure it works well and refactor around it.

can we hold this for a later refactor? i promise to get to it this week, if so.

I am good with putting this in another PR. Created an issue to track this... #4233

app/services/hyrax/custom_queries/find_file_metadata.rb

For valkyrie normalization, take advantage for the new `Hyrax::FileMetadata::Use.uri_for` module method to avoid reproducing logic. When the argument is already a URI, just return it to avoid casting unnecessarily. In the ActiveFedora normalizer, `RDF::URI` already implements equality appropriately, so `#casecmp` isn't needed and we can flatten to a simple switch statement.

The constant previously used an incorrect URI. `Thumbnail` should be `ThumbnailImage`.

Avoid typing `Hyrax.query_service.custom_queries` everywhere. Provide a method on the Hyrax level instead.

app/controllers/concerns/hyrax/embargoes_controller_behavior.rb

elrayle · 2020-01-29T13:15:31Z

app/services/hyrax/characterization/file_set_description.rb

+        self.file_set = file_set
+
+        @primary_file_type_uri =
+          Hyrax::FileMetadata::Use.uri_for(use: primary_file)


In other places, the file type is sometimes passed as a symbol (e.g. :original_file, :extracted_file, :thumbnail_image), and sometimes passed as a URI for custom types. Do you think that might happen here too? FileActor which calls Hydra::Works::AddFileToFileSet handles both symbol and URI.

@no-reply This is an open question for me.

i'm not sure i understand the question. this is a new interface, so callers will presumably use it as defined.

Custom types do not have a Symbol equivalent. So what happens here if someone wants to pass http://pcdm.org/use#remastered?

Based on recommendations in Issue #4228 addressing inconsistencies, I would suggest that this new interface accept RDF::URI for primary_file. And I would recommend renaming primary_file to primary_use to be consistent with the naming recommendations in #4228.

elrayle

I'm good with this PR. I only have one question remaining about whether file type may be either a symbol or a URI. I lean toward moving away from using symbols to only using URIs. Use of symbols was an artifact of AF using them to establish the directly_contains_one relationship from the FileSet to the File.

elrayle · 2020-01-29T17:52:23Z

app/actors/hyrax/actors/file_actor.rb

+          when Hyrax::FileMetadata::Use::EXTRACTED_TEXT
+            :extracted_file
+          when Hyrax::FileMetadata::Use::THUMBNAIL
+            :thumbnail_file


As I was exploring inconsistencies documented in Issue #4228, I noticed that these are using symbols that I think are incorrect. I'm pretty sure these should be :extracted_text and :thumbnail.

This part of why I propose putting any conversions between symbol and RDF::URI in a single place (e.g. FileMetadata::Use module).

i also noticed that the symbols were inconsistent across modules. i'm not sure that means they are incorrect(?). these are the symbols that pre-date this PR, right?

unless they got mixed up somewhere else recently, they should be the ones callers are already passing in.

in any case, i do think we should hold changes to behavior around these symbols for resolution as part of #4228. for now, the behavior exists in two places because it behaves slightly differently across them, and the goal here is to preserve that faithfully.

It looks like these were incorrectly entered in PR #3746 which was one of the first PRs for valkyrizing files. I don't think they have surfaced as a problem because Hyrax generally already passes relation as a Symbol unless it is a custom relation, so the conversion probably has never been used in a Hyrax app.

Looking back on PR #3746 for this part handling the conversion from URI to Symbol and vice versa, I am somewhat uncomfortable with the default being original file. I'm wondering what happens if a custom file type is passed in for relation.

I'm can update #4228 with this question or create a separate issue. It seems a bit beyond the refactoring for consistency issue.

re: :original_file i think this is just a straight refactor, and i'd prefer not to let its scope creep.

i'm in favor of wrapping any remaining issues into #4228.

Done... #4228 is updated.

elrayle

This is approved with the acknowledgement that there are several new issues with followup work.

jeremyf · 2020-01-31T00:19:24Z

👍

no-reply mentioned this pull request Jan 25, 2020

Extract characterization service to allow ActiveFedora and Valkyrie filesets #4213

Closed

no-reply force-pushed the valk-char branch 3 times, most recently from d0e2ef7 to 49b4584 Compare January 25, 2020 02:06

jeremyf reviewed Jan 27, 2020

View reviewed changes

app/services/hyrax/characterization/file_set_description.rb Show resolved Hide resolved

jeremyf previously approved these changes Jan 27, 2020

View reviewed changes

elrayle mentioned this pull request Jan 27, 2020

handle conversion of custom use types for files #4225

Merged

no-reply commented Jan 28, 2020

View reviewed changes

Tom Johnson added 2 commits January 28, 2020 08:48

Setup RSpec metadata for selecting a Valkyrie adapter

eb5004b

Use the test adapter in specs by doing: `context 'some context', valkyrie_adapter: :test_adapter do`

no-reply dismissed jeremyf’s stale review via f93029a January 28, 2020 16:50

no-reply force-pushed the valk-char branch from 49b4584 to f93029a Compare January 28, 2020 16:50

Tom Johnson added 2 commits January 28, 2020 10:06

Move PCDM Use URI constants to FileMetadata::Use

6edec00

The concept of "use" is more properly localized to `FileMetadata` than `FileSet`, since `FileMetadata` holds the actual use (`#type`) attribute.

Set a default FileMetadata#type

9989e38

Rather than using some branching logic in `FileMetadata.for`, take advantage of Valkyrie attributes' default values. When initializing a `FileMetadata`, assume it's an `ORIGINAL_FILE` unless a different use is passed.

no-reply requested a review from jeremyf January 28, 2020 19:59

no-reply commented Jan 28, 2020

View reviewed changes

lib/wings/hydra/works/services/add_file_to_file_set.rb Outdated Show resolved Hide resolved

Tom Johnson added 2 commits January 28, 2020 12:13

Register the new :find_many_file_metadata_by_use query

a65e59a

This query was added, but never registered. It needs to be added to the `.queries` method for the query handler in order to be picked up by most adapters.

Drop Hyrax::FileMetadata#used_for?

bb79e44

This method is supplanted by a new custom query. Use that query instead whereever `used_for?` was called.

no-reply force-pushed the valk-char branch 6 times, most recently from 387eb3b to e26a38f Compare January 28, 2020 21:39

no-reply force-pushed the valk-char branch from e26a38f to 675e7f1 Compare January 28, 2020 21:45

elrayle reviewed Jan 28, 2020

View reviewed changes

no-reply force-pushed the valk-char branch 3 times, most recently from 7be1297 to e185d58 Compare January 28, 2020 22:45

Tom Johnson added 3 commits January 28, 2020 15:49

Correct the ThumbnailImage URI in the PCDM Use vocab

98298fe

The constant previously used an incorrect URI. `Thumbnail` should be `ThumbnailImage`.

Introduce Hyrax.custom_queries

a749f23

Avoid typing `Hyrax.query_service.custom_queries` everywhere. Provide a method on the Hyrax level instead.

no-reply force-pushed the valk-char branch from e185d58 to a749f23 Compare January 28, 2020 23:53

elrayle reviewed Jan 29, 2020

View reviewed changes

app/controllers/concerns/hyrax/embargoes_controller_behavior.rb Show resolved Hide resolved

elrayle reviewed Jan 29, 2020

View reviewed changes

elrayle mentioned this pull request Jan 29, 2020

inconsistency in relation/use/type #4228

Open

elrayle reviewed Jan 29, 2020

View reviewed changes

elrayle mentioned this pull request Jan 30, 2020

Replacement for file_set.original_file #4233

Closed

elrayle approved these changes Jan 30, 2020

View reviewed changes

elrayle merged commit 1d5c2f2 into master Jan 30, 2020

elrayle deleted the valk-char branch January 30, 2020 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `FileSetDescription` to find characterization for FileSets #4215

Create `FileSetDescription` to find characterization for FileSets #4215

no-reply commented Jan 25, 2020 •

edited

Loading

no-reply left a comment

no-reply Jan 28, 2020

no-reply Jan 28, 2020

no-reply Jan 28, 2020

elrayle left a comment

elrayle Jan 28, 2020

no-reply Jan 28, 2020

no-reply Jan 29, 2020

elrayle Jan 30, 2020

elrayle Jan 29, 2020

elrayle Jan 30, 2020

no-reply Jan 30, 2020

elrayle Jan 30, 2020

elrayle Jan 30, 2020

elrayle left a comment

elrayle Jan 29, 2020

no-reply Jan 29, 2020

elrayle Jan 30, 2020 •

edited

Loading

elrayle Jan 30, 2020 •

edited

Loading

no-reply Jan 30, 2020

elrayle Jan 30, 2020

elrayle left a comment

jeremyf commented Jan 31, 2020

Create FileSetDescription to find characterization for FileSets #4215

Create FileSetDescription to find characterization for FileSets #4215

Conversation

no-reply commented Jan 25, 2020 • edited Loading

no-reply left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

elrayle Jan 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elrayle left a comment

Choose a reason for hiding this comment

jeremyf commented Jan 31, 2020

Create `FileSetDescription` to find characterization for FileSets #4215

Create `FileSetDescription` to find characterization for FileSets #4215

no-reply commented Jan 25, 2020 •

edited

Loading

elrayle Jan 30, 2020 •

edited

Loading

elrayle Jan 30, 2020 •

edited

Loading