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

Add support for multipart MIME in Savon2 #424

Closed
freakyfelt opened this issue Apr 1, 2013 · 18 comments
Closed

Add support for multipart MIME in Savon2 #424

freakyfelt opened this issue Apr 1, 2013 · 18 comments
Milestone

Comments

@freakyfelt
Copy link

In working with a SOAP response I discovered that it contains a multipart MIME response in the body that causes Savon to barf. I see that this was fixed with savon-multipart, however that was for Savon 1.0. Is there anything in the road map to add multipart support to Savon 2?

@rubiii
Copy link
Contributor

rubiii commented Apr 5, 2013

no. that really has to be pushed by people who need multipart support.

@tjarratt
Copy link
Contributor

tjarratt commented Apr 9, 2013

I also just discovered the multipart response support in Savon 1.0 and was surprised it wasn't present in 2.0; it sounds like you're open to pull requests, do you have any thoughts re: how that might work?

Looking at the old implementation, as far as I can tell, the code strips apart the boundaries and just concatenates the body together. In my case, I expect to get back some XML followed by several multi-part messages that are base64 encoded images. I was thinking of exposing those as an #attachments method on the response.

@rubiii
Copy link
Contributor

rubiii commented Apr 9, 2013

i think you're right in that multipart support has been ported to v2 yet.
and i think nobody complained about that up until now.

so, since almost noone used this feature in v1, i don't think it should ship with savon. what i would like to see instead,
is a proper new implementation of the savon-multipart gem that works with v2 and uses a proper interface instead of
overwriting classes and methods.

as far as i know, your observation about how savon-multipart currently works is right.
not sure about how this could solve your problem though.

@ebeigarts maintains the savon-multipart gem (thanks!) , so let's see what he thinks.

@tjarratt
Copy link
Contributor

tjarratt commented Apr 9, 2013

Many thanks for responding so quickly and pointing out savon-multipart -- I wasn't aware of that gem.

That's an interesting approach that should work; I can definitely see the appeal of not cluttering the savon gem with knowledge of how to decode multipart http responses since very few people will actually use it.

My current thinking is that maybe savon-multipart could just override Savon::Operation#call, taking a local argument that indicates whether the response is expected to be multipart or not -- this would change whether we return a Savon::Response or some Savon::MultipartResponse. Ideally it wouldn't need to patch any methods, but with the current design of Savon::Client, Operation and Response, I can't see how else you would inject a different response that can decode multipart messages.

@rubiii
Copy link
Contributor

rubiii commented Apr 9, 2013

i would really not go with overwriting savon. that was a bad idea back then and it still is.
it's just too difficult to not break compatibility and it limits you to the overwritten behavior once it's loaded.

i think we can come up with a better interface to properly support this. what i understand is that you would
basically just have savon return a different response which implements multipart, right?

i would have to look at the code to know if that can also cover the current use cases for savon-multipart
(those that have tests). would be great to keep the current features.

@tjarratt
Copy link
Contributor

tjarratt commented Apr 9, 2013

Absolutely, I want to preserve everything that's already in savon-multipart and make it work with Savon 2.0.

I'll take a closer look at how the Savon::Response is created, maybe there's a simpler interface that doesn't require overwriting any methods at all. Or perhaps @ebeigarts will have some thoughts.

@rubiii
Copy link
Contributor

rubiii commented Apr 9, 2013

what about a new :multipart option which tries to load savon-multipart.
if it's available, multipart is enabled for the client or the operation/request (not sure which makes sense).

then savon-multipart would somehow need to tell savon to use a different class for the response.
basically instead of calling Response.new it would call response_class.new, where response_class
would need to be injected.

@tjarratt
Copy link
Contributor

tjarratt commented Apr 9, 2013

👍

It's a big change from the previous implementation, but I do like how it would indicate multipart support from savon itself. I wasn't even aware of the savon-multipart gem today after having poked around in the code and read a few old issues.

I'll fork this in a bit, write some tests and see if I can get a pull request together.

@rubiii
Copy link
Contributor

rubiii commented Apr 9, 2013

great! let me know how it's going.

@ebeigarts
Copy link

I'm still using savon 1, haven't looked at savon 2 yet. But this sounds great :)

@tjarratt
Copy link
Contributor

tjarratt commented May 7, 2013

@freakyfelt if you're interesting in using savon with multipart HTTP responses, could you try using the latest savon with my branch of savon-multipart? I'm already using it in production, but it would be great to get some more feedback from other people using savon2.

This gist might be helpful to get everything setup for your tests: https://gist.github.com/tjarratt/5530566

@freakyfelt
Copy link
Author

Sure, I'll take a look later this week. Thanks for the help

Bruce Felt
Sent from mobile

On May 6, 2013, at 11:12 PM, Tim Jarratt notifications@github.com wrote:

@freakyfelt if you're interesting in using savon with multipart HTTP responses, could you try using the latest savon with my branch of savon-multipart? I'm already using it in production, but it would be great to get some more feedback from other people using savon2.

This gist might be helpful to get everything setup for your tests: https://gist.github.com/tjarratt/5530566


Reply to this email directly or view it on GitHub.

@armstrtw
Copy link
Contributor

armstrtw commented Jun 3, 2013

+1 for multipart support.

happy to help in testing.

@soundarapandian
Copy link

👍 👍

@martinbaciga
Copy link

Sorry guys, could you tell me how to attach a file on client message, and then receive it on the server side with savon-multipart. Thanks!!

@rubiii
Copy link
Contributor

rubiii commented Jul 26, 2013

this will be released with version 2.3.0. see #481.

@rubiii rubiii closed this as completed Jul 26, 2013
@rubiii rubiii mentioned this issue Jul 26, 2013
27 tasks
@rubiii
Copy link
Contributor

rubiii commented Jul 27, 2013

@tjarratt i'm going to release v2.3.0 later today and then i think we should get the savon-multipart
repository up to date and released. what do you think?

@tjarratt
Copy link
Contributor

Sounds great! I actually moved away from using savon-multipart for some projects at work, but I am still using it for a few smaller things. Been looking forward to getting it released for a while 👍

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

No branches or pull requests

7 participants