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? |
2 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