Skip to content

Add transform that adds the @key directives to our Node types#214

Open
alf wants to merge 1 commit intomainfrom
addKeyDirectives
Open

Add transform that adds the @key directives to our Node types#214
alf wants to merge 1 commit intomainfrom
addKeyDirectives

Conversation

@alf
Copy link
Copy Markdown
Collaborator

@alf alf commented Oct 21, 2025

Kort forklart trenger vi at sis-subgrafen setter @key-direktivene på alle nodene for at node-subgrafen skal kunne fungere. Jeg mener å huske at du allerede har skrevet koden som skal til for å generere entity resolvere for ting som har @key-direktivet. Må vi gjøre noe mer her for at dette skal fungere sammen?

@alf alf requested a review from Erkelinux October 21, 2025 20:20
@Erkelinux
Copy link
Copy Markdown
Collaborator

Erkelinux commented Oct 23, 2025

Jeg ser bare en hindring her, og det er at Graphitron mangler den lille biten som ekskluderer eksterne felt i en subgraf, hvor de skal federeres inn fra andre grafer. Det burde være enklet å fikse siden det er bare å sjekke for et direktiv.

Det eneste jeg saver i denne PR-en er en eksplisitt sjekk for om Node faktisk finnes, for nå tror jeg det caset er udefinert.

@alf alf force-pushed the addKeyDirectives branch from 10398a3 to 3860962 Compare October 23, 2025 17:16
@alf
Copy link
Copy Markdown
Collaborator Author

alf commented Oct 23, 2025

Jeg ser bare en hindring her, og det er at Graphitron mangler den lille biten som ekskluderer eksterne felt i en subgraf, hvor de skal federeres inn fra andre grafer. Det burde være enklet å fikse siden det er bare å sjekke for et direktiv.

Finnes det en oppgave på denne?

@Erkelinux
Copy link
Copy Markdown
Collaborator

Jeg ser bare en hindring her, og det er at Graphitron mangler den lille biten som ekskluderer eksterne felt i en subgraf, hvor de skal federeres inn fra andre grafer. Det burde være enklet å fikse siden det er bare å sjekke for et direktiv.

Finnes det en oppgave på denne?

Jeg var sikker på at det var det, men finner ikke en. Kan lage en ny og slette den gamle hvis den dukker opp.

return schema.transform(builder -> builder.clearAdditionalTypes().additionalTypes(reachableTypes));
}

private Set<GraphQLType> findReachableTypes(GraphQLSchema schema) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Kan være vi vil skille på ting som blir unreachable som følge av transform i motsetning til ting som var unreachable fra før, men usikker på om det bør løses her.

@alf alf marked this pull request as ready for review October 28, 2025 11:36
@andreahn
Copy link
Copy Markdown
Collaborator

andreahn commented Feb 3, 2026

Den her begynner å bli veldig gammel. Skal det skje noe mer her eller kan vi lukke den?

@alf
Copy link
Copy Markdown
Collaborator Author

alf commented Mar 12, 2026

Jeg venter vel i grunn på at den skal kunne merges. Det krever en approval. Kan rebase siden den er så gammel, kanskje det hjelper?

Before this we were relying on not visiting all nodes for pruning the schema, I
added some extra code to remove unreachable types. This uncovered a bug in our
existing test, so I fixed those tests as well.

During my changes here I triggered some strage behaviour related to running the
Federation transforms multiple times when `removeFederationDefinitions` is
false. Because of this I made the code skip this step if the _Entity union type
already exists.
@alf alf force-pushed the addKeyDirectives branch from 61f3908 to 60f4a56 Compare March 12, 2026 19:53
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

Successfully merging this pull request may close these issues.

3 participants