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

Revert "Restrict graphql content types (#458)" #504

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

MathieuLegault1
Copy link
Contributor

This reverts commit 6a4173a.

📖 Description

We unfortunately cannot remove the Plug.Parser from endpoint since the plug Plug.MethodOverride requires the body to be parsed before calling it. I've tried to move the plug MethodOverride to router.ex but unfortunately it doesn't work since a pipeline is only executed if a route matches.

In the documentation:

Note that router pipelines are only invoked after a route is found. No plug is invoked in case no matches were found.

For now we revert this commit and we can check after what is the best way to restrict the GraphQL endpoint to only json.

📝 Notes

📓 References

🦀 Dispatch

  • #dispatch/elixir

@mirego-builds
Copy link

🦀 Requesting reviewers for this pull request:

  • @matkev (contributor with 3 commits in the last 90 days and 3 commits overall)
  • @bducharme (reviewer for the elixir stack)

@gcauchon
Copy link
Contributor

I personnaly challenge the approach to break routing into multiple modules… It does not bring any value, and we even have to add a plug to overcome undesired side effects!

https://github.com/mirego/elixir-boilerplate/blob/main/lib/elixir_boilerplate_web/endpoint.ex#L53

In the large PR a did a few months ago to re-sync our boilerplate with the community approach, it was actually one of the first clean-up I did… Too bad it was never merged 🤷🏻‍♂️

#249

pass: [],
json_decoder: Phoenix.json_library()
)

Copy link
Member

Choose a reason for hiding this comment

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

@MathieuLegault1 But we could add a custom plug here to make sure the request content-type is application/json right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could. I'll merge this PR so nobody has this problem if someone forks it and I'll look to do it this week.

@MathieuLegault1 MathieuLegault1 merged commit 0470b05 into main Feb 21, 2024
1 check passed
@MathieuLegault1 MathieuLegault1 deleted the revert/restrict-content-type branch February 21, 2024 18:53
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.

4 participants