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

Issues with the "Override Source Has Override" specification #113

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

Issues with the "Override Source Has Override" specification #113

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

Comments

@glen-84
Copy link
Contributor

glen-84 commented Jan 7, 2025

While from is not null

(1) When would from be null?

from must not be in visited.

(2) If it is, should you log the error and break?


(3) I can't get the 3rd counter-example test case to pass with the current algorithm, unless I move the "from must not be in visited" and "Add from to visited" below "If sourceField is not annotated with @override". If C is added to visited (along with A), then "The size of visited must be equal to the size of overrides" will be true, when false is expected.

I can share code if it helps.


(4) We may also want to consider splitting this rule for improved error codes and messages, for example:

  1. The @override directive on field 'Bill.amount' in schema 'A' contains a circular reference ('A' -> 'B' -> 'A').
  2. The field 'Bill.amount' in schema 'C' is overridden in multiple schemas ('A', 'B').
@dariuszkuc
Copy link

Per @override definition (Apollo and this spec) from is non-nullable. Guessing validation should ensure that it is non-empty value and points to a known subgraph name.

directive @override(from: String!) on FIELD_DEFINITION

@glen-84
Copy link
Contributor Author

glen-84 commented Jan 7, 2025

Per @override definition (Apollo and this spec) from is non-nullable.

The from here refers to a variable, not to the directive attribute. 🙂

Guessing validation should ensure that it is non-empty value and points to a known subgraph name.

I think that would be a separate rule, which I was going to suggest. 👍 Edit: I've opened #114.

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