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

Questions regarding post-merge validation rules #147

Open
glen-84 opened this issue Jan 28, 2025 · 2 comments
Open

Questions regarding post-merge validation rules #147

glen-84 opened this issue Jan 28, 2025 · 2 comments

Comments

@glen-84
Copy link
Contributor

glen-84 commented Jan 28, 2025

Empty Merged Object Type

No Queries

  • Covered by Empty Merged Object Type?
  • "IsExposed" – where is this defined? Is it just something like "does not have the inaccessible directive, nor does the type that it is defined on"?
    • Actually, nothing inaccessible exists in the composite schema, so this just has to look for an empty object (see previous rule).

Implemented by Inaccessible

  • "Let schema be the merged composite execution schema" – I thought that this spec is not supposed to reference the execution schema?
  • "If type is not marked with @inaccessible" – this wouldn't happen in the composite schema?
  • "If field is marked with @inaccessible" – same, this would only be applicable in an execution schema?

Interface Field No Implementation

  • "that are visible in the merged schema" – everything is visible in the merged (composite) schema, this is only not true in the execution schema?

Invalid Field Sharing

  • Could this not be implemented as a pre-merge validation rule?
    • The Subscription part could even be a source schema validation rule?

Invalid Shareable Usage

  • Could this not be implemented as a source schema validation rule?

Only Inaccessible Children

  • "If IsExposed(type) is false" – all exposed in composite (not execution) schema?
  • "If field is not marked with @inaccessible and not @internal" – there definitely won't be any internal fields in the composite schema.
  • If only inaccessible, then this rule is basically the same as the "Empty Merged Object Type" (but for other type system members as well).

Require Invalid Fields

  • parentType is used without being defined.
  • The first counter-example is syntactically invalid, so would be caught by the Require Invalid Syntax source schema validation rule.

Provides Invalid Fields

  • "If selectedField returns a composite type then selection" – huh? 🙂

Input Fields cannot reference inaccessible type

  • "If field is not declared as @inaccessible" – no concept of inaccessible in composite (not execution) schema?

General thoughts

I think that we need to clarify the separation of composite (client-facing) and execution schemas. It was my understanding that the spec would not mention the latter at all. Any rules involving @inaccessible in the merged schema don't seem applicable to this specification?

@PascalSenn
Copy link
Contributor

Empty Merged Object Type
Should this also cover empty interfaces, or will there be a separate rule for that?

yes, i checked the merge algorithm, we return an interface without any fields in it #152

I have also seen that we currenlty silently remove unions that are empty. This seems wrong, as this way we cannot validate it properly

No Queries
Covered by Empty Merged Object Type?

I think this should be an exception. Same for all root types, i will take this to the WG.

"IsExposed" – where is this defined? Is it just something like "does not have the inaccessible directive, nor does the type that it is defined on"?

  • Actually, nothing inaccessible exists in the composite schema, so this just has to look for an empty object (see previous rule).

Yes, is exposed is no longer needed. I created #153

Implemented by Inaccessible
"Let schema be the merged composite execution schema" – I thought that this spec is not supposed to reference the execution schema?
"If type is not marked with @inaccessible" – this wouldn't happen in the composite schema?
"If field is marked with @inaccessible" – same, this would only be applicable in an execution schema?
Interface Field No Implementation
"that are visible in the merged schema" – everything is visible in the merged (composite) schema, this is only not true in the execution schema?

Yes that algorithm is wrong. I think that's one of the algorithms that we need to ensure that an interface is still valid after the merge. We could theoretically just run the GraphQL Validation and would get a similar result, but this is a more explicit implementation of it. The name of the rule is bad too, but we need to iterate over the names before the release anyway.
There is another issue with this rule. Interfaces can implement interfaces and have to define all fields too.

#154

Invalid Field Sharing
Could this not be implemented as a pre-merge validation rule?
The Subscription part could even be a source schema validation rule?

Debatable, you'd have to get all possible keys from all source schemas to know which fields are shareable and which are not. yet, also nested keys would make fields shareable. So you'd need to verify the whole type hierarchy. It could be a pre merge validation rule, yet isnt the idea of pre merge validation rules that they should validate that we can safely merge?

Invalid Shareable Usage
Could this not be implemented as a source schema validation rule?

Yes this should be a pre merge validation rule (also the rule is wrong) #155

Only Inaccessible Children
"If IsExposed(type) is false" – all exposed in composite (not execution) schema?
"If field is not marked with @inaccessible and not @internal" – there definitely won't be any internal fields in the composite schema.
If only inaccessible, then this rule is basically the same as the "Empty Merged Object Type" (but for other type system members as well).

Yes i have this of the list for the WG.

Require Invalid Fields

  • parentType is used without being defined.
  • The first counter-example is syntactically invalid, so would be caught by the Require Invalid Syntax source schema validation rule.

#156

Provides Invalid Fields

  • "If selectedField returns a composite type then selection" – huh? 🙂

Should probably just be If selectedField returns a composite type

#157

Input Fields cannot reference inaccessible type
"If field is not declared as @inaccessible" – no concept of inaccessible in composite (not execution) schema?

#158

@glen-84
Copy link
Contributor Author

glen-84 commented Jan 30, 2025

I think it's possible that No Queries could be implemented as a pre-merge validation rule, since it should just be:

"At least one source schema has a non-internal and non-inaccessible field on the Query type"

Right? Or are there other reasons why such a field may not make it into the merged schema?

Do let me know, but for now I'll implement it as a post-merge validation rule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants