Schema validation: interfaces #229

[TobiasPfeifer]

initial schema validation method layout for interfaces for #229

@ghostdogpr can you do an early stage review of private def validateInterface(t: __Type)

[ghostdogpr]

@TobiasPfeifer looks good so far!

[TobiasPfeifer]

@ghostdogpr I've implemented fieldname and argumentname validation. I'm now thinking about return type and input type validation. Should I try to lift this validation to the type level?

A __Field could define a `type` of type __TypeReturn that has a kind: __TypeKindReturn that only has instances of the valid return type kinds.
Same for the __InputValue thats `type` can be __TypeInput that has kind: __TypeKindInput

That way __Type conceptually becomes a sealed trait with instances such as Scalar, Object, Union etc.

However it's possible for me to leave the types as they are and validate on them at runtime

[ghostdogpr]

@TobiasPfeifer doing it at the type level would be better in the general case, however __Type and other classes prefixed with __ are modeled after the introspection schema and they are directly used for introspection in Caliban. If you use a sealed trait, __Type would become a union or an interface and wouldn't match with the schema from the spec. For that reason I think we have to stick to that and perform the check at runtime. Makes sense?

[ghostdogpr]

One more thing I told to @zach-albia who's working on #228:
Enum and union were very simple and didn't really need unit tests (and I think Caliban by default can only generate valid ones) but here rules are more complex so I think it would be good to have some. The existing tests in ValidationSpec are all about query validation, so yours will be a bit different: you'll need to create a GraphQL object, and check if .interpreter returns the appropriate ValidationError.

[zalbia]

@ghostdogpr Something came up so I haven't gotten around to pushing a WIP PR #228 yet. Looking to get back to it this Saturday.

[ghostdogpr]

@zach-albia no problem. There are a few rules in common in your 2 issues (and the one @mriceron is doing too) so keep an eye on the other PRs. Whoever does it first, the others can reuse 😄

[TobiasPfeifer]

@ghostdogpr I'll add tests to ensure correct ValidationError when calling .interpreter

@TobiasPfeifer doing it at the type level would be better in the general case, however __Type and other classes prefixed with __ are modeled after the introspection schema and they are directly used for introspection in Caliban. If you use a sealed trait, __Type would become a union or an interface and wouldn't match with the schema from the spec. For that reason I think we have to stick to that and perform the check at runtime. Makes sense?

Sure. I'll implement runtime validation with this PR. If I feel challenged to find a way to do this on the type level without altering the __XYZ types for the introspection schema, I'll tackle this in a separate PR

[ghostdogpr]

#244 was merged so you can re-use the same file for tests (and maybe some of the code)

[TobiasPfeifer]

@ghostdogpr I restructured my code to match the structure of #244 and reused common validation methods

Regarding the tests: I think it's not possible to construct an Interface that has a field returning an INPUT_OBJECT, so there is no test for that rule. Same for the rule "no two fields may share the same name"

[ghostdogpr]

Looks good! Just a few remarks to get better error messages and we can merge 👍

[ghostdogpr]

All good! Thanks for contributing!