Manfred / Reynard

Minimal OpenAPI client for Ruby.
MIT License
12 stars 2 forks source link

Extend support for schemas with `multipart/form-data` content #54

Closed alexmalus closed 5 months ago

alexmalus commented 5 months ago

Added Reynard::Context#multipart_form so consumers can pass form data to upload files.

alexmalus commented 5 months ago

@Manfred is this something you can find time to review please?

Manfred commented 5 months ago

Thanks for suggesting this, but I can't accept this PR. Params should be automatically serialized as multipart form data when the requested content type in the spec is multipart/form-data. Sort of similar to how we would support XML and form URL encoded if we supported that.

Implementation-wise I would expect Specification#build_body to return a subclass of serialized body that is specific to the specced request body. Probably with some sort of registration class that allows you do hook in additional serializers.

alexmalus commented 5 months ago

Thanks for looking into this!

I can't accept this PR [..]

No worries, I didn't expect an all-or-nothing response. My team uses Reynard across several applications, and since we now need to send files following an OpenAPI specification, we'd like to continue using Reynard for this task if possible. Please answer some questions that can help me address some of the concerns you're having.

Params should be automatically serialized as multipart form data when the requested content type in the spec is multipart/form-data

I'd like to clarify the API Reynard offer to consumers for the use case this PR tries to address. When you say "params", do you mean the existing API should be reused, like so?

response = reynard.
  operation('createBookCover').
  params('size' => 'preview', 'subject' => 'In a Nutshell', 'attachment' => attachment').
  execute

I would expect Specification#build_body to return a subclass of serialized body that is specific to the specced request body.

With the assumption that Reynard::Context#params will be used to register the information, are you then expecting this method to be extended in a similar way as Reynard::Context#body is constructed, building a serialized body that you mention and expect?

Probably with some sort of registration class that allows you do hook in additional serializers

Where would you expect the registration class to be? Do you have an example that I can follow?

Manfred commented 5 months ago

Again, I'm sorry I can't help you right now, this is a personal project and I'm going to work on it at my own pace. I included a few technical details to explain why I'm not going to merge this PR, it wasn't meant as a definitive implementation guide.

I have a plan for changing Reynard to add and remove (de)serializers, setting model and attribute naming instances, and a model registry. Explaining exactly how I would like it to be implemented would take more time than the implementation and that would be less fulfilling to me.

Maybe it's best to fork the project if you are in a hurry or you can hire me through Fingertips if would like support. I will add a contributing guide to the project to explain this a bit more.

alexmalus commented 5 months ago

No worries, I appreciate the timely and sincere responses. Closing the PR since it's not mergeable in this state.