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

Configured Resource Content-Type header overwritten by Resource.request() functions #262

Closed
falcon283 opened this issue Jul 4, 2018 · 4 comments

Comments

@falcon283
Copy link

I found that using the new Siesta 1.4 some network requests are broken.
As short explanation: the Content-Type header, placed by Service configuration, got overwritten by the Resource.request(...) method

When you use a Resource R1 load using another resource R2 request you end up having the Resource R2 request Content-Type overwritten by the request contentType default parameter.

Please refer #247 comment for a better explanation

I'm not sure the actual #247 fix is the wanted behaviour and if it is, it will broke quite some code around I guess 😄

@falcon283
Copy link
Author

Probably there is a misuse of Resource.request(.post, text: "SomeText") in my app, thus I solved it using Resource.request(.post, json: jsonObj)

It's still valid anyway that the current behaviour is different from the hold one and it's up to you decide if this new behaviour is the real behaviour you wanted or not.

The thing is that the Resource "Content-Type" value provided by the configuration, got completely overwritten by Resource.request(.post, text: "SomeText")

@falcon283
Copy link
Author

Ok I found your thought here thus I guess we can close this issue.
Thanks

@pcantrell
Copy link
Member

Just because I had thoughts doesn’t mean they’re right! I can see a case to be made that the configuration should have precedence, or perhaps that using the default (e.g. request(.post, json: foo)) should read the config which an explicitly specific one takes precedence, or….

I'm looking for something that:

  • is easy to understand & reason about, and
  • suits the common use cases.

@falcon283
Copy link
Author

falcon283 commented Jul 13, 2018

I also thought about it.

Content-type does make sense to be unique, I guess it’s specified also in the HTTP specs.
It’s the media type of the body you are sending. So multiple values doesn’t make really sense.
Only exception, if I’m not wrong, would be multipart ... but it has its own content-type and the Body, as multipart, define its content type per body part.

Regarding the .request methods ... well they are utilities so text/plain does make sense for text utility and application/json make sense for the json utility.

I’m thinking if it would be nice for those two methods to completely hide the contentType just to avoid confusion.
If you use them it’s because you want to send plain text or json object.

There is always the full .request method to create totally custom request with Data.

This anyway will have chance to break devs code due to API signature change

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

2 participants