Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Not compatible with ActiveStorage #10

Open
bastengao opened this issue Apr 19, 2018 · 28 comments
Open

Not compatible with ActiveStorage #10

bastengao opened this issue Apr 19, 2018 · 28 comments

Comments

@bastengao
Copy link
Contributor

bastengao commented Apr 19, 2018

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
Copy link
Collaborator

fuelen commented Apr 19, 2018

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
Copy link

awinograd commented Apr 29, 2018

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
Copy link
Collaborator

fuelen commented Apr 29, 2018

@awinograd yes, sure.

awinograd added a commit to awinograd/apollo_upload_server-ruby that referenced this issue Apr 29, 2018
@awinograd
Copy link

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

awinograd@e2c68cc

Looks like it was introduced in awinograd@0a9e17d to solve a different but

@fuelen
Copy link
Collaborator

fuelen commented Apr 30, 2018

@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
Copy link

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
Copy link

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
Copy link

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
Copy link

karensg commented Jun 5, 2018

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
Copy link

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
Copy link
Collaborator

fuelen commented Jun 6, 2018

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

@vimox-shah
Copy link

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
Copy link
Collaborator

fuelen commented Jun 6, 2018

@vimox-shah was file really sent?

@karensg
Copy link

karensg commented Jun 7, 2018

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

@vimox-shah
Copy link

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
Copy link

vimox-shah commented Jun 7, 2018

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
Copy link

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

@fuelen
Copy link
Collaborator

fuelen commented Jun 7, 2018

@vimox-shah could you show parameters from logs?

@vimox-shah
Copy link

screen shot 2018-06-07 at 11 31 27 pm

yes you can see logs in attached screenshot @fuelen

@djGrill
Copy link

djGrill commented Jul 23, 2018

@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
Copy link

etwillms commented Aug 3, 2018

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
Copy link
Collaborator

fuelen commented Sep 18, 2018

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
Copy link

yskkin commented Oct 29, 2018

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
Copy link
Collaborator

fuelen commented Nov 7, 2018

@yskkin We don't remember that reason.

@maltoe
Copy link

maltoe commented Dec 7, 2018

Just for sake of completeness: rails/rails#25250

@bastengao
Copy link
Contributor Author

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
Copy link

mintyfresh commented Feb 17, 2021

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
Copy link

sujh commented May 22, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

14 participants