[WIP] Add support for external files via RFC2017 mime types #1234

cjcolvar · 2017-04-20T18:05:59Z

This PR takes the pattern used by Fedora and some Hydra implementors (see related links below) and enables it at the ActiveFedora::File level. The use case for Avalon is that we want to store technical metadata and track derivative files that do not live in our repository but somewhere external, possibly a streaming server.

Since the only way to determine if there is external content is to inspect the mime type, a request to fcr:metadata is required to get the file's content. You can see I had to remove the test in spec/integration/attached_files_spec.rb due to this additional request. Certain calls now require requesting fcr:metadata to get the mime type to check if there is external content.

I'm guess there are things I've overlooked or edge cases that aren't handled so please give feedback on improvements!

~~The last failing test is related to #1233 and should disappear if I rebase once that is merged.~~

Related tickets and code:
#1156
samvera/ldp#71
https://github.com/projecthydra/hydra-works/blob/master/lib/hydra/works/services/add_external_file_to_file_set.rb
https://github.com/lbiedinger/ldp/tree/redirects
https://github.com/pulibrary/plum/blob/master/app/jobs/ingest_file_job.rb

escowles

This looks good to me — I've got just a couple of questions.

escowles · 2017-04-20T18:46:47Z

lib/active_fedora/file.rb

      content_will_change! unless @content == string_or_io
      @content = string_or_io
    end

    def content
-      local_or_remote_content(true)
+      external_content? ? external_content : local_or_remote_content(true)


Do we need to explicitly retrieve the external content URL from the description, and then retrieve it? Can't we just follow Fedora's redirect?

escowles · 2017-04-20T18:49:14Z

lib/active_fedora/file.rb

