chore(ADR): Proposal to add flag type #1746
Conversation
Signed-off-by: Andrey <andreyturkov@google.com>
Signed-off-by: Andrey <andreyturkov@google.com>
✅ Deploy Preview for polite-licorice-3db33c ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @andreyturkov, 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 proposal to enhance flagd by adding an optional type field to flag definitions. This aims to resolve existing inconsistencies in type validation between different flag resolution methods, providing a single source of truth for flag types. The change will lead to more predictable and reliable flag evaluations, improving overall system robustness and developer experience.
Highlights
- Problem Addressed: The pull request addresses inconsistent type validation in flagd between its Resolve and ResolveAll API methods, which can lead to unpredictable flag evaluation behavior.
- Proposed Solution: An optional type property will be added to the flag definition (e.g., "boolean", "string", "number", "object") to serve as the authoritative source for type validation, ensuring consistency across evaluation methods.
- Validation and Compatibility: When the type field is present, flagd will validate all flag variants against this type during initialization. The field is optional to maintain backward compatibility, treating untyped flags as Object.
- Benefits: This change is expected to improve the reliability and predictability of flag evaluations, enable early detection of type mismatches, and enhance the developer experience by making the API more explicit.
- Schema Updates: The proposal includes specific JSON schema modifications to schemas/json/flags.json to incorporate the new type property and enforce type constraints for specialized flag definitions like booleanFlag.
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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| 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 issue 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
-
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. ↩
There was a problem hiding this comment.
Code Review
This pull request proposes adding an optional type property to the flag definition in flagd to address inconsistencies in type validation. The proposal is well-documented in a new architecture decision record. My feedback focuses on improving the clarity and consistency of the proposal document. Specifically, I've pointed out an inconsistency in the naming of the 'object' type and suggested clarifying a point about the impact on the flag manifest schema.
Signed-off-by: Andrey <andreyturkov@google.com>
Signed-off-by: Andrey <andreyturkov@google.com>
Signed-off-by: Andrey <andreyturkov@google.com>
dosubot
bot
added
the
size:L
Signed-off-by: Andrey <andreyturkov@google.com>
There was a problem hiding this comment.
Hey @andreyturkov, I think we're getting close. The core problem and proposed solution makes sense. I left some feedback around the proposed name. That's the only outstanding issue I see. Thanks!
chore: updated flag type ADR according to the remarks from PR - fixed naming
fix: reverted schemas
fix: fixed schema naming for flag type
There was a problem hiding this comment.
All in all, I appreciate the proposal of being explicit in the flag definition.
However, after reviewing the flag schema, I am not sure if the original issue #1481 it tries to solve is still present.
One important thing to clarify is what validation of the introduced flag type in evaluation methods mean:
- We validate that requested type and flag type match exactly
- We validate that requested type and flag type can be converted
| * The new `flagdType`field in the flag definition must be optional to ensure backward compatibility. | ||
| * If the `flagdType`field is present, `flagd` must validate that all variant values of the flag conform to this type during initialization. | ||
| * Type mismatches found during initialization must be reported as errors. | ||
| * The `Resolve<T>` and `ResolveAll` methods must use the `flagdType`field for validation when it is available. |
There was a problem hiding this comment.
What is the expected behavior? How does this behavior relate to flagd providers?
Providers currently try to convert the returned value to the requested evaluation type. They return an evaluation error when conversion between resolved and requested type is not supported by the underlying type system.
You can for example, call evaluateStringFlag(...) on a boolean flag and if the boolean value can be converted to a String, it will do so.
Do we expect with this change, that the before mentioned example should now return an evaluation error in flagd? If yes, this behavior should be consistent in flagd providers, so there is no behavioral difference between RPC and inProcess evaluation modes.
What do we expect when calling Resolve<Float> on a flag with flagType: "integer". Should this fail?
There was a problem hiding this comment.
Added a section about float/integer types.
fix: markdown check fix - added new line to the end of the file
fix: cleaned up flag type proposal by removing redundant sections
|
|
||
| ### API changes | ||
|
|
||
| The `flagd` flag definition will be updated to include an optional `flagType`property. This property will be a string enum with the following possible values: `"boolean"`, `"string"`, `"integer"`, `"float"`, and `"object"`. This aligns with the OpenFeature CLI and the flag manifest schema. |
There was a problem hiding this comment.
I'm not 100% sure it's a good idea to differentiate between float/int for this purpose. A couple reasons:
- I think we could easily write a JSON schema to enforce that flags with
"flagType":booleanonly have variants of type boolean, and"flagType":stringflags only have variants of type string, etc, etc... but I don't think we can enforce that ints are ints and not floats (we might be able to do this with multipleOf but I'd like to see a PoC first). - Our providers don't care about float/int as it is; they convert instead of indicating a type error.
- JSON itself doesn't distinguish between numeric types, so attempting to do so might be fraught.
Very interested in other thoughts on this.
There was a problem hiding this comment.
The main reason to have both float and int is to match the flag manifest used in the OpenFeature cli. I think it's important to keep them in sync. However, we may need to support casting from at least int to float.
There was a problem hiding this comment.
Our providers don't care about float/int as it is; they convert instead of indicating a type error.
I see the type system of JSONLogic as a limitation, as Number is essentially an IEEE 754 Double.
Conversions between Double and Integer64 may lose precision in both directions. I'd like to see a stronger typing is enforced if possible (depending on the language), to prevent the case that the client gets a different value than what's put in the evaluation rules, which can be quite dangerous for some situations of using Number flags, for example, when the flag represents a GCP project number
There was a problem hiding this comment.
Do you think this is something we need to enhance the OpenFeature specification to support?
IMO, I think numeric precision is a fuzzy part of the spec, mostly because it's non-trivial to write language-neutral specification about that sort of thing.
There was a problem hiding this comment.
I don't have enough contexts and knowledge to form a mature opinion for OpenFeature spec.... I guess we could explore the extension in flagd and OFREP first and we'll learn more about the feasibility. For now, I think drawing the boundary for each type in the flagd can be helpful.
There was a problem hiding this comment.
Added a section about float/integer types.
fix: fixed markdown errors in flag-type.md
fix: adressed review comments
|
@tangenti, @toddbaert, @guidobrei could you please take a look the latest version of this ADR? |
| By introducing an explicit `flagdType`field, it establishes a single source of truth for the flag's type, independent of its variants. This allows for early and consistent type validation during flag definition parsing, preventing type-related errors at runtime. | ||
|
|
||
| The new `type` field will be optional to maintain backward compatibility with existing flag configurations. If the field is omitted, `flagd` will treat the flag as having `object`, and no type validation will be performed against the `defaultVariant`. When the `type` field is present, `flagd` will enforce that all variants of the flag conform to the specified type. | ||
| The new `flagdType`field will be optional to maintain backward compatibility with existing flag configurations. If the field is omitted, `flagd` will treat the flag as having `object`, and no type validation will be performed against the `defaultVariant`. When the `flagdType`field is present, `flagd` will enforce that all variants of the flag conform to the specified type. |
There was a problem hiding this comment.
Could you add more details about how the validation would work?
I'm particularly interested in the cases that the literal values set to the variants do not fit with the flag type. Currently flagd seems to just cast the value to the type, with precision lost (this is perhaps more related to the JSONLogic engine impl).
CC @toddbaert
fix: fixed typo in flag-type ADR
andreyturkov
changed the title
andreyturkov
changed the title
This PR is a proposal to add flag type
Related Issues
#1481