Skip to content

fix: reject circular aliases before expansion - #2931

Open
Dcastroro wants to merge 1 commit into
mikefarah:masterfrom
Dcastroro:fix/reject-circular-alias-expansion-2167
Open

Dcastroro wants to merge 1 commit into
mikefarah:masterfrom
Dcastroro:fix/reject-circular-alias-expansion-2167

Conversation

@Dcastroro

Copy link
Copy Markdown

AI disclosure: This contribution and description were prepared by OpenAI Codex, an AI agent acting on behalf of @Dcastroro, not by the user personally.

Fixes #2167.

Converting &self [*self] to JSON currently succeeds with [[[]]], silently losing the circular reference. Check the selected nodes for alias cycles before explode mutates them, and return cannot explode circular alias reference when expansion cannot produce a finite tree.

The check distinguishes nodes on the current traversal path from completed nodes, so repeated references to the same acyclic value remain valid. YAML output still preserves circular aliases; selecting an acyclic value from a cyclic document still works. No dependency, workflow, or security-check configuration changes are included.

Validation on Linux with Go 1.26.9, based on a814ff5166040ec745a96bddd79ee31c3a548e83:

  • The new sequence-cycle regression fails on the base: expected an error, received [[[]]].
  • go test ./pkg/yqlib -run 'TestJSONAliasCycles|TestAnchorAliasOperator' -count=1: passes, including both merge-anchor modes.
  • Seven CLI checks pass: sequence, mapping, indirect and merge cycles exit 1 without output; repeated aliases, YAML round-tripping and scalar selection succeed.
  • Formatting, spelling, gosec (0 issues), and golangci-lint (0 issues) pass. All 175 acceptance tests across 17 suites pass, as does TestGoInstallCompatibility.
  • Full-suite limitation: go test ./... has the same single failing test, TestJSONScenarios, on the untouched base and on this branch. Its existing malformed-JSON case expects json: string of object unexpected end of JSON input, but the current JSON dependency returns invalid character 'b' after object key:value pair. This change leaves that unrelated assertion intact. make local build returns success despite that unit-test failure because scripts/test.sh subsequently runs the install-compatibility test, so the full suite is not being reported as green.

The issue timeline had no assignee or work claim when checked, and searches of open and closed PRs for the issue number, circular references and alias expansion found no competing fix.

Copilot AI balanced review requested due to automatic review settings October 19, 2026 10:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

It changes user-facing behavior and exit codes of the core explode path for all non-YAML encoders, so a maintainer should confirm the error-on-cycle semantics and message wording.

No open issues 🎉

What changed in this PR

This PR fixes #2167, where converting a self-referential YAML structure (e.g. &self [*self]) to JSON silently produced incorrect output ([[[]]]) with exit code 0 instead of erroring. The fix adds a cycle check in explodeOperator that runs before explodeNode mutates nodes (which would otherwise hide the cycle), returning cannot explode circular alias reference when a finite expansion is impossible. Because non-YAML encoders explode aliases at print time (printer.go runs explode when CanHandleAliases() is false), this surfaces as an error for JSON/other formats while YAML output (which preserves aliases) is unaffected.

The new checkForAliasCycles is a standard depth-first grey/black cycle detector: visited[node]=false marks a node on the current traversal path and true marks a completed node, so a back-edge to an in-progress node is a true cycle, while repeated references to already-completed acyclic values are allowed. It follows both AliasNode.Alias and Content, covering sequence, mapping, indirect, and merge-anchor cycles. I verified it does not produce false positives for repeated aliases or diamond (shared-subtree) structures, and only inspects the selected nodes so acyclic selections from cyclic documents still succeed.

Changes:

  • Add a pre-explode cycle guard (checkForAliasCycles) in explodeOperator that errors on circular alias references.
  • Add expressionScenario tests covering sequence/merge cycles, repeated (non-cyclic) aliases, and YAML passthrough.
  • Add TestJSONAliasCycles covering sequence/mapping/indirect cycles, repeated aliases, and acyclic selection from a cyclic document.
File Description
pkg/​yqlib/​operator_anchors_aliases.go Adds checkForAliasCycles and invokes it in explodeOperator before explodeNode mutates nodes.
pkg/​yqlib/​operator_anchors_aliases_test.go Adds scenarios for cycle rejection (sequence + merge) and YAML alias preservation.
pkg/​yqlib/​json_test.go Adds TestJSONAliasCycles validating JSON-output cycle errors and acyclic cases.

Review effort: Balanced

This branch has not been deployed

No deployments
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.

Data structures not representable in output format should result in an error.

2 participants