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

router-bridge: Introduce native graphql (JS) validate to the plan #37

Merged
merged 1 commit into from
Feb 15, 2022

Conversation

abernix
Copy link
Member

@abernix abernix commented Feb 14, 2022

While Federation's buildQueryPlan function does a fair bit of validation in order to properly build our plan (and leverages graphql's validate function do most of that), it does not do all of the default validations that are specified.

To be thorough, we must call validate, as we did prior to updating to the new Federation v2 alpha query planner.

This also introduces a number of tests which exercise specific validations which we know as of today are not part of federation:

I chose three random but different types to make sure we're capturing it.

Closes: #25

While Federation's `buildQueryPlan` function does a fair bit of validation
in order to properly build our plan (and leverages `graphql`'s [`validate`]
function do most of that, it *does not* do all of the [default validations]
that are specified.

To be thorough, we must call `validate`, as we did [prior] to updating to the
new Federation v2 alpha query planner.

This also introduces a number of tests which exercise specific validations
which we know as of today are not part of federation:

 - invalid_graphql_validation_1_is_caught: NoFragmentCyclesRule
 - invalid_graphql_validation_2_is_caught: ScalarLeafsRule
 - invalid_graphql_validation_3_is_caught: NoUnusedFragmentsRule

I chose three random but different types to make sure we're capturing it.

[prior]: apollographql/federation@9cb2201
[`validate`]: https://github.com/graphql/graphql-js/blob/95dac43fd4bff037e06adaa7cfb44f497bca94a7/src/validation/validate.ts#L38
[default validations]: https://github.com/graphql/graphql-js/blob/95dac43fd4bff037e06adaa7cfb44f497bca94a7/src/validation/specifiedRules.ts#L76-L103

Closes: #25
@@ -225,7 +361,7 @@ mod tests {
}

#[test]
fn invalid_query_is_caught() {
fn syntactically_incorrect_query_is_caught() {
Copy link
Member Author

Choose a reason for hiding this comment

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

I renamed this just to add clarity — since this is a parsing error, rather than a validation error.

Copy link
Contributor

@o0Ignition0o o0Ignition0o 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 to me!

I feel like some of the validation steps can occur before the actual query pipeline, eg when updating the supergraph schema.

Would it be worth opening an issue on the router, to add schema validation capabilities that would allow us to refuse to update the router with an invalid schema?

@abernix
Copy link
Member Author

abernix commented Feb 15, 2022

@o0Ignition0o That's a prudent observation, and you're correct. We could use validateSDL to do some validations on the schema (which ultimately happen as part of validate, but we could do one better to avoid that schema from going into operation in the Router). (Though practically speaking, I believe our incoming schemas will typically be validated as part of Studio + Composition ahead of time).

I'll open an issue.

@abernix abernix merged commit 135a2c5 into main Feb 15, 2022
@abernix abernix self-assigned this Feb 15, 2022
@abernix abernix deleted the abernix/add-validate-to-plan branch February 15, 2022 10:41
abernix added a commit to apollographql/router that referenced this pull request Feb 15, 2022
Specifically, to bring in the update which adds `validate` to `plan`:

 - apollographql/federation-rs#37
abernix added a commit to apollographql/router that referenced this pull request Feb 15, 2022
Specifically, to bring in the update which adds `validate` to `plan`:

 - apollographql/federation-rs#37
dingxiangfei2009 pushed a commit to dingxiangfei2009/federation-rs that referenced this pull request Jun 23, 2022
feat: set up cargo-deny

fixes apollographql#37

This commit sets up cargo-deny with the following settings:
- Security advisories: warn for unmaintained and yanked crates. deny CVEs except `RUSTSEC-2020-0159` (pending chronotope/chrono#499 to close somehow)
- Licenses: Allow ELv2 compatible licenses: `"Apache-2.0", "Apache-2.0 WITH LLVM-exception", "BSD-2-Clause", "BSD-3-Clause", "CC0-1.0", "LicenseRef-ELv2", "ISC", "MIT",`. `LicenseRef-ELv2` is a placeholder while we're waiting for a SPDX identifier.
- Sources: Explicitly allow references to `apollographql` and `opentelemetry` github repositories only.
- Bans: No crates are banned as of yet

This commit also adds `cargo-deny -L error check` on Linux and OSX in CI. Note it doesn't run on windows, possibly because of an environment variable issue. However this is fine given [cargo-deny checks for every targets by default](https://github.com/EmbarkStudios/cargo-deny/blob/d0ea0a2e7d7376ee212faec756cb12ab05f55c4d/examples/08_target_filtering/README.md#description).
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.

router-bridge: Re-introduce validate on the main (v2) branch
2 participants