korbit-ai[bot] commented on code in PR #33037:
URL: https://github.com/apache/superset/pull/33037#discussion_r2033210232
##########
superset/migrations/shared/utils.py:
##########
@@ -172,11 +172,7 @@ def paginated_update(
def try_load_json(data: Optional[str]) -> dict[str, Any]:
- try:
- return data and json.loads(data) or {}
- except json.JSONDecodeError:
- print(f"Failed to parse: {data}")
- return {}
+ return data and json.loads(data) or {}
Review Comment:
### Missing JSON Error Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function no longer handles JSON decoding errors, which means any
malformed JSON will raise an unhandled json.JSONDecodeError.
###### Why this matters
If this function receives invalid JSON data, it will crash the application
instead of gracefully returning an empty dictionary as it did before.
###### Suggested change ∙ *Feature Preview*
Restore the error handling to preserve the function's reliability:
```python
def try_load_json(data: Optional[str]) -> dict[str, Any]:
try:
return data and json.loads(data) or {}
except json.JSONDecodeError:
logger.warning(f"Failed to parse JSON: {data}")
return {}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28fa0ca2-3184-430e-a7e6-a95704427513/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28fa0ca2-3184-430e-a7e6-a95704427513?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28fa0ca2-3184-430e-a7e6-a95704427513?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28fa0ca2-3184-430e-a7e6-a95704427513?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28fa0ca2-3184-430e-a7e6-a95704427513)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cbd744d1-2e38-412d-b997-b1695c58ab5f -->
[](cbd744d1-2e38-412d-b997-b1695c58ab5f)
##########
superset/migrations/shared/migrate_viz/base.py:
##########
@@ -121,34 +121,44 @@ def _migrate_temporal_filter(self, rv_data: dict[str,
Any]) -> None:
@classmethod
def upgrade_slice(cls, slc: Slice) -> None:
- clz = cls(slc.params)
- form_data_bak = copy.deepcopy(clz.data)
+ try:
+ clz = cls(slc.params)
+ form_data_bak = copy.deepcopy(clz.data)
- clz._pre_action()
- clz._migrate()
- clz._post_action()
+ clz._pre_action()
+ clz._migrate()
+ clz._post_action()
- # viz_type depends on the migration and should be set after its
execution
- # because a source viz can be mapped to different target viz types
- slc.viz_type = clz.target_viz_type
+ # viz_type depends on the migration and should be set after its
execution
+ # because a source viz can be mapped to different target viz types
+ slc.viz_type = clz.target_viz_type
- # only backup params
- slc.params = json.dumps({**clz.data, FORM_DATA_BAK_FIELD_NAME:
form_data_bak})
+ # only backup params
+ slc.params = json.dumps(
+ {**clz.data, FORM_DATA_BAK_FIELD_NAME: form_data_bak}
+ )
- if "form_data" in (query_context := try_load_json(slc.query_context)):
- query_context["form_data"] = clz.data
- slc.query_context = json.dumps(query_context)
+ if "form_data" in (query_context :=
try_load_json(slc.query_context)):
+ query_context["form_data"] = clz.data
+ slc.query_context = json.dumps(query_context)
+ except Exception as e:
Review Comment:
### Over-broad Exception Handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using a broad Exception catch without specific error handling for different
error types
###### Why this matters
Catching all exceptions makes it difficult to handle different error cases
appropriately and can mask serious problems that should be handled differently.
###### Suggested change ∙ *Feature Preview*
Catch specific exceptions:
```python
except (ValueError, JSONDecodeError) as e:
logging.error(f"Failed to parse slice {slc.id} data", exc_info=e)
except AttributeError as e:
logging.error(f"Invalid slice {slc.id} structure", exc_info=e)
except Exception as e:
logging.error(f"Unexpected error migrating slice {slc.id}", exc_info=e)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0d1dd12-df3a-4640-8831-2c2b2c132819/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0d1dd12-df3a-4640-8831-2c2b2c132819?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0d1dd12-df3a-4640-8831-2c2b2c132819?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0d1dd12-df3a-4640-8831-2c2b2c132819?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c0d1dd12-df3a-4640-8831-2c2b2c132819)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:29e2adca-9023-4cc4-81ff-4f67214ddeed -->
[](29e2adca-9023-4cc4-81ff-4f67214ddeed)
--
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]
