Skip to content

Fix setup export policy name collisions - #5216

Open
mattfaltyn wants to merge 1 commit into
apache:mainfrom
mattfaltyn:fix-5215-setup-policy-collision
Open

Fix setup export policy name collisions#5216
mattfaltyn wants to merge 1 commit into
apache:mainfrom
mattfaltyn:fix-5215-setup-policy-collision

Conversation

@mattfaltyn

@mattfaltyn mattfaltyn commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • export policies as a list with explicit name and namespace fields so namespace-scoped identities cannot collide
  • keep setup apply compatible with existing policy mappings keyed by name
  • cover an export/YAML/apply round trip with duplicate policy names in different namespaces
  • document the exported representation and record the user-visible fix in the changelog

Rationale

Policy names are unique only within a namespace. The previous catalog-wide mapping used policy name as its sole key, so a later namespace silently replaced an earlier same-named policy and made setup backups incomplete.

Fixes #5215

Validation

  • client/python/.venv/bin/pytest client/python/tests/test_setup_command.py -q (18 passed, 3 subtests passed)
  • .venv/bin/pytest tests/ from client/python (171 passed)
  • repository client pre-commit hooks: trailing whitespace, EOF, debug statements, Ruff check/format, and mypy (all passed)
  • ./gradlew format compileAll (BUILD SUCCESSFUL, 992 tasks)
  • uv build --clear from client/python (source distribution and wheel built successfully)
  • ./gradlew rat --rerun-tasks (BUILD SUCCESSFUL)
  • complete GitHub PR CI: required Gradle, runtime, integration, regression, Python 3.10-3.14, image, Helm, docs, and security checks (all passed)

AI disclosure

This change was developed with Codex assistance. I reviewed the implementation, tests, documentation, and generated diff and take responsibility for the contribution.

Checklist

  • 🛡️ This is a public, non-security correctness fix; no security issue is disclosed
  • 🔗 The need is explained above and linked with Fixes #5215
  • 🧪 Added a regression test covering the failing export/apply round trip
  • 💡 The normalization is direct and does not require additional explanatory comments
  • 🧾 Updated CHANGELOG.md
  • 📚 Updated documentation in site/content/in-dev/unreleased

@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Aug 1, 2026
@mattfaltyn
mattfaltyn marked this pull request as ready for review August 1, 2026 19:58

@MonkeyCanCode MonkeyCanCode 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.

Thanks for the fix. Somehow I was under impression policy name is global unique when I added this one. But looking at the code, the parent can be a catalog or a namespace which means policy name doesn't need to be global unique as resolving is based on the resolvedParent.

I think there are couple things which I think we should also fix:

  1. policy can be catalog level. For the export to include all, we would need to include case where namespace is "" (we should update command-line-interface.md as well).
  2. We should update reference-setup-config.yaml to reflect this change as well

Another thing is if we have something likes following:

      compaction-policy:
        namespace: dev_namespace
        ...
        content: {
             ...
        }
      compaction-policy:
        namespace: dev_namespace2
        ...
        content: {
             ...
          }

The current code would still only loaded the second one only.

The backward-compatible path via Union[Dict, List] only help us here when we don't have overlap key name for policy. Maybe worth bring a breaking change here or update doc to mention about this limitation when using the dict route?

@flyrain

@flyrain

flyrain commented Aug 3, 2026

Copy link
Copy Markdown
Contributor
  • policy can be catalog level. For the export to include all, we would need to include case where namespace is "" (we should update command-line-interface.md as well).

You can assign a policy to a catalog, but a policy is always hosted within a namespace,

/polaris/v1/{prefix}/namespaces/{namespace}/policies:
.

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.

Python CLI setup export drops same-named policies across namespaces

3 participants