korbit-ai[bot] commented on code in PR #35920:
URL: https://github.com/apache/superset/pull/35920#discussion_r2486626581
##########
superset/themes/schemas.py:
##########
@@ -56,7 +62,7 @@ def validate_json_data(self, value: dict[str, Any]) -> None:
value.clear()
value.update(sanitized_config)
else:
- self.context["sanitized_json_data"] =
json.dumps(sanitized_config)
+ sanitized_json_context.set(json.dumps(sanitized_config))
Review Comment:
### Sanitized data stored but not consumed <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The sanitized JSON data is stored in a ContextVar but never retrieved or
used by the consuming code, making the sanitization ineffective.
###### Why this matters
The sanitized data will be lost after validation completes, and the original
unsanitized data will be processed and stored in the database, potentially
introducing security vulnerabilities or data corruption.
###### Suggested change ā *Feature Preview*
The consuming code needs to retrieve the sanitized data from the ContextVar
after validation. Add a method to retrieve sanitized data:
```python
def get_sanitized_json_data() -> str | None:
return sanitized_json_context.get()
```
Then modify the API endpoints to use this sanitized data instead of the
original input when storing to the database.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6af5f09d-5f34-4415-bfa2-64f49522c8d5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6af5f09d-5f34-4415-bfa2-64f49522c8d5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6af5f09d-5f34-4415-bfa2-64f49522c8d5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6af5f09d-5f34-4415-bfa2-64f49522c8d5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6af5f09d-5f34-4415-bfa2-64f49522c8d5)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9d599a9d-e5c7-48ee-940e-3a166b7a47f2 -->
[](9d599a9d-e5c7-48ee-940e-3a166b7a47f2)
##########
superset/charts/schemas.py:
##########
@@ -459,7 +459,7 @@ class
ChartDataAggregateOptionsSchema(ChartDataPostProcessingOperationOptionsSch
allow_none=False,
metadata={"description": "Columns by which to group by"},
),
- minLength=1,
+ validate=Length(min=1),
Review Comment:
### Missing import for Length validator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The `Length` validator is being used without proper import, which will cause
a NameError at runtime.
###### Why this matters
This will cause the application to crash when these schema classes are
instantiated or used for validation, as `Length` is not defined in the current
scope.
###### Suggested change ā *Feature Preview*
The `Length` validator is already imported at the top of the file: `from
marshmallow.validate import Length, Range`. The code should work correctly as
`Length` is properly imported.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93343215-1372-40da-8bde-682cfd0bcc72/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93343215-1372-40da-8bde-682cfd0bcc72?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93343215-1372-40da-8bde-682cfd0bcc72?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93343215-1372-40da-8bde-682cfd0bcc72?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/93343215-1372-40da-8bde-682cfd0bcc72)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:aadc3a53-c875-4c81-bde6-24b1c58b9b60 -->
[](aadc3a53-c875-4c81-bde6-24b1c58b9b60)
##########
superset/marshmallow_compatibility.py:
##########
@@ -0,0 +1,68 @@
+"""
+Marshmallow 4.x Compatibility Module for Flask-AppBuilder 5.0.0
+
+This module provides compatibility between Flask-AppBuilder 5.0.0 and
+marshmallow 4.x, specifically handling missing auto-generated fields
+during schema initialization.
+"""
+
+from marshmallow import fields
+
+
+def patch_marshmallow_for_flask_appbuilder():
+ """
+ Patches marshmallow Schema._init_fields to handle Flask-AppBuilder 5.0.0
+ compatibility with marshmallow 4.x.
+
+ Flask-AppBuilder 5.0.0 automatically generates schema fields that reference
+ SQL relationship fields that may not exist in marshmallow 4.x's stricter
+ field validation. This patch dynamically adds missing fields as Raw fields
+ to prevent KeyError exceptions during schema initialization.
+ """
+ import marshmallow
+
+ # Store the original method
+ original_init_fields = marshmallow.Schema._init_fields
+
+ def patched_init_fields(self):
+ """Patched version that handles missing declared fields."""
+ max_retries = 10 # Prevent infinite loops in case of unexpected errors
+ retries = 0
+
+ while retries < max_retries:
+ try:
+ return original_init_fields(self)
+ except KeyError as e:
+ # Extract the missing field name from the KeyError
+ missing_field = str(e).strip("'\"")
+
+ # Initialize declared_fields if it doesn't exist
+ if not hasattr(self, "declared_fields"):
+ self.declared_fields = {}
+
+ # Only add if it doesn't already exist
+ if missing_field not in self.declared_fields:
+ # Use Raw field as a safe fallback for unknown
auto-generated fields
+ self.declared_fields[missing_field] = fields.Raw(
+ allow_none=True,
+ dump_only=True, # Prevent validation issues during
serialization
+ )
+
+ print(
+ f"Marshmallow compatibility: Added missing field "
+ f"'{missing_field}' as Raw field"
+ )
+
+ retries += 1
Review Comment:
### Inefficient retry loop with redundant field processing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The retry loop performs redundant work by repeatedly calling
original_init_fields() and re-processing the same KeyError exceptions without
accumulating missing fields in a single pass.
###### Why this matters
This approach leads to O(nĀ²) time complexity where n is the number of
missing fields, as each retry processes all previously encountered missing
fields again, causing unnecessary computation overhead.
###### Suggested change ā *Feature Preview*
Collect all missing fields in a single pass by catching all KeyErrors in one
try-except block, then add all missing fields at once before calling
original_init_fields() only once:
```python
def patched_init_fields(self):
"""Patched version that handles missing declared fields."""
# Initialize declared_fields if it doesn't exist
if not hasattr(self, "declared_fields"):
self.declared_fields = {}
# Try to identify all missing fields upfront
missing_fields = set()
max_attempts = 10
for attempt in range(max_attempts):
try:
return original_init_fields(self)
except KeyError as e:
missing_field = str(e).strip("'\"")
if missing_field not in missing_fields and missing_field not in
self.declared_fields:
missing_fields.add(missing_field)
self.declared_fields[missing_field] = fields.Raw(
allow_none=True,
dump_only=True
)
print(f"Marshmallow compatibility: Added missing field
'{missing_field}' as Raw field")
else:
# If we're seeing the same field again, something else is
wrong
break
except Exception:
raise
raise RuntimeError(f"Marshmallow field initialization failed after
{max_attempts} attempts")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ba4b961-a547-42cc-92c4-ddcc60a10dea/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ba4b961-a547-42cc-92c4-ddcc60a10dea?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ba4b961-a547-42cc-92c4-ddcc60a10dea?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ba4b961-a547-42cc-92c4-ddcc60a10dea?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6ba4b961-a547-42cc-92c4-ddcc60a10dea)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f592693b-ae03-4a42-929a-4c251cf2c3fb -->
[](f592693b-ae03-4a42-929a-4c251cf2c3fb)
##########
superset/marshmallow_compatibility.py:
##########
@@ -0,0 +1,68 @@
+"""
+Marshmallow 4.x Compatibility Module for Flask-AppBuilder 5.0.0
+
+This module provides compatibility between Flask-AppBuilder 5.0.0 and
+marshmallow 4.x, specifically handling missing auto-generated fields
+during schema initialization.
+"""
+
+from marshmallow import fields
+
+
+def patch_marshmallow_for_flask_appbuilder():
+ """
+ Patches marshmallow Schema._init_fields to handle Flask-AppBuilder 5.0.0
+ compatibility with marshmallow 4.x.
+
+ Flask-AppBuilder 5.0.0 automatically generates schema fields that reference
+ SQL relationship fields that may not exist in marshmallow 4.x's stricter
+ field validation. This patch dynamically adds missing fields as Raw fields
+ to prevent KeyError exceptions during schema initialization.
+ """
+ import marshmallow
+
+ # Store the original method
+ original_init_fields = marshmallow.Schema._init_fields
+
+ def patched_init_fields(self):
+ """Patched version that handles missing declared fields."""
+ max_retries = 10 # Prevent infinite loops in case of unexpected errors
+ retries = 0
+
+ while retries < max_retries:
+ try:
+ return original_init_fields(self)
+ except KeyError as e:
+ # Extract the missing field name from the KeyError
+ missing_field = str(e).strip("'\"")
+
+ # Initialize declared_fields if it doesn't exist
+ if not hasattr(self, "declared_fields"):
+ self.declared_fields = {}
+
+ # Only add if it doesn't already exist
+ if missing_field not in self.declared_fields:
+ # Use Raw field as a safe fallback for unknown
auto-generated fields
+ self.declared_fields[missing_field] = fields.Raw(
+ allow_none=True,
+ dump_only=True, # Prevent validation issues during
serialization
+ )
+
+ print(
+ f"Marshmallow compatibility: Added missing field "
+ f"'{missing_field}' as Raw field"
+ )
+
+ retries += 1
+ # Continue the loop to retry initialization
+ except Exception:
+ # For any other type of error, just propagate it
+ raise
+
+ # If we've exhausted retries, something is seriously wrong
+ raise RuntimeError(
+ f"Marshmallow field initialization failed after {max_retries}
retries"
+ )
+
+ # Apply the patch
+ marshmallow.Schema._init_fields = patched_init_fields
Review Comment:
### Unmanaged Monkey-Patching of External Library <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function directly modifies a core class method of an external library
(marshmallow.Schema._init_fields) without providing a way to revert the changes
or manage the patching lifecycle.
###### Why this matters
Monkey-patching without proper lifecycle management can lead to
hard-to-debug issues, especially in larger applications where multiple
components might need different behaviors or when the patch needs to be
temporarily disabled.
###### Suggested change ā *Feature Preview*
Implement a context manager or explicit enable/disable functions to manage
the patch lifecycle:
```python
class MarshmallowPatcher:
def __init__(self):
self.original_init_fields = None
def __enter__(self):
import marshmallow
self.original_init_fields = marshmallow.Schema._init_fields
marshmallow.Schema._init_fields = self.patched_init_fields
return self
def __exit__(self, *args):
import marshmallow
marshmallow.Schema._init_fields = self.original_init_fields
@staticmethod
def patched_init_fields(self):
# existing patch logic
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/018674a9-4243-4e69-b477-e1fe602ba68b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/018674a9-4243-4e69-b477-e1fe602ba68b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/018674a9-4243-4e69-b477-e1fe602ba68b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/018674a9-4243-4e69-b477-e1fe602ba68b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/018674a9-4243-4e69-b477-e1fe602ba68b)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:afdf2a03-15f8-4253-8157-513a9793dffd -->
[](afdf2a03-15f8-4253-8157-513a9793dffd)
##########
superset/themes/schemas.py:
##########
@@ -56,7 +62,7 @@ def validate_json_data(self, value: dict[str, Any]) -> None:
value.clear()
value.update(sanitized_config)
else:
- self.context["sanitized_json_data"] =
json.dumps(sanitized_config)
+ sanitized_json_context.set(json.dumps(sanitized_config))
Review Comment:
### Redundant JSON serialization without caching <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
JSON serialization is performed multiple times for the same data without
caching the result.
###### Why this matters
The `json.dumps()` operation is computationally expensive for large theme
configurations and is called repeatedly in validation methods without storing
the result for reuse, leading to unnecessary CPU overhead.
###### Suggested change ā *Feature Preview*
Cache the serialized JSON result to avoid repeated serialization of the same
data:
```python
if sanitized_config != theme_config:
# Cache the serialized result to avoid repeated computation
if not hasattr(self, '_cached_sanitized_json') or
self._cached_sanitized_json is None:
self._cached_sanitized_json = json.dumps(sanitized_config)
sanitized_json_context.set(self._cached_sanitized_json)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e801c6c3-c36d-451f-b980-3a9f12f1dc49/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e801c6c3-c36d-451f-b980-3a9f12f1dc49?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e801c6c3-c36d-451f-b980-3a9f12f1dc49?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e801c6c3-c36d-451f-b980-3a9f12f1dc49?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e801c6c3-c36d-451f-b980-3a9f12f1dc49)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cce461c5-5758-4afb-8bdd-a161142b5951 -->
[](cce461c5-5758-4afb-8bdd-a161142b5951)
##########
superset/app.py:
##########
@@ -43,6 +43,16 @@
logger = logging.getLogger(__name__)
+# Apply marshmallow 4.x compatibility patch for Flask-AppBuilder
+try:
+ from superset.marshmallow_compatibility import (
+ patch_marshmallow_for_flask_appbuilder,
+ )
+
+ patch_marshmallow_for_flask_appbuilder()
+except ImportError:
+ logger.debug("marshmallow_compatibility module not found, skipping patch")
Review Comment:
### Module-level patch execution adds import overhead <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The marshmallow compatibility patch is executed at module import time, which
adds overhead to every import of the app module regardless of whether the patch
is needed.
###### Why this matters
This causes unnecessary computation during module imports, potentially
slowing down application startup and any code that imports this module,
especially in scenarios where the patch may not be required.
###### Suggested change ā *Feature Preview*
Move the patch execution to the `create_app()` function or make it lazy by
only applying the patch when actually needed. For example:
```python
def create_app(superset_config_module: Optional[str] = None,
superset_app_root: Optional[str] = None) -> Flask:
# Apply marshmallow compatibility patch if needed
try:
from superset.marshmallow_compatibility import
patch_marshmallow_for_flask_appbuilder
patch_marshmallow_for_flask_appbuilder()
except ImportError:
logger.debug("marshmallow_compatibility module not found, skipping
patch")
app = SupersetApp(__name__)
# ... rest of function
```
This ensures the patch is only applied when an app is actually being created.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21e16f6f-223e-41a0-be89-0ce1a940d903/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21e16f6f-223e-41a0-be89-0ce1a940d903?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21e16f6f-223e-41a0-be89-0ce1a940d903?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21e16f6f-223e-41a0-be89-0ce1a940d903?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/21e16f6f-223e-41a0-be89-0ce1a940d903)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9f8b7141-0c47-4571-b6dd-a418f1e1b929 -->
[](9f8b7141-0c47-4571-b6dd-a418f1e1b929)
##########
superset/marshmallow_compatibility.py:
##########
@@ -0,0 +1,68 @@
+"""
+Marshmallow 4.x Compatibility Module for Flask-AppBuilder 5.0.0
+
+This module provides compatibility between Flask-AppBuilder 5.0.0 and
+marshmallow 4.x, specifically handling missing auto-generated fields
+during schema initialization.
+"""
+
+from marshmallow import fields
+
+
+def patch_marshmallow_for_flask_appbuilder():
+ """
+ Patches marshmallow Schema._init_fields to handle Flask-AppBuilder 5.0.0
+ compatibility with marshmallow 4.x.
+
+ Flask-AppBuilder 5.0.0 automatically generates schema fields that reference
+ SQL relationship fields that may not exist in marshmallow 4.x's stricter
+ field validation. This patch dynamically adds missing fields as Raw fields
+ to prevent KeyError exceptions during schema initialization.
+ """
+ import marshmallow
+
+ # Store the original method
+ original_init_fields = marshmallow.Schema._init_fields
+
+ def patched_init_fields(self):
+ """Patched version that handles missing declared fields."""
+ max_retries = 10 # Prevent infinite loops in case of unexpected errors
+ retries = 0
+
+ while retries < max_retries:
+ try:
+ return original_init_fields(self)
+ except KeyError as e:
+ # Extract the missing field name from the KeyError
+ missing_field = str(e).strip("'\"")
+
+ # Initialize declared_fields if it doesn't exist
+ if not hasattr(self, "declared_fields"):
+ self.declared_fields = {}
+
+ # Only add if it doesn't already exist
+ if missing_field not in self.declared_fields:
+ # Use Raw field as a safe fallback for unknown
auto-generated fields
+ self.declared_fields[missing_field] = fields.Raw(
+ allow_none=True,
+ dump_only=True, # Prevent validation issues during
serialization
+ )
+
+ print(
+ f"Marshmallow compatibility: Added missing field "
+ f"'{missing_field}' as Raw field"
+ )
Review Comment:
### Improper Logging Mechanism <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using print statements for logging in a library module instead of proper
logging mechanisms.
###### Why this matters
Print statements cannot be configured, filtered, or redirected like proper
logging, making it difficult to manage output in production environments.
###### Suggested change ā *Feature Preview*
Replace print with proper logging:
```python
import logging
logger = logging.getLogger(__name__)
# Later in the code:
logger.debug(f"Added missing field '{missing_field}' as Raw field")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c31a285f-56be-404f-a1b8-062e8fdb11c1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c31a285f-56be-404f-a1b8-062e8fdb11c1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c31a285f-56be-404f-a1b8-062e8fdb11c1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c31a285f-56be-404f-a1b8-062e8fdb11c1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c31a285f-56be-404f-a1b8-062e8fdb11c1)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:72b68619-6fe4-4000-9684-1527d949a77a -->
[](72b68619-6fe4-4000-9684-1527d949a77a)
##########
superset/marshmallow_compatibility.py:
##########
@@ -0,0 +1,68 @@
+"""
+Marshmallow 4.x Compatibility Module for Flask-AppBuilder 5.0.0
+
+This module provides compatibility between Flask-AppBuilder 5.0.0 and
+marshmallow 4.x, specifically handling missing auto-generated fields
+during schema initialization.
+"""
+
+from marshmallow import fields
+
+
+def patch_marshmallow_for_flask_appbuilder():
+ """
+ Patches marshmallow Schema._init_fields to handle Flask-AppBuilder 5.0.0
+ compatibility with marshmallow 4.x.
+
+ Flask-AppBuilder 5.0.0 automatically generates schema fields that reference
+ SQL relationship fields that may not exist in marshmallow 4.x's stricter
+ field validation. This patch dynamically adds missing fields as Raw fields
+ to prevent KeyError exceptions during schema initialization.
+ """
+ import marshmallow
+
+ # Store the original method
+ original_init_fields = marshmallow.Schema._init_fields
+
+ def patched_init_fields(self):
+ """Patched version that handles missing declared fields."""
+ max_retries = 10 # Prevent infinite loops in case of unexpected errors
+ retries = 0
+
+ while retries < max_retries:
+ try:
+ return original_init_fields(self)
+ except KeyError as e:
+ # Extract the missing field name from the KeyError
+ missing_field = str(e).strip("'\"")
+
+ # Initialize declared_fields if it doesn't exist
+ if not hasattr(self, "declared_fields"):
+ self.declared_fields = {}
Review Comment:
### Unsafe declared_fields modification <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes it can modify the schema instance's declared_fields
attribute, but this may not be the correct approach for all marshmallow schema
types.
###### Why this matters
Some schema classes may have declared_fields as a class attribute or
property, and directly assigning to it on the instance could cause unexpected
behavior or not have the intended effect on field resolution.
###### Suggested change ā *Feature Preview*
Check if declared_fields exists and handle it more safely:
```python
# Get declared_fields, creating a copy if it's a class attribute
if hasattr(self.__class__, 'declared_fields'):
if not hasattr(self, '_instance_declared_fields'):
self._instance_declared_fields =
self.__class__.declared_fields.copy()
self.declared_fields = self._instance_declared_fields
else:
if not hasattr(self, 'declared_fields'):
self.declared_fields = {}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e5e7a52-e2d6-4973-9b5a-0d6134fc2f16/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e5e7a52-e2d6-4973-9b5a-0d6134fc2f16?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e5e7a52-e2d6-4973-9b5a-0d6134fc2f16?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e5e7a52-e2d6-4973-9b5a-0d6134fc2f16?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4e5e7a52-e2d6-4973-9b5a-0d6134fc2f16)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:dcb4e5da-03da-4883-bcb2-9e670693d75f -->
[](dcb4e5da-03da-4883-bcb2-9e670693d75f)
##########
superset/app.py:
##########
@@ -43,6 +43,16 @@
logger = logging.getLogger(__name__)
+# Apply marshmallow 4.x compatibility patch for Flask-AppBuilder
+try:
+ from superset.marshmallow_compatibility import (
+ patch_marshmallow_for_flask_appbuilder,
+ )
+
+ patch_marshmallow_for_flask_appbuilder()
+except ImportError:
+ logger.debug("marshmallow_compatibility module not found, skipping patch")
Review Comment:
### Marshmallow patch applied at module level <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The marshmallow compatibility patch is applied at module import time before
the Flask app is created, which may cause the patch to be applied multiple
times in multi-process environments or when the module is imported multiple
times.
###### Why this matters
This can lead to unexpected behavior where the patch function runs multiple
times, potentially causing duplicate monkey-patching or side effects that
weren't intended. In production environments with multiple workers or during
testing, this could result in inconsistent application state.
###### Suggested change ā *Feature Preview*
Move the marshmallow compatibility patch inside the `create_app()` function
or add a guard to ensure it's only applied once:
```python
# At module level, add a flag
_marshmallow_patched = False
def create_app(
superset_config_module: Optional[str] = None,
superset_app_root: Optional[str] = None,
) -> Flask:
global _marshmallow_patched
# Apply marshmallow 4.x compatibility patch for Flask-AppBuilder
if not _marshmallow_patched:
try:
from superset.marshmallow_compatibility import (
patch_marshmallow_for_flask_appbuilder,
)
patch_marshmallow_for_flask_appbuilder()
_marshmallow_patched = True
except ImportError:
logger.debug("marshmallow_compatibility module not found,
skipping patch")
app = SupersetApp(__name__)
# ... rest of the function
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45e10cfd-1dec-4af6-8a0c-e8a501b64ec0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45e10cfd-1dec-4af6-8a0c-e8a501b64ec0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45e10cfd-1dec-4af6-8a0c-e8a501b64ec0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45e10cfd-1dec-4af6-8a0c-e8a501b64ec0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/45e10cfd-1dec-4af6-8a0c-e8a501b64ec0)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b3ab318c-3aa5-4b83-8672-8d5b1a43d8ea -->
[](b3ab318c-3aa5-4b83-8672-8d5b1a43d8ea)
##########
superset/marshmallow_compatibility.py:
##########
@@ -0,0 +1,68 @@
+"""
+Marshmallow 4.x Compatibility Module for Flask-AppBuilder 5.0.0
+
+This module provides compatibility between Flask-AppBuilder 5.0.0 and
+marshmallow 4.x, specifically handling missing auto-generated fields
+during schema initialization.
+"""
+
+from marshmallow import fields
+
+
+def patch_marshmallow_for_flask_appbuilder():
+ """
+ Patches marshmallow Schema._init_fields to handle Flask-AppBuilder 5.0.0
+ compatibility with marshmallow 4.x.
+
+ Flask-AppBuilder 5.0.0 automatically generates schema fields that reference
+ SQL relationship fields that may not exist in marshmallow 4.x's stricter
+ field validation. This patch dynamically adds missing fields as Raw fields
+ to prevent KeyError exceptions during schema initialization.
+ """
+ import marshmallow
+
+ # Store the original method
+ original_init_fields = marshmallow.Schema._init_fields
+
+ def patched_init_fields(self):
+ """Patched version that handles missing declared fields."""
+ max_retries = 10 # Prevent infinite loops in case of unexpected errors
+ retries = 0
+
+ while retries < max_retries:
+ try:
+ return original_init_fields(self)
+ except KeyError as e:
+ # Extract the missing field name from the KeyError
+ missing_field = str(e).strip("'\"")
Review Comment:
### Unreliable KeyError message parsing <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The KeyError exception message parsing is unreliable and may not correctly
extract the field name from different KeyError formats.
###### Why this matters
KeyError messages can vary in format across Python versions and contexts.
Simply stripping quotes may not work for all cases, potentially leading to
incorrect field names being added or the patch failing to work properly.
###### Suggested change ā *Feature Preview*
Use a more robust method to extract the field name from KeyError:
```python
def extract_field_name_from_keyerror(e):
"""Extract field name from KeyError exception."""
# KeyError args[0] contains the actual key that was missing
if e.args:
return str(e.args[0])
# Fallback to string parsing if args is empty
return str(e).strip("'\"")
missing_field = extract_field_name_from_keyerror(e)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5954bcc0-6588-4aca-afd6-d7c979b6a197/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5954bcc0-6588-4aca-afd6-d7c979b6a197?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5954bcc0-6588-4aca-afd6-d7c979b6a197?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5954bcc0-6588-4aca-afd6-d7c979b6a197?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5954bcc0-6588-4aca-afd6-d7c979b6a197)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1fc3d49e-4cf6-4a8d-b11b-2a3ad2629a6f -->
[](1fc3d49e-4cf6-4a8d-b11b-2a3ad2629a6f)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
