apollo_upload_server-ruby icon indicating copy to clipboard operation
apollo_upload_server-ruby copied to clipboard

Not compatible with ActiveStorage

Open bastengao opened this issue 7 years ago • 28 comments

ActiveStorage accept ActionDispatch::Http::UploadedFile generally, see https://github.com/rails/rails/blob/master/activestorage/lib/active_storage/attached.rb#L18 , but uploaded file type is ApolloUploadServer::Wrappers:: UploadedFile, it is not accepted by ActiveStorage.

bastengao avatar Apr 19 '18 01:04 bastengao

Oh that ActiveStorage... we use shrine for our applications, so I don't know if we add support for ActiveStorage in the nearest future.

fuelen avatar Apr 19 '18 08:04 fuelen

I'm running into this issue too. @bastengao have you found a solution?

Also @fuelen, would you be open to PRs that add ActiveStorage compatibility?

awinograd avatar Apr 29 '18 13:04 awinograd

@awinograd yes, sure.

fuelen avatar Apr 29 '18 14:04 fuelen

@fuelen still trying to wrap my head around the code as I haven't worked with middlewares or ActionDispatch::Http::UploadedFile directly before, but this commit seems be to be working for me.

https://github.com/awinograd/apollo_upload_server-ruby/commit/e2c68cc275617946db46aa80a3a4d8ec7415c379

Looks like it was introduced in https://github.com/awinograd/apollo_upload_server-ruby/commit/0a9e17d387082a9731036f5a782f246209af7d83 to solve a different but

awinograd avatar Apr 29 '18 19:04 awinograd

@awinograd without wrapped_file you have another issue :) When some error with file field happens graphql tries to convert file object to JSON. And we have error exceptions because of binary data in the file.

I think the only way for now to add support is to monkey-patch ActiveStorage::Attached class. Or add an ability to graphql-ruby to serialize errors somehow in a custom way.

fuelen avatar Apr 30 '18 06:04 fuelen

Ahh I see. I knew there must have been a reason but I had trouble figuring out what the history was around that change. I'll likely take some more time to look at this when I'm trying to productionize the system. Then I can take some time to dig into the ActiveStorage internals and see what fix might work.

Thanks for the info!

awinograd avatar Apr 30 '18 18:04 awinograd

After digging through the initializer for ActionDispatch::Http::UploadedFile, I was able to create ActionDispatch::Http::UploadedFiles on the fly. ( ApolloUploadServer::Wrappers::UploadedFile is mostly just ActionDispatch::Http::UploadedFile, so it isn't hard how to pull out the attributes I need to recreate an ActionDispatch::Http::UploadedFile.)

file = args['file'] # ApolloUploadServer::Wrappers::UploadedFile
normal_file = ActionDispatch::Http::UploadedFile.new(
  filename: file.original_filename,
  type: file.content_type,
  head: file.headers,
  tempfile: file.tempfile,
) # ActionDispatch::Http::UploadedFile
record.annotated_images.create!(
  file: normal_file
)

(record.annotated_image has_one_attached :file)

Hope this helps with those who are struggling with ActiveStorage.

Not sure if this the best way to handle it, but it might help to expose a helper method in ApolloUploadServer::Wrappers::UploadedFile to expose the original ActionDispatch::Http::UploadedFile for those who need to use this with ActionDispatch.

liang3404814 avatar May 16 '18 08:05 liang3404814

I also hit another issue with this now, while using ActiveData which has type checks, and ApolloUploadServer::Wrappers::UploadedFile is not the same class that was expected (and not even an Object :) ).

FWIW, it is possible to obtain the unwrapped object via __getobj__ so no new method should be needed, (but might be useful) but I would suggest overriding the inspect/to_s methods to simplify debugging this issue for future people.

riffraff avatar May 22 '18 07:05 riffraff

I used @liang3404814 solution for own scalar to move the logic out of the mutation.

Scalars::FileType = GraphQL::ScalarType.define do
  name 'File'
  description 'action_dispatch_uploaded_file'
  coerce_input lambda { |file, _ctx|
    ActionDispatch::Http::UploadedFile.new(
      filename: file.original_filename,
      type: file.content_type,
      head: file.headers,
      tempfile: file.tempfile,
    )
  }
end

It would be good to support ActionStorage out of the box in the final version. I guess that just the key names are different.

karensg avatar Jun 05 '18 14:06 karensg

this means there is not support for active storage now. right? @fuelen and I didn't get code that you mentioned. do I need to write that code in resolver? above @liang3404814

vimox-shah avatar Jun 06 '18 08:06 vimox-shah

Yes, no support for active storage. For now, you can use custom scalar provided by @karensg

fuelen avatar Jun 06 '18 08:06 fuelen

I have tried that also that did not work. @karensg and whenever we try to access file it is coming as null in variable like variables: {'id': 1, file: null} the how can we access args[:file] @liang3404814

vimox-shah avatar Jun 06 '18 08:06 vimox-shah

@vimox-shah was file really sent?

fuelen avatar Jun 06 '18 09:06 fuelen

Do you have apollo-upload-client installed and configured on frontend?

karensg avatar Jun 07 '18 15:06 karensg

