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.

Sign up for GitHub

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

Jump to bottom

router-bridge: Re-introduce validate on the main (v2) branch #25

Closed
abernix opened this issue Feb 9, 2022 · 2 comments · Fixed by #37
Closed

router-bridge: Re-introduce validate on the main (v2) branch #25

abernix opened this issue Feb 9, 2022 · 2 comments · Fixed by #37

Comments

@abernix
Copy link
Member

abernix commented Feb 9, 2022

When I initially switched to using the new query-planner, via apollographql/federation@9cb2201, I removed our invocation of validate on the premise that we did all the validations inside of the Federation's buildQueryPlan and operationFromDocument. While we do a lot in those, we do not do the complete suite of validations and we should add that back.

This can be seen rather clearly in the diff on the above commit, but roughly this consists of:

  1. Adding back 
      const validationErrors = validate(composedSchema, query);
  if (validationErrors.length > 0) {
    return { errors: validationErrors };
  }
  2. Making sure the Router itself can recognize that as a validation error and thread that through to this code (see initial studio agent, client awareness and operation reporting router#309)
  3. Adding a test to the Router to make sure that we test for validation to have happened.
@abernix
Copy link
Member Author

abernix commented Feb 9, 2022

I've marked this as blocked until #21 lands.

This was referenced Feb 9, 2022
Merged
Merged
@abernix abernix changed the title Re-introduce validate to the router-bridge on the main (v2) branch router-bridge: Re-introduce validate on the main (v2) branch Feb 10, 2022
@abernix
Copy link
Member Author

abernix commented Feb 14, 2022

Ok, this is unblocked.

abernix added a commit that referenced this issue Feb 14, 2022
@abernix
router-bridge: Introduce native graphql (JS) validate to the plan
39ada0a 
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
@abernix abernix mentioned this issue Feb 14, 2022
Merged
abernix added a commit that referenced this issue Feb 15, 2022
@abernix
router-bridge: Introduce native graphql (JS) validate to the `pla…
135a2c5 
…n` (#37)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

router-bridge: Introduce native graphql (JS) validate to the plan apollographql/federation-rs
1 participant
@abernix