feat: deprecate selector body field #199
Conversation
Summary of ChangesHello @toddbaert, 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 introduces a significant update to how flag selectors are managed, deprecating the Highlights
Using Gemini Code AssistThe 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
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 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
|
toddbaert
requested review from
aepfli,
alexandraoberaigner,
beeme1mr,
djosephsen and
guidobrei
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request deprecates the selector field in SyncFlagsRequest within the legacy sync.v1 proto definition, favoring a flagd-selector header instead. The change itself is correct. However, this deprecation is not applied consistently. The selector field also exists in FetchAllFlagsRequest in the same file, and in both SyncFlagsRequest and FetchAllFlagsRequest in the current flagd.sync.v1 API definition, but these have not been updated. For API consistency, it's important to apply this deprecation across all relevant messages. I've left a comment with more details.
toddbaert
force-pushed
the
feat/deprecate-selector
branch
from
fad0550 to
37febd6
Compare
Signed-off-by: Todd Baert <[email protected]>
toddbaert
force-pushed
the
feat/deprecate-selector
branch
from
37febd6 to
7e11b77
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request deprecates the selector field in SyncFlagsRequest and FetchAllFlagsRequest messages within the flagd.sync.v1 protobuf definition. The change is in favor of using a Flagd-Selector header, which aligns with the OFREP specification and provides more flexibility for infrastructure-level manipulation. The changes are correct, but I've suggested improving the comments associated with the deprecation to be more informative for API consumers by indicating the replacement mechanism.
| // filtering mechanism. | ||
| // ex:- selector: 'source=database,app=weatherapp' | ||
| string selector = 2; | ||
| string selector = 2 [deprecated = true]; // remember to reserve field number 2 if this is removed; |
There was a problem hiding this comment.
The comment explaining the deprecation could be more helpful to users of this API. It's good practice to mention what should be used as a replacement. The information about reserving the field is a good implementation detail for maintainers, but guiding users is more important for a deprecation message.
| string selector = 2 [deprecated = true]; // remember to reserve field number 2 if this is removed; | |
| string selector = 2 [deprecated = true]; // Deprecated: Use the 'Flagd-Selector' header instead. |
| // filtering mechanism. | ||
| // ex:- selector: 'source=database,app=weatherapp' | ||
| string selector = 2; | ||
| string selector = 2 [deprecated = true]; // remember to reserve field number 2 if this is removed; |
There was a problem hiding this comment.
The comment explaining the deprecation could be more helpful to users of this API. It's good practice to mention what should be used as a replacement. The information about reserving the field is a good implementation detail for maintainers, but guiding users is more important for a deprecation message.
| string selector = 2 [deprecated = true]; // remember to reserve field number 2 if this is removed; | |
| string selector = 2 [deprecated = true]; // Deprecated: Use the 'Flagd-Selector' header instead. |
) ## Problem Currently, flagd services have inconsistent handling of selectors: - **Sync Service**: Uses `selector` field from the request body - **Evaluation Service** (v1 & v2): Uses `Flagd-Selector` header - **OFREP Service**: Uses `Flagd-Selector` header This inconsistency makes the API surface less predictable and can lead to confusion for users. ## Solution This PR normalizes selector handling by updating the sync service to accept the `Flagd-Selector` header as the primary method, aligning it with the evaluation and OFREP services. The implementation maintains full backward compatibility by falling back to the request body selector when the header is not present. ## Changes ### Core Implementation Modified `flagd/pkg/service/flag-sync/handler.go`: - Added `getSelectorExpression()` helper method that checks for `Flagd-Selector` header (via gRPC metadata) first - Falls back to request body `selector` field if header is not present - Logs a deprecation warning when the request body selector is used - Applied the new logic to both `SyncFlags()` and `FetchAllFlags()` methods ### Test Coverage Added 5 comprehensive test cases in `handler_test.go`: - Header-based selector (verifies no deprecation warning) - Request body selector (verifies backward compatibility and deprecation warning) - Header precedence (verifies header takes priority when both are provided) - FetchAllFlags with header selector - FetchAllFlags with request body selector All existing tests continue to pass. ## Migration Path The change is fully backward compatible. Users can migrate at their convenience: **Current (deprecated):** ```go req := &syncv1.FetchAllFlagsRequest{ Selector: "source:my-source", } resp, err := client.FetchAllFlags(ctx, req) ``` **Recommended:** ```go md := metadata.New(map[string]string{ "Flagd-Selector": "source:my-source", }) ctx := metadata.NewOutgoingContext(context.Background(), md) req := &syncv1.FetchAllFlagsRequest{} resp, err := client.FetchAllFlags(ctx, req) ``` ## Benefits - **Consistency**: All flagd services now use the same `Flagd-Selector` header pattern - **Backward Compatibility**: Existing clients continue to work without modification - **Clear Migration Path**: Deprecation warnings guide users to adopt the new approach - **Aligns with Standards**: Using headers/metadata for query/filter parameters is a common gRPC pattern ## Breaking Changes None. The request body selector will be removed in a future major version (e.g., v2.0.0). ## Related Addresses #[issue_number] <!-- START COPILOT CODING AGENT SUFFIX --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>[FEATURE] Normalize selector handling across sync, evaluation, and OFREP services</issue_title> > <issue_description>### Requirements > > ### Problem > Currently, we have inconsistent handling of selectors across different flagd services: > > - **Sync Service** (`flagd/pkg/service/flag-sync/handler.go:34`): Uses `req.GetSelector()` from the request body > - **Evaluation Service**: Uses `flagd-selector` header > - **OFREP Service**: Uses `flagd-selector` header > > This inconsistency can lead to confusion for users and makes the API surface less predictable. > > ### Proposal > Normalize selector handling across all services by: > > 1. **Preferred approach**: Use the `flagd-selector` header as the primary method for all services > 2. **Backward compatibility**: For the sync service, continue to accept `selector` in the request body as a fallback for a deprecation period > > ### Implementation Strategy > 1. Update sync service to check for `flagd-selector` header first > 2. Fall back to request body `selector` field if header is not present > 3. Log deprecation warning when using request body selector > 4. Document the change and migration path > 5. Remove request body selector support in a future major version (e.g., v2.0.0) > > ### Benefits > - Consistent API across all flagd services > - Easier to understand and use for developers > - Aligns with common patterns for query/filter parameters in gRPC metadata > > ### Breaking Changes > None initially (backward compatible), but request body selector would be removed in a future major release. > > ### Additional Context > The sync service currently processes the selector at: > ```go > selectorExpression := req.GetSelector() > selector := store.NewSelector(selectorExpression) > ``` > > This should be updated to prioritize metadata/header values while maintaining backward compatibility.</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> Fixes #1814 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/open-feature/flagd/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. Relates to: open-feature/flagd-schemas#199 --------- Signed-off-by: Simon Schrottner <[email protected]> Signed-off-by: Todd Baert <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: aepfli <[email protected]> Co-authored-by: Simon Schrottner <[email protected]> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Todd Baert <[email protected]>
Deprecates the
selectorfield in favor of a theFlagd-selectorheader. This:-H Flagd-Selectorfor the reasons above)