Skip to content

Service: Refactor on grant operation#4059

Merged
flyrain merged 2 commits intoapache:mainfrom
flyrain:grant-op-refactor
Mar 26, 2026
Merged

Service: Refactor on grant operation#4059
flyrain merged 2 commits intoapache:mainfrom
flyrain:grant-op-refactor

Conversation

@flyrain
Copy link
Copy Markdown
Contributor

@flyrain flyrain commented Mar 25, 2026

Refactor on the grant operation to reduce code duplication.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

@flyrain flyrain closed this Mar 25, 2026
@github-project-automation github-project-automation bot moved this from PRs In Progress to Done in Basic Kanban Board Mar 25, 2026
@flyrain flyrain reopened this Mar 25, 2026
@github-project-automation github-project-automation bot moved this from Done to PRs In Progress in Basic Kanban Board Mar 25, 2026
Comment on lines +635 to +639
direction == GrantDirection.GRANT
? adminService.grantPrivilegeOnViewToRole(
catalogName, catalogRoleName, identifier, privilege)
: adminService.revokePrivilegeOnViewFromRole(
catalogName, catalogRoleName, identifier, privilege);
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.

nit: pls consider adding helper methods to the enum then : direction.applyToView(adminService, catalogName, catalogRoleName, identifier, privilege). This would be more readable, IMHO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tried locally. It turns out removing 20 lines of code by adding 80 LOC. Net negative. Not sure if it's worth to pursue, but I'm open to suggestion

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.

Interesting 🤔 No worries, let's skip that.

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 25, 2026
Copy link
Copy Markdown
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

LGTM.

The logic is the same, and the code is cleaner now.

Thanks @flyrain !

@flyrain flyrain merged commit 6fe509f into apache:main Mar 26, 2026
50 of 54 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Mar 26, 2026
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.

3 participants