Dependent features #176

Tymek · 2023-09-27T10:55:34Z

Edit: Moved this into draft so can iterate a little on it

Description

Introducing concept of "dependency" on a feature toggle. Feature toggle, in order to be enabled an return a variant, can require the specified state of another feature toggle. There will be a dependencies property on each feature.

{
 ...
 "dependencies": {
   "feature": "string-featureId",
   "enabled: true, // boolean, state that is expected defaults to `true`
   "variants": ["variant1", "variant2"] // optional, variants allowed for feature to be enabled
 }
}

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Unit tests
Spec Tests
Integration tests / Manual Tests

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

src/DefaultUnleash.php

kwasniew · 2023-10-03T07:24:51Z

src/DefaultUnleash.php

@@ -59,21 +63,157 @@ public function isEnabled(string $featureName, ?Context $context = null, bool $d
            }
        }

-        return $this->isFeatureEnabled($feature, $context, $default)->isEnabled();
+        return $this->isFeatureEnabled($feature, $context, $dependencies, $default)->isEnabled();


why can't dependencies be taken from the feature and need to be passed as arguments

RikudouSage · 2023-10-03T07:29:22Z

src/DefaultUnleash.php

+        }
+
+        $dependencyVariants = $dependency->getVariants();
+        if (!empty($dependencyVariants)) {


Please don't ever use empty(), that does like a million things. Be specific, you probably want count() instead of empty().

Me: Arrives on this pull request
Me: Reads this comment
Me: Can empty() really be that bad?
Me: Reads for 5 minutes
Me: Please don't ever use empty(), that does like a million things

kwasniew · 2023-10-03T08:00:24Z

src/DefaultUnleash.php

@@ -43,7 +45,9 @@ public function __construct(
    public function isEnabled(string $featureName, ?Context $context = null, bool $default = false): bool
    {
        $context ??= $this->configuration->getContextProvider()->getContext();
-        $feature = $this->findFeature($featureName, $context);
+        $featureAndDependencies = $this->findFeatureAndDependencies($featureName, $context);


can findFeature return feature with dependencies same way as our API returns feature with dependencies?

I think we're thinking the same thing here, I have a similar comment on materializing this from cache in the correct DTO format. I think we're saying the same thing. If we are, I definitely agree

kwasniew · 2023-10-03T08:03:37Z

src/DefaultUnleash.php

+            foreach ($dependencies as $dependency) {
+                $parentFeature = $parentFeatures[$dependency->getFeature()] ?? null;
+                if (!$this->isDependencySatisfied($dependency, $parentFeature, $context)) {
+                    $this->metricsHandler->handleMetrics($feature, false);


this reminds me to add a test that verifies that metrics are not counted for the parent features

kwasniew · 2023-10-03T08:21:26Z

src/DefaultUnleash.php

+
+            return [
+                'feature' => null,
+                'dependencies' => [],


actualDependencies

Tymek · 2023-10-03T12:15:09Z

Can we somehow solve it in another way? Right now the issue is that it's not possible to get feature toggles from repository multiple times in multiple places in main class. This should be doable by holding in-memory copy of the response or state of the Repository, since it's already a property on main class. Am I missing something?
@sighphyre @RikudouSage

RikudouSage · 2023-10-03T12:46:49Z

@Tymek I'm not entirely sure what this feature is about (no one told me), but it seems that features now can have a dependency on another feature. If any parent feature evaluates to false, all child features are also false. Is that a correct assumption?

In that case, why not construct it recursively? The DTOs pretty much should be the JSON response but as PHP DTOs.

There's also the method findFeature(), why can't you use that to fetch the toggles multiple times?

If you describe the issue in more detail, I might be able to assist further.

sighphyre · 2023-10-04T13:10:34Z

src/DefaultUnleash.php

+     *   dependencies: array<string, Feature>,
+     * }
+     */
+    public function findFeatureAndDependencies(string $featureName, ?Context $context): ?array


Does this have to be public?

Also, I'm not sure this is the right layer to be doing this in. Not hitting the cache twice? Yes! Doing this... less yes.

Can we not materialise this from cache in a more complete state rather? I want to say the parseFeatures method in the repository is a good target for this. That can return a DTO that looks something like (in very pseudocode)

DefaultFeature {
name: String,
parent?: DefaultFeature
}

sighphyre · 2023-10-04T13:12:41Z

src/DefaultUnleash.php

+     *   dependencies: array<string, Feature>,
+     * }
+     */
+    public function findFeatureAndDependencies(string $featureName, ?Context $context): ?array


Also, I don't think this method should take context. It's strange. It doesn't affect the outcome in any way outside of meta data in the events. Especially for a function marked public that feels off

sighphyre · 2023-10-04T13:16:19Z

src/Repository/DefaultUnleashRepository.php

@@ -83,9 +90,9 @@ public function findFeature(string $featureName): ?Feature
     * @throws ClientExceptionInterface
     * @throws JsonException
     *
-     * @return iterable<Feature>
+     * @return array<Feature>


I see we're shifted from an iterable to an array in a few places in this PR but I can't see why we would need to. What's happening here?

sighphyre

I don't believe it's enough to rely on the client specs here. This is feature that can go wrong in a lot of subtle ways, even though it's simple on the surface. The PHP implementation is always going to have details that most of the other SDK implementations don't - you know the details better than anyone, cover them with tests

If not to make me happy, then to make the quality gates on this repo happy

sighphyre

Can we also put a proper description on this PR? This is a significant feature and it's helpful for others to understand what it does, why it does it and how it works. Especially for reviewers who are seeing this feature for the first time

sighphyre · 2023-10-04T13:23:37Z

In that case, why not construct it recursively? The DTOs pretty much should be the JSON response but as PHP DTOs.

I think we can and should do that!

There's also the method findFeature(), why can't you use that to fetch the toggles multiple times?

findFeature() will load and parse everything from cache every time, it's expensive(ish) with large feature sets. I'd rather avoid it if we can. If it's hard to do, I'm happy to go with better, more understandable code over fast code. But I think just constructing the DTOs in the repo layer recursively would give us nice code that's also cheap

* refactor: load dependencies when parsing * fix: phpstan errors * fix: variant hash seed * chore: update client specification * fix: stickiness calculator type * fix: variant selection * fix: finding dependency

RikudouSage · 2023-11-24T14:08:23Z

Replaced with #187

Tymek added 4 commits September 27, 2023 12:55

update client specification

e5d3992

update spec version

0f7d34e

update DTO

8980058

feat: support feature dependencies

9d284cb

Tymek force-pushed the feat/dependent-features branch from a15a9a3 to 012093c Compare September 27, 2023 16:36

fix: type issues

38b2757

Tymek force-pushed the feat/dependent-features branch from 012093c to 38b2757 Compare September 27, 2023 16:45

Tymek added 3 commits October 2, 2023 17:51

refactor: finding feature with dependencies

958cc74

revert composer file update

7ada2d5

run fixer

919a2c6

Tymek requested review from sjaanus and sighphyre October 2, 2023 16:17

fix: getVariantForFeature

93f57fb

Tymek marked this pull request as ready for review October 2, 2023 16:24

Tymek requested a review from kwasniew October 3, 2023 07:16

kwasniew reviewed Oct 3, 2023

View reviewed changes

src/DefaultUnleash.php Show resolved Hide resolved

kwasniew reviewed Oct 3, 2023

View reviewed changes

RikudouSage reviewed Oct 3, 2023

View reviewed changes

kwasniew reviewed Oct 3, 2023

View reviewed changes

src/DefaultUnleash.php Outdated

return [

'feature' => null,

'dependencies' => [],

Copy link

kwasniew Oct 3, 2023

Choose a reason for hiding this comment

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

actualDependencies

RikudouSage self-requested a review October 3, 2023 12:47

sighphyre reviewed Oct 4, 2023

View reviewed changes

refactor: load dependencies when parsing (#178)

f9958ab

* refactor: load dependencies when parsing * fix: phpstan errors * fix: variant hash seed * chore: update client specification * fix: stickiness calculator type * fix: variant selection * fix: finding dependency

Tymek requested a review from sighphyre October 31, 2023 12:22

sighphyre added 2 commits November 8, 2023 11:12

merge main

258253f

wip: apply composer fix so we get a nice baseline

4c50856

sighphyre marked this pull request as draft November 8, 2023 09:14

sighphyre added 4 commits November 8, 2023 11:15

wip: remove strange looking null check

05a17d4

remove odd looking null coalesce

d788737

wip: revert type change from iterable to array

68c5881

wip: rollback more suspicious type changes

a338436

RikudouSage mentioned this pull request Nov 24, 2023

Feat: Dependent features #187

Merged

11 tasks

RikudouSage closed this Nov 24, 2023

RikudouSage deleted the feat/dependent-features branch January 12, 2024 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependent features #176

Dependent features #176

Tymek commented Sep 27, 2023 •

edited by sighphyre

kwasniew Oct 3, 2023

RikudouSage Oct 3, 2023

sighphyre Oct 4, 2023

kwasniew Oct 3, 2023

sighphyre Oct 4, 2023

kwasniew Oct 3, 2023

kwasniew Oct 3, 2023

Tymek commented Oct 3, 2023 •

edited

RikudouSage commented Oct 3, 2023

sighphyre Oct 4, 2023

sighphyre Oct 4, 2023

sighphyre Oct 4, 2023

sighphyre left a comment

sighphyre left a comment

sighphyre commented Oct 4, 2023

RikudouSage commented Nov 24, 2023

Dependent features #176

Dependent features #176

Conversation

Tymek commented Sep 27, 2023 • edited by sighphyre

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tymek commented Oct 3, 2023 • edited

RikudouSage commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

sighphyre commented Oct 4, 2023

RikudouSage commented Nov 24, 2023

Tymek commented Sep 27, 2023 •

edited by sighphyre

Tymek commented Oct 3, 2023 •

edited