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

Serialize integers in multipart forms #761

Closed
wants to merge 1 commit into from

Conversation

szastupov
Copy link

I know this change might be too specific, but users coming from requests are used to passing integers without converting them to strings 1.

@szastupov
Copy link
Author

Any updates on this one? I can write more tests if necessary.

@asvetlov
Copy link
Member

Looks good to me.
@kxepal what do you think about?

@kxepal
Copy link
Member

kxepal commented Feb 10, 2016

Hi @szastupov!
Sorry, I don't have a strong opinion on this. The origin of this behaviour lasts from urllib3/urllib3@9796a76 commit when urllib3 blindly casted any value to a string. After that commit, int's support was preserved for backward compatibility (with the tests I think). So this feature seems only remains by that reason for almost 6 years already.

Should we support old past days decisions of the thirdparty libs? I'm not sure.
What's the practical reason for sending a single number as a body part? I'm not sure at all.

I think here would be better to allow users to register own serializers (and override default ones), but that's will require a bit more work. As for this PR I'm -0.

@asvetlov
Copy link
Member

@kxepal agree with your objections

@szastupov
Copy link
Author

What's the practical reason for sending a single number as a body part?

The first reason is when people want to simply pass an object as data parameter to request. They could use json but then you have to do it manually so they fall back to passing object via data. Btw, this case can be covered by a separate patch that adds json parameter to request (again, similar to requests interface).

The second case is when server API accepts both arguments and a file (for example). In this case I usually manually convert integers to to strings so I could get used to it, but I found that for some people it's confusing and it takes them time to figure out why the code doesn't work.

Tl;DR I just want aiohttp to be a bit more user friendly (it already does a good job), so why not copy copy some ideas from requests? :)

@fafhrd91
Copy link
Member

Provide real example for int use case. Right now I am -1

@fafhrd91
Copy link
Member

Sending int and receiving string doesn't sound strait forward

@kxepal
Copy link
Member

kxepal commented Feb 10, 2016

@szastupov

The first reason is when people want to simply pass an object as data parameter to request. They could use json but then you have to do it manually so they fall back to passing object via data.

Explicit is better than implicit. By passing an object I'm not sure what the request will be made: with application/json data or multipart/form-data - both can be valid. XML may be?

The second case is when server API accepts both arguments and a file (for example). In this case I usually manually convert integers to to strings so I could get used to it, but I found that for some people it's confusing and it takes them time to figure out why the code doesn't work.

Ok, assume that's it. Then somehow in your or else code True value will be passed to a body part writer. Since isinstance(True, int) is True that value will be serialized without any issues, but a server will be very surprised to get True string instead of a number. Whom to blame in this case? Should we share the same bug as the requests eventually does? I'm not sure.

so why not copy copy some ideas from requests?

As the commit that I referenced shows, that idea is not a part of requests ideas, but a legacy of underlying http client. Also buggy as I wrote above.

Sending int and receiving string doesn't sound strait forward

+1. So I'm -1 as well now.

@szastupov
Copy link
Author

Ok, you convinced me about multipart. So, what about providing a convenience method for posting json, something like this:

session.post('http://httpbin.org/post', json=mydata)

Which will set content-type header and dump json. What dou you think?

@kxepal
Copy link
Member

kxepal commented Feb 11, 2016

@szastupov I think that would need a new issue ticket for the further discussion. Let's not mix the ideas (:

Closing this one.

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants