samvera / active_fedora

A Rails interface to the Fedora repository, akin to ActiveModel
Other
54 stars 63 forks source link

Update external content handling for Fedora 4.7.0-RC #1156

Closed escowles closed 7 years ago

escowles commented 7 years ago

Fedora 4.6 returned a 200 OK response when doing a HEAD on an external content resource, but [4.7.0-RC-1]((https://github.com/fcrepo4/fcrepo4/releases/tag/fcrepo-4.7.0-RC-1) returns a 307 Temporary Redirect instead.

This change results in two test failures (error messages below), so we should update to handle the 307 response code:

  1) ActiveFedora::File with a sub-resource a binary file streaming the response when there are more than 3 requests because of redirects raises a HTTP redirect too deep Error
     Failure/Error: super

     Ldp::HttpError:
       STATUS: 307 ...
     # ./lib/active_fedora/initializing_connection.rb:22:in `head'
     # ./lib/active_fedora/file.rb:63:in `new_record?'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:24:in `metadata_uri'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:38:in `ldp_source'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:15:in `initialize'
     # ./lib/active_fedora/file.rb:110:in `new'
     # ./lib/active_fedora/file.rb:110:in `metadata'
     # ./lib/active_fedora/file.rb:167:in `block in create_or_update'
     # ./lib/active_fedora/file.rb:166:in `tap'
     # ./lib/active_fedora/file.rb:166:in `create_or_update'
     # ./lib/active_fedora/persistence.rb:29:in `save'
     # ./lib/active_fedora/persistence.rb:237:in `block in save_contained_resources'
     # ./lib/active_fedora/persistence.rb:236:in `each'
     # ./lib/active_fedora/persistence.rb:236:in `save_contained_resources'
     # ./lib/active_fedora/persistence.rb:176:in `_update_record'
     # ./lib/active_fedora/indexing.rb:70:in `_update_record'
     # ./lib/active_fedora/callbacks.rb:244:in `block in _update_record'
     # ./lib/active_fedora/callbacks.rb:244:in `_update_record'
     # ./lib/active_fedora/persistence.rb:159:in `create_or_update'
     # ./lib/active_fedora/callbacks.rb:236:in `block in create_or_update'
     # ./lib/active_fedora/callbacks.rb:236:in `create_or_update'
     # ./lib/active_fedora/persistence.rb:33:in `save!'
     # ./lib/active_fedora/validations.rb:56:in `save!'
     # ./spec/integration/file_spec.rb:179:in `block (6 levels) in <top (required)>'

  2) ActiveFedora::File with a sub-resource a binary file streaming the response when the request results in a redirect 
     Failure/Error: super

     Ldp::HttpError:
       STATUS: 307 ...
     # ./lib/active_fedora/initializing_connection.rb:22:in `head'
     # ./lib/active_fedora/file.rb:63:in `new_record?'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:24:in `metadata_uri'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:38:in `ldp_source'
     # ./lib/active_fedora/with_metadata/metadata_node.rb:15:in `initialize'
     # ./lib/active_fedora/file.rb:110:in `new'
     # ./lib/active_fedora/file.rb:110:in `metadata'
     # ./lib/active_fedora/file.rb:167:in `block in create_or_update'
     # ./lib/active_fedora/file.rb:166:in `tap'
     # ./lib/active_fedora/file.rb:166:in `create_or_update'
     # ./lib/active_fedora/persistence.rb:29:in `save'
     # ./lib/active_fedora/persistence.rb:237:in `block in save_contained_resources'
     # ./lib/active_fedora/persistence.rb:236:in `each'
     # ./lib/active_fedora/persistence.rb:236:in `save_contained_resources'
     # ./lib/active_fedora/persistence.rb:176:in `_update_record'
     # ./lib/active_fedora/indexing.rb:70:in `_update_record'
     # ./lib/active_fedora/callbacks.rb:244:in `block in _update_record'
     # ./lib/active_fedora/callbacks.rb:244:in `_update_record'
     # ./lib/active_fedora/persistence.rb:159:in `create_or_update'
     # ./lib/active_fedora/callbacks.rb:236:in `block in create_or_update'
     # ./lib/active_fedora/callbacks.rb:236:in `create_or_update'
     # ./lib/active_fedora/persistence.rb:33:in `save!'
     # ./lib/active_fedora/validations.rb:56:in `save!'
     # ./spec/integration/file_spec.rb:168:in `block (6 levels) in <top (required)>'
escowles commented 7 years ago

It may be best to update the ldp gem to accept 307 Temporary Redirect as successful for a head request: https://github.com/projecthydra/ldp/blob/master/lib/ldp/client/methods.rb#L116

escowles commented 7 years ago

Just updating ldp to accept 307 as successful fixes the AF errors: https://github.com/projecthydra/ldp/compare/master...escowles:307-redirect

Though I know I'm missing something because that method doesn't take redirects into account at all, and I don't see how Faraday is either (is it treating 307 redirects differently from 302/303?)

Thoughts @jcoyne @cbeer?