+    def external_url
+      return nil unless mime_type.start_with? "message/external-body"
+      url = mime_type.split(';').at(2)
+      url.nil? ? nil : url[/\"(.*?)\"/, 1]


Is there a HTTP header parser we can use that would be more robust against slight changes to the external-body format? E.g., I know the spaces are optional, and I think you might be able to use single quotes or change the order of the tokens so the URL was in the second position, etc.

I couldn't really find anything to do this parsing for us. Any suggestions?

I thought one of the HTTP clients would have this (and they must internally), but I couldn't find anything. I did find a gem called content-type that will do this: https://rubygems.org/gems/content-type

ContentType.parse("message/external-body; access-type=\"URL\"; URL=\"http://example.org/1\"").parameters['url']

Is it worth pulling in this gem (of 2 commits from 3 years ago) along with its dependency parslet for this? Maybe it would be better to come up with something good enough that we own instead.

Yes, I'm overthinking this — we could probably even just use a regex like:

url.match(/url=['"](.+?)['"]/i).captures.first

awead · 2017-04-26T16:09:06Z

lib/active_fedora/file.rb

+    end
+
+    def external_url=(external_file_url)
+      self.mime_type = "message/external-body; access-type=URL; URL=\"#{external_file_url}\""


@cjcolvar This definitely makes it easy. I'm wondering if we could use another property. Could we make content itself an HTML redirect so that when someone hits the resource it actually redirects to the location? Or does that work with using mime_type?

Fedora currently redirects if the file has a mime_type using message/external-body (RFC2017). This means the content is still stored but is never exposed.

@cjcolvar interesting. so you could still have binary content on content but the override to mime_type would force a redirect?

I'd have to check to make sure, but I think you could store content but you wouldn't be able to retrieve it. But once you changed the mime type to something else, then you could probably retrieve the content again.

That is currently how File and Fedora work. I'm attempting to have .content return the bits for either external or internal. Thus making the File act like it either has external or internal content but never both at the same time. I had started with a separate .external_content method but thought this approach clearer.

@escowles Are you suggesting if I want to create an AF::File that points to a non-resolvable or non-HTTP URL then I might be better off storing that in another field (like ebucore:locator) and setting empty content instead of using message/external-body in the mime_type?

I figured it would be better to use the same approach to store the external reference no matter the protocol or resolvability and mark the AF::File as external content in all of those cases because that is what it is representing.

@cjcolvar I don't know about this. Right now, Fedora sends redirects, but there is a lot of interest in having Fedora proxy content instead (both HTTP and potentially local files too). My main concern is that if we support anything beyond the currently-implemented behavior (which is already in use by several apps), it will complicate implementing whatever gets decided about proxying or supporting files instead of just HTTP. I'm hoping to discuss this with the new Fedora API spec group that's meeting tomorrow.

@escowles Great! Looking forward to the outcome of that then!

@escowles Any updates from the Fedora API spec group meeting?

The current proposal for the API spec is here: https://github.com/fcrepo/fcrepo-specification/wiki/External-Content (this wiki page has been generally agreed upon and will be incorporated into the spec shortly).

The basic pattern is different from the current implementation:

Either url or local-file are supported, and they are to be interpreted by the server.

The server will then proxy the content and the client doesn't need to know anything about the content being external.

The location of the external content will be exposed to the client in a Content-Location header.

Supporting redirects like the current implementation is still undecided — if anyone has a use case for redirects that proxying won't cover, I'd recommend opening an issue in https://github.com/fcrepo/fcrepo-specification/issues

So, I think we should support the current behavior now, but I don't think we need to do anything about local-file or other types right now.

awead · 2017-04-26T16:09:42Z

lib/active_fedora/file.rb

@@ -147,6 +161,7 @@ def to_solr(solr_doc = {}, _opts = {})
    end

    def content=(string_or_io)
+      raise "This file has external content.  First call external_url=nil if you want to use content=." if external_content?


@cjcolvar might be nice to have a external_content_reset method that does the setting to nil for you.

afred · 2017-04-26T18:44:34Z

lib/active_fedora/file.rb

@@ -196,7 +216,7 @@ def uploaded_file?(payload)
      end

      def local_or_remote_content(ensure_fetch = true)


Would it make sense to settle on the word "external" instead of "remote" for this method (and any others that may be applicable)? We could alias :local_or_remote_content, :local_or_external_content, and add a deprecation warning.

My understanding is that remote_content here means the version of the content that lives in fedora as opposed to the local yet to be persisted version of the content. External content is different from those two and thus a new name. It might be appropriate to call this method :local_or_remote_or_external_content now but that seems wordy and confusing.

cjcolvar · 2018-01-24T21:05:55Z

Closing because I'm no longer working on this and might be irrelevant now.

cjcolvar requested review from jcoyne, escowles and afred April 20, 2017 18:05

escowles reviewed Apr 20, 2017

View reviewed changes

cjcolvar force-pushed the external_files branch 2 times, most recently from fcad9a9 to a8f601a Compare April 21, 2017 13:31

cjcolvar mentioned this pull request Apr 21, 2017

Persist active encode results in ActiveFedora::File objects with external content samvera/hydra-derivatives#157

Merged

awead reviewed Apr 26, 2017

View reviewed changes

cjcolvar added 4 commits April 26, 2017 12:52

Add support for external files via RFC2017 mime types

4a6ed78

Follow fedora's 307 redirect

3348ee8

Restore test

8ad1e33

More rebust parsing of content-type header

966ee74

cjcolvar force-pushed the external_files branch from a8f601a to 966ee74 Compare April 26, 2017 16:53

afred reviewed Apr 26, 2017

View reviewed changes

cjcolvar mentioned this pull request Apr 28, 2017

ActiveFedora External File PR avalonmediasystem/avalon#1859

Closed

4 tasks

awead and others added 2 commits May 4, 2017 12:56

Merge branch 'master' into external_files

24317a2

Merge branch 'master' into external_files

1f3a0ad

cjcolvar closed this Jan 24, 2018

awead mentioned this pull request May 17, 2018

Move Binary Data outside of Fedora psu-libraries/scholarsphere-3#1128

Closed

tpendragon deleted the external_files branch May 8, 2019 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add support for external files via RFC2017 mime types #1234

[WIP] Add support for external files via RFC2017 mime types #1234

cjcolvar commented Apr 20, 2017 •

edited

Loading

escowles left a comment

escowles Apr 20, 2017

escowles Apr 20, 2017

cjcolvar Apr 20, 2017

escowles Apr 20, 2017

cjcolvar Apr 21, 2017

escowles Apr 21, 2017

awead Apr 26, 2017

cjcolvar Apr 26, 2017

awead Apr 26, 2017

escowles Apr 26, 2017

cjcolvar Apr 26, 2017

cjcolvar May 2, 2017

escowles May 2, 2017

cjcolvar May 2, 2017

cjcolvar May 10, 2017

escowles May 11, 2017

awead Apr 26, 2017

afred Apr 26, 2017

cjcolvar Apr 26, 2017

cjcolvar commented Jan 24, 2018

		@@ -196,7 +216,7 @@ def uploaded_file?(payload)
		end

		def local_or_remote_content(ensure_fetch = true)

[WIP] Add support for external files via RFC2017 mime types #1234

[WIP] Add support for external files via RFC2017 mime types #1234

Conversation

cjcolvar commented Apr 20, 2017 • edited Loading

escowles 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

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

cjcolvar commented Jan 24, 2018

cjcolvar commented Apr 20, 2017 •

edited

Loading