Browse Source

fix(options): Fix `ProjectOption.isset` (#62036)

Currently, `ProjectOptionsManager.isset` always returns `True`, whether or not the given option is set on the given project. This happens because the signature of `Project.get_option` is 

```
def get_option(self, key, default, validate=None)
```

but in `isset` we're calling it like 

```
project.get_option(project, key, Ellipsis)
```

making `key` the value of the `default` parameter. Thus, when the option in question isn't set and the default is returned, what's returned is `key` rather than `Ellipses`. We then compare that result to `Ellipses`, and since they never match, we never detect that the default has been returned and the option isn't set. (I suspect this was never caught because we don't currently use `isset` anywhere. I only noticed because I'm planning to use it in an upcoming PR.)

This fixes the `get_option` call (and passes `default` as a kwarg, just for good measure). It also un-xfails the relevant tests, since the behavior is now correct.
Katie Byers 1 year ago
parent
commit
a4eead7789
2 changed files with 1 additions and 13 deletions
  1. 1 1
      src/sentry/projectoptions/manager.py
  2. 0 12
      tests/sentry/projectoptions/test_basic.py

+ 1 - 1
src/sentry/projectoptions/manager.py

@@ -56,7 +56,7 @@ class ProjectOptionsManager:
         return ProjectOption.objects.set_value(project, key, value)
 
     def isset(self, project, key):
-        return project.get_option(project, key, Ellipsis) is not Ellipsis
+        return project.get_option(key, default=Ellipsis) is not Ellipsis
 
     def get(self, project, key, default=None, validate=None):
         from sentry.models.options.project_option import ProjectOption

+ 0 - 12
tests/sentry/projectoptions/test_basic.py

@@ -1,7 +1,5 @@
 from unittest import mock
 
-import pytest
-
 from sentry.models.options.project_option import ProjectOption
 from sentry.projectoptions import default_manager, defaults
 from sentry.projectoptions.manager import WellKnownProjectOption
@@ -51,11 +49,6 @@ def test_epoch_defaults():
     assert option.get_default(epoch=100) == "latest-value"
 
 
-@pytest.mark.xfail(
-    reason="`ProjectOption.isset` always returns True",
-    raises=AssertionError,
-    strict=True,
-)
 @django_db_all
 def test_isset_simple(default_project):
     default_manager.register("best_dogs", default="all dogs")
@@ -68,11 +61,6 @@ def test_isset_simple(default_project):
     assert default_manager.isset(default_project, "best_dogs") is True
 
 
-@pytest.mark.xfail(
-    reason="`ProjectOption.isset` always returns True",
-    raises=AssertionError,
-    strict=True,
-)
 @django_db_all
 def test_isset_differentiates_unset_from_set_to_default(default_project):
     default_manager.register("best_dogs", default="all dogs")