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

Header "content-type" for multipart-request #347

Closed
xml-project opened this issue Jan 30, 2020 · 3 comments · Fixed by #348
Closed

Header "content-type" for multipart-request #347

xml-project opened this issue Jan 30, 2020 · 3 comments · Fixed by #348
Assignees

Comments

@xml-project
Copy link
Contributor

@xml-project xml-project commented Jan 30, 2020

While implementing the new approach, the following problem came up:
The specs says:

The overall content type of the request is taken from the “multipart-content-type” parameter; if that parameter is absent, the value “multipart/mixed” is used.

What to do, if a "content-type" is present in option "headers":
(1) Simply ignore,
(2) Raise error,
(3) Raise error only, if the two entries differ.

As we have all three approaches on other occassions in p:http-request, none seems more natural to me than the other.

But we need to say something - And I need to know, what to implement.

@ndw

This comment has been minimized.

Copy link
Collaborator

@ndw ndw commented Jan 30, 2020

It's a good question. If I step back, it seems like we could make a high-level choice: either the implementation is supposed to interpret the values in the headers map and those values influence its behavior or the headers map is just copied to the request and exists for extra things that aren't part of the core HTTP spec.

I had the latter interpretation in mind, but we now have several places where that leads to potential conflicts. We should probably treat those conflicts in the same way. If the goal is to take the latter approach, I don't think option 3, comparing them and raising an error only if the values differ is the best approach. We should either ignore them or it should be an error if they exist at all in the headers map.

But part of my reason for thinking we would do the "just copy" approach is that it used to be much harder to specify headers than it was to specify options so it was at least a little bit about ease of use. Today, there's not much difference between:

  <p:http-request parameters='{"multipart-content-type": "multipart/mixed"}'/>

and

  <p:http-request headers='{"content-type": "multipart/mixed"}'/>

In fact, I think you could make an argument that the headers case is simpler (it's less typing) and easier (it uses HTTP-technology not our invented parameters).

If we took the former approach, going back to my original statement, and argued that header values influence step behavior, we could remove the multipart-boundary, 'multipart-content-type, and transfer-encoding properties.

Would that be better?

@xml-project

This comment has been minimized.

Copy link
Contributor Author

@xml-project xml-project commented Jan 30, 2020

Ok, now I understand the reason why you made "multipart-content-type" etc. a parameter. To my understanding (before you made the last change) parameters control the configuration of the http client and the post-processing of the response. While headers are http headers which are all send with the request and partly influence the inner logic of the p:http-request step when constructing the request.

Therefor for me it would seem more natural to remove multipart-boundary etc. from parameters and use the normal HTTP headers. I would argue that it could be more natural for authors to put all HTTP headers into headers (and let us sort it out), than to look up which is a header and which is a parameter.

Does that make sense?

@ndw

This comment has been minimized.

Copy link
Collaborator

@ndw ndw commented Jan 30, 2020

Yes. Let me see if I can write up a quick pull request.

@ndw ndw self-assigned this Jan 30, 2020
@ndw ndw closed this in #348 Jan 30, 2020
ndw added a commit that referenced this issue Jan 30, 2020
Fix #347 by reworking headers and parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.