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 · 25 comments

Comments

Projects
None yet
@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

awinograd commented Apr 29, 2018

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

awinograd commented Apr 30, 2018

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

This comment has been minimized.

Copy link

liang3404814 commented May 16, 2018

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

This comment has been minimized.

Copy link

riffraff commented May 22, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

vimox-shah commented Jun 6, 2018

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

This comment has been minimized.

Copy link
Member

fuelen commented Jun 6, 2018

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

@vimox-shah

This comment has been minimized.

Copy link

vimox-shah commented Jun 6, 2018

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

This comment has been minimized.

Copy link
Member

fuelen commented Jun 6, 2018

@vimox-shah was file really sent?

@karensg

This comment has been minimized.

Copy link

karensg commented Jun 7, 2018

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

@vimox-shah

This comment has been minimized.

Copy link

vimox-shah commented Jun 7, 2018

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

vimox-shah commented Jun 7, 2018

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

@fuelen

This comment has been minimized.

Copy link
Member

fuelen commented Jun 7, 2018

@vimox-shah could you show parameters from logs?

@vimox-shah

This comment has been minimized.

Copy link

vimox-shah commented Jun 7, 2018

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

yes you can see logs in attached screenshot @fuelen
@djGrill

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

fuelen commented Nov 7, 2018

@yskkin We don't remember that reason.

@maltoe

This comment has been minimized.

Copy link

maltoe commented Dec 7, 2018

Just for sake of completeness: rails/rails#25250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment