Refactor code to extract subgraphs from supergraphs #2655
Conversation
|
| Name | Link |
|---|---|
| 🔨 Latest commit | fb86f7d8e8827016270988396f9a69cdeb2f91d6 |
🦋 Changeset detectedLatest commit: c40fbf6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
pcmanus
force-pushed
the
refactor-subgraph-extraction
branch
from
3c85a43 to
b46272f
Compare
pcmanus
force-pushed
the
refactor-subgraph-extraction
branch
from
b46272f to
fb86f7d
Compare
| .filter((sg) => includeTypeInSubgraph(type, sg.name)) | ||
| .map(sg => sg.schema).forEach(schema => schema.addType(newNamedType(type.kind, type.name))); |
There was a problem hiding this comment.
I know this is previously existing code but this filter/map is pretty trivial and adds 100s of iterations (in the case we're hearing about lately). Fine to leave as is but this looks like an easy win.
| .filter((sg) => includeTypeInSubgraph(type, sg.name)) | |
| .map(sg => sg.schema).forEach(schema => schema.addType(newNamedType(type.kind, type.name))); | |
| .filter((sg) => includeTypeInSubgraph(type, sg.name)) | |
| .map(sg => sg.schema) | |
| .forEach(sg => { | |
| if (!includeTypeInSubgraph(type, sg.name)) return; | |
| sg.schema.addType(newNamedType(type.kind, type.name)); | |
| }); |
There was a problem hiding this comment.
I'm ok changing that. Made it a good'ol for-loop while at it though as it's imo more readable at that point.
I'll note though that this is code that only run for fed1 supergraphs, so only for use that never ever composed with the fed2 composition code, and so not necessary where performance matters much.
| // TODO: Not sure that code is needed anymore (any field necessary to validate an interface will have been marked | ||
| // external)? | ||
|
|
||
| // We now make a pass on every field of every interface and check that all implementers do have that field (even if | ||
| // external). If not (which can happen because, again, the v0.1 spec had no information on where an interface was | ||
| // truly defined, so we've so far added them everywhere with all their fields, but some fields may have been part | ||
| // of an extension and be only in a few subgraphs), we remove the field or the subgraph would be invalid. | ||
| for (const subgraph of subgraphs) { | ||
| for (const itf of subgraph.schema.interfaceTypes()) { | ||
| // We only look at objects because interfaces are handled by this own loop in practice. | ||
| const implementations = itf.possibleRuntimeTypes(); | ||
| for (const field of itf.fields()) { | ||
| if (!implementations.every(implem => implem.field(field.name))) { | ||
| field.remove(); | ||
| } | ||
| } | ||
| // And it may be that the interface wasn't part of the subgraph at all! | ||
| if (!itf.hasFields()) { | ||
| itf.remove(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Maybe doesn't have to be this PR but should we remove this as suggested by the comment? Looks like tests pass without it but that's the extent of my understanding of its importance. Another potentially hundreds/thousands of iterations.
There was a problem hiding this comment.
Good call. When I noticed this, I was doing something unrelated and didn't wanted to risk breaking something, hence the TODO instead of removing it right away. But this patch is a refactor, so it's as good a place as any to make the removal. And double checked that this code indeed is not needed anymore (as the TODO says, the code that adds missing external fields that is called before that will ensure there is no implementation missing a field needed for an interface).
But I'll also point out that I rebased this/will merge this against next because this is a refactor, not a bug fix, so really belong in next (and the other PR that rely on this also really belong in next anyway). Which is all the more reason not to be shy about removing this.
The subgraph extraction code is currently written as a single method that handles all join spec versions. There is however a fair number of differences betwee the version 0.1 of the join spec (the one generated by federation 1) and its successor (generated by federation 2), due to 1) having no ownership in fed2 and 2) the join spec 0.2+ versions tracking types much more systematically. So this commit actually separate the code for extracting each versions, or more precisely, has one method for join 0.1 and one for join 0.2+ (the difference between join 0.2 and 0.3 are a lot more minor). Doing may increase the total LOC technically, but it also makes each version a lot clearer. This also allows to refactor the 0.2+ version a bit to use the fact that those version are more systematic in the metadata they provide, and do things a bit more efficiently.
pcmanus
force-pushed
the
refactor-subgraph-extraction
branch
from
56a8503 to
7d3d699
Compare
pcmanus
force-pushed
the
refactor-subgraph-extraction
branch
from
7d3d699 to
c40fbf6
Compare
The subgraph extraction code is currently written as a single method that handles all join spec versions. There is however a fair number of differences betwee the version 0.1 of the join spec (the one generated by federation 1) and its successor (generated by federation 2), due to 1) having no ownership in fed2 and 2) the join spec 0.2+ versions tracking types much more systematically.
So this commit actually separate the code for extracting each versions, or more precisely, has one method for join 0.1 and one for join 0.2+ (the difference between join 0.2 and 0.3 are a lot more minor). Doing may increase the total LOC technically, but it also makes each version a lot clearer. This also allows to refactor the 0.2+ version a bit to use the fact that those version are more systematic in the metadata they provide, and do things a bit more efficiently.