korbit-ai[bot] commented on code in PR #36076:
URL: https://github.com/apache/superset/pull/36076#discussion_r2515136301
##########
superset/utils/schema.py:
##########
@@ -51,3 +51,43 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
json.validate_json(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
+
+
+def validate_query_context_metadata(value: bytes | bytearray | str | None) ->
None:
+ """
+ Validator for query_context field to ensure it contains required metadata.
+
+ Validates that the query_context JSON contains the required 'datasource'
and
+ 'queries' fields needed for chart data retrieval.
+
+ :raises ValidationError: if value is not valid JSON or missing required
fields
+ :param value: a JSON string that should contain datasource and queries
metadata
+ """
+ if value is None or not value:
+ return # Allow None values and empty strings
+
+ try:
+ parsed_data = json.loads(value)
+ except json.JSONDecodeError as ex:
+ raise ValidationError("JSON not valid") from ex
+
+ # Validate required fields exist in the query_context
+ if not isinstance(parsed_data, dict):
+ raise ValidationError("Query context must be a valid JSON object")
+
+ missing_fields = []
+
+ # When query_context is provided (not None), validate it has required
fields
+ if "datasource" not in parsed_data:
+ missing_fields.append("datasource")
+
+ if "queries" not in parsed_data:
+ missing_fields.append("queries")
+
+ if missing_fields:
+ error_msg = (
+ f"Query context is missing required fields: {',
'.join(missing_fields)}"
+ )
+ raise ValidationError(
+ error_msg,
+ )
Review Comment:
### Inefficient field validation with list collection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code uses a list to collect missing fields and then checks if the list
is non-empty, which is inefficient for validation that could fail fast.
###### Why this matters
This approach performs unnecessary list operations and string formatting
when validation could fail immediately upon finding the first missing field,
reducing both time complexity and memory allocation.
###### Suggested change ∙ *Feature Preview*
Replace the list-based approach with immediate validation that raises an
exception on the first missing field:
```python
if "datasource" not in parsed_data:
raise ValidationError("Query context is missing required field:
datasource")
if "queries" not in parsed_data:
raise ValidationError("Query context is missing required field: queries")
```
Alternatively, use a set-based approach for better performance:
```python
required_fields = {"datasource", "queries"}
missing_fields = required_fields - parsed_data.keys()
if missing_fields:
raise ValidationError(f"Query context is missing required fields: {',
'.join(sorted(missing_fields))}")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b76e40de-69a1-4154-8fa0-74c6e2b6f2f8/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b76e40de-69a1-4154-8fa0-74c6e2b6f2f8?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b76e40de-69a1-4154-8fa0-74c6e2b6f2f8?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b76e40de-69a1-4154-8fa0-74c6e2b6f2f8?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b76e40de-69a1-4154-8fa0-74c6e2b6f2f8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4a2117f6-ecbc-4729-b699-e0af46d35596 -->
[](4a2117f6-ecbc-4729-b699-e0af46d35596)
##########
superset/utils/schema.py:
##########
@@ -51,3 +51,43 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
json.validate_json(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
+
+
+def validate_query_context_metadata(value: bytes | bytearray | str | None) ->
None:
+ """
+ Validator for query_context field to ensure it contains required metadata.
+
+ Validates that the query_context JSON contains the required 'datasource'
and
+ 'queries' fields needed for chart data retrieval.
+
+ :raises ValidationError: if value is not valid JSON or missing required
fields
+ :param value: a JSON string that should contain datasource and queries
metadata
+ """
+ if value is None or not value:
+ return # Allow None values and empty strings
+
+ try:
+ parsed_data = json.loads(value)
+ except json.JSONDecodeError as ex:
+ raise ValidationError("JSON not valid") from ex
+
+ # Validate required fields exist in the query_context
+ if not isinstance(parsed_data, dict):
+ raise ValidationError("Query context must be a valid JSON object")
+
+ missing_fields = []
+
+ # When query_context is provided (not None), validate it has required
fields
+ if "datasource" not in parsed_data:
+ missing_fields.append("datasource")
+
+ if "queries" not in parsed_data:
+ missing_fields.append("queries")
+
+ if missing_fields:
Review Comment:
### Imperative field validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The required fields validation uses imperative logic with multiple steps
when it could be more declarative.
###### Why this matters
The current approach is more verbose and harder to maintain than necessary.
Adding new required fields would require modifying multiple lines of code.
###### Suggested change ∙ *Feature Preview*
Use a more declarative approach with a set of required fields:
```python
def validate_query_context_metadata(value: bytes | bytearray | str | None)
-> None:
...
REQUIRED_FIELDS = {"datasource", "queries"}
missing_fields = REQUIRED_FIELDS - parsed_data.keys()
if missing_fields:
raise ValidationError(f"Query context is missing required fields:
{', '.join(missing_fields)}")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2ddbc62-af2e-4bd9-99bd-2202c7eb8b78/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2ddbc62-af2e-4bd9-99bd-2202c7eb8b78?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2ddbc62-af2e-4bd9-99bd-2202c7eb8b78?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2ddbc62-af2e-4bd9-99bd-2202c7eb8b78?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2ddbc62-af2e-4bd9-99bd-2202c7eb8b78)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5488fadf-d34b-4c97-a59a-e77aaf388769 -->
[](5488fadf-d34b-4c97-a59a-e77aaf388769)
##########
superset/utils/schema.py:
##########
@@ -51,3 +51,43 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
json.validate_json(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
+
+
+def validate_query_context_metadata(value: bytes | bytearray | str | None) ->
None:
+ """
+ Validator for query_context field to ensure it contains required metadata.
+
+ Validates that the query_context JSON contains the required 'datasource'
and
+ 'queries' fields needed for chart data retrieval.
+
+ :raises ValidationError: if value is not valid JSON or missing required
fields
+ :param value: a JSON string that should contain datasource and queries
metadata
+ """
+ if value is None or not value:
+ return # Allow None values and empty strings
+
+ try:
+ parsed_data = json.loads(value)
+ except json.JSONDecodeError as ex:
+ raise ValidationError("JSON not valid") from ex
Review Comment:
### Duplicate JSON validation logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The function duplicates JSON validation logic that already exists in the
validate_json function above it.
###### Why this matters
Code duplication increases maintenance burden and risk of inconsistencies
when changes are needed. If the JSON validation logic needs to change, it would
need to be updated in multiple places.
###### Suggested change ∙ *Feature Preview*
Modify the function to reuse the existing validate_json function:
```python
def validate_query_context_metadata(value: bytes | bytearray | str | None)
-> None:
if value is None or not value:
return
validate_json(value)
parsed_data = json.loads(value)
# Continue with metadata validation...
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/14f5ec67-fff0-4243-af4f-0fb3bbc2422f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/14f5ec67-fff0-4243-af4f-0fb3bbc2422f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/14f5ec67-fff0-4243-af4f-0fb3bbc2422f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/14f5ec67-fff0-4243-af4f-0fb3bbc2422f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/14f5ec67-fff0-4243-af4f-0fb3bbc2422f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:7e7d6da0-7ce0-4ff4-8e7a-42b8e61476c9 -->
[](7e7d6da0-7ce0-4ff4-8e7a-42b8e61476c9)
##########
superset/utils/schema.py:
##########
@@ -51,3 +51,43 @@ def validate_json(value: Union[bytes, bytearray, str]) ->
None:
json.validate_json(value)
except json.JSONDecodeError as ex:
raise ValidationError("JSON not valid") from ex
+
+
+def validate_query_context_metadata(value: bytes | bytearray | str | None) ->
None:
+ """
+ Validator for query_context field to ensure it contains required metadata.
+
+ Validates that the query_context JSON contains the required 'datasource'
and
+ 'queries' fields needed for chart data retrieval.
+
+ :raises ValidationError: if value is not valid JSON or missing required
fields
+ :param value: a JSON string that should contain datasource and queries
metadata
+ """
+ if value is None or not value:
+ return # Allow None values and empty strings
Review Comment:
### Falsy JSON strings incorrectly bypass validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The condition `not value` will incorrectly return early for valid JSON
strings like '0', 'false', or '[]' because these evaluate to falsy in Python.
###### Why this matters
Valid JSON strings that represent falsy values (like the string '0' or
'false') will be incorrectly skipped during validation, potentially allowing
invalid data to pass through when these strings should be parsed and validated
for required fields.
###### Suggested change ∙ *Feature Preview*
Change the condition to explicitly check for empty strings:
```python
if value is None or value == "":
return # Allow None values and empty strings
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a978654e-8450-4230-831e-b3e438b7d3a1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a978654e-8450-4230-831e-b3e438b7d3a1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a978654e-8450-4230-831e-b3e438b7d3a1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a978654e-8450-4230-831e-b3e438b7d3a1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a978654e-8450-4230-831e-b3e438b7d3a1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0dff758-2f37-461b-aaab-253246fc868e -->
[](b0dff758-2f37-461b-aaab-253246fc868e)
--
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]
