fix: getState mandatory on EventProvider #531

toddbaert · 2023-07-26T15:18:18Z

In 3 event provider implementations, flagd-java, go-feature-flag-web, and even my own implementation of flagd-js, authors have forgotten to properly implement getState (which means the SDK assumes the provider is already READY and doesn't run initialize().)

This is an easy pitfall. To help with this, I'm making the implementation of getState required in the abstract EventProvider class, and will do similarly in the JS-SDK. I think this is a reasonable proposal because the EventProvider class is very new, and in the majority of cases a provider that can fire async events requires some kind of remote connection, so it will need initialization. This is technically a breaking change, but it would only impact the experimental events API, specifically the EventProvider implementations which could only be a few days old at this point, and which (due to my previous point) likely should have implemented this anyway.

It also adds some more doc about the importance of this method.

Relates to: open-feature/js-sdk#506

codecov · 2023-07-26T15:23:54Z

Codecov Report

Merging #531 (527c9f7) into main (8789f90) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #531   +/-   ##
=========================================
  Coverage     95.36%   95.36%           
  Complexity      330      330           
=========================================
  Files            31       31           
  Lines           755      755           
  Branches         37       37           
=========================================
  Hits            720      720           
  Misses           18       18           
  Partials         17       17

Flag	Coverage Δ
unittests	`95.36% <ø> (ø)`

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

Files Changed	Coverage Δ
...c/main/java/dev/openfeature/sdk/EventProvider.java	`100.00% <ø> (ø)`
...main/java/dev/openfeature/sdk/FeatureProvider.java	`100.00% <ø> (ø)`

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

In 3 event provider implementations, [flagd-java](open-feature/java-sdk-contrib#361 (review)), [go-feature-flag-web](open-feature/js-sdk-contrib#474 (comment)), and even my own implementation of flagd-js, authors have forgotten to properly implement `getState` (which means the SDK assumes the provider is already `READY` and doesn't run `initialize()`.) This is an easy pitfall. To help with this, I'm adding doc. We may be able to improve the interfaces to help with this, but I think this is a safe and helpful change for now. Relates to: open-feature/java-sdk#531 Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

thisthat

I support this change 👍
Should we provide an annotation to mark specific APIs experimental to feel less guilty when we do breaking changes? 🤔
Something similar exists in other projects:

Guava
gRPC

src/test/java/dev/openfeature/sdk/EventProviderTest.java

toddbaert · 2023-07-27T12:17:54Z

I support this change +1 Should we provide an annotation to mark specific APIs experimental to feel less guilty when we do breaking changes? 🤔 Something similar exists in other projects:

Guava

gRPC

This is an interesting idea IMO. Can you elaborate more on the user-experience of this? Does the annotation add something that developers can easily see in their IDE? Does it suppose a dev would open the source file? @thisthat

thisthat · 2023-07-27T12:40:03Z

This is an interesting idea IMO. Can you elaborate more on the user-experience of this? Does the annotation add something that developers can easily see in their IDE? Does it suppose a dev would open the source file? @thisthat

AFAIK, no IDE prints any special info for annotations. However, when I work on implementing an interface/abstract class I do look in the source file. Annotations are preserved in case of decompilation. It would be just a "FYI" for implementers.

toddbaert · 2023-07-27T14:46:41Z

This is an interesting idea IMO. Can you elaborate more on the user-experience of this? Does the annotation add something that developers can easily see in their IDE? Does it suppose a dev would open the source file? @thisthat

AFAIK, no IDE prints any special info for annotations. However, when I work on implementing an interface/abstract class I do look in the source file. Annotations are preserved in case of decompilation. It would be just a "FYI" for implementers.

OK that's kinda what I was assuming. I still like the idea.

toddbaert · 2023-07-31T17:09:00Z

src/main/java/dev/openfeature/sdk/FeatureProvider.java

+     * If the provider needs to be initialized, it should return {@link ProviderState#NOT_READY}.
+     * If the provider is in an error state, it should return {@link ProviderState#ERROR}.
+     * If the provider is functioning normally, it should return {@link ProviderState#READY}.
+     * 
+     * <p><i>Providers which do not implement this method are assumed to be ready immediately.</i></p>


At a minimum we should add these docs, but I think the small breaking change below is worth adding.

Kavindu-Dodan · 2023-08-01T21:21:07Z

Approved. We can make this a breaking change and bump the minor version. Given that the eventing support is new, I assume there are only a handful of adoptions of the previous sdk version.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

sonarcloud · 2023-08-04T19:34:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

toddbaert requested a review from a team as a code owner July 26, 2023 15:18

toddbaert requested review from beeme1mr, lukas-reining, thomaspoignant and kinyoklion July 26, 2023 15:18

toddbaert mentioned this pull request Jul 26, 2023

fix: provider js-doc improvments open-feature/js-sdk#506

Merged

toddbaert requested a review from thisthat July 26, 2023 16:25

toddbaert force-pushed the fix/event-provider-getstate branch from 4cc7e49 to ab90798 Compare July 26, 2023 17:47

thisthat approved these changes Jul 27, 2023

View reviewed changes

src/test/java/dev/openfeature/sdk/EventProviderTest.java Outdated Show resolved Hide resolved

toddbaert force-pushed the fix/event-provider-getstate branch from ca8636a to 734ddc6 Compare July 27, 2023 12:18

toddbaert force-pushed the fix/event-provider-getstate branch from 734ddc6 to ecb5a8f Compare July 31, 2023 17:07

toddbaert requested a review from justinabrahms July 31, 2023 17:07

toddbaert commented Jul 31, 2023

View reviewed changes

toddbaert force-pushed the fix/event-provider-getstate branch from ecb5a8f to dd26e2c Compare August 1, 2023 13:42

beeme1mr approved these changes Aug 1, 2023

View reviewed changes

Kavindu-Dodan approved these changes Aug 1, 2023

View reviewed changes

toddbaert force-pushed the fix/event-provider-getstate branch 2 times, most recently from 6061f26 to 8541a2a Compare August 4, 2023 17:45

toddbaert and others added 3 commits August 4, 2023 15:34

fix: getState mandatory on EventProvider

6cf7387

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

fixup: update test class

c2c4e87

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Update src/test/java/dev/openfeature/sdk/EventProviderTest.java

527c9f7

Co-authored-by: Giovanni Liva <giovanni.liva@dynatrace.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

toddbaert force-pushed the fix/event-provider-getstate branch from 8541a2a to 527c9f7 Compare August 4, 2023 19:34

toddbaert merged commit 37fd2be into main Aug 4, 2023
8 checks passed

toddbaert deleted the fix/event-provider-getstate branch August 4, 2023 19:45

github-actions bot mentioned this pull request Aug 4, 2023

chore(main): release 1.4.2 #527

Merged

toddbaert mentioned this pull request Nov 1, 2023

chore: add liran2000 and thisthat to Java approvers open-feature/community#273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: getState mandatory on EventProvider #531

fix: getState mandatory on EventProvider #531

toddbaert commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

thisthat left a comment •

edited

Loading

toddbaert commented Jul 27, 2023 •

edited

Loading

thisthat commented Jul 27, 2023

toddbaert commented Jul 27, 2023

toddbaert Jul 31, 2023

Kavindu-Dodan commented Aug 1, 2023

sonarcloud bot commented Aug 4, 2023

fix: getState mandatory on EventProvider #531

fix: getState mandatory on EventProvider #531

Conversation

toddbaert commented Jul 26, 2023 • edited Loading

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

thisthat left a comment • edited Loading

Choose a reason for hiding this comment

toddbaert commented Jul 27, 2023 • edited Loading

thisthat commented Jul 27, 2023

toddbaert commented Jul 27, 2023

toddbaert Jul 31, 2023

Choose a reason for hiding this comment

Kavindu-Dodan commented Aug 1, 2023

sonarcloud bot commented Aug 4, 2023

toddbaert commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

thisthat left a comment •

edited

Loading

toddbaert commented Jul 27, 2023 •

edited

Loading