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

Handle GET request query parameters. #429

Merged
merged 22 commits into from
Feb 15, 2022

Conversation

o0Ignition0o
Copy link
Contributor

@o0Ignition0o o0Ignition0o commented Feb 9, 2022

This commit allows us to parse --data-urlencoded get parameters, to handle a graphql request.

This commit adds plugin utilities that will allow users to craft Request and Responses, and MockServices so they will be able to test their Layers and Plugins in isolation.
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

One small thing to add a reference to the documentation on '+' treatment.

@@ -46,6 +46,34 @@ where
}

impl Request {
pub fn from_urlencoded_query(url_encoded_query: String) -> Result<Request, serde_json::Error> {
// decode percent encoded string
// from the docs `Unencoded `+` is preserved literally, and _not_ changed to a space.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Which docs? Specify a reference here please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also probably could do with a larger comment here with background on the problem.

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Do we currently have a way to forbid mutations from happening over GET? Mutations should not be allowed over GET. (And we should return HTTP 405 Method Not Allowed.)

Marking as requested changes, mostly just because it's important. (Haven't reviewed the rest!)

@o0Ignition0o
Copy link
Contributor Author

it somehow looks like i can't comment on the thread anymore, I've adressed the comment related issue in 1e4dc1a

@Geal
Copy link
Contributor

Geal commented Feb 10, 2022

is forbidding mutations due to CSRF (cf https://blog.doyensec.com/2021/05/20/graphql-csrf.html )?

Base automatically changed from igni/plugin_utils to geal-all-along-the-tower-service February 10, 2022 15:57
@o0Ignition0o
Copy link
Contributor Author

@abernix getting there! Just need to make sure the response body matches our expectations (or the gateways)

@abernix
Copy link
Member

abernix commented Feb 11, 2022

is forbidding mutations due to CSRF (cf blog.doyensec.com/2021/05/20/graphql-csrf.html )?

That definitely comes into play though I don't think that this alone would be sufficient to tick that box since POST is also CSRF-able.

At the core of it for me is that GET must be Idempotent and mutations would often be expected to have side-effects.

Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

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

Approved with one suggestion.

apollo-router-core/src/services/router_service.rs Outdated Show resolved Hide resolved
@o0Ignition0o
Copy link
Contributor Author

@abernix @garypen friendly ping on this, if you have a bit of spare time :)

Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Looks good. I think adding Default to Request seems useful, but let me know if you disagree.

apollo-router-core/Cargo.toml Outdated Show resolved Hide resolved
apollo-router-core/src/plugin_utils/structures.rs Outdated Show resolved Hide resolved
apollo-router-core/src/plugin_utils/structures.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Nice work!

@abernix abernix dismissed their stale review February 15, 2022 09:16

My concerns have been addressed!

@Geal Geal mentioned this pull request Feb 15, 2022
@o0Ignition0o o0Ignition0o merged commit d147f97 into geal-all-along-the-tower-service Feb 15, 2022
@o0Ignition0o o0Ignition0o deleted the igni/http_get_support branch February 15, 2022 09:49
@abernix abernix mentioned this pull request Feb 15, 2022
abernix added a commit that referenced this pull request Feb 15, 2022
We can use this changelog in the future!

Ref: #429
@abernix abernix mentioned this pull request Feb 18, 2022
@abernix abernix added this to the v0.1.0-alpha.5 milestone Feb 18, 2022
@o0Ignition0o o0Ignition0o self-assigned this Mar 25, 2022
abernix added a commit that referenced this pull request Sep 21, 2022
This removes `Request::from_bytes()` from the public API.  We were no
longer using it and we don't expect anyone external to have been relying on
it.

This was discovered this function during an exercise of documenting our
entire public API.  We considered keeping it briefly, but it doesn't
necessarily meet our requirements for shipping it in the public API.  It's
internal usage was removed in
[`d147f97d`](d147f97d as part
of [PR #429](#429).

We can consider re-introducing this in the future (it even has a matching
`Response::from_bytes()` which it composes against nicely!), but it seemed
worth remove it for the time-being until the use-cases are here.  (We can
already see the use-cases, but we want to actually design them first.)

Closes #1855
abernix added a commit that referenced this pull request Sep 21, 2022
This removes `Request::from_bytes()` from the public API.  We were no
longer using it and we don't expect anyone external to have been relying on
it.

This was discovered this function during an exercise of documenting our
entire public API.  We considered keeping it briefly, but it doesn't
necessarily meet our requirements for shipping it in the public API.  It's
internal usage was removed in
[`d147f97d`](d147f97d) as part
of [PR #429](#429).

We can consider re-introducing this in the future (it even has a matching
`Response::from_bytes()` which it composes against nicely!), but it seemed
worth remove it for the time-being until the use-cases are here.  (We can
already see the use-cases, but we want to actually design them first.)

Closes #1855
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

6 participants