fix(federation): differentiate field and input value names #280

patrick91 · 2020-11-15T17:58:33Z

This only contains the test at the moment :)

I found a bug where I had at type like this:

type Campaign implements Node {
  id: ID!
  identifier: String
  event(identifier: String!): Event
}

in two services, and apollo was still complaining about the types being different.

Turns out that the diff is comparing the field identifier with the argument identifier,
and they have different types.

So, I've added a test for that. I suspect we might have a similar issue with different fields
having the same argument but with different type. I might add a test for that later :)

EDIT: not sure why the tests didn't fail here

Summary of all failing tests
 FAIL  federation-js/src/composition/validate/sdl/__tests__/uniqueFieldDefinitionNames.test.ts (5.973s)
  ● UniqueFieldDefinitionNames › permits duplicate field names for › object type definitions field and arguments with different type

    expect(received).toHaveLength(expected)

    Expected length: 0
    Received length: 2
    Received array:  [[GraphQLError: Field "Campaign.identifier" can only be defined once.], [GraphQLError: Field "Campaign.event" can only be defined once.]]

      288 |         UniqueFieldDefinitionNames,
      289 |       ]);
    > 290 |       expect(errors).toHaveLength(0);
          |                      ^
      291 |     });
      292 |   });
      293 | });

      at Object.<anonymous> (src/composition/validate/sdl/__tests__/uniqueFieldDefinitionNames.test.ts:290:22)

Ah, the tests were not running: #281

apollo-cla · 2020-11-15T17:58:35Z

@patrick91: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

trevor-scheer · 2020-11-19T22:02:14Z

Interesting bug, and good catch! Thanks for opening this - is this something you're interested in fixing as well?

patrick91 · 2020-11-19T23:33:21Z

@trevor-scheer I can give a try on the weekend! Do you have any pointer?

trevor-scheer · 2020-11-20T16:28:24Z

@patrick91 Awesome! I see the issue - let me know if you'd like me to go into any more detail. I don't want to steal the solution from you if you're interested in the solve, but happy to provide as much assistance as you're interested in.

This object seems to be doing a bit more work than it should within the validator:

federation/federation-js/src/composition/validate/sdl/uniqueFieldDefinitionNames.ts

Lines 68 to 70 in e8fae64

    
           const knownFieldNames: { 
        
             [typeName: string]: FieldToNameNodeMap; 
        
           } = Object.create(null);

patrick91 · 2020-11-22T00:21:29Z

@trevor-scheer I think I'm having some troubles understanding the code. I think the issue is in diffTypeNodes here: https://github.com/apollographql/federation/blob/main/federation-js/src/composition/utils.ts#L379-L424

I wonder why a visitor has been used for this function, aren't type definitions basically flat? If I understood correctly with the current code what happens is that both fields names and argument names are put inside fieldsDiff which doesn't seem to be correct. I'm not sure what the best solution would be. Do you have any suggestions?

trevor-scheer · 2020-11-23T22:31:12Z

@patrick91 you're on the right track! typeDefs can be iterated over but it's an AST - it's flat in some senses but you will end up doing a fair amount of "walking" the tree yourself without the visitor.

You're right that fieldsDiff is doing double duty here, so we need an inputValuesDiff as well.

As far as the function goes, I can imagine two solutions:

Currently this abstraction works for both visitors, but it needs to operate on 2 different diff objects. We can turn fieldVisitor into a function which returns a visitor function. The argument to our factory function would be the diff.

function fieldVisitorFactory(diff) {
  return function visitor(node) {
    //...previous fieldVisitor logic captured here
  }
}

Then where we call visit:

visit(document, {
    FieldDefinition: fieldVisitorFactory(fieldsDiff),
    InputValueDefinition: fieldVisitorFactory(inputValuesDiff),
    // ...
});

Split the abstraction and have two very similar visitor functions which are single purposed and operate on different diffs (one for FieldDefinition and one for InputValueDefinition).

Is that helpful? Let me know if you have any more questions 🙂

P.S. I lean toward 2 myself. It's less clever. I wrote this code and find it a bit of a stretch to combine this logic.

patrick91 · 2020-11-29T12:36:12Z

@trevor-scheer took me a while to get back to you, but I think I managed to make it work, I went for suggestion 2, let me know how it looks. The tests are passing on my machine 😊

trevor-scheer

Thanks @patrick91! This is looking great, I have one more comment for you to address and this should be ready to ship.

Please add a changelog entry to the federation-js/CHANGELOG.md file as well.

federation-js/src/composition/utils.ts

patrick91 · 2020-12-01T13:34:27Z

