You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "ysinghc (via GitHub)" <gi...@apache.org> on 2025/11/11 17:07:01 UTC
[PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
ysinghc opened a new pull request, #36075:
URL: https://github.com/apache/superset/pull/36075
### 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:
```bash
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
- [x] Has associated issue: Fixes #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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "ysinghc (via GitHub)" <gi...@apache.org>.
ysinghc commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3518014346
/review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #36075:
URL: https://github.com/apache/superset/pull/36075#discussion_r2519387372
##########
tests/integration_tests/tags/mysql_compatibility_test.py:
##########
@@ -0,0 +1,285 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for tag creation with MySQL compatibility."""
+
+import pytest
+from markupsafe import Markup
+from sqlalchemy.exc import ProgrammingError
+
+from superset import db
+from superset.tags.models import get_tag, Tag, TagType
+from tests.integration_tests.base_tests import SupersetTestCase
+
+
+class TestTagCreationMySQLCompatibility(SupersetTestCase):
+ """Test suite to verify the MySQL compatibility fix for tag creation."""
+
+ def setUp(self) -> None:
+ """Set up test fixtures."""
+ super().setUp()
+ # Clean up any existing test tags
+ self.cleanup_test_tags()
+
+ def tearDown(self):
+ """Clean up after tests."""
+ self.cleanup_test_tags()
+ super().tearDown()
+
+ def cleanup_test_tags(self):
+ """Remove test tags from the database."""
+ test_tag_prefixes = [
+ "mysql-fix-verification",
+ "test-tag",
+ "integration-test",
+ ]
+ for prefix in test_tag_prefixes:
+ tags = db.session.query(Tag).filter(Tag.name.like(f"{prefix}%")).all()
+ for tag in tags:
+ db.session.delete(tag)
+ db.session.commit()
+
+ def test_create_tag_returns_string_not_markup(self) -> None:
+ """
+ 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.
+ """
+ tag_name = "mysql-fix-verification-20251111"
+
+ # Create tag using get_tag function
+ tag = get_tag(tag_name, db.session, TagType.custom)
+
+ # Verify the tag was created
+ assert tag is not None, "Tag should be created"
+ assert tag.id is not None, "Tag should have an ID after creation"
+
+ # Critical checks for MySQL compatibility
+ assert isinstance(tag.name, str), "Tag name should be a plain string"
+ assert not isinstance(tag.name, Markup), (
+ "Tag name should NOT be a Markup object"
+ )
+ assert tag.name.__class__ is str, "Tag name should be exactly str type"
+ assert tag.name == tag_name, f"Tag name should be '{tag_name}'"
+
+ # Verify the tag persists correctly in the database
+ db.session.commit()
+
+ # Retrieve the tag from database to ensure it was stored correctly
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, "Tag should be retrievable from database"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag name should be a string"
+ )
+ assert not isinstance(retrieved_tag.name, Markup), (
+ "Retrieved tag name should NOT be Markup"
+ )
+ assert retrieved_tag.name == tag_name, (
+ "Retrieved tag name should match original"
+ )
+
+ def test_create_multiple_tags_no_sql_error(self) -> None:
+ """
+ Test creating multiple tags in sequence without SQL errors.
+
+ This ensures the fix works consistently across multiple operations.
+ """
+ tag_names = [
+ "integration-test-tag-1",
+ "integration-test-tag-2",
+ "integration-test-tag-3",
+ ]
+
+ created_tags = []
+ 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}",
+ )
+
+ db.session.commit()
+
+ # Verify all tags were created successfully
+ assert len(created_tags) == len(tag_names), "All tags should be created"
+
+ for tag_name in tag_names:
+ tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert tag is not None, f"Tag '{tag_name}' should exist in database"
+ assert isinstance(tag.name, str), (
+ f"Tag '{tag_name}' should have string name"
+ )
+
+ def test_create_tag_with_special_characters(self) -> None:
+ """
+ Test creating tags with special characters that might trigger HTML escaping.
+
+ Ensures these characters don't cause the tag name to become a Markup object.
+ """
+ tag_names = [
+ "test-tag-with-dashes",
+ "test_tag_with_underscores",
+ "test.tag.with.dots",
+ "test:tag:with:colons",
+ "test tag with spaces",
+ ]
+
+ for tag_name in tag_names:
+ try:
+ tag = get_tag(tag_name, db.session, TagType.custom)
+
+ assert isinstance(tag.name, str), f"Tag '{tag_name}' should be a string"
+ assert not isinstance(tag.name, Markup), (
+ f"Tag '{tag_name}' should NOT be Markup"
+ )
+ assert tag.name == tag_name, (
+ f"Tag name should match input: '{tag_name}'"
+ )
+
+ db.session.commit()
+
+ # Verify database persistence
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, (
+ f"Tag '{tag_name}' should be in database"
+ )
+ assert retrieved_tag.name == tag_name, (
+ f"Retrieved tag name should match: '{tag_name}'"
+ )
+
+ except ProgrammingError as e:
+ pytest.fail(
+ f"ProgrammingError should not occur for tag '{tag_name}': {e}"
+ )
+
+ def test_get_existing_tag_returns_string(self) -> None:
+ """Test that retrieving an existing tag also returns a string name."""
+ tag_name = "test-tag-for-retrieval"
+
+ # Create tag first
+ original_tag = get_tag(tag_name, db.session, TagType.custom)
+ db.session.commit()
+
+ # Retrieve the same tag
+ retrieved_tag = get_tag(tag_name, db.session, TagType.custom)
+
+ assert retrieved_tag.id == original_tag.id, "Should retrieve the same tag"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag name should be a string"
+ )
+ assert not isinstance(retrieved_tag.name, Markup), (
+ "Retrieved tag name should NOT be Markup"
+ )
+
+ def test_tag_with_whitespace_handling(self) -> None:
+ """Test that tags with leading/trailing whitespace are handled correctly."""
+ input_name = " test-tag-with-whitespace "
+ expected_name = "test-tag-with-whitespace"
+
+ tag = get_tag(input_name, db.session, TagType.custom)
+
+ assert isinstance(tag.name, str), "Tag name should be a string"
+ assert not isinstance(tag.name, Markup), "Tag name should NOT be Markup"
+ assert tag.name == expected_name, "Tag name should be stripped of whitespace"
+
+ db.session.commit()
+
+ # Verify in database
+ retrieved_tag = db.session.query(Tag).filter_by(name=expected_name).first()
+ assert retrieved_tag is not None, "Tag should exist with stripped name"
+
+ def test_tag_type_variations(self) -> None:
+ """Test creating tags with different TagType values."""
+ tag_types = [
+ (TagType.custom, "test-custom-tag"),
+ (TagType.type, "type:chart"),
+ (TagType.owner, "owner:123"),
+ (TagType.favorited_by, "favorited_by:456"),
+ ]
+
+ for tag_type, tag_name in tag_types:
+ try:
+ tag = get_tag(tag_name, db.session, tag_type)
+
+ assert isinstance(tag.name, str), (
+ f"Tag name for {tag_type} should be a string"
+ )
+ assert not isinstance(tag.name, Markup), (
+ f"Tag name for {tag_type} should NOT be Markup"
+ )
+ assert tag.type == tag_type, f"Tag type should be {tag_type}"
+
+ db.session.commit()
+
+ except ProgrammingError as e:
+ pytest.fail(f"ProgrammingError for {tag_type} tag '{tag_name}': {e}")
+
+ def test_no_sql_syntax_error_on_commit(self) -> None:
+ """
+ Test that committing a tag to the database doesn't cause SQL syntax errors.
+
+ This is the main error that occurred in issue #32484 with MySQL.
+ """
+ tag_name = "mysql-compatibility-test"
+
+ try:
+ get_tag(tag_name, db.session, TagType.custom)
+
+ # This commit should not raise a ProgrammingError
+ db.session.commit()
+
+ # Verify the tag exists in the database
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, "Tag should exist after commit"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag should have string name"
+ )
+
+ except ProgrammingError as e:
+ pytest.fail(f"ProgrammingError should not occur during commit: {e}")
+ except BaseException as e:
Review Comment:
Except block directly handles BaseException.
```suggestion
except Exception as e:
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #36075:
URL: https://github.com/apache/superset/pull/36075
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3517958377
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36075?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.44%. Comparing base ([`d123249`](https://app.codecov.io/gh/apache/superset/commit/d123249bd2d39ff04ce3e57a5bf6ed261beec976?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`40bddf5`](https://app.codecov.io/gh/apache/superset/commit/40bddf539ca64f2d3f70a6bb0cb77fc036abe140?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/36075?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/tags/models.py](https://app.codecov.io/gh/apache/superset/pull/36075?src=pr&el=tree&filepath=superset%2Ftags%2Fmodels.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGFncy9tb2RlbHMucHk=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36075?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36075 +/- ##
===========================================
+ Coverage 0 48.44% +48.44%
===========================================
Files 0 622 +622
Lines 0 45756 +45756
Branches 0 4979 +4979
===========================================
+ Hits 0 22166 +22166
- Misses 0 22965 +22965
- Partials 0 625 +625
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/36075/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/36075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `44.17% <0.00%> (?)` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/36075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.72% <0.00%> (?)` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/36075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.43% <0.00%> (?)` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/36075/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/36075?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3517923260
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #36075:
URL: https://github.com/apache/superset/pull/36075#discussion_r2522420271
##########
tests/unit_tests/tags/models_test.py:
##########
@@ -0,0 +1,263 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for tag models, specifically testing the MySQL fix for tag creation."""
+
+from unittest.mock import MagicMock
+
+from markupsafe import Markup
+from sqlalchemy.orm import Session
+
+from superset.tags.models import get_tag, Tag, TagType
+
+
+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
+ in Markup objects, causing MySQL driver errors.
+ """
+ # Setup
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ tag_name = "test-tag-name"
+ tag_type = TagType.custom
+
+ # Execute
+ result = get_tag(tag_name, mock_session, tag_type)
+
+ # Verify
+ assert isinstance(result.name, str), "Tag name should be a plain string"
+ assert not isinstance(result.name, Markup), "Tag name should NOT be a Markup object"
+ assert result.name == tag_name, f"Tag name should be '{tag_name}'"
+ assert result.type == tag_type, f"Tag type should be {tag_type}"
+
+
+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.
+ """
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ # Test with various special characters that might trigger HTML escaping
+ tag_names = [
+ "tag-with-dashes",
+ "tag_with_underscores",
+ "tag.with.dots",
+ "tag:with:colons",
+ "tag/with/slashes",
+ "tag with spaces",
+ "mysql-fix-verification-20251111",
+ ]
+
+ for tag_name in tag_names:
+ result = get_tag(tag_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), (
+ f"Tag name '{tag_name}' should be a plain string"
+ )
+ assert not isinstance(result.name, Markup), (
+ f"Tag name '{tag_name}' should NOT be a Markup object"
+ )
+ assert result.name == tag_name, f"Tag name should match input: '{tag_name}'"
+
+
+def test_get_tag_with_html_characters() -> None:
+ """
+ Test that get_tag() handles HTML special characters correctly.
+
+ Even though these characters might have been escaped before, they should
+ now be stored as plain strings to avoid MySQL driver issues.
+ """
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ # Test with HTML special characters
+ tag_names = [
+ "tag<with>brackets",
+ "tag&with&ersands",
+ 'tag"with"quotes',
+ "tag'with'apostrophes",
+ ]
+
+ for tag_name in tag_names:
+ result = get_tag(tag_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), (
+ f"Tag name '{tag_name}' should be a plain string"
+ )
+ assert not isinstance(result.name, Markup), (
+ f"Tag name '{tag_name}' should NOT be a Markup object"
+ )
+ # Name should remain unchanged (not HTML-escaped)
+ assert result.name == tag_name, f"Tag name should not be escaped: '{tag_name}'"
+
+
+def test_get_tag_strips_whitespace() -> None:
+ """Test that get_tag() strips leading and trailing whitespace from tag names."""
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ tag_names_with_whitespace = [
+ (" tag-with-leading-spaces", "tag-with-leading-spaces"),
+ ("tag-with-trailing-spaces ", "tag-with-trailing-spaces"),
+ (" tag-with-both ", "tag-with-both"),
+ ("\ttag-with-tabs\t", "tag-with-tabs"),
+ ("\ntag-with-newlines\n", "tag-with-newlines"),
+ ]
+
+ for input_name, expected_name in tag_names_with_whitespace:
+ result = get_tag(input_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), "Tag name should be a plain string"
+ assert not isinstance(result.name, Markup), (
+ "Tag name should NOT be a Markup object"
+ )
+ assert result.name == expected_name, (
+ f"Tag name should be stripped: '{expected_name}'"
+ )
+
+
+def test_get_tag_returns_existing_tag() -> None:
+ """
+ Test that get_tag() returns existing tag from database.
+
+ Verifies it doesn't create a new one.
+ """
+ mock_session = MagicMock(spec=Session)
+
+ # Create an existing tag
+ existing_tag = Tag(name="existing-tag", type=TagType.custom)
+ existing_tag.id = 42
+
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = existing_tag
+
+ # Execute
+ result = get_tag("existing-tag", mock_session, TagType.custom)
+
+ # Verify
+ assert result is existing_tag, "Should return the existing tag"
+ assert result.id == 42, "Should have the existing tag's ID"
+ mock_session.add.assert_not_called(), "Should not add a new tag"
Review Comment:
The assertion call has incorrect syntax. `assert_not_called()` should be called without the `assert` keyword. The correct syntax should be:
```python
mock_session.add.assert_not_called()
```
without the comma and the string at the end. The assertion message cannot be added to mock assertions this way.
```suggestion
mock_session.add.assert_not_called()
```
##########
tests/unit_tests/tags/models_test.py:
##########
@@ -0,0 +1,263 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Unit tests for tag models, specifically testing the MySQL fix for tag creation."""
+
+from unittest.mock import MagicMock
+
+from markupsafe import Markup
+from sqlalchemy.orm import Session
+
+from superset.tags.models import get_tag, Tag, TagType
+
+
+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
+ in Markup objects, causing MySQL driver errors.
+ """
+ # Setup
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ tag_name = "test-tag-name"
+ tag_type = TagType.custom
+
+ # Execute
+ result = get_tag(tag_name, mock_session, tag_type)
+
+ # Verify
+ assert isinstance(result.name, str), "Tag name should be a plain string"
+ assert not isinstance(result.name, Markup), "Tag name should NOT be a Markup object"
+ assert result.name == tag_name, f"Tag name should be '{tag_name}'"
+ assert result.type == tag_type, f"Tag type should be {tag_type}"
+
+
+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.
+ """
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ # Test with various special characters that might trigger HTML escaping
+ tag_names = [
+ "tag-with-dashes",
+ "tag_with_underscores",
+ "tag.with.dots",
+ "tag:with:colons",
+ "tag/with/slashes",
+ "tag with spaces",
+ "mysql-fix-verification-20251111",
+ ]
+
+ for tag_name in tag_names:
+ result = get_tag(tag_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), (
+ f"Tag name '{tag_name}' should be a plain string"
+ )
+ assert not isinstance(result.name, Markup), (
+ f"Tag name '{tag_name}' should NOT be a Markup object"
+ )
+ assert result.name == tag_name, f"Tag name should match input: '{tag_name}'"
+
+
+def test_get_tag_with_html_characters() -> None:
+ """
+ Test that get_tag() handles HTML special characters correctly.
+
+ Even though these characters might have been escaped before, they should
+ now be stored as plain strings to avoid MySQL driver issues.
+ """
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ # Test with HTML special characters
+ tag_names = [
+ "tag<with>brackets",
+ "tag&with&ersands",
+ 'tag"with"quotes',
+ "tag'with'apostrophes",
+ ]
+
+ for tag_name in tag_names:
+ result = get_tag(tag_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), (
+ f"Tag name '{tag_name}' should be a plain string"
+ )
+ assert not isinstance(result.name, Markup), (
+ f"Tag name '{tag_name}' should NOT be a Markup object"
+ )
+ # Name should remain unchanged (not HTML-escaped)
+ assert result.name == tag_name, f"Tag name should not be escaped: '{tag_name}'"
+
+
+def test_get_tag_strips_whitespace() -> None:
+ """Test that get_tag() strips leading and trailing whitespace from tag names."""
+ mock_session = MagicMock(spec=Session)
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = None
+
+ tag_names_with_whitespace = [
+ (" tag-with-leading-spaces", "tag-with-leading-spaces"),
+ ("tag-with-trailing-spaces ", "tag-with-trailing-spaces"),
+ (" tag-with-both ", "tag-with-both"),
+ ("\ttag-with-tabs\t", "tag-with-tabs"),
+ ("\ntag-with-newlines\n", "tag-with-newlines"),
+ ]
+
+ for input_name, expected_name in tag_names_with_whitespace:
+ result = get_tag(input_name, mock_session, TagType.custom)
+
+ assert isinstance(result.name, str), "Tag name should be a plain string"
+ assert not isinstance(result.name, Markup), (
+ "Tag name should NOT be a Markup object"
+ )
+ assert result.name == expected_name, (
+ f"Tag name should be stripped: '{expected_name}'"
+ )
+
+
+def test_get_tag_returns_existing_tag() -> None:
+ """
+ Test that get_tag() returns existing tag from database.
+
+ Verifies it doesn't create a new one.
+ """
+ mock_session = MagicMock(spec=Session)
+
+ # Create an existing tag
+ existing_tag = Tag(name="existing-tag", type=TagType.custom)
+ existing_tag.id = 42
+
+ mock_query = MagicMock()
+ mock_session.query.return_value = mock_query
+ mock_query.filter_by.return_value.one_or_none.return_value = existing_tag
+
+ # Execute
+ result = get_tag("existing-tag", mock_session, TagType.custom)
+
+ # Verify
+ assert result is existing_tag, "Should return the existing tag"
+ assert result.id == 42, "Should have the existing tag's ID"
+ mock_session.add.assert_not_called(), "Should not add a new tag"
+ mock_session.commit.assert_not_called(), "Should not commit"
Review Comment:
The assertion call has incorrect syntax. `assert_not_called()` should be called without the `assert` keyword. The correct syntax should be:
```python
mock_session.commit.assert_not_called()
```
without the comma and the string at the end. The assertion message cannot be added to mock assertions this way.
```suggestion
mock_session.commit.assert_not_called()
```
##########
tests/integration_tests/tags/api_tests.py:
##########
@@ -784,3 +784,48 @@ def test_post_bulk_tag_skipped_tags_perm(self):
result = rv.json["result"]
assert len(result["objects_tagged"]) == 2
assert len(result["objects_skipped"]) == 1
+
+ def test_create_tag_mysql_compatibility(self) -> None:
+ """
+ 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.
+ """
+ from markupsafe import Markup
Review Comment:
The import statement `from markupsafe import Markup` should be placed at the top of the file with other imports, not inside the function. This violates Python's PEP 8 style guide and established best practices for import organization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3523378252
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36075:
URL: https://github.com/apache/superset/pull/36075#discussion_r2515090974
##########
tests/integration_tests/tags/mysql_compatibility_test.py:
##########
@@ -0,0 +1,285 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Integration tests for tag creation with MySQL compatibility."""
+
+import pytest
+from markupsafe import Markup
+from sqlalchemy.exc import ProgrammingError
+
+from superset import db
+from superset.tags.models import get_tag, Tag, TagType
+from tests.integration_tests.base_tests import SupersetTestCase
+
+
+class TestTagCreationMySQLCompatibility(SupersetTestCase):
+ """Test suite to verify the MySQL compatibility fix for tag creation."""
+
+ def setUp(self) -> None:
+ """Set up test fixtures."""
+ super().setUp()
+ # Clean up any existing test tags
+ self.cleanup_test_tags()
+
+ def tearDown(self):
+ """Clean up after tests."""
+ self.cleanup_test_tags()
+ super().tearDown()
+
+ def cleanup_test_tags(self):
+ """Remove test tags from the database."""
+ test_tag_prefixes = [
+ "mysql-fix-verification",
+ "test-tag",
+ "integration-test",
+ ]
+ for prefix in test_tag_prefixes:
+ tags = db.session.query(Tag).filter(Tag.name.like(f"{prefix}%")).all()
+ for tag in tags:
+ db.session.delete(tag)
+ db.session.commit()
+
+ def test_create_tag_returns_string_not_markup(self) -> None:
+ """
+ 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.
+ """
+ tag_name = "mysql-fix-verification-20251111"
+
+ # Create tag using get_tag function
+ tag = get_tag(tag_name, db.session, TagType.custom)
+
+ # Verify the tag was created
+ assert tag is not None, "Tag should be created"
+ assert tag.id is not None, "Tag should have an ID after creation"
+
+ # Critical checks for MySQL compatibility
+ assert isinstance(tag.name, str), "Tag name should be a plain string"
+ assert not isinstance(tag.name, Markup), (
+ "Tag name should NOT be a Markup object"
+ )
+ assert tag.name.__class__ is str, "Tag name should be exactly str type"
+ assert tag.name == tag_name, f"Tag name should be '{tag_name}'"
+
+ # Verify the tag persists correctly in the database
+ db.session.commit()
+
+ # Retrieve the tag from database to ensure it was stored correctly
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, "Tag should be retrievable from database"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag name should be a string"
+ )
+ assert not isinstance(retrieved_tag.name, Markup), (
+ "Retrieved tag name should NOT be Markup"
+ )
+ assert retrieved_tag.name == tag_name, (
+ "Retrieved tag name should match original"
+ )
+
+ def test_create_multiple_tags_no_sql_error(self) -> None:
+ """
+ Test creating multiple tags in sequence without SQL errors.
+
+ This ensures the fix works consistently across multiple operations.
+ """
+ tag_names = [
+ "integration-test-tag-1",
+ "integration-test-tag-2",
+ "integration-test-tag-3",
+ ]
+
+ created_tags = []
+ 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}",
+ )
+
+ db.session.commit()
+
+ # Verify all tags were created successfully
+ assert len(created_tags) == len(tag_names), "All tags should be created"
+
+ for tag_name in tag_names:
+ tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert tag is not None, f"Tag '{tag_name}' should exist in database"
+ assert isinstance(tag.name, str), (
+ f"Tag '{tag_name}' should have string name"
+ )
+
+ def test_create_tag_with_special_characters(self) -> None:
+ """
+ Test creating tags with special characters that might trigger HTML escaping.
+
+ Ensures these characters don't cause the tag name to become a Markup object.
+ """
+ tag_names = [
+ "test-tag-with-dashes",
+ "test_tag_with_underscores",
+ "test.tag.with.dots",
+ "test:tag:with:colons",
+ "test tag with spaces",
+ ]
+
+ for tag_name in tag_names:
+ try:
+ tag = get_tag(tag_name, db.session, TagType.custom)
+
+ assert isinstance(tag.name, str), f"Tag '{tag_name}' should be a string"
+ assert not isinstance(tag.name, Markup), (
+ f"Tag '{tag_name}' should NOT be Markup"
+ )
+ assert tag.name == tag_name, (
+ f"Tag name should match input: '{tag_name}'"
+ )
+
+ db.session.commit()
+
+ # Verify database persistence
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, (
+ f"Tag '{tag_name}' should be in database"
+ )
+ assert retrieved_tag.name == tag_name, (
+ f"Retrieved tag name should match: '{tag_name}'"
+ )
+
+ except ProgrammingError as e:
+ pytest.fail(
+ f"ProgrammingError should not occur for tag '{tag_name}': {e}"
+ )
+
+ def test_get_existing_tag_returns_string(self) -> None:
+ """Test that retrieving an existing tag also returns a string name."""
+ tag_name = "test-tag-for-retrieval"
+
+ # Create tag first
+ original_tag = get_tag(tag_name, db.session, TagType.custom)
+ db.session.commit()
+
+ # Retrieve the same tag
+ retrieved_tag = get_tag(tag_name, db.session, TagType.custom)
+
+ assert retrieved_tag.id == original_tag.id, "Should retrieve the same tag"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag name should be a string"
+ )
+ assert not isinstance(retrieved_tag.name, Markup), (
+ "Retrieved tag name should NOT be Markup"
+ )
+
+ def test_tag_with_whitespace_handling(self) -> None:
+ """Test that tags with leading/trailing whitespace are handled correctly."""
+ input_name = " test-tag-with-whitespace "
+ expected_name = "test-tag-with-whitespace"
+
+ tag = get_tag(input_name, db.session, TagType.custom)
+
+ assert isinstance(tag.name, str), "Tag name should be a string"
+ assert not isinstance(tag.name, Markup), "Tag name should NOT be Markup"
+ assert tag.name == expected_name, "Tag name should be stripped of whitespace"
+
+ db.session.commit()
+
+ # Verify in database
+ retrieved_tag = db.session.query(Tag).filter_by(name=expected_name).first()
+ assert retrieved_tag is not None, "Tag should exist with stripped name"
+
+ def test_tag_type_variations(self) -> None:
+ """Test creating tags with different TagType values."""
+ tag_types = [
+ (TagType.custom, "test-custom-tag"),
+ (TagType.type, "type:chart"),
+ (TagType.owner, "owner:123"),
+ (TagType.favorited_by, "favorited_by:456"),
+ ]
+
+ for tag_type, tag_name in tag_types:
+ try:
+ tag = get_tag(tag_name, db.session, tag_type)
+
+ assert isinstance(tag.name, str), (
+ f"Tag name for {tag_type} should be a string"
+ )
+ assert not isinstance(tag.name, Markup), (
+ f"Tag name for {tag_type} should NOT be Markup"
+ )
+ assert tag.type == tag_type, f"Tag type should be {tag_type}"
+
+ db.session.commit()
+
+ except ProgrammingError as e:
+ pytest.fail(f"ProgrammingError for {tag_type} tag '{tag_name}': {e}")
+
+ def test_no_sql_syntax_error_on_commit(self) -> None:
+ """
+ Test that committing a tag to the database doesn't cause SQL syntax errors.
+
+ This is the main error that occurred in issue #32484 with MySQL.
+ """
+ tag_name = "mysql-compatibility-test"
+
+ try:
+ get_tag(tag_name, db.session, TagType.custom)
+
+ # This commit should not raise a ProgrammingError
+ db.session.commit()
+
+ # Verify the tag exists in the database
+ retrieved_tag = db.session.query(Tag).filter_by(name=tag_name).first()
+ assert retrieved_tag is not None, "Tag should exist after commit"
+ assert isinstance(retrieved_tag.name, str), (
+ "Retrieved tag should have string name"
+ )
+
+ except ProgrammingError as e:
+ pytest.fail(f"ProgrammingError should not occur during commit: {e}")
+ except BaseException as e:
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Catching blind BaseException is too broad</b></div>
<div id="fix">
Catching `BaseException` is too broad and can mask system exits and keyboard interrupts. Consider catching more specific exceptions or use `Exception` instead.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
except Exception as e:
````
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36075#issuecomment-3518014988>#6ee5c0</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3518014988
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(tags): ensure tag creation is compatible with MySQL by avoiding Markup objects [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #36075:
URL: https://github.com/apache/superset/pull/36075#issuecomment-3523376479
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org