docs: improve fractional non-string adr#1963
Conversation
Signed-off-by: Marcin Olko <molko@google.com>
dosubot
Bot
added
the
size:M
✅ 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.
Code Review
This pull request updates the architectural decision for the fractional operator to improve cross-language consistency. Key changes include explicitly rejecting null as a primary argument, implementing strict number normalization (Integer vs. Float), and requiring CBOR deterministic encoding for map ordering and preferred serialization. Feedback suggests restoring null to the list of supported CBOR major types to handle nested values correctly and recommends explicitly rejecting datetime types with an error instead of leaving their behavior undefined.
| * `boolean` is major type 7 | ||
| * `null` is major type 7 | ||
| * `string` is major type 3 |
There was a problem hiding this comment.
While 'null' is explicitly rejected as the first argument to the 'fractional' operator (to prevent silent errors from evaluation failures), it remains a valid type within complex structures like maps (objects). The removal of 'null' from this list of major types leaves its encoding undefined when it appears as a nested value. It should be restored to ensure consistent hashing of objects containing null values.
| * `boolean` is major type 7 | |
| * `null` is major type 7 | |
| * `string` is major type 3 | |
| * boolean is major type 7 | |
| * null is major type 7 | |
| * string is major type 3 |
| * `array` (list, sequence) is major type 4 | ||
| * `datetime` is converted to POSIX epoch time (including fractional seconds for sub-second precision) and CBOR Tag 1 is used | ||
|
|
||
| **NOTE: As JSONLogic doesn’t have any datetime type, currently we don’t leverage CBOR Tag 1. Any datetime type used within provider implementation and passed to the fractional operator causes undefined behavior. If a user wants to manage datetime, they can do it by leveraging POSIX epoch encoded as integer value, or as ISO 8601 standard encoded as string.** |
There was a problem hiding this comment.
Marking 'datetime' usage as 'undefined behavior' contradicts the goal of ensuring strict cross-language consistency. Since 'null' is explicitly rejected with an error, 'datetime' types should be handled similarly to prevent inconsistent implementations across different providers. If a provider encounters a native datetime type that hasn't been converted to a string or integer by the user, it should report an error.
| **NOTE: As JSONLogic doesn’t have any datetime type, currently we don’t leverage CBOR Tag 1. Any datetime type used within provider implementation and passed to the fractional operator causes undefined behavior. If a user wants to manage datetime, they can do it by leveraging POSIX epoch encoded as integer value, or as ISO 8601 standard encoded as string.** | |
| NOTE: As JSONLogic doesn’t have any datetime type, currently we don’t leverage CBOR Tag 1. Any datetime type used within provider implementation and passed to the fractional operator must be rejected with an error. If a user wants to manage datetime, they can do it by leveraging POSIX epoch encoded as integer value, or as ISO 8601 standard encoded as string. |
References
- When a feature designed to prevent user misconfiguration introduces significant implementation complexity, re-evaluate if a simpler architectural approach can solve the problem more elegantly.
|
@cupofcat @toddbaert @beeme1mr Could you take a look and let me know what do you think of those changes? |
|
Ping @toddbaert what are your thoughts on this? |
| To prevent overflow errors in strongly-typed languages (e.g. Go) and inconsistent BigInt tagging in languages with arbitrary-precision integers (e.g. Python), the number normalization is restricted to the range $[-2^{63}, 2^{64}-1]$ (covering both signed and unsigned 64-bit integers): | ||
|
|
||
| 1. If a numeric value has no fractional part (e.g., `val == math.Trunc(val)` in Go, or `val.is_integer()` in Python), the provider must attempt to cast it to a signed (if <0) or unsigned (if >=0) integer before encoding. | ||
| 2. If a numeric value has fractional part, or if it falls outside the range $[-2^{63}, 2^{64}-1]$ (e.g., `1.0e+176`), it **must not** be normalized to an integer. It must be encoded as a float (Major Type 7). |
There was a problem hiding this comment.
We may want to apply this to anything larger than the Javascript MAX_SAFE_INTEGER (2^53 - 1). Things could get messy, especially in JS, otherwise.
Even Gson I think out of the box, would yield double here, IIUC.
If the goal here is just consistency across all languages including JS, I think encoding as float outside the "SAFE" JS range will be easiest to accomplish. We could also perhaps support parsing to bigint in JS, but that seems somewhat heavy for just this purpose.
| 4. Otherwise, if `targetingKey` is missing, report an error and return nil | ||
| 1. If the first element in `fractional` evaluates to `null`, we report an error and return `nil`. | ||
| 2. If the first element in `fractional` evaluates to a non-array type then deterministically encode it to a well defined byte array and hash the bytes. | ||
| 3. Otherwise, if `targetingKey` is a string, build a 2-elements array of `flagKey` and `targetingKey`, deterministically encode that and hash (**NOTE:** This is different than string concatenation used today). |
There was a problem hiding this comment.
Is it possible to maintain the current cat behavior?
If not, that's okay but we did get some complains about the re-random assignment this caused. Not a huge deal <1.0.
There was a problem hiding this comment.
Left a couple comments... this one sticks out to me: https://github.com/open-feature/flagd/pull/1963/changes#r3305815236.
I think as long as we have a solution for JS this all sounds good.
4 participants
This PR
While working on the non-string fractional ADR, we identified several critical pitfalls in the initial proposal and wanted to expand the requirements to ensure strict cross-language consistency.
This PR updates the ADR (fractional-non-string-rand-units.md) with the following key changes:
nullRejection: Explicitly rejectnullas the first argument to prevent silent errors from evaluation failures.Related Issues
Under scope of #1737
Notes
No Breaking Change: As none of the languages implemented this ADR yet, those changes won't affect any of the implementations, while avoiding pitfalls that could arise later on. The changes are purely semantic and do not affect the flagd JSON schema and the rest of the adr.
Follow-up Tasks
We should update reference specification of fractional operator on flagd page