federation-js/src/composition/validate/sdl/uniqueTypeNamesWithFields.ts


-      const fieldsDiff = Object.entries(fields);
+      const fieldsDiff = Object.entries(fields).concat(Object.entries(inputValues));


does this make sense @trevor-scheer ?

I think we might need to update checkFieldUniquenessExcludingValueTypes as well :)

I think it might work but I'm not sure the error reporting will be very accurate if we group these diffs into the fieldsDiff. Notice down below (L97) when we iterate over fieldsDiff, we might push error messages that relate to a field, but the error code and message don't really hint at input values. It seems appropriate to introduce another error code, like VALUE_TYPE_INPUT_VALUE_MISMATCH and have some similar iteration over the inputValuesDiff which contributes to the typesHaveSameShape evaluation. Does that make sense? It's similar to what you're already doing, but it does split out the iteration over inputValues so we can provide a more targeted error message.

Feel free to draw inspiration from the format and wording of existing error messages, or I'd be happy to provide guidance there. There's also some documentation around our error codes which would need a matching update:

federation/docs/source/errors.md

Line 89 in 8b5ebc6

| `VALUE_TYPE_KIND_MISMATCH` | An implementing service defines a type with the same name and fields as a type in another service, but there is a declaration mismatch. For example, `type MyType` is invalid if another service defines `interface MyType`. |

I think you're right that an update to checkFieldUniquenessExcludingValueTypes makes sense as well.

not really happy with the code duplication, I was thinking of moving the check to a function, but we'd need to change a bit how we report the error, so might not be worth it :)

federation-js/src/composition/__tests__/compose.test.ts

trevor-scheer · 2020-12-02T16:05:41Z

federation-js/src/composition/validate/sdl/uniqueTypeNamesWithFields.ts

+          if (types.length === 2) {
+            possibleErrors.push(
+              errorWithCode(
+                'VALUE_TYPE_INPUT_VALUE_MISMATCH',


Can we get a test case that exercises this code path and demonstrates what will trigger this error?

done! I wish we could access the field name to improve the error message, but alas I wasn't able to :)

I see. I think we can find a way to pass that info down through the diff but I'm really happy with where we've landed here. This feels ready to land to me and I don't want to block getting this released. If you don't mind opening an issue that references this PR comment that would be wonderful!

federation-js/src/composition/validate/sdl/uniqueTypeNamesWithFields.ts

trevor-scheer

This has come together really nicely, @patrick91. Huge thank you for your initial efforts and timely updates! 🎉

Did you have any lingering concerns with checkFieldUniquenessExcludingValueTypes? I don't think we addressed that yet. After quickly refamiliarizing myself and reading the comments, it seems right to leave it as is, but I'm open to hearing otherwise.

federation-js/CHANGELOG.md

trevor-scheer · 2020-12-03T20:26:26Z

federation-js/src/composition/validate/sdl/__tests__/uniqueFieldDefinitionNames.test.ts

+    });
+  });
+
+  it('object type definitions field different order', () => {


Is this test just asserting that order is irrelevant for fields when determining value types? Just want to make sure I understand this test isn't related to your changes (but let's keep it!)

yup! it was failing before we did the change to fieldDiff, oddly :)

I've updated the test to have a better name (and made the schema nicer too) :)

trevor-scheer · 2020-12-03T20:29:20Z

federation-js/src/composition/validate/sdl/uniqueTypeNamesWithFields.ts

+          if (types.length === 2) {
+            possibleErrors.push(
+              errorWithCode(
+                'VALUE_TYPE_INPUT_VALUE_MISMATCH',


I see. I think we can find a way to pass that info down through the diff but I'm really happy with where we've landed here. This feels ready to land to me and I don't want to block getting this released. If you don't mind opening an issue that references this PR comment that would be wonderful!

patrick91 · 2020-12-03T20:47:56Z

@trevor-scheer thank you for the patience!

Regarding checkFieldUniquenessExcludingValueTypes it seems fine to me now, my previous commit fixed the regression I added, so now the behaviour is the same as before, unless there are inputValues, see here: https://github.com/apollographql/federation/pull/280/files#diff-73bf495bb0ca51439e86b55e70ed176aa970de874bd6801cf8950fa90d22d964R161

If this is ready to merge, do you want me to squash the commits?

trevor-scheer · 2020-12-03T21:01:58Z

@patrick91 That would be great!

…ame name

trevor-scheer · 2020-12-03T22:57:13Z

Thanks again @patrick91! I'll aim to get a patch release out for this tomorrow.

trevor-scheer · 2020-12-04T22:41:38Z

This is released via ab39394!

@apollo/federation@0.20.7
@apollo/gateway@0.21.4

patrick91 force-pushed the issue-with-same-name-arg-field branch from 3898e9f to bc9f13a Compare November 30, 2020 09:29

trevor-scheer requested changes Nov 30, 2020

View reviewed changes

federation-js/src/composition/utils.ts Outdated Show resolved Hide resolved

patrick91 force-pushed the issue-with-same-name-arg-field branch from db3a46b to 6cade24 Compare December 1, 2020 12:12

patrick91 commented Dec 1, 2020

View reviewed changes

patrick91 force-pushed the issue-with-same-name-arg-field branch from c7a77e5 to f3656ee Compare December 2, 2020 10:52

trevor-scheer reviewed Dec 2, 2020

View reviewed changes

federation-js/src/composition/__tests__/compose.test.ts Show resolved Hide resolved

trevor-scheer reviewed Dec 2, 2020

View reviewed changes

federation-js/src/composition/validate/sdl/uniqueTypeNamesWithFields.ts Outdated Show resolved Hide resolved

patrick91 force-pushed the issue-with-same-name-arg-field branch from f3656ee to f0841d4 Compare December 3, 2020 11:36

trevor-scheer approved these changes Dec 3, 2020

View reviewed changes

patrick91 mentioned this pull request Dec 3, 2020

Improve error messages for value types mismatches #296

Open

Fix check for value types when having fields and arguments with the s…

c9356f4

…ame name

patrick91 changed the title ~~Fix check for identical types~~ Fix check for value types when having fields and arguments with the same name Dec 3, 2020

patrick91 force-pushed the issue-with-same-name-arg-field branch from da1cad3 to c9356f4 Compare December 3, 2020 21:05

trevor-scheer changed the title ~~Fix check for value types when having fields and arguments with the same name~~ fix(federation): differentiate field and input value names Dec 3, 2020

trevor-scheer merged commit 0bf5dce into apollographql:main Dec 3, 2020

patrick91 deleted the issue-with-same-name-arg-field branch December 4, 2020 23:35

dependabot bot mentioned this pull request Mar 15, 2021

Bump @apollo/federation from 0.20.2 to 0.22.0 piersroberts/strapi#14

Closed

trevor-scheer mentioned this pull request Nov 2, 2021

Value type with similar field argument signatures breaks composition #1100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(federation): differentiate field and input value names #280

fix(federation): differentiate field and input value names #280

patrick91 commented Nov 15, 2020 •

edited

Loading

apollo-cla commented Nov 15, 2020

trevor-scheer commented Nov 19, 2020

patrick91 commented Nov 19, 2020

trevor-scheer commented Nov 20, 2020 •

edited

Loading

patrick91 commented Nov 22, 2020

trevor-scheer commented Nov 23, 2020 •

edited

Loading

patrick91 commented Nov 29, 2020

trevor-scheer left a comment

patrick91 Dec 1, 2020

trevor-scheer Dec 2, 2020

trevor-scheer Dec 2, 2020

patrick91 Dec 2, 2020

trevor-scheer Dec 2, 2020

patrick91 Dec 3, 2020

trevor-scheer Dec 3, 2020

patrick91 Dec 3, 2020

trevor-scheer left a comment

trevor-scheer Dec 3, 2020

patrick91 Dec 3, 2020

trevor-scheer Dec 3, 2020

patrick91 commented Dec 3, 2020

trevor-scheer commented Dec 3, 2020

trevor-scheer commented Dec 3, 2020

trevor-scheer commented Dec 4, 2020


		const fieldsDiff = Object.entries(fields);
		const fieldsDiff = Object.entries(fields).concat(Object.entries(inputValues));

fix(federation): differentiate field and input value names #280

fix(federation): differentiate field and input value names #280

Conversation

patrick91 commented Nov 15, 2020 • edited Loading

apollo-cla commented Nov 15, 2020

trevor-scheer commented Nov 19, 2020

patrick91 commented Nov 19, 2020

trevor-scheer commented Nov 20, 2020 • edited Loading

patrick91 commented Nov 22, 2020

trevor-scheer commented Nov 23, 2020 • edited Loading

patrick91 commented Nov 29, 2020

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick91 commented Dec 3, 2020

trevor-scheer commented Dec 3, 2020

trevor-scheer commented Dec 3, 2020

trevor-scheer commented Dec 4, 2020

patrick91 commented Nov 15, 2020 •

edited

Loading

trevor-scheer commented Nov 20, 2020 •

edited

Loading

trevor-scheer commented Nov 23, 2020 •

edited

Loading