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

Basic support for virtual fields using hydration #600

Merged
merged 18 commits into from
Oct 31, 2024
Merged

Conversation

gnawf
Copy link
Collaborator

@gnawf gnawf commented Oct 21, 2024

So the phrase virtual fields here refers to a @hydrated field at the top level that doesn't use any $source arguments.

Though in a new branch I've repurposed it and renamed hydratedField etc. to virtual field and actor field to backing field.

@gnawf gnawf requested a review from felipe-gdr October 29, 2024 21:36
sourceNode: JsonNode,
sourceIds: List<List<Any?>>,
resultNodesByObjectId: Map<List<Any?>, JsonMap>,
): NadelResultInstruction {
val hydratedFieldDef = NadelTransformUtil.getOverallFieldDef(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is redundant, it looks up the field definition using the __typename in the result, but that's already been done because we've resolved an instruction at the call site.

This change just passes in the instruction and looks an the field definition in the instruction.

Likely this was a mistake due to copy paste during refactoring.

"VirtualEcho" to "BackingEcho",
"VirtualEchoInfo" to "BackingEchoInfo",
),
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question -

So when we make a virtual type - we say that all fields in other types get made "virtual" as well - eg the shape of virtual type is actually the backing type shapes

What happens if the types reference themselves

 type VirtualEcho {
                    echo: String
                    info: VirtualEchoInfo
                    selfReference : VirtualEcho.  # what happens here
                }

Copy link
Collaborator

Choose a reason for hiding this comment

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

will it go into a infinite loop and if not can we have a test for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's code to protect against that, I've created a test for that though.

}
]
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this code have a "get all other parameters" and stuff them into a map code yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, coming in another PR.

Branch is https://github.com/atlassian-labs/nadel/tree/remaining-arguments

bbakerman
bbakerman previously approved these changes Oct 30, 2024
@gnawf gnawf merged commit f148c32 into master Oct 31, 2024
2 checks passed
@gnawf gnawf deleted the virtual-fields branch October 31, 2024 02:16
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