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.

Sign up for GitHub

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

Jump to bottom

feat: Support mapping a client to a given provider. #388

Merged
merged 13 commits into from
May 19, 2023
Merged

feat: Support mapping a client to a given provider. #388

merged 13 commits into from
May 19, 2023

Conversation

justinabrahms
Copy link
Member

@justinabrahms justinabrahms commented Apr 11, 2023 •
edited by toddbaert
Loading

This PR

This adds support for mapping a client name onto a given provider, while still providing backwards compatibility to existing clients.

Related Issues

Refs open-feature/ofep#56
Fixes: #442

Follow-up Tasks

I still need to clean up documentation around this to note the difference between a "default provider" and a client-specific provider.

How to test

Not sure about testing, but I'd especially love eyes on the thread safety. I don't think ConcurrentHashMap is doing a lot of work here, given we have to use locks to support getProviderForClientOrDefault. If you have a better way, I'm all ears.

@justinabrahms justinabrahms requested a review from a team as a code owner April 11, 2023 21:15
@justinabrahms justinabrahms marked this pull request as draft April 11, 2023 21:16
@justinabrahms justinabrahms changed the title Support mapping a client to a given provider. feat: Support mapping a client to a given provider. Apr 11, 2023
@justinabrahms justinabrahms mentioned this pull request Apr 11, 2023
Merged
@justinabrahms justinabrahms force-pushed the client-provider-mappings branch from a1f948e to a1bbf38 Compare April 13, 2023 21:37
@codecov
Copy link

codecov bot commented Apr 14, 2023 •
edited
Loading

Codecov Report

Merging #388 (c08b895) into main (1af8e96) will increase coverage by 1.66%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #388      +/-   ##
============================================
+ Coverage     91.53%   93.20%   +1.66%     
- Complexity      213      220       +7     
============================================
  Files            23       23              
  Lines           496      500       +4     
  Branches         32       32              
============================================
+ Hits            454      466      +12     
+ Misses           24       17       -7     
+ Partials         18       17       -1
Flag Coverage Δ
unittests 93.20% <100.00%> (+1.66%) ⬆️

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

Impacted Files Coverage Δ
.../main/java/dev/openfeature/sdk/MutableContext.java 76.47% <100.00%> (+3.92%) ⬆️
.../main/java/dev/openfeature/sdk/OpenFeatureAPI.java 100.00% <100.00%> (ø)
...in/java/dev/openfeature/sdk/OpenFeatureClient.java 97.84% <100.00%> (-0.07%) ⬇️

... and 2 files with indirect coverage changes

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

@justinabrahms justinabrahms force-pushed the client-provider-mappings branch from e9a298f to 9932825 Compare April 14, 2023 06:17
@justinabrahms justinabrahms mentioned this pull request Apr 27, 2023
Merged
@justinabrahms justinabrahms marked this pull request as ready for review May 3, 2023 16:01
@toddbaert
Copy link
Member

I still need to clean up documentation around this to note the difference between a "default provider" and a client-specific provider.

If you think it's worth it, it could be something we mention in the glossary.

@justinabrahms justinabrahms mentioned this pull request May 3, 2023
Merged
@justinabrahms
Copy link
Member Author

Doc improvement: open-feature/spec#186

@toddbaert toddbaert mentioned this pull request May 4, 2023
Closed
@justinabrahms justinabrahms force-pushed the client-provider-mappings branch from 7e5ec3d to f2abe39 Compare May 4, 2023 18:21
@toddbaert toddbaert requested a review from lopitz May 4, 2023 18:55
justinabrahms and others added 10 commits May 6, 2023 23:11
@justinabrahms
Add a few javadocs.
69e729a 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Special case the null client name
ece1909 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Add some missing test cases.
1318157 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Moving to an object map unwraps the values.
319c600 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Fix equality test.
ceceffe 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Carry targeting key when copying over null object.
7e10c98 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Test provider name, not object equality.
25acfcf 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Client-based getProvider is now an overload; Use read lock, not write…
75fbf27 
… lock.

Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms @lopitz
Update src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java
b5a40d8 
Co-authored-by: Lars Opitz <lars@lars-opitz.de>
Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms
Simplify locking logic around providers.
f5c60b5 
There's no such thing as "API without a provider set" anymore. We now default to NoOpProvider in the API (not client).

Signed-off-by: Justin Abrahms <justin@abrah.ms>
@justinabrahms justinabrahms force-pushed the client-provider-mappings branch from 777cb0b to f5c60b5 Compare May 7, 2023 06:11
@justinabrahms
Add a few missing tests
efae477 
Signed-off-by: Justin Abrahms <justin@abrah.ms>
thiyagu06
thiyagu06 approved these changes May 7, 2023
View reviewed changes
Copy link
Member

@thiyagu06 thiyagu06 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👍

lopitz
lopitz approved these changes May 7, 2023
View reviewed changes
Copy link
Contributor

@lopitz lopitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work! ❤️

Kavindu-Dodan
Kavindu-Dodan reviewed May 8, 2023
View reviewed changes
src/main/java/dev/openfeature/sdk/OpenFeatureAPI.java
private EvaluationContext evaluationContext;
private List<Hook> apiHooks;
private final List<Hook> apiHooks;
private FeatureProvider defaultProvider = new NoOpProvider();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for the noop default provider. This ensures null safety where possible.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@beeme1mr
Copy link
Member

@justinabrahms @toddbaert any concerns about merging this?

@toddbaert
Copy link
Member

toddbaert commented May 18, 2023 •
edited
Loading

@justinabrahms @toddbaert any concerns about merging this?

Nope (but lets' not release until all this is done, IMO).

This was referenced May 19, 2023
Closed
Closed
@justinabrahms justinabrahms merged commit d4c43d7 into main May 19, 2023
@justinabrahms justinabrahms deleted the client-provider-mappings branch May 19, 2023 22:27
@github-actions github-actions bot mentioned this pull request May 19, 2023
Merged
@beeme1mr beeme1mr mentioned this pull request May 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiyagu06 thiyagu06 thiyagu06 approved these changes

@lopitz lopitz lopitz approved these changes

@Kavindu-Dodan Kavindu-Dodan Kavindu-Dodan approved these changes

@thomaspoignant thomaspoignant thomaspoignant approved these changes

@toddbaert toddbaert toddbaert approved these changes

@thisthat thisthat Awaiting requested review from thisthat

@kinyoklion kinyoklion Awaiting requested review from kinyoklion

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

implement named client support: open-feature/spec@4cf8229
7 participants
@justinabrahms @toddbaert @beeme1mr @thiyagu06 @lopitz @Kavindu-Dodan @thomaspoignant