Refactor strategy instantiation for more extensitiliby #1254

adamsachs · 2022-09-02T21:54:34Z

Purpose

Make our strategies more extensible by creating a Strategy abstract class using the builtin __subclasses__() method to find and instantiate Strategy subclases.

Before this change, many of our strategies (with the exception of MaskingStrategys, which had been refactored in #560) were registered by means of a hardcoded enums in the core fidesops codebase. If a developer wanted to implement their own strategy, it required an update to the core fidesops codebase.

With this change, developers outside of core fidesops can implement their own strategy (whether that's an AuthenticationStrategy, MaskingStrategy, PaginationStrategy, or PostProcessorStrategy) and leverage it in the system by simply importing their subclass and ensuring that it defines a unique name class variable, along with a configuration_model variable pointing to the strategy's pydantic configuration class. As an example:

class SomeStrategyConfiguration(StrategyConfiguration):
    some_key: str = "default value"

class SomeStrategy(PostProcessorStrategy):
    name = "some postprocessor strategy"
    configuration_model = SomeStrategyConfiguration

    def __init__(self, configuration: SomeStrategyConfiguration):
        self.some_config = configuration.some_key

    def process(
        self, data: Any, identity_data: Dict[str, Any] = None
    ) -> Union[List[Dict[str, Any]], Dict[str, Any]]:
        pass

Changes

an abstract base class Strategy that defines logic for strategy retrieval and instantiation, through a generic get_strategy method
- update existing strategy types to inherit from this new base class.
- the base class defines standardized class variables name and configuration_model that are used to identify and instantiate strategy subtypes in a consistent manner
remove existing strategy factories as they are no longer needed; update references in the codebase to use the new get_strategy method for the corresponding Strategy subtype

Checklist

Ticket

Fixes #562

adamsachs · 2022-09-05T17:29:11Z

the shopify extrnal unsafe integration test is failing but i don't think that's related to my changes?

looks like unsafe tests were never run on the shopify PR that was merged a few days ago, and looks like the unsafe tests have failed all runs since then, besides for @galvana's draft PR which actually seems like it is amending the shopify issue.

@galvana is that accurate? if so, then i think we can ignore the failure here...

adamsachs · 2022-09-06T14:18:38Z

src/fidesops/ops/migrations/versions/55d61eb8ed12_add_default_policies.py

@@ -48,6 +45,7 @@
 FIDESOPS_AUTOGENERATED_STORAGE_KEY = "fidesops_autogenerated_storage_destination"
 AUTOGENERATED_ACCESS_KEY = "download"
 AUTOGENERATED_ERASURE_KEY = "delete"
+STRING_REWRITE_STRATEGY_NAME = "string_rewrite"


is it OK to edit this file by hand? i hesitated before doing so since it's a generated migration file...

This is fine since it's functionally equivalent, we're just cleaning up the constants.

Worth noting that this file wasn't actually auto-generated either, it's a data migration instead of a schema migration and automatically adds several rows to multiple tables in the fidesops application database!

oh, nice - thanks for clarifying that @pattisdr!

src/fidesops/ops/service/strategy.py

galvana · 2022-09-06T16:06:02Z

@adamsachs, yes, we can ignore the Shopify issues as part of this ticket

sanders41 · 2022-09-06T16:18:01Z

I believe I found the shopify issue and fixed it as part of #1260 🤞

pattisdr

Nice work, main thing is fixing up _find_all_strategy_subclasses - the latest commit broke this, and adding tests around it. I do like this implementation generally.

pattisdr · 2022-09-06T22:35:02Z

tests/ops/api/v1/endpoints/test_masking_endpoints.py

    def test_read_strategies(self, api_client: TestClient):
        expected_response = []
-        for strategy in MaskingStrategyFactory.get_strategies():
+        for strategy in MaskingStrategy.get_strategies():
            expected_response.append(strategy.get_description())

        response = api_client.get(V1_URL_PREFIX + MASKING_STRATEGY)


This test doesn't pick up the error. Currently get_strategies is broken and returns an empty list, but the expected_response here is also incorrectly an empty list

yeah, i've been a bit uncertain about this test since i first saw it. but i'm also not really sure on the best way to resolve it - i feel like any approach to getting a "true" list of strategies is either going to rely on duplicating the logic of get_strategies(), or it's going to be prone to false negatives if/when more masking strategies are added to the testing runtime (whether that's core fidesops updates or, potentially, some extended runtime like -plus).

obviously we need more robust testing for get_strategies(), as you've correctly pointed out. but what do you think about keeping this as is, and just focusing on firming up the tests around get_strategies() itself?

The get_strategies() improvement was the main thing, but perhaps asserting that the response is non-empty at least?

src/fidesops/ops/service/strategy.py

adamsachs · 2022-09-07T13:02:22Z

@ethyca/docs-authors would you be able to take a quick look at the docs changes here? this PR is instead of #1163, as we decided on a different implementation approach -- sorry for the double-work! let me know if you've got suggestions.

i've also tweaked the description of related docs ticket #1169 to reference this PR, rather than the now outdated #1163

pattisdr

Good work @adamsachs 🏆

A small improvement might be adding #1254 (comment), but otherwise this looks good to me.

I'll let your team take care of merging in case there's more left to do -

conceptualshark

Just a little tweak - otherwise this looks good!

conceptualshark · 2022-09-07T18:06:44Z

docs/fidesops/docs/guides/masking_strategies.md

-In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be registered with the `MaskingStrategyFactory`. To register a new `MaskingStrategy`, use the `register` decorator on the `MaskingStrategy` subclass definition, as shown in the above example.
-
-The value passed as the argument to the decorator must be the registered name of the `MaskingStrategy` subclass. This is the same value defined by [callers](#using-fidesops-as-a-masking-service) in the `"masking_strategy"."strategy"` field.
+In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be imported into the application runtime. Also, the `MaskingStrategy` class must define two class variables: `name`, which is the unique, registered name that callers [callers](#using-fidesops-as-a-masking-service) will use in their `"masking_strategy"."strategy"` field to invoke the strategy; and `configuration_model`, which references the configuration class used to parameterize the strategy.


Suggested change

In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be imported into the application runtime. Also, the `MaskingStrategy` class must define two class variables: `name`, which is the unique, registered name that callers [callers](#using-fidesops-as-a-masking-service) will use in their `"masking_strategy"."strategy"` field to invoke the strategy; and `configuration_model`, which references the configuration class used to parameterize the strategy.

In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be imported into the application runtime. Also, the `MaskingStrategy` class must define two class variables: `name`, which is the unique, registered name that [callers](#using-fidesops-as-a-masking-service) will use in their `"masking_strategy"."strategy"` field to invoke the strategy; and `configuration_model`, which references the configuration class used to parameterize the strategy.

A generalized Strategy abstract base class provides generalized getter methods that instantiate strategy subclasses (implementations). These methods rely on the builtin __subclasses__() method to identify Strategy subclasses, which allows for more dynamic and extensible strategy implementation, removing the need for a hardcoded enumeration of supported Strategy implementations. Abstract strategy types inherit from this new abstract base class, and strategy subclasses (implementations) must provide `name` and `configuration_model` attributes that are leveraged by new instantiation mechanism in the abstract base class.

This allows the method to leverage the new `name` class variable rather than relying on a static constant variable.

Strategy factories are no longer needed with refactored Strategy getters. Update the uses (references) of strategy factories throughout the codebase to now rely on the new Strategy getters. Strategy subclasses (implementations) now need to be imported explicitly in __init__.py's because they used to be imported in factory modules. Also remove the old MaskingStrategy registration/factory mechanisms.

Now that the abstract Strategy base class enforces implementation subclasses to have a `name` class attribute, this attribute should be relied upon rather than the arbitrary name constants declared previously. The get_strategy_name() abstract method is also superfluous, as the `name` class attribute can be used as a standardized way to retrieve the strategy name.

The generalized strategy getter now relies upon the `configuration_model` class variable that's on each Strategy. Therefore we no longer need the get_configuration_model() getter on each Strategy subclass.

Update associated tests to make sure the recursion is properly tested

galvana

I like this approach! I also like how there's a distinction between the different strategy "types" instead of just one combined pool of strategies.

* Instantiate strategies via abstract Strategy base class A generalized Strategy abstract base class provides generalized getter methods that instantiate strategy subclasses (implementations). These methods rely on the builtin __subclasses__() method to identify Strategy subclasses, which allows for more dynamic and extensible strategy implementation, removing the need for a hardcoded enumeration of supported Strategy implementations. Abstract strategy types inherit from this new abstract base class, and strategy subclasses (implementations) must provide `name` and `configuration_model` attributes that are leveraged by new instantiation mechanism in the abstract base class. * Update get_description() to be a class rather than static method This allows the method to leverage the new `name` class variable rather than relying on a static constant variable. * Remove strategy factories and update references Strategy factories are no longer needed with refactored Strategy getters. Update the uses (references) of strategy factories throughout the codebase to now rely on the new Strategy getters. Strategy subclasses (implementations) now need to be imported explicitly in __init__.py's because they used to be imported in factory modules. Also remove the old MaskingStrategy registration/factory mechanisms. * Remove strategy name constants Now that the abstract Strategy base class enforces implementation subclasses to have a `name` class attribute, this attribute should be relied upon rather than the arbitrary name constants declared previously. The get_strategy_name() abstract method is also superfluous, as the `name` class attribute can be used as a standardized way to retrieve the strategy name. * Remove get_configuration_model() abstract method The generalized strategy getter now relies upon the `configuration_model` class variable that's on each Strategy. Therefore we no longer need the get_configuration_model() getter on each Strategy subclass. * Update MaskingStrategy docs with new Strategy functionality * Update changelog * Improve recursion in _find_all_strategy_subclasses * Fix recursion bug when finding all strategies Update associated tests to make sure the recursion is properly tested * Tweak conditional for falsy check * Make get_strategies endpoint test more robust * Fix typo in documentation Co-authored-by: Adam Sachs <[email protected]>

adamsachs added run unsafe ci checks Triggers running of unsafe CI checks Needs doc review SaaS Connector The issue indicates development work for a specific SaaS application labels Sep 2, 2022

adamsachs requested a review from pattisdr September 2, 2022 21:54

adamsachs self-assigned this Sep 2, 2022

adamsachs requested a review from galvana September 2, 2022 21:55

adamsachs mentioned this pull request Sep 2, 2022

unified strategy factory with decorator to register strategies #1163

Closed

10 tasks

adamsachs changed the title ~~562 refactor strategy instantiation for more extensitiliby~~ Refactor strategy instantiation for more extensitiliby Sep 2, 2022

adamsachs force-pushed the 562-subclasses-builtin branch from 7879b07 to 765b6b2 Compare September 6, 2022 14:16

adamsachs commented Sep 6, 2022

View reviewed changes

src/fidesops/ops/service/strategy.py Show resolved Hide resolved

adamsachs force-pushed the 562-subclasses-builtin branch from 50723e7 to fc0fa76 Compare September 6, 2022 19:00

pattisdr suggested changes Sep 6, 2022

View reviewed changes

adamsachs force-pushed the 562-subclasses-builtin branch from c0a48d2 to 5f0241a Compare September 7, 2022 12:50

adamsachs mentioned this pull request Sep 7, 2022

Document how developers can implement their own "strategy" #1169

Open

pattisdr approved these changes Sep 7, 2022

View reviewed changes

conceptualshark reviewed Sep 7, 2022

View reviewed changes

Adam Sachs added 9 commits September 7, 2022 14:54

Update get_description() to be a class rather than static method

991a2e0

This allows the method to leverage the new `name` class variable rather than relying on a static constant variable.

Remove get_configuration_model() abstract method

7204d11

The generalized strategy getter now relies upon the `configuration_model` class variable that's on each Strategy. Therefore we no longer need the get_configuration_model() getter on each Strategy subclass.

Update MaskingStrategy docs with new Strategy functionality

244d67d

Update changelog

d3221ec

Improve recursion in _find_all_strategy_subclasses

44ece6b

Fix recursion bug when finding all strategies

c8001aa

Update associated tests to make sure the recursion is properly tested

Adam Sachs added 3 commits September 7, 2022 14:54

Tweak conditional for falsy check

6ed4701

Make get_strategies endpoint test more robust

393814e

Fix typo in documentation

48ecfa0

adamsachs force-pushed the 562-subclasses-builtin branch from 32da9b5 to 48ecfa0 Compare September 7, 2022 18:54

galvana approved these changes Sep 7, 2022

View reviewed changes

adamsachs merged commit f478a6a into main Sep 7, 2022

adamsachs deleted the 562-subclasses-builtin branch September 7, 2022 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor strategy instantiation for more extensitiliby #1254

Refactor strategy instantiation for more extensitiliby #1254

adamsachs commented Sep 2, 2022 •

edited

Loading

adamsachs commented Sep 5, 2022

adamsachs Sep 6, 2022

galvana Sep 6, 2022

pattisdr Sep 6, 2022

adamsachs Sep 6, 2022

galvana commented Sep 6, 2022

sanders41 commented Sep 6, 2022

pattisdr left a comment

pattisdr Sep 6, 2022

adamsachs Sep 7, 2022

pattisdr Sep 7, 2022

adamsachs commented Sep 7, 2022 •

edited

Loading

pattisdr left a comment

conceptualshark left a comment

conceptualshark Sep 7, 2022

galvana left a comment

Refactor strategy instantiation for more extensitiliby #1254

Refactor strategy instantiation for more extensitiliby #1254

Conversation

adamsachs commented Sep 2, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

adamsachs commented Sep 5, 2022

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

galvana commented Sep 6, 2022

sanders41 commented Sep 6, 2022

pattisdr left a 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

adamsachs commented Sep 7, 2022 • edited Loading

pattisdr left a comment

Choose a reason for hiding this comment

conceptualshark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galvana left a comment

Choose a reason for hiding this comment

adamsachs commented Sep 2, 2022 •

edited

Loading

adamsachs commented Sep 7, 2022 •

edited

Loading