Conversation
…ter/attachments into fix/multi-level-draft-support
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix Cyclic Structure Bug in Draft Save Handler for Deeply Nested AttachmentsBug Fix🐛 Resolves a crash caused by cyclic entity structures when processing deeply nested attachment compositions during draft save. The bug originated in Changes
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces buildBackAssocChain and a new deeply-nested attachment test, but has several correctness issues: the computed backAssocChain is never actually used in the WHERE clause (it's a dead expression), the hardcoded .columns() expansion with up_ and replyTo will break for non-cyclic/shallow attachment entities, the draft content response in the test is fetched but never asserted, and the early-return path in buildBackAssocChain silently returns a partial chain that callers cannot distinguish from a valid result. These issues should be addressed before merging.
PR Bot Information
Version: 1.18.4 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
29443710-1e1c-11f1-9c1c-1abb4807201e - Event Trigger:
pull_request.opened
| const backAssocChain = buildBackAssocChain(rootEntity, compositionPath) | ||
|
|
||
| return async (_, req) => { | ||
| backAssocChain |
There was a problem hiding this comment.
Bug: backAssocChain is referenced as a standalone expression statement with no effect
Line 163 evaluates backAssocChain as a bare expression — this is a no-op. The variable is computed but never used in the query. The original commented-out .where(...) block (lines 173–178) was the intended use, and it has been replaced by a .columns(...) expansion that hardcodes up_ and replyTo associations. This means the WHERE clause that scopes draft attachments to the current root entity's draft is entirely missing, so the handler may load all draft attachments across all root drafts instead of only those belonging to the current save request.
Should either restore the .where(...) using backAssocChain, or remove the dead backAssocChain reference.
| backAssocChain | |
| // backAssocChain is computed above for future use in .where() filtering |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| .columns((a) => { | ||
| ;(a`.*`, | ||
| a.up_((u) => { | ||
| ;(u`.*`, u.replyTo(`*`)) | ||
| })) | ||
| }) |
There was a problem hiding this comment.
Bug: Hardcoded .columns() expansion assumes up_ and replyTo associations exist on every attachment entity
The new .columns() block unconditionally navigates up_ and then replyTo on every attachment entity regardless of the actual schema. For shallow (depth-1) compositions like Incidents.attachments or Test.attachments, there is no replyTo association, so this expansion will either silently return null columns or cause a runtime error. The old code used a simple .where() predicate scoped to the root key which worked generically across all depths.
Consider making the columns expansion conditional on the actual composition depth / entity shape, or restoring the generic WHERE approach once the backAssocChain filtering is correctly wired up.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const contentResponseDraft = await GET( | ||
| `odata/v4/processor/Comments_attachments(up__ID=${replyID},ID=${attachmentID},IsActiveEntity=false)/content`, | ||
| ) |
There was a problem hiding this comment.
Bug: contentResponseDraft is fetched but its result is never asserted
The response from the draft-mode content GET (line 59–61) is stored in contentResponseDraft but no expect(...) is called on it. If this intermediate download is meant to verify content is readable before activation, the assertion is missing. If it is not needed, the variable and GET call should be removed to avoid confusion.
Consider adding:
expect(contentResponseDraft.status).toEqual(200)
expect(contentResponseDraft.data).toBeTruthy()or removing the call entirely.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| let current = rootEntity | ||
| for (const compName of compositionPath) { | ||
| const comp = current.compositions && current.compositions[compName] | ||
| if (!comp || !comp._target) return chain.reverse() |
There was a problem hiding this comment.
Logic Error: Early return on missing composition skips already-accumulated chain items without reversing correctly
When !comp || !comp._target is encountered mid-path (i.e., chain already has entries), return chain.reverse() returns a partial reversed chain. While reversing is correct, silently returning a partial chain could cause incorrect WHERE predicates to be built from the incomplete result. The caller in draftSaveHandler has no way to distinguish a successfully completed chain from a partially built one due to a missing composition.
Consider returning an empty array [] or throwing/logging a warning so callers can detect that the path could not be fully resolved:
| if (!comp || !comp._target) return chain.reverse() | |
| if (!comp || !comp._target) { | |
| LOG.warn(`buildBackAssocChain: could not resolve composition "${compName}" on "${current.name}"`) | |
| return [] | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
3 participants
Running the one test in tests/integration/attachments.test.js shows a bug due to the cyclic structure. The bug originated in srv/basic.js. Run the test in debug mode to see the error.