From 3db17e3a4a754f47fcc41225755d7f24dd52d0e7 Mon Sep 17 00:00:00 2001 From: William Mak Date: Wed, 15 Sep 2021 16:37:42 -0400 Subject: [PATCH 1/4] fix(features): Skip the entity handler after batch checking - This adds the ability to skip the entity handler entirely when calling features.has - This is becaue when there are a lot of non entity features to check we're actually spending time checking that the feature is indeed not an entity feature --- src/sentry/api/serializers/models/organization.py | 3 ++- src/sentry/features/handler.py | 2 +- src/sentry/features/manager.py | 6 ++++-- tests/sentry/features/test_manager.py | 10 +++++++++- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index 9d880cc072d69f..7774f560934dbb 100644 --- a/src/sentry/api/serializers/models/organization.py +++ b/src/sentry/api/serializers/models/organization.py @@ -136,7 +136,8 @@ def serialize(self, obj, attrs, user): org_features.remove(feature_name) for feature_name in org_features: - if features.has(feature_name, obj, actor=user): + # Skip the entity handler entirely since batch_has would've checked it + if features.has(feature_name, obj, skip_entity=True, actor=user): # Remove the organization scope prefix feature_list.add(feature_name[len(_ORGANIZATION_SCOPE_PREFIX) :]) diff --git a/src/sentry/features/handler.py b/src/sentry/features/handler.py index 42de302829672f..242341be7e83ad 100644 --- a/src/sentry/features/handler.py +++ b/src/sentry/features/handler.py @@ -19,7 +19,7 @@ def __call__(self, feature: "Feature", actor: "User") -> Optional[bool]: return self.has(feature, actor) @abc.abstractmethod - def has(self, feature: "Feature", actor: "User") -> bool: + def has(self, feature: "Feature", actor: "User", skip_entity: Optional[bool] = False) -> bool: raise NotImplementedError def has_for_batch(self, batch: "FeatureCheckBatch") -> Mapping["Project", bool]: diff --git a/src/sentry/features/manager.py b/src/sentry/features/manager.py index e33e7fb46d6ddc..4aae60accb93a3 100644 --- a/src/sentry/features/manager.py +++ b/src/sentry/features/manager.py @@ -174,7 +174,9 @@ def add_entity_handler(self, handler: "FeatureHandler") -> None: """ self._entity_handler = handler - def has(self, name: str, *args: Any, **kwargs: Any) -> bool: + def has( + self, name: str, *args: Any, skip_entity: Optional[bool] = False, **kwargs: Any + ) -> bool: """ Determine if a feature is enabled. If a handler returns None, then the next mechanism is used for feature checking. @@ -213,7 +215,7 @@ def has(self, name: str, *args: Any, **kwargs: Any) -> bool: if rv is not None: return rv - if self._entity_handler: + if self._entity_handler and not skip_entity: rv = self._entity_handler.has(feature, actor) if rv is not None: return rv diff --git a/tests/sentry/features/test_manager.py b/tests/sentry/features/test_manager.py index 4f44ddbafd65ff..de04a8b87bb245 100644 --- a/tests/sentry/features/test_manager.py +++ b/tests/sentry/features/test_manager.py @@ -13,7 +13,10 @@ class MockBatchHandler(features.BatchFeatureHandler): features = frozenset(["auth:register", "organizations:feature", "projects:feature"]) def has( - self, feature: Feature, actor: User + self, + feature: Feature, + actor: User, + skip_entity: Optional[bool] = False, ) -> Union[Optional[bool], Mapping[str, Optional[bool]]]: return {feature.name: True} @@ -117,6 +120,11 @@ def test_entity_handler(self): assert len(entity_handler.has.mock_calls) == 1 assert len(registered_handler.mock_calls) == 1 + # The feature isn't registered, but lets skip the entity_handler + manager.has("organizations:unregistered-feature", test_org, skip_entity=True) + assert len(entity_handler.has.mock_calls) == 1 + assert len(registered_handler.mock_calls) == 1 + # The entity_handler doesn't have a response for this feature either, so settings should be checked instead entity_handler.has.return_value = None settings.SENTRY_FEATURES["organizations:settings-feature"] = "test" From 8fb05e00aa7382eec8ac89290ae60aa67c902dc6 Mon Sep 17 00:00:00 2001 From: William Mak Date: Fri, 17 Sep 2021 13:51:19 -0400 Subject: [PATCH 2/4] fix: Typing --- src/sentry/features/handler.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/features/handler.py b/src/sentry/features/handler.py index 242341be7e83ad..1f81c66ad1711f 100644 --- a/src/sentry/features/handler.py +++ b/src/sentry/features/handler.py @@ -54,7 +54,7 @@ def _check_for_batch( ) -> bool: raise NotImplementedError - def has(self, feature: "Feature", actor: "User") -> bool: + def has(self, feature: "Feature", actor: "User", skip_entity: Optional[bool] = False) -> bool: return self._check_for_batch(feature.name, feature.get_organization(), actor) def has_for_batch(self, batch: "FeatureCheckBatch") -> Mapping["Project", bool]: From 161e568fce5ef247b1f0c9df84c39bab3c201b72 Mon Sep 17 00:00:00 2001 From: William Mak Date: Fri, 17 Sep 2021 14:15:55 -0400 Subject: [PATCH 3/4] ref: Update comments to be clearer --- src/sentry/api/serializers/models/organization.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index 7774f560934dbb..a007bd22e42a0a 100644 --- a/src/sentry/api/serializers/models/organization.py +++ b/src/sentry/api/serializers/models/organization.py @@ -123,6 +123,7 @@ def serialize(self, obj, attrs, user): ] feature_list = set() + # Check features in batch using the entity handler batch_features = features.batch_has(org_features, actor=user, organization=obj) # batch_has has found some features @@ -135,8 +136,8 @@ def serialize(self, obj, attrs, user): # This feature_name was found via `batch_has`, don't check again using `has` org_features.remove(feature_name) + # Remaining features should not be checked via the entity handler for feature_name in org_features: - # Skip the entity handler entirely since batch_has would've checked it if features.has(feature_name, obj, skip_entity=True, actor=user): # Remove the organization scope prefix feature_list.add(feature_name[len(_ORGANIZATION_SCOPE_PREFIX) :]) From 75da6df554b229a004337f1b09bb352876b468c4 Mon Sep 17 00:00:00 2001 From: William Mak Date: Wed, 22 Sep 2021 12:50:57 -0400 Subject: [PATCH 4/4] reorder args --- src/sentry/api/serializers/models/organization.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index a007bd22e42a0a..6ec033f52648e7 100644 --- a/src/sentry/api/serializers/models/organization.py +++ b/src/sentry/api/serializers/models/organization.py @@ -138,7 +138,7 @@ def serialize(self, obj, attrs, user): # Remaining features should not be checked via the entity handler for feature_name in org_features: - if features.has(feature_name, obj, skip_entity=True, actor=user): + if features.has(feature_name, obj, actor=user, skip_entity=True): # Remove the organization scope prefix feature_list.add(feature_name[len(_ORGANIZATION_SCOPE_PREFIX) :])