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: argument and variable validation during execution #902

Merged
merged 6 commits into from
Oct 4, 2024

Conversation

mattjohnsonpint
Copy link
Contributor

@mattjohnsonpint mattjohnsonpint commented Sep 28, 2024

When using engine.Execute from the execution package, I generally expect that all inputs will be validated before the operation is actually executed against the data source. As discussed in #885, this turned out to not be the case for:

  • Input arguments that are specified but are invalid when compared to their schema
  • Input arguments that are an input object type, where one of the fields of the input object is invalid when compared to its schema
  • User-supplied variables that are invalid when compared to the variable definition in the operation

After much research, I believe there are two core issues:

  • The default normalization options include WithExtractVariables, which converts arguments to variables. Since normalization happens before validation, a document with invalid arguments is normalized to a valid document with invalid variables. Thus validation of the document passes erroneously.
  • Variable validation is not performed at all. Even if it were, it would give incorrect messages about invalid arguments - because they would have been converted to variables using assigned names rather than the original argument name and path.

See #885 for examples of both cases.

The solution is to break up the normalization process into multiple stages.

  1. First, normalize with all the default options except WithExtractVariables.
  2. Next, validate the operation document
  3. Then validate the variables the user supplied with the original request
  4. Finally, normalize again, extracting document arguments to variables so the execution plan caching continues to work as originally designed.

After applying these changes, two of the existing tests failed. On closer inspection, they were actually false-positive tests for the very problems I was experiencing. One of them was allowing null values through to the data source where it should have errored, and the other was being fed a single integer to a variable that was expecting a list of integers. I updated those tests to the correct behavior.

Let me know if you need any adjustments or more tests added. Thanks. 🙂

@mattjohnsonpint mattjohnsonpint changed the title Fix argument and variable validation failures during execution via engine.Execute fix: argument and variable validation during execution Sep 28, 2024
@jensneuse jensneuse added the internally-reviewed Internally reviewed label Oct 1, 2024
@devsergiy devsergiy merged commit 895e332 into wundergraph:master Oct 4, 2024
4 checks passed
@mattjohnsonpint mattjohnsonpint deleted the validation branch October 4, 2024 18:24
devsergiy pushed a commit that referenced this pull request Nov 15, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.1.0](execution/v1.0.9...execution/v1.1.0)
(2024-11-14)


### Features

* add data source ID to trace
([#870](#870))
([beb8720](beb8720))
* add further apollo-compatible error support
([#939](#939))
([2d08eb6](2d08eb6))
* add query plans
([#871](#871))
([da79d7e](da79d7e))
* expose acquire resolver wait time in loader hooks
([#854](#854))
([b85148d](b85148d))
* expose compose method of engine federation config factory
([#878](#878))
([95e943e](95e943e))
* improve performance and memory usage of loader & resolbable
([#851](#851))
([27670b7](27670b7))
* improve resolve performance by solving merge abstract nodes in
postprocessing
([#826](#826))
([6566e02](6566e02))
* include subgraph name in ART
([#929](#929))
([fc0993d](fc0993d))
* rewrite variable renderer to use astjson
([#946](#946))
([0d2d642](0d2d642))
* subgraph error propagation improvements
([#883](#883))
([13cb695](13cb695))
* support multiple pubsub providers
([#788](#788))
([ea8b3d3](ea8b3d3))
* validate returned enum values
([#936](#936))
([7aa4add](7aa4add))


### Bug Fixes

* argument and variable validation during execution
([#902](#902))
([895e332](895e332))
* correctly render trace and query plan together
([#874](#874))
([2fc364f](2fc364f))
* execution validation order, do not reuse planner
([#925](#925))
([3ffce8b](3ffce8b))
* ignore empty errors
([#890](#890))
([4c5556f](4c5556f))
* improve ws subprotocol selection
([1fc0dd9](1fc0dd9))
* improve ws subprotocol selection
([#795](#795))
([ad67dbb](ad67dbb))
* keep scalar order when merging fields in post processing
([#835](#835))
([d27fb6e](d27fb6e))
* keep unused variables during normalization
([#802](#802))
([15ae7b3](15ae7b3))
* level of null data propagation
([#810](#810))
([537f4d6](537f4d6))
* merging fields correctly
([0dfb6a2](0dfb6a2))
* merging fields correctly
([#836](#836))
([3c4cb17](3c4cb17))
* merging response nodes
([#772](#772))
([5e89693](5e89693))
* planning of provides, parent entity jump, conditional implicit keys,
handling of external fields
([#818](#818))
([fe6ffd6](fe6ffd6)),
closes
[#830](#830)
[#847](#847)
* return empty data when all root fields was skipped
([#910](#910))
([4607dc0](4607dc0))
* variables normalization for the anonymous operations
([#965](#965))
([267aef8](267aef8))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internally-reviewed Internally reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants