Skip to content
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

fix: isList check in Value checks type of list #70

Merged
merged 3 commits into from
Sep 13, 2022

Conversation

rgrassian-split
Copy link
Contributor

A list of any type would be able to be created due to the new object constructor. This makes it so that the isList check only returns true if the list contains objects of type Value (or is empty).

Signed-off-by: Robert Grassian <robert.grassian@split.io>
Signed-off-by: Robert Grassian <robert.grassian@split.io>
@rgrassian-split rgrassian-split changed the title isList check in Value checks type of list fix: isList check in Value checks type of list Sep 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Merging #70 (d13a310) into main (00af2f8) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main      #70      +/-   ##
============================================
+ Coverage     91.27%   91.32%   +0.05%     
- Complexity      159      162       +3     
============================================
  Files            19       19              
  Lines           344      346       +2     
  Branches         14       16       +2     
============================================
+ Hits            314      316       +2     
  Misses           21       21              
  Partials          9        9              
Flag Coverage Δ
unittests 91.32% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/java/dev/openfeature/javasdk/Value.java 88.40% <100.00%> (+0.34%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rgrassian-split
Copy link
Contributor Author

@justinabrahms @toddbaert please check out this PR, as I was updating the Split Provider I noticed it would be possible to set a list of any time due to the new constructor. This change should make it so that only lists containing Values are allowed to be created through that constructor

@justinabrahms justinabrahms merged commit 81ab071 into open-feature:main Sep 13, 2022
@rgrassian-split rgrassian-split deleted the list_type_check branch September 13, 2022 22:17
@toddbaert
Copy link
Member

Thanks for adding this check. I think we'll have a similar issue in dotnet too. I will address it there.

@toddbaert
Copy link
Member

toddbaert commented Sep 13, 2022

Thinking about it more, would it be better to instead check that the list type is sane when we instantiate the value? The Object constructor already throws. We could add this same test on the object constructor and throw if the List is not of type value. That way, it's impossible to ever create such a value. @rgrassian-split

@rgrassian-split
Copy link
Contributor Author

rgrassian-split commented Sep 14, 2022

Thinking about it more, would it be better to instead check that the list type is sane when we instantiate the value? The Object constructor already throws. We could add this same test on the object constructor and throw if the List is not of type value. That way, it's impossible to ever create such a value. @rgrassian-split

@toddbaert Can you elaborate, I'm a bit confused. Right now the object constructor calls the isList method which would fail so it's impossible to create a value that is a list of things other than values.

Also as an fyi, it turned out that this object constructor was less useful than we thought it would be and I'm not sure we will end up using it for our Split Provider. The reason being if we have on our end something like a List<String> and want it as a value we would really want to turn this into something like Value<List<Value<String>>>, but the object constructor can't do that for us so that's a case we still have to handle special.

The fetching as an object has been very useful though thanks for that

@toddbaert
Copy link
Member

@rgrassian-split oof, I misread the test, and didn't recall that isList() is called from the object constructor - so the net behavior you've implemented is basically what I was hoping for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants