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

Variable types are not validated #62

Closed
BrynCooke opened this issue Aug 5, 2021 · 6 comments · Fixed by #166 or #227
Closed

Variable types are not validated #62

BrynCooke opened this issue Aug 5, 2021 · 6 comments · Fixed by #166 or #227
Assignees

Comments

@BrynCooke
Copy link
Contributor

Describe the bug
When passing in variables their types should be validated against the schema and the entire request rejected if they are incorrect.

Requires schema parsing to complete.

@o0Ignition0o o0Ignition0o transferred this issue from another repository Nov 4, 2021
@cecton
Copy link
Contributor

cecton commented Nov 17, 2021

I analyzed a bit and I would need to make quite some abstraction to make it all work. I think it would be best to wait for the higher level API @lrlna will be making on apollo-parser.

@BrynCooke I was wondering if implementing the code to solve this issue is actually a good idea: this will slow down the router a little bit and since it's the variables it can't be entirely cached. The type validation itself is already done by the subgraphs. Why not just raise the errors and call it a day?

@cecton
Copy link
Contributor

cecton commented Nov 17, 2021

Oh I see. The errors will match the sub queries variables, not the original variables which might differ.

@BrynCooke
Copy link
Contributor Author

BrynCooke commented Nov 18, 2021

If it's simple I would be tempted to implement this:

  1. to get us towards where we need to be in terms of router functionality.
  2. to enable us to feed requirements back to apollo-rs.

What do you think though? It's throw away code for sure.

@lrlna The timescales for the HIR will inform our decision on the above. Do you think it would be feasible to jump ahead and start contributing to an HIR in apollo-rs relatively soon? Otherwise there is probably still benefit from a stop-gap solution.

@cecton cecton mentioned this issue Nov 18, 2021
6 tasks
@lrlna
Copy link
Member

lrlna commented Nov 18, 2021

@BrynCooke here is the current plan for semantic analysis that I think will be able to accommodate for other people to contribute to the work some time in the second part of January:

  1. I have a pretty good idea of how I'd like to design the library, but I am very keen to do a spike first.
  2. Once the spike is complete I will be in a position to thoroughly breakdown the work into smaller chunks that could be divided between myself and another implementor (or 2!). This is important as I like to make sure that the work we are doing is thought out, deliberate, and facilitates the needs of other tooling that will also be using this library. Timeline-wise, this will bring us close to holiday season.
  3. If my current idea is proved to be reasonable during the spike, it will be easiest if I set up a barebones architecture first before other contributors come on board. Otherwise we are in a situation of too many cooks. This is likely to spill over to just after the holidays.
  4. After this, I think you can start contributing with a lot more ease.

How does this sound to you?

@BrynCooke
Copy link
Contributor Author

Thanks @lrlna, this sounds great.

Given this information I am +1 to a stopgap on the router side with the intention of replacing with the apollo-rs HIR when it makes sense to do so.

@cecton
Copy link
Contributor

cecton commented Nov 24, 2021

Closed by mistake

@cecton cecton reopened this Nov 24, 2021
@abernix abernix added 2021-12 and removed 2021-11 labels Dec 3, 2021
o0Ignition0o added a commit that referenced this issue Jan 11, 2022
# [v0.1.0-alpha.3] 2022-01-11

## 🚀🌒 Public alpha release

> An alpha or beta release is in volatile, active development. The release might not be feature-complete, and breaking API changes are possible between individual versions.

## ✨ Features

- Trace sampling [#228](#228): Tracing each request can be expensive. The router now supports sampling, which allows us to only send a fraction of the received requests.

- Health check [#54](#54)

## 🐛 Fixes

- Schema parse errors [#136](#136): The router wouldn't display what went wrong when parsing an invalid Schema. It now displays exactly where a the parsing error occurred, and why.

- Various tracing and telemetry fixes [#237](#237): The router wouldn't display what went wrong when parsing an invalid Schema. It now displays exactly where a the parsing error occurred, and why.

- Query variables validation [#62](#62): Now that we have a schema parsing feature, we can validate the variables and their types against the schemas and queries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants