jetruby / apollo_upload_server-ruby

MIT License
180 stars 50 forks source link

Not compatible with ActiveStorage #10

Open bastengao opened 6 years ago

bastengao commented 6 years ago

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.

fuelen commented 6 years ago

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

awinograd commented 6 years ago

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?

fuelen commented 6 years ago

@awinograd yes, sure.

awinograd commented 6 years ago

@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

fuelen commented 6 years ago

@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.

awinograd commented 6 years ago

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!

liang3404814 commented 6 years ago

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.

riffraff commented 6 years ago

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.

karensg commented 6 years ago

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.

vimox-shah commented 6 years ago

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

fuelen commented 6 years ago

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

vimox-shah commented 6 years ago

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

fuelen commented 6 years ago

@vimox-shah was file really sent?

karensg commented 6 years ago

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

vimox-shah commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

fuelen commented 6 years ago

@vimox-shah could you show parameters from logs?

vimox-shah commented 6 years ago
screen shot 2018-06-07 at 11 31 27 pm

yes you can see logs in attached screenshot @fuelen

djGrill commented 6 years ago

@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)
etwillms commented 6 years ago

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)

fuelen commented 5 years ago

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?

yskkin commented 5 years ago
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?

fuelen commented 5 years ago

@yskkin We don't remember that reason.

maltoe commented 5 years ago

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

bastengao commented 5 years ago

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
mintyfresh commented 3 years ago

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.

EDIT: Still an issue in 2024. As a recommendation for quality of life, it's possible to extend the GraphQL DSL of input objects to work around this, e.g.

module Types
  class BaseInputObject < GraphQL::Schema::InputObject
    # ...

    def self.upload_argument(name, *args, **options)
      argument(name, ApolloUploadServer::Upload, *args, **options, prepare: -> (upload, _) {
        upload&.__getobj__ # Unwrap value for ActiveStorage
      })
    end
  end
end

Which slightly cleans up the argument preparation to:

upload_argument :image, required: false

This bit is up to personal preference, of course.

sujh commented 1 year ago

@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.