Fix detached-instance crash on GraphQL dimensionLinks.foreignKeys#2034
Merged
shangyian merged 4 commits intoDataJunction:mainfrom Apr 20, 2026
Merged
Fix detached-instance crash on GraphQL dimensionLinks.foreignKeys#2034shangyian merged 4 commits intoDataJunction:mainfrom
dimensionLinks.foreignKeys#2034shangyian merged 4 commits intoDataJunction:mainfrom
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
shangyian
changed the title
dimensionLinks.foreignKeys
…rry objects rather than ORM objects
shangyian
force-pushed
the
fix-gql-find-nodes
branch
from
6da63ad to
fbd625d
Compare
jlhester
reviewed
Apr 20, 2026
| default_value=link.default_value, | ||
| ), | ||
| ) | ||
| return links |
Collaborator
There was a problem hiding this comment.
Should self.dimension_links be updated to be links at the end of the method here? What's the point of completely rebuilding each dimension link object?
Collaborator
Author
There was a problem hiding this comment.
I ended up completely simplifying this ... the only thing that's needed is the pre-seeding of the node revision relationship
jlhester
reviewed
Apr 20, 2026
| continue # hard-deleted or deactivated dimension node | ||
| set_committed_value(link, "node_revision", self) | ||
| links.append( | ||
| DimensionLink( # type: ignore[call-arg] |
Collaborator
There was a problem hiding this comment.
This could be fragile if DimensionLink is ever updated to include more fields
Collaborator
Author
There was a problem hiding this comment.
Ah yes good point... this is actually unnecessary now that we're returning a DimensionLink ORM obj with node_revision pre-seeded so foreign_keys can resolve without a lazy load.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Querying
findNodes { current { dimensionLinks { foreignKeys } } }was failing with a SQLAlchemy detached-instance error introduced by the per-resolver-session refactor (#2028). Thedimension_linksresolver was returning raw ORMDimensionLinkobjects and by the time Strawberry resolved the foreignKeys field, the session that loaded those objects had closed, so the lazy load offoreign_keysblew up.The fix is for the
dimension_linksresolver to turn each link into a StrawberryDimensionLinkobject, including foreign_keys, so that every attribute is realized while the parent resolver's session is still open and nothing lazy crosses the session boundary.Other resolvers in scalars/node.py that walk nested lazy relationships (e.g., availability.partitions, materializations.backfills, metric_metadata.unit) should be audited for similar errors. The general rule is that any property reaching through a non-eager-loaded relationship is now an issue under the session-per-resolver pattern.
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan