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 GET requests with content_type headers #3452

Merged

Conversation

vethan
Copy link
Contributor

@vethan vethan commented Apr 15, 2024

Description

Currently, if you perform a GET query while supplying the application/json header, strawberry will return a 400. By moving the method check first when parsing the query we can avoid these unneccessary 400s

Types of Changes

  • Core
  • [ x] Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

  • Issue not created

Checklist

  • [x ] My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [ x] I have read the CONTRIBUTING document.
  • [ x] I have added tests to cover my changes.
  • [ x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @vethan - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@botberry
Copy link
Member

botberry commented Apr 15, 2024

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This change fixes GET request queries returning a 400 if a content_type header is supplied

Here's the tweet text:

🆕 Release (next) is out! Thanks to @vethan4 for the PR 👏

Get it here 👉 https://strawberry.rocks/release/(next)

Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Merging #3452 (3327afd) into main (292b771) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3452      +/-   ##
==========================================
- Coverage   96.48%   96.47%   -0.01%     
==========================================
  Files         509      509              
  Lines       32717    32723       +6     
  Branches     5404     5404              
==========================================
+ Hits        31566    31571       +5     
- Misses        940      941       +1     
  Partials      211      211              

Copy link

codspeed-hq bot commented Apr 15, 2024

CodSpeed Performance Report

Merging #3452 will degrade performances by 26.72%

Comparing vethan:get-method-check-should-be-first (53e3874) with main (292b771)

Summary

❌ 1 regressions
✅ 12 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main vethan:get-method-check-should-be-first Change
test_execute_basic 10.2 ms 13.9 ms -26.72%

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! interesting!

The changes look good, but could you add a test? let me know if you need any info 😊

@vethan
Copy link
Contributor Author

vethan commented Apr 15, 2024

Ah! interesting!

The changes look good, but could you add a test? let me know if you need any info 😊

@patrick91 Test Added!

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you!

@patrick91 patrick91 merged commit 3443390 into strawberry-graphql:main Apr 15, 2024
61 of 63 checks passed
@botberry
Copy link
Member

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants