fix: cache plan eagerly to support self-referential responses#32
Open
hodfords-quang-th-backend wants to merge 1 commit into
Open
Conversation
Cache the in-progress plan before recursing into nested classes so that self-referential response types (e.g. ChatMessageResponse with a referencedMessage of the same type) resolve to a stable, shared plan instead of being dropped as an empty placeholder. Previously, getOrBuildPlan tracked the current target with a visiting set and returned an empty plan when a cycle was detected. The empty plan was then filtered out in collectNestedPlans, so transforms on the self-referenced field were never applied. Now the plan object is inserted into planCache before its children are walked. Recursive lookups receive the same object reference and any property transforms attached to the cycle are honored. The cycle in the plan mirrors the cycle in the data, and runPlan terminates naturally when the data graph terminates. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
hodfords-quang-th-backend
force-pushed
the
fix/plan-cache-cycle-handling
branch
from
a2ed446 to
94e731a
Compare
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
1 participant
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
planCachebefore recursing into nested classes so that self-referential response types resolve to a stable, shared plan instead of being dropped as empty placeholders.visitingset / empty-plan short-circuit ingetOrBuildPlan, and theif (nestedPlan.propertyTransforms.length > 0 || nestedPlan.nestedPlans.length > 0)filter incollectNestedPlansthat was filtering out the cycle.Why
Caught in
employer-backendchat tests after upgrading to@hodfords/nestjs-response@11.1.0. AChatMessageResponsewhosereferencedMessageis itself aChatMessageResponsewould lose its property transforms — the recursive call returned{ propertyTransforms: [], nestedPlans: [] }, then the empty plan was filtered out, so transforms attached to the referenced message were silently dropped (7 e2e failures).After this fix, the plan inserted before recursion is the same reference returned to the inner call. As subsequent loop iterations mutate
plan.propertyTransformsandplan.nestedPlans, the inner reference sees the updates. The cycle in the plan structure mirrors the cycle in the data;runPlanterminates naturally when the data graph terminates.Test plan
npm run build— clean tsc build, generated JS is byte-identical to the patched JS we are running in production.employer-backendchat e2e: 7 previously-failingreferencedMessageassertions now pass with the patched JS this PR produces.🤖 Generated with Claude Code