Skip to content

Fix/explicit requires with mutli entity resolver #3744

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jun 24, 2025
Merged

Fix/explicit requires with mutli entity resolver #3744

merged 17 commits into from
Jun 24, 2025

Conversation

parkerroan
Copy link
Contributor

@parkerroan parkerroan commented Jun 20, 2025 •
edited
Loading

This is a PR addressing this issue: #3742

These changes ensure that explicit_requires is respected and used even when @entityResolver(multi: true).

Prior to this schema generation would fail if you required fields from an field of type [object]. Also the Populate functions for explicit_requires would not be respected when @entityResolver(multi: true).

Example of failed generation schema:

type Product @key(fields: "id") @entityResolver(multi: true)  {
  id: ID!
  someField: [SomeItem!]
  price: Float! @requires(fields: "someField {key value}") @goField(forceResolver: true)
}

type SomeItem {
  key: String!
  value: String!
}

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

parkerroan
parkerroan commented Jun 20, 2025
View reviewed changes
@StevenACoffman StevenACoffman added the federation Related to Apollo federation label Jun 21, 2025
@StevenACoffman
Copy link
Collaborator

Thanks for working on this!

@coveralls
Copy link

coveralls commented Jun 24, 2025 •
edited
Loading

Coverage Status

coverage: 73.153% (+0.2%) from 72.991%
when pulling 27b3f9b on parkerroan:fix/explicit-requires-with-mutli-entity-resolver
into 3654b05 on 99designs:master.

@parkerroan parkerroan marked this pull request as ready for review June 24, 2025 15:33
@parkerroan
Copy link
Contributor Author

parkerroan commented Jun 24, 2025 •
edited
Loading

Thanks for working on this!

I decided to limit these changes to explicit requires. Computed requires will need a similar fix from what I can tell, but I am less familiar with it.

@clayne11 - tagging you as you seem to have introduced the computed_requires changes. From what I can tell, those changes will also fail generation when @entityResolver(multi: true) is set. I was not aware the portion I was working on was deprecated.

parkerroan
parkerroan commented Jun 24, 2025
View reviewed changes
plugin/federation/testdata/explicitrequires/schema.graphql
size: Int! @requires(fields: "world{ foo }")
sizes: [Int!] @requires(fields: "worlds{ foo }")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have failed generation in previous versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayne11 this is the bug I was referencing. It will fail generation for both options, explicit and computed when mutli is enabled.

I think I addressed both issues in this PR, the computed fix was incredibly simple.

@clayne11
Copy link
Contributor

clayne11 commented Jun 24, 2025 •
edited
Loading

As far as I know, @entityResolver(multi: true) does work with computed_requires — that was one of the issues that I addressed with the new design.

My opinion is that explicit_requires is fundamentally broken from a design perspective and is deprecated. I would encourage you to move away from it if you can.

@parkerroan
Copy link
Contributor Author

parkerroan commented Jun 24, 2025 •
edited
Loading

As far as I know, @entityResolver(multi: true) does work with computed_requires — that was one of the issues that I addressed with the new design.

My opinion is that explicit_requires is fundamentally broken from a design perspective and is deprecated. I would encourage you to move away from it if you can.

@clayne11 If you take a look at the old code it was still generating explicit mapping unlike resolveEntity which was skipping it. Also generation would fail on the bug I have listed in the PR using computed_requires also. I just pushed up a fix I would love for you to take a look?

parkerroan
parkerroan commented Jun 24, 2025
View reviewed changes
plugin/federation/federation_computedrequires_test.go
}),
&resp,
client.Var("representations", representations),
)

require.NoError(t, err)
require.Equal(t, "first name - 1", resp.Entities[0].Name)
require.Equal(t, "key1 - 1", resp.Entities[0].Key1)
Copy link
Contributor Author

@parkerroan parkerroan Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Computed requires should not populate external entity fields like this as far as I know, as they are now passed into the resolver. This would fail if not for Multi true, which still has the explicit mapping in its path.

@StevenACoffman
Copy link
Collaborator

I really appreciate your engaging with this. There haven't been enough folks carefully going over federation recently and the more eyes on this the better. I'm going to merge this as is for now, but I would love for further follow-up PRs.

@StevenACoffman StevenACoffman merged commit 74ff0f8 into 99designs:master Jun 24, 2025
18 checks passed
@StevenACoffman
Copy link
Collaborator

@parkerroan I tried to connect with you on https://www.linkedin.com/in/steve-coffman-79322175/ because it might be easier to talk over a few things in a quick chat, if you are up for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
federation Related to Apollo federation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@parkerroan @StevenACoffman @coveralls @clayne11