feat: Make IFeatureClient interface public. #102

cdonnellytx · 2023-01-13T22:12:00Z

Signed-off-by: Chris Donnelly [email protected]

This PR

Exposes the IFeatureClient interface to consumers by making it public.
Pulls up the XMLDoc from the concrete class to the interface, filling in gaps (e.g., the interface-level docs) from the Java SDK.

Background and Motivation

This allows consumers using dependency injection to properly create a mock of the feature client. Currently, the exposed FeatureClient class is sealed, therefore making it extremely difficult (though not impossible) for consumers to mock in parallel without side-effects.

Consider

public class MyService
{
    private readonly FeatureClient _featureClient;

    public MyService(FeatureClient featureClient, ...)
    {
        _featureClient = featureClient;
    }

    public async Task<int> DoSomethingAsync() => await _featureClient.GetBooleanValue("flag") ? 42 : 47;
}

A unit test for MyService would be dependent on the concrete FeatureClient class, and thus on the Api.Instance singleton, limiting tests to be more brittle and dependent on state.

Allowing MyService to use IFeatureClient would get around this problem, and allow for unit tests to not depend on shared state, allowing for test parallelization where possible.

Notes

Modifying a public interface in any way (even adding new methods/properties) is considered a breaking change, so each client surface enhancement/change would be breaking by that definition. We may wish to pull up methods from the concrete class to the interface (e.g., GetContext / SetContext) now before committing to the existing interface.

Follow-up Tasks

Avoiding unrelated changes is also why I opted not to make GetMetadata() into a Metadata read-only property on the interface -- while this makes sense from a .NET idiomatic view, it would conflict with the current way the concrete class exposes GetMetadata(). Fixing it later would be a breaking change, though.

Similarly, I opted to keep Api.Instance.GetClient() returning the concrete class, as it would be a breaking change to make it return IFeatureClient. I am open to that idea, though.

How to test

No additional tests should be necessary at this time.

Additional necessary changes: - Pulls up the documentation from the concrete class to the interface. Signed-off-by: Chris Donnelly <[email protected]>

codecov-commenter · 2023-01-16T19:22:03Z

Codecov Report

Merging #102 (7aa75d7) into main (7cc7ab4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #102   +/-   ##
=======================================
  Coverage   94.54%   94.54%           
=======================================
  Files          20       20           
  Lines         532      532           
  Branches       37       37           
=======================================
  Hits          503      503           
  Misses         16       16           
  Partials       13       13

Impacted Files	Coverage Δ
src/OpenFeature/OpenFeatureClient.cs	`98.74% <ø> (ø)`

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

toddbaert · 2023-01-16T19:22:53Z

src/OpenFeature/IFeatureClient.cs

+        /// <param name="defaultValue">Default value</param>
+        /// <param name="context"><see cref="EvaluationContext">Evaluation Context</see></param>
+        /// <param name="config"><see cref="EvaluationContext">Flag Evaluation Options</see></param>
+        /// <returns>Resolved flag details <see cref="FlagEvaluationDetails{T}"/></returns>


This wasn't something you introduced, @cdonnellytx, so I don't mind if you address it or not, but these docs are slightly innacurate. This just resolves the flag value not wrapped in the FlagEvaluationDetails wrapper.

I can fix it; do you want me to do it in a separate PR or in this one?

I'll leave that up to you, I'll review/approve either way.

OK, I have updated the docs.

toddbaert

Thanks for your contribution and well-reasoned PR, @cdonnellytx ! I agree with the overall goal and solution.

I would like input from @kinyoklion and @benjiro on this point:

Modifying a public interface in any way (even adding new methods/properties) is considered a breaking change, so each client surface enhancement/change would be breaking by that definition. We may wish to pull up methods from the concrete class to the interface (e.g., GetContext / SetContext) now before committing to the existing interface.

I'm leaning towards adding more methods to the interface, so they could be easily mocked as well.

cdonnellytx · 2023-01-17T15:46:31Z

I'm leaning towards adding more methods to the interface, so they could be easily mocked as well.

I would recommend doing so. At a minimum I would recommend what's exposed on the Java interface:

GetContext / SetContext -- either as-is or as a property
GetHooks

(An aside: if you do want to convert the getter/setter methods to properties, backward compatibility on the class itself can be preserved.)

kinyoklion · 2023-01-17T17:23:10Z

I'm leaning towards adding more methods to the interface, so they could be easily mocked as well.

I would recommend doing so. At a minimum I would recommend what's exposed on the Java interface:

GetContext / SetContext -- either as-is or as a property

GetHooks

