Fix/489 auto enum value allowed chars #494

fabien-michel · 2024-03-05T10:01:06Z

Use regex to replace invalid characters in auto generated enums.

This occur because django's choice values are strings and can contain any chars.

Exemple This

class Choice(models.TextChoices):
   EN_US = "en-US", "English"

Choice.choices return (("en-US", "English"),), and hyphen is not a valid char for GraphQL naming convention. So it would fail.
The PR replace non valid chars to _.

Types of Changes

Issues Fixed or Closed by This PR

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

…lacing invalid chars by _

sourcery-ai

Hey fabien-michel - Here's my review!

General suggestions:

Consider implementing a mechanism to ensure the uniqueness of sanitized enum names to avoid potential naming collisions.
Refactor the sanitization logic into a separate, reusable function to simplify the main code block and improve code readability.
Expand the test cases to cover a wider range of special characters and scenarios, ensuring comprehensive coverage.
Explicitly verify in tests that the original value of the enum is preserved and correctly associated with the transformed name, maintaining the integrity of the mapping.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

sourcery-ai · 2024-03-05T10:02:32Z

strawberry_django/fields/types.py

+                choice_name = re.sub(r"^[^a-zA-Z]|[^a-zA-Z0-9]", "_", c[0])
+                # use str() to trigger eventual django's gettext_lazy string
+                choice_value = EnumValueDefinition(value=c[0], description=str(c[1]))


suggestion (llm): The approach to sanitize choice names for GraphQL compatibility is sensible. However, consider edge cases where the sanitization might lead to name collisions. For instance, if the original choices contain both '1example' and 'example', both would be sanitized to '_example', causing a conflict. It might be beneficial to implement a mechanism to ensure uniqueness of the sanitized names.

sourcery-ai · 2024-03-05T10:02:32Z

strawberry_django/fields/types.py

        field_type = getattr(model_field, "_strawberry_enum", None)
        if field_type is None:
            meta = model_field.model._meta
+
+            enum_choices = {}
+            for c in model_field.choices:


issue (llm): The new implementation introduces additional complexity, particularly with the use of regular expressions and the iterative approach for constructing enum_choices. While ensuring compatibility with GraphQL naming conventions is important, we might consider streamlining this process to maintain code readability and reduce cognitive load.

One suggestion is to encapsulate the naming convention logic into a separate function, which could simplify the main code block. For example:

def sanitize_enum_name(name): # Replace chars not compatible with GraphQL naming convention return re.sub(r"^[^a-zA-Z]|[^a-zA-Z0-9]", "_", name)

And then integrate this function into the original dictionary comprehension:

field_type = strawberry.enum( enum.Enum( "".join( ( capitalize_first(to_camel_case(meta.app_label)), str(meta.object_name), capitalize_first(to_camel_case(model_field.name)), "Enum", ), ), { sanitize_enum_name(c[0]): EnumValueDefinition(value=c[0], description=str(c[1])) for c in model_field.choices }, ), description=( f"{meta.verbose_name} | {model_field.verbose_name}" if settings["FIELD_DESCRIPTION_FROM_HELP_TEXT"] else None ), )

This approach keeps the code concise and maintains readability, while still achieving the goal of ensuring GraphQL naming convention compatibility.

sourcery-ai · 2024-03-05T10:02:33Z

tests/test_enums.py

@@ -18,15 +19,16 @@ class Choice(models.TextChoices):

    A = "a", "A description"
    B = "b", "B description"
-    C = "c", "C description"
+    C = "c", gettext_lazy("C description")
+    D = "12d-d'éléphant_🐘", "D description"


suggestion (llm): It's great to see a test case added for the new functionality handling special characters in enum names. However, to ensure comprehensive coverage, consider adding more diverse test cases that include a wider range of special characters and scenarios. For example, strings starting with numbers, containing only special characters, or being empty.

sourcery-ai · 2024-03-05T10:02:33Z

tests/test_enums.py

@@ -296,6 +300,9 @@ def obj(self) -> ChoicesType:

      """C description"""
      c
