fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects

[ysinghc]

SUMMARY

Fixes issue #32484 where creating tags in Apache Superset with a MySQL database backend resulted in a fatal ProgrammingError. The root cause was the escape() function wrapping tag names in Markup objects instead of plain strings, which the MySQL driver could not process.

Changes:

• Removed the escape() call in superset/tags/models.py line 141, ensuring tag names remain as plain strings
• Removed the unused markupsafe.escape import
• Added comprehensive test suite with 17 tests (8 unit, 8 integration, 1 API) to verify the fix and prevent regression

Technical Details:
The Markup object is a subclass of str used by MarkupSafe for HTML escaping. While it works with some database drivers, MySQL's PyMySQL driver requires pure str types in SQL queries. Since tag names don't need HTML escaping at the model level (they should be escaped only when rendering in templates), removing the escape() call is the correct fix.

TESTING INSTRUCTIONS

Automated Tests

Run the comprehensive test suite:

pytest tests/unit_tests/tags/models_test.py \
       tests/integration_tests/tags/mysql_compatibility_test.py \
       tests/integration_tests/tags/api_tests.py::TestTagApi::test_create_tag_mysql_compatibility \
       -v

Test Coverage

The test suite verifies:

• Tag names are plain str type, not Markup objects
• No SQL errors during tag creation/commit
• Special characters handling (dashes, underscores, dots, colons, spaces)
• HTML characters don't trigger Markup conversion
• All TagType enum values work correctly
• Database persistence and retrieval
• API endpoint functionality

ADDITIONAL INFORMATION

• Has associated issue: Fixes Create tag raise Fatal error when superset deployed with mysql #32484
• Required feature flags: None
• Changes UI: No (only fixes backend bug)
• Includes DB Migration: No
• Introduces new feature or API: No (bug fix only)
• Removes existing feature or API: No

Files Changed:

• models.py - Removed escape() call and unused import
• models_test.py - Added 8 unit tests (NEW)
• mysql_compatibility_test.py - Added 8 integration tests (NEW)
• api_tests.py - Added 1 API test

[bito-code-review]

Code Review Agent Run #f0be31

Actionable Suggestions - 0

Additional Suggestions - 11

