diff --git a/src/sentry/api/serializers/models/organization.py b/src/sentry/api/serializers/models/organization.py index 9d880cc072d69f..6ec033f52648e7 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,9 @@ 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: - if features.has(feature_name, obj, 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) :]) diff --git a/src/sentry/features/handler.py b/src/sentry/features/handler.py index 42de302829672f..1f81c66ad1711f 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]: @@ -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]: 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"