+
+      """D description"""
+      _2d_d__l_phant__


suggestion (llm): I noticed that the test verifies the transformation of '12d-d'éléphant_🐘' to '2d_d__l_phant_', which aligns with the implementation. However, it would be beneficial to add assertions that explicitly check the underlying value of the enum (i.e., '12d-d'éléphant_🐘') is preserved and correctly associated with the transformed name. This ensures that the mapping between the GraphQL enum name and its original value remains intact.

sourcery-ai · 2024-03-05T10:02:33Z

strawberry_django/fields/types.py

+            enum_choices = {}
+            for c in model_field.choices:
+                # replace chars not compatible with GraphQL naming convention
+                choice_name = re.sub(r"^[^a-zA-Z]|[^a-zA-Z0-9]", "_", c[0])


suggestion (llm): The regex used for replacing non-valid characters seems to replace any character at the start of the string that is not an alphabet and any character in the string that is not alphanumeric. This approach might not handle cases where the string starts with a valid underscore or has valid underscores in between. Consider refining the regex to avoid unnecessary replacements of valid underscores and ensure it aligns with GraphQL naming conventions more accurately.

sourcery-ai · 2024-03-05T10:02:33Z

strawberry_django/fields/types.py

+            enum_choices = {}
+            for c in model_field.choices:
+                # replace chars not compatible with GraphQL naming convention
+                choice_name = re.sub(r"^[^a-zA-Z]|[^a-zA-Z0-9]", "_", c[0])


issue (llm): The implementation replaces invalid characters with underscores, which is a good approach. However, it might lead to potential naming collisions. For instance, '1-a' and '1_a' would both be converted to '_1_a'. It would be beneficial to add a mechanism to detect and resolve such collisions, ensuring the uniqueness of enum names.

codecov-commenter · 2024-03-05T10:17:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.98%. Comparing base (fc23726) to head (dcae186).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   87.95%   87.98%   +0.03%     
==========================================
  Files          37       37              
  Lines        3172     3180       +8     
==========================================
+ Hits         2790     2798       +8     
  Misses        382      382

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bellini666

TY! :)

fabien-michel added 2 commits March 4, 2024 17:42

fix: use str() to trigger eventual django's gettext_lazy string

a286504

fix: auto enum convert choice value to valid GraphQL enum name by rep…

073c247

…lacing invalid chars by _

sourcery-ai bot reviewed Mar 5, 2024

View reviewed changes

Merge branch 'main' into fix/489-auto-enum-value-allowed-chars

c7b6e1f

fabien-michel added 2 commits March 5, 2024 11:25

fix: underscores are allowed at start

c77c08e

fix: avoid auto enum name conflict

dcae186

bellini666 approved these changes Mar 5, 2024

View reviewed changes

bellini666 merged commit 475b03f into strawberry-graphql:main Mar 5, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/489 auto enum value allowed chars #494

Fix/489 auto enum value allowed chars #494

fabien-michel commented Mar 5, 2024 •

edited

Loading

sourcery-ai bot left a comment

sourcery-ai bot Mar 5, 2024

sourcery-ai bot Mar 5, 2024

sourcery-ai bot Mar 5, 2024

sourcery-ai bot Mar 5, 2024

sourcery-ai bot Mar 5, 2024

sourcery-ai bot Mar 5, 2024

codecov-commenter commented Mar 5, 2024 •

edited

Loading

bellini666 left a comment

Fix/489 auto enum value allowed chars #494

Fix/489 auto enum value allowed chars #494

Conversation

fabien-michel commented Mar 5, 2024 • edited Loading

Types of Changes

Issues Fixed or Closed by This PR

Checklist

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

sourcery-ai bot Mar 5, 2024

Choose a reason for hiding this comment

codecov-commenter commented Mar 5, 2024 • edited Loading

Codecov Report

bellini666 left a comment

Choose a reason for hiding this comment

fabien-michel commented Mar 5, 2024 •

edited

Loading

codecov-commenter commented Mar 5, 2024 •

edited

Loading