fix: do not mutate provider's ResolutionDetails on type mismatch#272
Conversation
Signed-off-by: Cristian Marta <cristian@caffe-lento.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents the SDK from mutating Provider::ResolutionDetails instances returned by providers when the SDK detects a type mismatch, avoiding potential corruption for providers that cache and reuse those objects across evaluations.
Changes:
- Add a spec asserting the provider-returned
ResolutionDetailsis not modified on type mismatch. - Update client type-mismatch handling to construct a new
ResolutionDetailsinstead of mutating the provider’s instance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/open_feature/sdk/client.rb |
Replaces in-place mutation on type mismatch with creation of a fresh ResolutionDetails error result. |
spec/open_feature/sdk/client_spec.rb |
Adds regression coverage ensuring provider-owned ResolutionDetails objects are not mutated by the SDK. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request ensures that ResolutionDetails are not mutated on type mismatch by creating a new instance instead of modifying the existing one. A test case was added to verify this behavior. Feedback suggests adding a descriptive error_message to the new instance to comply with OpenFeature specifications and improve error reporting.
| resolution_details = Provider::ResolutionDetails.new( | ||
| value: default_value, | ||
| error_code: Provider::ErrorCode::TYPE_MISMATCH, | ||
| reason: Provider::Reason::ERROR | ||
| ) |
There was a problem hiding this comment.
While creating a new ResolutionDetails object correctly avoids mutating the provider's response, this is also a good opportunity to provide a descriptive error_message. According to the OpenFeature specification, the evaluation details SHOULD contain an intelligible explanation of why an error occurred. Additionally, following repository guidelines, complex expressions like method chains should be extracted from string interpolations into local variables to improve readability.
actual_type = resolution_details.value.class
resolution_details = Provider::ResolutionDetails.new(
value: default_value,
error_code: Provider::ErrorCode::TYPE_MISMATCH,
reason: Provider::Reason::ERROR,
error_message: "Expected #{type}, but got #{actual_type}"
)References
- OpenFeature Specification Requirement 1.4.7: The evaluation details structure's error message field SHOULD contain a string containing an intelligible explanation of why an error occurred, if applicable.
- For readability, extract complex expressions like ternary operators from string interpolations into local variables.
The spec does not prohibit providers from caching the
ResolutionDetailsthey return. Mutating that object in place would cause silent data corruption for any provider that reuses it across calls — the provider owns what it returns, the SDK should not write to it.Construct a fresh
ResolutionDetailsinstead of mutating the one returned by the provider.