• tests/integration_tests/tags/mysql_compatibility_test.py - 5

  • Try-except within loop performance overhead · Line 119-119
    `try`-`except` within a loop incurs performance overhead. This affects multiple test methods. Consider restructuring to handle exceptions outside the loop or use a different approach.
    Code suggestion

     @@ -107,18 +107,21 @@
              created_tags = []
    -        for tag_name in tag_names:
    -            try:
    -                tag = get_tag(tag_name, db.session, TagType.custom)
    -                created_tags.append(tag)
    -
    -                assert isinstance(tag.name, str), (
    -                    f"Tag '{tag_name}' name should be a string"
    -                )
    -                assert not isinstance(tag.name, Markup), (
    -                    f"Tag '{tag_name}' should NOT be Markup"
    -                )
    -            except ProgrammingError as e:
    -                pytest.fail(
    -                    f"ProgrammingError should not occur when creating tag "
    -                    f"'{tag_name}': {e}"
    -                )
    +        try:
    +            for tag_name in tag_names:
    +                tag = get_tag(tag_name, db.session, TagType.custom)
    +                created_tags.append(tag)
    +
    +                assert isinstance(tag.name, str), (
    +                    f"Tag '{tag_name}' name should be a string"
    +                )
    +                assert not isinstance(tag.name, Markup), (
    +                    f"Tag '{tag_name}' should NOT be Markup"
    +                )
    +        except ProgrammingError as e:
    +            pytest.fail(
    +                f"ProgrammingError should not occur when creating tags: {e}"
    +            )

  • Blind exception catching detected · Line 264-264
    Do not catch blind exception: `Exception`. Replace with specific exception types or use `BaseException` if you need to catch all exceptions.
    Code suggestion

     @@ -264,2 +264,2 @@
                  pytest.fail(f"ProgrammingError should not occur during commit: {e}")
    -        except Exception as e:
    +        except BaseException as e:
                  pytest.fail(f"Unexpected error during tag creation and commit: {e}")

  • Missing return type annotation for setUp · Line 31-31
    Missing return type annotation for `setUp` method. This issue also affects `tearDown` and all test methods in this class. Add `-> None` return type annotations for consistency.
    Code suggestion

     @@ -31,1 +31,1 @@
    -    def setUp(self):
    +    def setUp(self) -> None:

  • Multi-line docstring summary formatting issue · Line 56-56
    Multi-line docstring summary should start at the first line. This issue affects multiple test methods. Move the summary to the same line as the opening quotes.
    Code suggestion

     @@ -55,6 +55,5 @@
          def test_create_tag_returns_string_not_markup(self):
    -        """
    -        Test that creating a tag results in a plain string name, not Markup.
    +        """Test that creating a tag results in a plain string name, not Markup.
     
              This is the core test for issue #32484 - ensures tag names are
              compatible with MySQL driver requirements.
              """

  • Missing trailing comma in function call · Line 122-122
    Trailing comma missing in `pytest.fail()` call. This issue affects multiple locations. Add trailing commas for consistency with project style.
    Code suggestion

     @@ -120,4 +120,4 @@
                  pytest.fail(
                      f"ProgrammingError should not occur when creating tag "
    -                f"'{tag_name}': {e}"
    +                f"'{tag_name}': {e}",
                  )

• tests/unit_tests/tags/models_test.py - 4

  • Missing return type annotation for test function · Line 27-27
    Test function `test_get_tag_returns_plain_string_not_markup` is missing return type annotation. Add `-> None` to all test functions. Multiple similar issues exist on lines 53, 86, 119, 146, 172, 200, 227.
    Code suggestion

     @@ -27,1 +27,1 @@
    -def test_get_tag_returns_plain_string_not_markup():
    +def test_get_tag_returns_plain_string_not_markup() -> None:

  • Multi-line docstring summary formatting issue · Line 28-28
    Docstring summary should start on the first line instead of a separate line. Multiple similar issues exist on lines 54, 87, 147, 228.
    Code suggestion

     @@ -27,4 +27,3 @@
    -def test_get_tag_returns_plain_string_not_markup():
    -    """
    -    Test that get_tag() returns a Tag with a plain string name, not a Markup object.
    -
    +def test_get_tag_returns_plain_string_not_markup():
    +    """Test that get_tag() returns a Tag with a plain string name, not a Markup object.
    +

  • Missing blank line after docstring summary · Line 54-54
    Docstring requires blank line between summary and description. Similar issue exists on line 228.
    Code suggestion

     @@ -53,4 +53,5 @@
    -def test_get_tag_with_special_characters():
    -    """
    -    Test that get_tag() correctly handles tag names with special characters
    -    without converting them to Markup objects.
    +def test_get_tag_with_special_characters():
    +    """Test that get_tag() correctly handles tag names with special characters.
    +
    +    Without converting them to Markup objects.
    +

  • Magic value used in comparison · Line 167-167
    Magic value `42` used in comparison. Consider replacing with a named constant for better readability.
    Code suggestion

     @@ -154,3 +154,4 @@
    -    # Create an existing tag
    -    existing_tag = Tag(name="existing-tag", type=TagType.custom)
    -    existing_tag.id = 42
    +    # Create an existing tag
    +    EXISTING_TAG_ID = 42
    +    existing_tag = Tag(name="existing-tag", type=TagType.custom)
    +    existing_tag.id = EXISTING_TAG_ID

• tests/integration_tests/tags/api_tests.py - 2

  • Missing return type annotation for test method · Line 788-788
    Test method `test_create_tag_mysql_compatibility` is missing return type annotation. Consider adding `-> None` for consistency with type hints.
    Code suggestion

     @@ -788,1 +788,1 @@
    -    def test_create_tag_mysql_compatibility(self):
    +    def test_create_tag_mysql_compatibility(self) -> None:

  • Multi-line docstring summary formatting issue · Line 789-789
    Multi-line docstring summary should start at the first line. Move the summary text to the same line as the opening triple quotes.
    Code suggestion

     @@ -789,6 +789,5 @@
    -        """
    -        Test creating a tag via API to ensure MySQL compatibility.
    -
    -        This test verifies the fix for issue #32484 where tag creation
    -        failed with MySQL due to Markup objects being used instead of strings.
    -        """
    +        """Test creating a tag via API to ensure MySQL compatibility.
    +
    +        This test verifies the fix for issue #32484 where tag creation
    +        failed with MySQL due to Markup objects being used instead of strings.
    +        """

Review Details

• Files reviewed - 4 · Commit Range: 40bddf5..40bddf5

  • superset/tags/models.py
  • tests/integration_tests/tags/api_tests.py
  • tests/integration_tests/tags/mysql_compatibility_test.py
  • tests/unit_tests/tags/models_test.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[korbit-ai]

I've completed my review and didn't find any issues.

Files scanned

​

File Path	Reviewed
superset/tags/models.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.37%. Comparing base (76d897e) to head (13df896).
⚠️ Report is 2892 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #36075      +/-   ##
==========================================
+ Coverage   60.48%   68.37%   +7.88%     
==========================================
  Files        1931      631    -1300     
  Lines       76236    45987   -30249     
  Branches     8568     4987    -3581     
==========================================
- Hits        46114    31445   -14669     
+ Misses      28017    13295   -14722     
+ Partials     2105     1247     -858     

Flag	Coverage Δ
hive	43.95% <0.00%> (-5.20%)	⬇️
javascript	?
mysql	67.46% <100.00%> (?)
postgres	67.51% <100.00%> (?)
presto	47.50% <0.00%> (-6.31%)	⬇️
python	68.34% <100.00%> (+4.83%)	⬆️
sqlite	67.12% <100.00%> (?)
unit	100.00% <ø> (+42.36%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #6ee5c0

Actionable Suggestions - 1

• tests/integration_tests/tags/mysql_compatibility_test.py - 1

  • Catching blind BaseException is too broad · Line 263-263

Additional Suggestions - 8

• tests/integration_tests/tags/mysql_compatibility_test.py - 4

  • Missing return type annotation for tearDown · Line 37-37
    The `tearDown` method is missing a return type annotation. Add `-> None` to maintain consistency with the `setUp` method which already has proper type annotation.
    Code suggestion

     @@ -37,1 +37,1 @@
    -    def tearDown(self):
    +    def tearDown(self) -> None:

  • Missing return type annotation for cleanup_test_tags · Line 42-42
    The `cleanup_test_tags` method is missing a return type annotation. Add `-> None` for consistency with other methods in the class.
    Code suggestion

     @@ -42,1 +42,1 @@
    -    def cleanup_test_tags(self):
    +    def cleanup_test_tags(self) -> None:

  • Multi-line docstring summary formatting issue · Line 56-56
    Multiple docstrings have formatting issues where the summary should start on the first line. This affects readability and violates PEP 257 conventions. Similar issues exist on lines 96, 137, 241, and 267.
    Code suggestion

     @@ -55,4 +55,3 @@
          def test_create_tag_returns_string_not_markup(self) -> None:
    -        """
    -        Test that creating a tag results in a plain string name, not Markup.
    +        """Test that creating a tag results in a plain string name, not Markup.
     
              This is the core test for issue #32484 - ensures tag names are

  • Missing trailing comma in function call · Line 175-175
    Missing trailing comma in the `pytest.fail()` function call. Adding trailing commas improves code consistency and makes future modifications cleaner.
    Code suggestion

     @@ -174,2 +174,2 @@
                      pytest.fail(
    -                    f"ProgrammingError should not occur for tag '{tag_name}': {e}"
    +                    f"ProgrammingError should not occur for tag '{tag_name}': {e}",

• tests/unit_tests/tags/models_test.py - 3

  • Multi-line docstring summary formatting issue · Line 28-28
    Multi-line docstring summary should start at the first line. This issue appears in multiple docstrings throughout the file (lines 28, 54, 87, 147, 228).
    Code suggestion

     @@ -27,5 +27,4 @@
    -def test_get_tag_returns_plain_string_not_markup() -> None:
    -    """
    -    Test that get_tag() returns a Tag with a plain string name, not a Markup object.
    +def test_get_tag_returns_plain_string_not_markup() -> None:
    +    """Test that get_tag() returns a Tag with a plain string name, not a Markup object.
     
          This verifies the fix for issue #32484 where escape() was wrapping tag names

  • Missing blank line after docstring summary · Line 54-54
    Docstring requires 1 blank line between summary line and description according to PEP 257 conventions.
    Code suggestion

     @@ -53,6 +53,7 @@
    -def test_get_tag_with_special_characters() -> None:
    -    """
    -    Test that get_tag() correctly handles tag names with special characters
    -    without converting them to Markup objects.
    -    """
    +def test_get_tag_with_special_characters() -> None:
    +    """Test that get_tag() correctly handles tag names with special characters
    +
    +    without converting them to Markup objects.
    +    """

  • Magic value comparison with hardcoded number · Line 167-167
    Magic value `42` used in comparison. Consider replacing with a named constant for better readability and maintainability.
    Code suggestion

     @@ -154,6 +154,7 @@
          # Create an existing tag
    +    EXISTING_TAG_ID = 42
          existing_tag = Tag(name="existing-tag", type=TagType.custom)
    -    existing_tag.id = 42
    +    existing_tag.id = EXISTING_TAG_ID
     
     @@ -165,7 +166,7 @@
          # Verify
          assert result is existing_tag, "Should return the existing tag"
    -    assert result.id == 42, "Should have the existing tag's ID"
    +    assert result.id == EXISTING_TAG_ID, "Should have the existing tag's ID"

• tests/integration_tests/tags/api_tests.py - 1

  • Multi-line docstring summary should start first line · Line 789-789
    The docstring summary should start on the first line instead of having an empty line. This follows PEP 257 conventions for multi-line docstrings.
    Code suggestion

     @@ -788,7 +788,6 @@
         def test_create_tag_mysql_compatibility(self) -> None:
    -        """
    -        Test creating a tag via API to ensure MySQL compatibility.
    +        """Test creating a tag via API to ensure MySQL compatibility.
     
              This test verifies the fix for issue #32484 where tag creation

Review Details

• Files reviewed - 4 · Commit Range: 40bddf5..f51a138

  • superset/tags/models.py
  • tests/integration_tests/tags/api_tests.py
  • tests/integration_tests/tags/mysql_compatibility_test.py
  • tests/unit_tests/tags/models_test.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[sadpandajoe]

@betodealmeida @mistercrunch thoughts of having the mysql tests as part of our core tests? If not do you have a recommendation on where they should live?

[rusackas]

Appreciate all the tests here! My question is about whether or not we do (or should) support MySQL as a default metadata database, and thus these should be "core" tests, or if this should be added somewhere else for those who DO elect to configure/install mysql for their instance. @mistercrunch @betodealmeida might have opinions here.

[Copilot]

Pull Request Overview

This pull request fixes issue #32484 where creating tags in Apache Superset with a MySQL database backend resulted in a ProgrammingError. The fix removes the escape() function call that was wrapping tag names in Markup objects, which MySQL's PyMySQL driver cannot process.

Key Changes:

• Removed escape() call from get_tag() function in superset/tags/models.py to ensure tag names remain plain strings
• Removed unused markupsafe.escape import
• Added comprehensive test coverage with 17 new tests across unit, integration, and API test files

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
superset/tags/models.py	Removed escape() call on line 140 and unused import, ensuring tag names are stored as plain strings instead of Markup objects
tests/unit_tests/tags/models_test.py	Added 8 unit tests using mocks to verify tag names are plain strings and not Markup objects
tests/integration_tests/tags/mysql_compatibility_test.py	Added 8 integration tests with real database operations to ensure MySQL compatibility
tests/integration_tests/tags/api_tests.py	Added 1 API test to verify the fix works through the REST API endpoint

[Copilot]

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

[rusackas]

LGTM!