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

fix(elements-core): Do not omit request body for certain HTTP methods #2607

Conversation

provokateurin
Copy link
Contributor

The HTTP spec does not forbid using request bodies for GET, DELETE and other methods (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/GET for example). OpenAPI doesn't forbid this either, so it is possible to have a valid specification that is not correctly executed by this library.

I'm not sure about adding tests, as I couldn't find any tests that cover this area of the code so far, or at least not directly unit testing this method. Let me know if you want to have tests for this change and how I can implement them best.

Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for stoplight-elements ready!

Name Link
🔨 Latest commit 3b8b13c
🔍 Latest deploy log https://app.netlify.com/sites/stoplight-elements/deploys/66a78f846d3e0f00084e827e
😎 Deploy Preview https://deploy-preview-2607--stoplight-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 1, 2024

Deploy Preview for stoplight-elements-demo ready!

Name Link
🔨 Latest commit 3b8b13c
🔍 Latest deploy log https://app.netlify.com/sites/stoplight-elements-demo/deploys/66a78f8493b2640008006a6e
😎 Deploy Preview https://deploy-preview-2607--stoplight-elements-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mnaumanali94
Copy link
Contributor

@provokateurin Tests seem to be failing particularly the lint and check step. Others probably are unrelated. Can you take a look and fix them please. Then you can ask for a re review.

@provokateurin provokateurin force-pushed the fix/elements-core/http-method-request-body-omission branch from d20e02d to ff22efd Compare July 12, 2024 12:41
@provokateurin
Copy link
Contributor Author

Done!

@provokateurin provokateurin force-pushed the fix/elements-core/http-method-request-body-omission branch from ff22efd to 0e36b14 Compare July 12, 2024 12:42
@provokateurin
Copy link
Contributor Author

Hey @mnaumanali94 do you mind taking another look? Would be nice to have this fix available soon :)

@provokateurin
Copy link
Contributor Author

Is the e2e failure related? I can't tell from the logs it's giving

@darekplawecki
Copy link
Contributor

@provokateurin The code looks fine, but if you want to release changes, you also need to upgrade version in package.json. Since you've made changes in elements-core, you would need to upgrade version in all three packages (read Versioning Guideline).

I'll check E2E Tests however they are in general flaky and therefore not really reliable (we have a ticket in our sprint to analyse it).

@provokateurin
Copy link
Contributor Author

Ok I can bump the version. Should I wait for you to check the tests or do it right away?

@darekplawecki
Copy link
Contributor

@provokateurin no need to wait, you can do it right away :)

@provokateurin provokateurin force-pushed the fix/elements-core/http-method-request-body-omission branch 2 times, most recently from de037dc to 231e8af Compare July 29, 2024 11:36
@provokateurin provokateurin force-pushed the fix/elements-core/http-method-request-body-omission branch from 231e8af to 3b8b13c Compare July 29, 2024 12:48
@darekplawecki
Copy link
Contributor

@provokateurin I checked E2E Tests. They are failing while trying to send a request:

Screenshot 2024-07-29 at 14 58 03

This is the error that is thrown:
Failed to execute 'fetch' on 'Window': Request with GET/HEAD method cannot have body.

So it seems like the fetch method from window API in browser forbids request body for GET requests and it breaks the "Try It" functionality.

@provokateurin
Copy link
Contributor Author

Even though I think it is wrong, Chrome seems to prevent this. I already saw this in a real codebase where a GET request had a body and failed, so I don't think there is a way around this :/
Maybe parts of this PR can still be merged, since e.g. curl probably allows this behavior and there is no reason to prevent it. For the TryIt a warning could be displayed that explains why the body isn't visible/in the request.

@darekplawecki
Copy link
Contributor

@provokateurin sounds good to me :)

@mnaumanali94
Copy link
Contributor

@provokateurin If you want to take out the stuff around try it, we can review this again.

@provokateurin
Copy link
Contributor Author

Sorry, I'm dealing with other problems at the moment. I'm not sure I will revisit this soon, so feel free to close this PR or pick up the working parts of it.

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

3 participants