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: getState mandatory on EventProvider #531

Merged
merged 3 commits into from
Aug 4, 2023

Conversation

toddbaert
Copy link
Member

@toddbaert toddbaert commented Jul 26, 2023

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
Copy link

codecov bot commented Jul 26, 2023

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

@toddbaert toddbaert requested a review from thisthat July 26, 2023 16:25
@toddbaert toddbaert force-pushed the fix/event-provider-getstate branch from 4cc7e49 to ab90798 Compare July 26, 2023 17:47
toddbaert added a commit to open-feature/js-sdk that referenced this pull request Jul 26, 2023
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 <[email protected]>
Copy link
Member

@thisthat thisthat left a comment

Choose a reason for hiding this comment

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

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:

src/test/java/dev/openfeature/sdk/EventProviderTest.java Outdated Show resolved Hide resolved
@toddbaert
Copy link
Member Author

toddbaert commented Jul 27, 2023

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:

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

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

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
Copy link
Member Author

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.

Comment on lines +60 to +64
* 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>
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@Kavindu-Dodan
Copy link
Contributor

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.

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

sonarcloud bot commented Aug 4, 2023

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 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@toddbaert toddbaert merged commit 37fd2be into main Aug 4, 2023
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