Repository navigation
fix(diffserver): bracket map keys that contain a period in field paths - #802
sarankumar1325 wants to merge 1 commit into
Conversation
The plan paths joined map keys with a plain ".", so a key such as the AWS tag "kubernetes.io/cluster/x" came back as spec.forProvider.tags.kubernetes.io/cluster/x, which fieldpath.Parse reads as three nested fields. Write such keys in crossplane-runtime's bracket form (tags[kubernetes.io/cluster/x]) in both the SDKv2 and the plugin framework path builders; other keys keep the .key form. Fixes crossplane#801 Signed-off-by: sarankumar1325 <120080287+sarankumar1325@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared map-key formatting pkg/diffserver/internal/plan/response.go |
mapKeySegment renders map keys as bracketed or dotted path segments based on their serialized form. |
Builder integration and tests pkg/diffserver/internal/plan/tfpluginfw.go, pkg/diffserver/internal/plan/tfpluginsdk.go, pkg/diffserver/internal/plan/tfpluginfw_test.go, pkg/diffserver/internal/plan/tfpluginsdk_test.go |
Both path builders use mapKeySegment. Tests check that dotted keys appear as one bracketed component. The SDKv2 test also checks the old and planned values and desired-state origin. |
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Medium
Suggested reviewers: sergenyalcin, ulucinar
Merge Risk: 
ae780
-
A literal
*map key can produce a wildcard field path instead of identifying only that entry; this is a narrow edge case suitable for follow-up.
Pre-merge checks: 
-
Check name Status Explanation Title check The title names the fix (bracketing map keys that contain a period) and the area (diffserver field paths), which matches the changeset. It is 70 characters, under the 72-character limit, so it meets t… Description check The description explains the bug (map keys with periods such as kubernetes.io/cluster/x are parsed as nested fields), names the helper and both path builders changed, lists the new test cases, and sta… Linked Issues check Issue #801requires correct FieldChange paths for map keys containing periods in both path builders.mapKeySegmentusesfieldpath.Segments.String()and emits bracket syntax when required. `fieldPa…Out of Scope Changes check The diff changes only the shared map-key formatter, the two affected path builders, and focused tests. These changes directly support issue #801. No unrelated production behavior or unrelated tests ar…Configuration Api Breaking Changes The pull request changes only files under pkg/diffserver/internal/plan. The authoritative diff contains no changes under pkg/config, so it does not remove, rename, or alter any configuration API. Generated Code Manual Edits PASS. The pull request changes five files under pkg/diffserver/internal/plan, and none has a basename matching zz_*.go. Therefore, the explicit generated-code manual-edit failure condition is not met. Template Breaking Changes PASS: The pull request changes only files under pkg/diffserver/internal/plan. It makes no changes to pkg/controller/external*.go templates, so it cannot introduce the template-generated controller beh… Full details: Title check
-
Explanation
The title names the fix (bracketing map keys that contain a period) and the area (diffserver field paths), which matches the changeset. It is 70 characters, under the 72-character limit, so it meets the stated requirement.
Full details: Description check
-
Explanation
The description explains the bug (map keys with periods such as kubernetes.io/cluster/x are parsed as nested fields), names the helper and both path builders changed, lists the new test cases, and states the out-of-scope keys. It is related to the changeset and gives substantial detail.
Full details: Linked Issues check
-
Explanation
Issue
#801requires correct FieldChange paths for map keys containing periods in both path builders.mapKeySegmentusesfieldpath.Segments.String()and emits bracket syntax when required.fieldPathapplies it to SDKv2stepMapKeyvalues, andframeworkFieldPathapplies it to plugin-framework string keys. The added tests coverkubernetes.io/cluster/xin both builders and expecttags[kubernetes.io/cluster/x].
Full details: Out of Scope Changes check
-
Explanation
The diff changes only the shared map-key formatter, the two affected path builders, and focused tests. These changes directly support issue
#801. No unrelated production behavior or unrelated tests are present.
Full details: Template Breaking Changes
-
Explanation
PASS: The pull request changes only files under pkg/diffserver/internal/plan. It makes no changes to pkg/controller/external*.go templates, so it cannot introduce the template-generated controller behavior covered by this check.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
blacksoxx
left a comment
blacksoxx
left a comment
There was a problem hiding this comment.
Reads right to me. mapKeySegment spells a key exactly the way fieldpath.Segments.String() does (brackets only for a period or *, .key otherwise), so the two builders now produce paths that fieldpath.Parse reads back as one map key, and every path without such a key is byte-for-byte what it was. The Team_Name case next to the new DottedMapKey one makes that visible. The limits you list (all-digit keys, keys containing brackets) match what the parser itself cannot express, so leaving them alone is the honest choice. Read against head ae7800e on main, not run locally.
I would approve this; the decision is with the maintainers.
Description of your changes
The diff server's plan paths joined map keys with a plain
.. A key that itself contains a period, such as the AWS tagkubernetes.io/cluster/x, came back asspec.forProvider.tags.kubernetes.io/cluster/x, andfieldpath.Parsereads that as three nested fields.Both path builders in
pkg/diffserver/internal/plannow write map keys the way crossplane-runtime'sfieldpath.Segments.String()does. The work is done in a smallmapKeySegmenthelper:fieldPath, for thestepMapKeystep.frameworkFieldPath, fortftypes.ElementKeyString.A key with a period (or
*) is written in brackets,tags[kubernetes.io/cluster/x]. Every other key keeps the current.keyform, so ordinary paths don't change.What this PR doesn't cover: keys that are all digits, or that contain
[/].fieldpathhas no unambiguous spelling for those, so they are left as they are.Fixes #801
I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
DottedMapKeyBracketedinTestPlanResponseOrigin(SDKv2) andDottedMapKeyinTestFrameworkFieldPath(plugin framework). Both fail on main and pass with this change.go test ./pkg/diffserver/...passes.golangci-lint run ./pkg/diffserver/...(v2, repo config) reports 0 issues.make reviewable; that's why its box is unchecked.This change was prepared with AI assistance and reviewed before submission.