-
Notifications
You must be signed in to change notification settings - Fork 80
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
Representation fields in entity queries are not validated at query execution time #237
Comments
@linstantnoodles Thanks for the issue submission. From a quick glance at the federation subgraph specification docs, I see only two constraints on the Each entry in the
While there is nothing that I can find that requires the validation behavior you suggest, I understand where you are coming from. I think the impact of adding extra invalid fields is that they are essentially dropped/ignored, so unless someone is abusing the representations for purposes of authentication or authorization, I don't know that there is any kind of risk in leaving the behavior as-is. What do you think? |
@grxy Thanks for the speedy reply! That's true - the federation specs don't actually specify validation behavior for entity fields. I also realized that since the input type of the representation objects are currently
I agree - our gateway currently issues these queries and we have a schema registry that performs schema validation so it's not going to include invalid fields in practice. Like you said if a bad actor chooses to issue invalid fields that don't exist, they'll just be ignored. The problem I'm pointing out specifically is within the context of pre-release testing (and as you pointed out I don't think this is a problem with this gem itself). The absence of any input validation of the representation inputs means that I can reference any key on the representation object inside the reference resolvers even if it doesn't correspond to an actual field. For example: Given this representation with an invalid field:
It's currently possible within a reference resolver to do I'll raise this with the apollo team and get their thoughts on this! Given the dynamic nature of representations in general, it's probably not straightforward to set more concrete type definitions for those inputs. |
Found this paragraph in the spec:
I didn't realize you can have different entity representations in an entities query! If that's the case, I don't see how it's possible to type the representation inputs to anything other than |
@linstantnoodles You're right that a schema-level validation would not be possible (until input unions are supported at least), but we could add an extra runtime validation check in theory. The idea that comes to mind is that we could potentially validate shapes based on the included |
@grxy Yes that's true - I think having that runtime validation would be nice because clients (even if it's just the router) should be notified if they're attempting to resolve types with bad input (representations containing fields that don't exist). I think that behavior would be consistent with the semantics of how we treat invalid arguments into query field i.e |
It's currently possible to issue an entities query with representations that have non-existent fields on the schema. Using an example for testing that's provided in your README:
The above works correctly. If you using the following representation with a bad typename:
You get a runtime error because the schema fails to resolve
BlogPostDoesNotExist
to a known type. This is good!However, If you do this:
The query runs without error despite the field
randomFieldThatDoesNotExist
being present (undefined in theBlogPost
type).Correct me if I'm wrong, but I believe all representations for a given type
K
that are being issued to a subgraph must only reference a set of fields that are a subset of fields defined forK
in the subgraph. So For example, if I have typeBlogPost
that has fields A, B, and C. All entities queries containing representations forBlogPost
must, at minimum, contain the relevant key fields and any other field that's defined onBlogPost
that's a subset of{A B C}
.PS: Thank you for this gem!
The text was updated successfully, but these errors were encountered: