diff --git a/api/features/serializers.py b/api/features/serializers.py index e6e1e577c85c..5cfeca4e76a5 100644 --- a/api/features/serializers.py +++ b/api/features/serializers.py @@ -360,6 +360,9 @@ def validate_name(self, name: str): # type: ignore[no-untyped-def] project = self.context["project"] feature_name_regex = project.feature_name_regex + if project.only_allow_lower_case_feature_names and name != name.lower(): + raise serializers.ValidationError("Feature name must be lower case.") + if not project.is_feature_name_valid(name): raise serializers.ValidationError( f"Feature name must match regex: {feature_name_regex}" diff --git a/api/projects/models.py b/api/projects/models.py index f56e43f63856..0cd00c37d77d 100644 --- a/api/projects/models.py +++ b/api/projects/models.py @@ -199,13 +199,17 @@ def live_segment_count(self) -> int: def is_feature_name_valid(self, feature_name: str) -> bool: """ - Validate the feature name based on the feature_name_regex attribute. + Validate the feature name based on the only_allow_lower_case_feature_names and + feature_name_regex attributes. Since we always want to evaluate the regex against the whole string, we're wrapping the attribute value in ^(...)$. Note that ^(...)$ and ^^(...)$$ are equivalent (in case the attribute already has the boundaries defined) """ return ( + not self.only_allow_lower_case_feature_names + or feature_name == feature_name.lower() + ) and ( not self.feature_name_regex or re.match(f"^{self.feature_name_regex}$", feature_name) is not None ) diff --git a/api/tests/unit/features/test_unit_features_views.py b/api/tests/unit/features/test_unit_features_views.py index cf7554904ed5..a7e113d4b657 100644 --- a/api/tests/unit/features/test_unit_features_views.py +++ b/api/tests/unit/features/test_unit_features_views.py @@ -641,7 +641,7 @@ def test_create_feature__dynamo_enabled__triggers_single_dynamo_write( project.save() url = reverse("api-v1:projects:project-features-list", args=[project.id]) - data = {"name": "Test feature flag", "type": STANDARD, "project": project.id} + data = {"name": "test_feature_flag", "type": STANDARD, "project": project.id} mock_dynamo_environment_wrapper.is_enabled = True mock_dynamo_environment_wrapper.reset_mock() @@ -1903,6 +1903,54 @@ def test_create_feature__name_does_not_match_regex__returns_400( ) +def test_create_feature__lower_case_only_and_mixed_case_name__returns_400( + admin_client_new: APIClient, project: Project +) -> None: + # Given + # only_allow_lower_case_feature_names defaults to True + url = reverse("api-v1:projects:project-features-list", args=[project.id]) + data = {"name": "MixedCaseFeature", "type": STANDARD, "project": project.id} + + # When + response = admin_client_new.post(url, data=data) + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["name"][0] == "Feature name must be lower case." + + +def test_create_feature__lower_case_only_and_lower_case_name__returns_201( + admin_client_new: APIClient, project: Project +) -> None: + # Given + # only_allow_lower_case_feature_names defaults to True + url = reverse("api-v1:projects:project-features-list", args=[project.id]) + data = {"name": "lower_case_feature", "type": STANDARD, "project": project.id} + + # When + response = admin_client_new.post(url, data=data) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + +def test_create_feature__lower_case_only_disabled_and_mixed_case_name__returns_201( + admin_client_new: APIClient, project: Project +) -> None: + # Given + project.only_allow_lower_case_feature_names = False + project.save() + + url = reverse("api-v1:projects:project-features-list", args=[project.id]) + data = {"name": "MixedCaseFeature", "type": STANDARD, "project": project.id} + + # When + response = admin_client_new.post(url, data=data) + + # Then + assert response.status_code == status.HTTP_201_CREATED + + def test_create_feature__valid_data__creates_audit_log( admin_client_new: APIClient, project: Project, @@ -1910,7 +1958,7 @@ def test_create_feature__valid_data__creates_audit_log( ) -> None: # Given url = reverse("api-v1:projects:project-features-list", args=[project.id]) - data = {"name": "Test feature flag", "type": STANDARD, "project": project.id} + data = {"name": "test_feature_flag", "type": STANDARD, "project": project.id} # When response = admin_client_new.post(url, data=data) @@ -1997,7 +2045,7 @@ def test_create_feature__with_tags__creates_tagged_feature( ) -> None: # Given - set up data default_value = "Test" - feature_name = "Test feature" + feature_name = "test_feature" data = { "name": feature_name, "project": project.id, @@ -3381,7 +3429,7 @@ def test_create_feature__missing_required_metadata__returns_400( url = reverse("api-v1:projects:project-features-list", args=[project.id]) description = "This is the description" data = { - "name": "Test feature", + "name": "test_feature", "description": description, } @@ -3406,7 +3454,7 @@ def test_create_feature__with_optional_metadata__returns_201( description = "This is the description" field_value = 10 data = { - "name": "Test feature", + "name": "test_feature", "description": description, "metadata": [ { @@ -3442,7 +3490,7 @@ def test_create_feature__with_required_metadata__returns_201( description = "This is the description" field_value = 10 data = { - "name": "Test feature", + "name": "test_feature", "description": description, "metadata": [ { @@ -3478,7 +3526,7 @@ def test_create_feature__required_metadata_org_content_type__returns_201( description = "This is the description" field_value = 10 data = { - "name": "Test feature", + "name": "test_feature", "description": description, "metadata": [ { @@ -4916,7 +4964,7 @@ def test_create_feature__duplicate_metadata_id__keeps_metadata_isolated( # Create first feature with metadata first_feature_data = { - "name": "First Feature", + "name": "first_feature", "description": "First feature description", "metadata": [ { @@ -4941,7 +4989,7 @@ def test_create_feature__duplicate_metadata_id__keeps_metadata_isolated( # Given - Create second feature second_feature_data = { - "name": "Second Feature", + "name": "second_feature", "description": "Second feature description", "metadata": [ { @@ -5013,7 +5061,7 @@ def test_create_feature__required_metadata_on_other_project__returns_201( model_field=model_field, ) url = reverse("api-v1:projects:project-features-list", args=[project.id]) - data = {"name": "Test feature cross project", "description": "desc"} + data = {"name": "test_feature_cross_project", "description": "desc"} # When response = admin_client.post( @@ -5082,7 +5130,7 @@ def test_create_feature__type_provided__validates_and_sets_type( ) -> None: # Given url = reverse("api-v1:projects:project-features-list", args=[project.id]) - data = {"name": f"test_feature_{feature_type}", "type": feature_type} + data = {"name": f"test_feature_{feature_type.lower()}", "type": feature_type} # When response = admin_client_new.post( diff --git a/api/tests/unit/projects/test_unit_projects_models.py b/api/tests/unit/projects/test_unit_projects_models.py index 21842c1646c3..4dde94f10bff 100644 --- a/api/tests/unit/projects/test_unit_projects_models.py +++ b/api/tests/unit/projects/test_unit_projects_models.py @@ -142,6 +142,31 @@ def test_is_feature_name_valid__regex_and_name__returns_expected( # type: ignor assert result == expected_result +@pytest.mark.parametrize( + "only_allow_lower_case_feature_names, feature_name, expected_result", + ( + (True, "lowercasefeature", True), + (True, "MixedCaseFeature", False), + (True, "UPPERCASEFEATURE", False), + (False, "MixedCaseFeature", True), + ), +) +def test_is_feature_name_valid__lower_case_setting_and_name__returns_expected( # type: ignore[no-untyped-def] + only_allow_lower_case_feature_names, feature_name, expected_result +): + # Given + project = Project( + name="test", + only_allow_lower_case_feature_names=only_allow_lower_case_feature_names, + ) + + # When + result = project.is_feature_name_valid(feature_name) + + # Then + assert result == expected_result + + def test_save_project__name_updated__clears_environment_caches( # type: ignore[no-untyped-def] environment, project, mocker ):