(An aside: if you do want to convert the getter/setter methods to properties, backward compatibility on the class itself can be preserved.

I think the adding the interface itself is a great change.

These methods I have some concerns with. Primarily I don't think they should have been public to begin with on the concrete type (probably should have been internal). They aren't required for any functionality an OF client is intended to provide, and don't seem to be part of the spec.

I guess adding them to the interface doesn't worsen that problem, and being as they had been exposed they may as well also be added to the interface.

Signed-off-by: Chris Donnelly <[email protected]>

cdonnellytx · 2023-01-17T17:46:56Z

I guess adding them to the interface doesn't worsen that problem, and being as they had been exposed they may as well also be added to the interface.

I agree.

I'll go ahead and add them to the PR.

- GetContext() / SetContext(EvaluationContext) - GetHooks() Signed-off-by: Chris Donnelly <[email protected]>

The extra "see" at the end made it read as "Gets the EvaluationContext of this clientEvaluationContext". Signed-off-by: Chris Donnelly <[email protected]>

toddbaert · 2023-01-17T19:27:37Z

Primarily I don't think they should have been public to begin with on the concrete type (probably should have been internal). They aren't required for any functionality an OF client is intended to provide, and don't seem to be part of the spec.

I think this is true for GetHooks/GetContext, but not for SetContext. The spec does mention in 3.2.1 that is means of providing context per client is required. It doesn't need to be implemented with a mutator though.

One justification is that we wanted to support a "client-per-request" paradigm, so that you could basically associate a particular thread or continuation with a state (the client's context).

kinyoklion · 2023-01-17T19:32:23Z

Primarily I don't think they should have been public to begin with on the concrete type (probably should have been internal). They aren't required for any functionality an OF client is intended to provide, and don't seem to be part of the spec.

I think this is true for GetHooks/GetContext, but not for SetContext. The spec does mention in 3.2.1 that is means of providing context per client is required. It doesn't need to be implemented with a mutator though.

One justification is that we wanted to support a "client-per-request" paradigm, so that you could basically associate a particular thread or continuation with a state, for instance, session data.

Yes, sorry, The set should be exposed for sure.

toddbaert · 2023-01-17T21:14:52Z

I plan on merging this and releasing it with 1.1 tomorrow, unless I hear objections.

benjiro

Thanks for your contribution @cdonnellytx

feat: Make IFeatureClient interface public.

937a962

Additional necessary changes: - Pulls up the documentation from the concrete class to the interface. Signed-off-by: Chris Donnelly <[email protected]>

toddbaert requested review from benjiro, kinyoklion and toddbaert January 16, 2023 14:30

toddbaert reviewed Jan 16, 2023

View reviewed changes

toddbaert approved these changes Jan 16, 2023

View reviewed changes

fix: Docs for value methods now state they return the value.

9710d1c

Signed-off-by: Chris Donnelly <[email protected]>

cdonnellytx added 2 commits January 17, 2023 11:54

feat: Pull up methods to interface.

a980412

- GetContext() / SetContext(EvaluationContext) - GetHooks() Signed-off-by: Chris Donnelly <[email protected]>

fix: Fixing Get/SetContext docs.

7aa75d7

The extra "see" at the end made it read as "Gets the EvaluationContext of this clientEvaluationContext". Signed-off-by: Chris Donnelly <[email protected]>

kinyoklion approved these changes Jan 17, 2023

View reviewed changes

benjiro approved these changes Jan 17, 2023

View reviewed changes

benjiro merged commit 5a09c4f into open-feature:main Jan 17, 2023

github-actions bot mentioned this pull request Jan 17, 2023

chore(main): release 1.1.0 #98

Merged

cdonnellytx deleted the feat/make-interface-public branch January 17, 2023 21:27

This was referenced Jan 18, 2023

chore(main): release 1.0.0 #103

Closed

chore(main): release 1.0.0 #104

Closed

chore(main): release 1.0.0 #105

Closed

chore(main): release 1.0.0 #106

Closed

chore(main): release 1.1.0 #107

Merged

say25 mentioned this pull request Jan 18, 2023

docs: inheritdoc at class level #108

Closed

toddbaert mentioned this pull request Jan 25, 2023

First class client-side support in OpenFeature spec open-feature/spec#167

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make IFeatureClient interface public. #102

feat: Make IFeatureClient interface public. #102

cdonnellytx commented Jan 13, 2023

codecov-commenter commented Jan 16, 2023 •

edited

Loading

toddbaert Jan 16, 2023

cdonnellytx Jan 17, 2023

toddbaert Jan 17, 2023

cdonnellytx Jan 17, 2023

toddbaert left a comment

cdonnellytx commented Jan 17, 2023

kinyoklion commented Jan 17, 2023

cdonnellytx commented Jan 17, 2023

toddbaert commented Jan 17, 2023 •

edited

Loading

kinyoklion commented Jan 17, 2023

toddbaert commented Jan 17, 2023

benjiro left a comment

feat: Make IFeatureClient interface public. #102

feat: Make IFeatureClient interface public. #102

Conversation

cdonnellytx commented Jan 13, 2023

This PR

Background and Motivation

Notes

Follow-up Tasks

How to test

codecov-commenter commented Jan 16, 2023 • edited Loading

Codecov Report

toddbaert Jan 16, 2023

Choose a reason for hiding this comment

cdonnellytx Jan 17, 2023

Choose a reason for hiding this comment

toddbaert Jan 17, 2023

Choose a reason for hiding this comment

cdonnellytx Jan 17, 2023

Choose a reason for hiding this comment

toddbaert left a comment

Choose a reason for hiding this comment

cdonnellytx commented Jan 17, 2023

kinyoklion commented Jan 17, 2023

cdonnellytx commented Jan 17, 2023

toddbaert commented Jan 17, 2023 • edited Loading

kinyoklion commented Jan 17, 2023

toddbaert commented Jan 17, 2023

benjiro left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 16, 2023 •

edited

Loading

toddbaert commented Jan 17, 2023 •

edited

Loading