yes I have already integrated apollo-upload-client in front end. I have used your code in at server side for defining file type but in args I did not get file object.

vimox-shah avatar Jun 07 '18 15:06 vimox-shah

I have uploaded sample code here https://stackoverflow.com/questions/50705123/how-can-i-pass-multipart-form-data-and-file-upload-using-graphql-ruby-at-servers can you please check and verify is it right or wrong? @karensg

vimox-shah avatar Jun 07 '18 15:06 vimox-shah

yes file was really sent. can you please share your code how did you manage upload file? @karensg

vimox-shah avatar Jun 07 '18 16:06 vimox-shah

@vimox-shah could you show parameters from logs?

fuelen avatar Jun 07 '18 17:06 fuelen

screen shot 2018-06-07 at 11 31 27 pm yes you can see logs in attached screenshot @fuelen

vimox-shah avatar Jun 07 '18 18:06 vimox-shah

@bastengao besides doing all the basic apollo-upload-client, apollo_upload_server and Active Storage configuration; by attaching the file as IO it works for me:

project.file.attach(io: args[:file].to_io,
                    filename: args[:file].original_filename)

djGrill avatar Jul 23 '18 03:07 djGrill

thanks @djGrill, that worked for me!

With 'graphql', '~> 1.8', '>= 1.8.2' I was able to: <model>.<argument>.attach(io: <argument>, filename: <argument>.original_filename)

or event.image.attach(io: image, filename: image.original_filename)

etwillms avatar Aug 03 '18 01:08 etwillms

I think we can do something like this in Upload class and get rid of ApolloUploadServer::Wrappers::UploadedFile

coerce_input ->(value, _ctx) {
  return if value.nil?

  def value.as_json(options = nil)
     instance_values.except('tempfile').as_json(options)
  end
  value
}

Could anyone test this approach?

fuelen avatar Sep 18 '18 09:09 fuelen

module ApolloUploadServer
  class GraphQLDataBuilder
    def assign_file(field, splited_path, file)
      if field.is_a? Hash
        field.merge!(splited_path.last => file)
      elsif field.is_a? Array
        field[splited_path.last.to_i] = file
      end
    end
  end

  remove_const :Upload
  Upload = GraphQL::ScalarType.define do
    name "Upload"

    coerce_input ->(value, _ctx) { value }
    coerce_result lambda { |value, _ctx|
      return if value.nil?

      def value.as_json(options = nil)
        instance_values.except("tempfile").as_json(options)
      end
      value
    }
  end
end

Putting this monkey patch in config/initializer worked for me, but is there any reason not making ApolloUploadServer::Wrappers::UploadedFile subclass of ActionDispatch::Http::UploadedFile instead of using delegation?

yskkin avatar Oct 29 '18 02:10 yskkin

@yskkin We don't remember that reason.

fuelen avatar Nov 07 '18 19:11 fuelen

Just for sake of completeness: https://github.com/rails/rails/issues/25250

maltoe avatar Dec 07 '18 14:12 maltoe

I make a patch, unwrap ApolloUploadServer::Wrappers::UploadedFile to get ActionDispatch::Http::UploadedFile

ApolloUploadServer::Upload = ApolloUploadServer::Upload.redefine do
  coerce_input ->(value, _ctx) do
    value.is_a?(ApolloUploadServer::Wrappers::UploadedFile) ? value.__getobj__ : value
  end
end

bastengao avatar Jul 14 '19 15:07 bastengao

Oh that ActiveStorage... we use shrine for our applications, so I don't know if we add support for ActiveStorage in the nearest future.

I'm using shrine but still cannot figure out how this gem works.

bravemaster619 avatar Jun 25 '20 12:06 bravemaster619

Bump this because it's still an issue, and one that's particularly annoying to debug since the error message produced is mind-bendingly confusing (the wrapper object being transparent to all logging).

Could not find or build blob: expected attachable, got #<ActionDispatch::Http::UploadedFile:0x00007f954b82b580 @tempfile=#<Tempfile:/var/folders/70/g1f45d555d399x3jqnsrj7yr0000gn/T/RackMultipart20210216-69192-1u8bciw.JPG>, @original_filename="IMG_7421D.JPG", @content_type="image/jpeg", @headers="Content-Disposition: form-data; name=\"1\"; filename=\"IMG_7421D.JPG\"\r\nContent-Type: image/jpeg\r\n">

(Included above for searchability from Google.)

We've worked around this temporarily by using:

argument :image, ApolloUploadServer::Upload, required: false, prepare: -> (upload, _) {
  upload&.__getobj__ # Unwrap value for ActiveStorage
}

This is less than ideal however, since the argument needs to be prepared every time we accept a file.

With ActiveStorage being the out-of-box file management solution for Rails, this issue is probably going to cause a lot of headaches.

mintyfresh avatar Feb 17 '21 15:02 mintyfresh

@mintyfresh Yeah, the error message is quite confusing and took me a while to identify the actual problem.

In our new Rails projects, we've switched from Carrierwave to ActiveStorage. So far, we haven't found a straightforward solution to resolve it in an elegant manner.

sujh avatar May 22 '23 03:05 sujh