Skip to content

fix(core): refactor ListAttributesValues to use Get#769

Merged
alkalescent merged 5 commits intomainfrom
fix/DSPX-2530
Mar 6, 2026
Merged

fix(core): refactor ListAttributesValues to use Get#769
alkalescent merged 5 commits intomainfrom
fix/DSPX-2530

Conversation

@alkalescent
Copy link
Copy Markdown
Contributor

@alkalescent alkalescent commented Mar 6, 2026

This PR refactors policy attributes values list to use get internally.

@alkalescent alkalescent requested a review from a team as a code owner March 6, 2026 16:46
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request focuses on maintaining the relevance of the test suite by removing an end-to-end test case. Specifically, the List attribute values - Good test was removed because the Remote Procedure Call (RPC) it was designed to validate is slated for deprecation. This change ensures that testing efforts are aligned with the current and future state of the API, preventing unnecessary maintenance of tests for soon-to-be-removed functionality.

Highlights

  • Test Removal: Removed the List attribute values - Good end-to-end test from e2e/attributes.bats as the corresponding RPC is being deprecated.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • e2e/attributes.bats
    • Removed the List attribute values - Good test case.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request removes an end-to-end test for the policy attributes values list command, stating that the underlying RPC is being deprecated. However, the command's implementation, documentation, and even its 'bad path' test case remain in the codebase. Removing tests for existing functionality is a concern as it leaves the code untested and can lead to maintenance issues. For a clean deprecation, all parts of the feature—including its implementation, documentation, and all related tests—should be removed together. I've added a comment with a suggestion to either restore the test or proceed with a complete removal of the feature.

I am having trouble creating individual review comments. Click here to see my feedback.

e2e/attributes.bats (435-496)

high

You've removed the 'good path' test for values list, but the 'bad path' test, the command implementation, and its documentation remain. Removing tests for a feature that still exists in the codebase is risky as it leaves the feature untested. If the values list command is being deprecated, it would be better to remove all related code, including all tests and documentation, in a single pull request. If the feature is not being removed at this time, this test should be kept to ensure continued correctness.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

@jakedoublev jakedoublev left a comment

Choose a reason for hiding this comment

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

As discussed, e2e test should still pass with underlying API called by subcommand changed.

@alkalescent alkalescent marked this pull request as draft March 6, 2026 17:41
@alkalescent alkalescent changed the title fix(core): remove ListAttributes test fix(core): refactor policy attributes values list to use get internally Mar 6, 2026
@alkalescent alkalescent changed the title fix(core): refactor policy attributes values list to use get internally fix(core): refactor ListAttributesValues to use Get internally Mar 6, 2026
@alkalescent alkalescent changed the title fix(core): refactor ListAttributesValues to use Get internally fix(core): refactor ListAttributesValues to use Get Mar 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 6, 2026

@alkalescent
Copy link
Copy Markdown
Contributor Author

/gemini review

@alkalescent alkalescent marked this pull request as ready for review March 6, 2026 19:43
@alkalescent alkalescent requested a review from a team as a code owner March 6, 2026 19:44
@alkalescent alkalescent requested a review from Copilot March 6, 2026 19:48
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors ListAttributesValues to fetch all attribute values from an attribute and then perform client-side filtering and pagination. While functional, this change introduces a potential runtime panic in the paginateValues function due to a lack of validation for negative offset values, which could be exploited to crash the CLI tool. It is recommended to add a check to ensure the offset is non-negative. Furthermore, fetching all data at once can lead to high network traffic and memory usage on the client, especially for attributes with many values, and there's an opportunity to reduce code duplication in the new filtering logic for better maintainability.

Comment thread pkg/handlers/attributeValues.go
Comment thread cmd/policy/attributeValues.go
Comment thread cmd/policy/attributeValues.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors policy attributes values list to avoid a dedicated ListAttributeValues SDK call and instead retrieves values via the existing GetAttribute call. Client-side filtering by state and client-side pagination are introduced to replicate the server-side behavior previously delegated to the ListAttributeValues API endpoint. The protocol/go dependency is also bumped from v0.16.0 to v0.17.0.

Changes:

  • pkg/handlers/attributeValues.go: ListAttributeValues now calls GetAttribute and returns attr.GetValues() instead of calling the dedicated list API with pagination/state params.
  • cmd/policy/attributeValues.go: Adds filterValuesByState and paginateValues helper functions to replicate server-side filtering/pagination logic locally; the listAttributeValue command now uses these to produce equivalent output.
  • go.mod / go.sum: Bumps github.com/opentdf/platform/protocol/go from v0.16.0 to v0.17.0.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File Description
pkg/handlers/attributeValues.go Replaces sdk.Attributes.ListAttributeValues call with a GetAttribute + GetValues approach
cmd/policy/attributeValues.go Adds filterValuesByState and paginateValues helpers; wires them into listAttributeValue; constructs ListAttributeValuesResponse for JSON output
go.mod Bumps protocol/go dependency to v0.17.0
go.sum Updates checksums for new protocol/go version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmd/policy/attributeValues.go
Comment thread pkg/handlers/attributeValues.go
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 6, 2026

@alkalescent alkalescent requested a review from jakedoublev March 6, 2026 20:34
@alkalescent alkalescent merged commit a82f7b7 into main Mar 6, 2026
20 checks passed
@alkalescent alkalescent deleted the fix/DSPX-2530 branch March 6, 2026 21:25
alkalescent pushed a commit that referenced this pull request Mar 31, 2026
🤖 I have created a release *beep* *boop*
---


##
[0.30.0](v0.29.0...v0.30.0)
(2026-03-31)


### Features

* **core:** Add optional namespace flag for subject mappings and
condtion sets ([#779](#779))
([9e849c4](9e849c4))
* **core:** add scope support for client creds
([#752](#752))
([9ca9e43](9ca9e43))
* **core:** migrate registered resources
([#772](#772))
([2b49a7d](2b49a7d))
* **core:** optional namespace in actions commands and re-enable
actions/RR tests ([#775](#775))
([29a2eb1](29a2eb1))
* **core:** support namespaced registered resources
([#767](#767))
([4d786b5](4d786b5))


### Bug Fixes

* **ci:** Temporarily skip namespaced-actions impacted BATS cases
([#773](#773))
([633728a](633728a))
* **core:** bump toolchain to go 1.24.13
([#747](#747))
([6804b93](6804b93))
* **core:** disable RR E2E tests
([#768](#768))
([0821b8c](0821b8c))
* **core:** make namespacing registered resources optional
([#785](#785))
([8e6eb31](8e6eb31))
* **core:** refactor `ListAttributesValues` to use `Get`
([#769](#769))
([a82f7b7](a82f7b7))
* **core:** unsafe update result output values order
([#759](#759))
([baeba0f](baeba0f))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
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