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

clarify merge order #132

Open
dariuszkuc opened this issue Jan 10, 2025 · 5 comments
Open

clarify merge order #132

dariuszkuc opened this issue Jan 10, 2025 · 5 comments

Comments

@dariuszkuc
Copy link

dariuszkuc commented Jan 10, 2025

Currently we don't specify the order in which we process the source schemas (at least I haven't seen it here). Should this be explicitly called out?

This just came up with the discussion around which description should be picked up during the merge process

For each type in types:
    If description is null:
        Set description to the description of type.

If we don't specify the order (e.g. source schemas should be sorted lexographically) then we may potentially end up with different supergraphs.

related: #88

@PascalSenn
Copy link
Contributor

@dariuszkuc yes, we should also do this with the schemas, order them by name

@glen-84
Copy link
Contributor

glen-84 commented Jan 12, 2025

Would it not make more sense to respect the order in which the source schemas are passed in?

Ordering by name:

  • Creates a mostly-rigid (but semantically-arbitrary) order. The order would also change unexpectedly if one of the schemas is renamed.
  • Does not allow the developer to control the order.

Of course, this depends on how the schemas are passed in (and I'm not sure which method this issue assumes):

  1. Method call (developer can decide).
  2. CLI call (developer can decide).
  3. Pointing to a directory (probably has to be based on the file name, but not necessarily the schema name)

@PascalSenn
Copy link
Contributor

The challenge is that, in practice, subgraphs are often composed one after another without a specific order, as seen in CI/CD pipelines. Ordering by name allows for the option to prefix with an order key if needed.

@dariuszkuc
Copy link
Author

dariuszkuc commented Jan 13, 2025

IMHO composition should generate exactly the same supergraph regardless of user provided order, i.e. A + B + C = B +A + C. Depending on the merge order you will get slightly different (but semantically equivalent*) schemas, e.g. order of the fields may differ on the merged types. If you can get different schemas depending on the order this means you have to do semantic comparison to check whether your supergraph changed (yes you could potentially detect those on a subgraph level as well... but it is just simpler to recompose after each subgraph publish and compare the final result).

Depending on the order you process those subgraphs you will also end up with slightly different query graphs so you may also end up with different query plans (there could be multiple plans with the same cost).

*there could be a difference in the descriptions but other than that they should be the same

@glen-84
Copy link
Contributor

glen-84 commented Jan 15, 2025

The challenge is that, in practice, subgraphs are often composed one after another without a specific order, as seen in CI/CD pipelines. Ordering by name allows for the option to prefix with an order key if needed.

Ah, right. The only alternative that I can think of would be adding an order attribute to the directive that will be used for the name:

@schema(name: "Products", order: 1)

But even if that became necessary, it's likely not needed for a "v1".

IMHO composition should generate exactly the same supergraph regardless of user provided order

I agree, I didn't think about recomposition with single source schemas. The only issue would be in renaming source schemas, in which case, as you mention, descriptions and query plans may change.

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

3 participants