-
Notifications
You must be signed in to change notification settings - Fork 6
feat: pass Dictionary to flag evaluation #193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cb71d25 to
a33bb3e
Compare
b250056 to
765d4de
Compare
64d7580 to
4385b93
Compare
nicklasl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off; great work taking this on and pushing through! And 👏 for writing such detailed tests, really helped me!
Secondly; I'm a bit mixed with all of this. It just becomes a bit less type safe than what I would like when you might get back a Dict<String, Any>. However; in those cases that was actually exactly what you passed in, so maybe it's fine?
I would have suggested we went straight to custom Struct support but we do need this to comply with OpenFeature though so 🤷.
Somewhere in my review I left a comment about the case when we have an additional key in the default value and that "breaks" the flag evaluation. If we could allow that, I'd be all good with this.
| // New behavior: should fail because the "error" key is missing from the flag structure | ||
| XCTAssertEqual(evaluation.value as? [String: AnyHashable], ["width": 100, "color": "black", "error": "Unknown"]) | ||
| XCTAssertEqual(evaluation.errorCode, .typeMismatch( | ||
| message: "Default value key \'error\' not found in flag")) | ||
| XCTAssertEqual(evaluation.errorMessage, "Default value key \'error\' not found in flag") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this behaviour TBH.
Why am I as a user of the SDK not allowed to have other dict keys in the default values compared to what Confidence assigns?
As a developer, this would prevent me from evolving my feature (or how I am using the feature flag) before implementing the new schema in Confidence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a potential pitfall with the mixed approach: if we have a remote variant { "size": 3 } and a default value { "sze": 10 } (with a typo), there would be no evaluation errors and possibly the devs will keep using size 10 around their codebase even if the exposure is for size 3. Not sure how much we should care about this, but it's definitely something more bug prone than failing with: "sze" not found in the remote flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another potential pitfall of having certain fields only in the default value is that if we later add the corresponding fields in the remote schema but with a different type we will break existing evaluations. Adding properties to a flag in the UI should always be a "safe operation".
As a developer, this would prevent me from evolving my feature (or how I am using the feature flag) before implementing the new schema in Confidence.
I am thinking devs can still try out the flag locally using the default value. But in order to resolve remote correctly, all the default keys must match the flag's remote schema: perhaps this is a reasonable ask
Signed-off-by: Fabrizio Demaria <[email protected]>
4385b93 to
205b2bc
Compare
fd76151 to
4a33b6c
Compare
4a33b6c to
bbad09a
Compare
a1e5a01 to
adee881
Compare
dc1254f to
d839c94
Compare
8ab8052 to
4179a7a
Compare
4179a7a to
0e7dabd
Compare
Behaviour
OpenFeature.Valueis required a default value.Supported complex types
✅ defaultValue: ["size": 0] # Dictionary
❓ defaultValue: DefVal(size: 0) # struct (see wip. Could we expose this too, eventually?)
❌ defaultValue: ["size": ConfidenceValue.init(integer: 0)] # Dict with ConfidenceValue
❌ defaultValue: ConfidenceValue.init(structure: ["size": ConfidenceValue.init(integer: 0)]) # ConfidenceValue
Address #190