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

Don't set the content-type when there is no payload #325

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lionelschiepers
Copy link

Hi,

I can't use the Mock Web Server https://github.com/stoplightio/prism because the GET operations set a content-type & content-length in the http headers. I think it's an error. a content-type & content-length must be set when there is a payload with operations like 'PUT' or 'POST'...

Web server like https://github.com/stoplightio/prism (mock of openapi) rejects the calls because it validate the request and a content-type has nothing to do with GET operations.

Exemple of a call made by openapi-to-graphql to my openapi web server:

GET http://127.0.0.1:4010/RESTMethod HTTP/1.1
content-type: application/json (bad)
accept: application/json
host: 127.0.0.1:4010
content-length: 0 (bad)
Connection: close

The HTTP Get operation actually set a content-type of 'application/json' when there is no payload. The request adds a content-length of 0 because there is no payload.

Web server like https://github.com/stoplightio/prism (mock of openapi) reject the calls because the content-type has nothing to do with GET operations.

Exemple:
GET http://127.0.0.1:4010/RESTMethod HTTP/1.1
content-type: application/json
accept: application/json
host: 127.0.0.1:4010
content-length: 0
Connection: close

Signed-off-by: Lionel Schiepers <[email protected]>
fix: check if there is a payload by checking the http method instead of payloadRequired because that field is not always correctly initialized during unit test
Signed-off-by: Lionel Schiepers <[email protected]>
@lionelschiepers
Copy link
Author

Hi, I'm not a specialist of node.js/javascript.

I tried to find the reason why payloadRequired was not correctly initialized during the unit test but didn't find the reason. That's why I changed the logic by checking the method instead of payloadRequired. Do you have any advise?

Lionel.

@Alan-Cha
Copy link
Collaborator

@lionelschiepers I apologize for the late reply! A lot of things have been going on at my end.

We set the content-type here. This will be probably a super simple fix, we just need to check for the operation.method to see if it is a GET request or not. I'm not sure where we set the content-length, however. I assume that's set by the request.js library by default when we do not include a payload.

Let me give it a shot...

@Alan-Cha
Copy link
Collaborator

Ah sorry, I got a bit ahead of myself. It seems that you have already made the changes.

This is the way I would have done as well. I do not think we should make things more complicated by checking if the payload is required.

Do these changes address your issue? Do you have any other problems with OtG?

@lionelschiepers
Copy link
Author

Hi,

Yes. This pullrequest solves my problem. If every body agree on the change I would be the most happiest man in the world if the pullrequest is merged :-)

Thanks,
Lionel.

@Alan-Cha
Copy link
Collaborator

Alan-Cha commented Jul 1, 2020

@lionelschiepers I can try to get this checked in later today. I think we need a test case which I will look into.

@Alan-Cha Alan-Cha mentioned this pull request Jul 1, 2020
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

Successfully merging this pull request may close these issues.

None yet

2 participants