Repository: Unleash/unleash-client-python
Pull request #127 (2021-02-04 16:12:05)

Fix issue with updating in-memory variants in the loader
# Description

When the new features are downloaded, the loader first updates the file cache before updating
the in-memory cache.

This PR attempts to fix an issue where the loader did not properly update the variants part of the cache.
This caused the variants to be stale until the next cache initialisation on client restart.

## Type of change

Please delete options that are not relevant.

- [x] Bug fix (non-breaking change which fixes an issue)

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

- [x] Unit tests
- [ ] Spec Tests
- [ ] Integration tests / Manual Tests

# Checklist:

- [x] My code follows the style guidelines of this project
- [x] 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
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published in downstream modules

ivarconr: Awesome, thanks! 
Funny that these bugs are allowed to live for so long! 

All in all a good catch. I would however prefer that the internal of Feature just used `variants` instead of `variations` (https://github.com/Unleash/unleash-client-python/blob/master/UnleashClient/features/Feature.py#L24). This would have kept the naming of things close the the protocol, and we would probably not have fooled ourselves. 
ivanklee86: @simenaasland Ack, good catch!  Thanks for the PR!

@ivarconr I can certainly line up the names better, though I'd prefer to do that in a separate PR.  I'll get to it in the next few days.

I'll do a bugfix release later today/early tomorrow with this fix. :)
simenaasland: @ivarconr @ivanklee86 Great that the fix is appreciated, happy to help! It makes sense to use `variants` in `Feature`. We considered changing it that way, and can amend the PR now. Totally up to you.

@ivanklee86 Thanks a lot for making a quick bugfix release! Let us know if you want us to do any renaming here, or if you want to take care of that. :)
ivanklee86: If you've got time, I'd be more than happy to include the renaming in this PR. :) If not, I'll try and tackle it next time I get some coding time. ^^
simenaasland: > If you've got time, I'd be more than happy to include the renaming in this PR. :) If not, I'll try and tackle it next time I get some coding time. ^^

I have time to fix it. However, having thought about it a bit more, I'm wondering if there is any risk involved in changing the attribute name without keeping the old one. If external users for some reason looked up the old attribute on Feature, it could be a problem. I assume this is unlikely, but I wanted to confirm with you before making a potential breaking change. :)
ivanklee86: Hmm, fair point. 

I'd propose:
* Making a bugfix on the current semver (3.6.1) to fix the problem for everyone.
* Rename the variable in the next major version bump (4.x.x) - there's another deprecation queued up.

It's not the best reason to do a major version bump, but I think it'd be OK to clearly communicate to users that there's backwards incompatible changes.

Does that make sense?  If so, I think we can merge this PR (to fix the issue)  and I can create a v4.0.0 branch for the next major release.
simenaasland: >>> Does that make sense? If so, I think we can merge this PR (to fix the issue) and I can create a v4.0.0 branch for the next major release.

That sounds like a good plan to me. :)

Patch:
diff --git a/UnleashClient/loader.py b/UnleashClient/loader.py
index eecbf70..b16db8c 100644
--- a/UnleashClient/loader.py
+++ b/UnleashClient/loader.py
@@ -95,7 +95,7 @@ def load_features(cache: FileCache,
                 feature_for_update.strategies = parsed_strategies
 
             if 'variants' in parsed_features[feature]:
-                feature_for_update.variants = Variants(
+                feature_for_update.variations = Variants(
                     parsed_features[feature]['variants'],
                     parsed_features[feature]['name']
                 )


Test patch:
diff --git a/tests/unit_tests/test_loader.py b/tests/unit_tests/test_loader.py
index 752456e..8ebb37b 100644
--- a/tests/unit_tests/test_loader.py
+++ b/tests/unit_tests/test_loader.py
@@ -8,9 +8,6 @@ from tests.utilities.mocks import MOCK_ALL_FEATURES
 from tests.utilities.testing_constants import DEFAULT_STRATEGY_MAPPING
 from tests.utilities.decorators import cache_full, cache_custom  # noqa: F401
 
-MOCK_UPDATED = copy.deepcopy(MOCK_ALL_FEATURES)
-MOCK_UPDATED["features"][4]["strategies"][0]["parameters"]["percentage"] = 60
-
 
 def test_loader_initialization(cache_full):  # noqa: F811
     # Set up variables
@@ -41,7 +38,7 @@ def test_loader_initialization(cache_full):  # noqa: F811
             assert strategy.variants
 
 
-def test_loader_refresh(cache_full):  # noqa: F811
+def test_loader_refresh_strategies(cache_full):  # noqa: F811
     # Set up variables
     in_memory_features = {}
     temp_cache = cache_full
@@ -49,7 +46,9 @@ def test_loader_refresh(cache_full):  # noqa: F811
     load_features(temp_cache, in_memory_features, DEFAULT_STRATEGY_MAPPING)
 
     # Simulate update mutation
-    temp_cache[FEATURES_URL] = MOCK_UPDATED
+    mock_updated = copy.deepcopy(MOCK_ALL_FEATURES)
+    mock_updated["features"][4]["strategies"][0]["parameters"]["percentage"] = 60
+    temp_cache[FEATURES_URL] = mock_updated
     temp_cache.sync()
 
     load_features(temp_cache, in_memory_features, DEFAULT_STRATEGY_MAPPING)
@@ -58,6 +57,24 @@ def test_loader_refresh(cache_full):  # noqa: F811
     assert len(temp_cache[FAILED_STRATEGIES]) == 1
 
 
+def test_loader_refresh_variants(cache_full):  # noqa: F811
+    # Set up variables
+    in_memory_features = {}
+    temp_cache = cache_full
+
+    load_features(temp_cache, in_memory_features, DEFAULT_STRATEGY_MAPPING)
+
+    # Simulate update mutation
+    mock_updated = copy.deepcopy(MOCK_ALL_FEATURES)
+    mock_updated["features"][8]["variants"][0]["name"] = "VariantA"
+    temp_cache[FEATURES_URL] = mock_updated
+    temp_cache.sync()
+
+    load_features(temp_cache, in_memory_features, DEFAULT_STRATEGY_MAPPING)
+
+    assert in_memory_features["Variations"].variations.variants[0]["name"] == "VariantA"
+
+
 def test_loader_initialization_failure(cache_custom):  # noqa: F811
     # Set up variables
     in_memory_features = {}