[CDF-28123] fix(graphql): surface real API error when upsertGraphQlDmlVersion returns null#3088
[CDF-28123] fix(graphql): surface real API error when upsertGraphQlDmlVersion returns null#3088ronpal wants to merge 8 commits into
Conversation
## Bump - [x] Patch - [ ] Skip ## Changelog ### Fixed - `cdf build` rejecting unquoted timestamp-like values in config.yaml variables
…urns null
When the CDF DML API rejects a mutation it returns
{"data": {"upsertGraphQlDmlVersion": null}, "errors": [...]} — the
previous client parsed the response with Pydantic first, crashing on
the null before ever reaching the error list, leaving the user with an
opaque "Input should be an object" message.
Now check top-level GraphQL errors before Pydantic validation so the
real rejection reason is always surfaced. Also fix UpsertResponseData:
errors was typed as dict but the API returns a list of DMLError objects,
and result is now correctly nullable for when DML validation fails.
|
/gemini review |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
This reverts commit f1a33bb.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the GraphQL Data Models API by parsing raw JSON responses to surface top-level GraphQL and DML validation errors before Pydantic validation, and adds corresponding regression tests. The review feedback suggests enhancing robustness by ensuring the parsed response is a dictionary and handling cases where error messages are missing to avoid empty error outputs.
| raw = json.loads(response.body) | ||
| if top_errors := raw.get("errors"): | ||
| messages = [e.get("message", str(e)) for e in top_errors if isinstance(e, dict)] | ||
| raise ToolkitAPIError(f"GraphQL mutation failed: {humanize_collection(messages)}") |
There was a problem hiding this comment.
If the response body is not a JSON object (e.g., if it is a JSON array, a primitive, or if an intermediate proxy returns an unexpected format), raw will not be a dictionary and calling raw.get will raise an AttributeError. Additionally, if top_errors is not a list or contains non-dictionary elements, the current list comprehension will either ignore them or fail. Checking that raw is a dictionary and handling non-list/non-dict errors gracefully ensures robust error surfacing.
| raw = json.loads(response.body) | |
| if top_errors := raw.get("errors"): | |
| messages = [e.get("message", str(e)) for e in top_errors if isinstance(e, dict)] | |
| raise ToolkitAPIError(f"GraphQL mutation failed: {humanize_collection(messages)}") | |
| raw = json.loads(response.body) | |
| if isinstance(raw, dict) and (top_errors := raw.get("errors")): | |
| if isinstance(top_errors, list): | |
| messages = [e.get("message", str(e)) if isinstance(e, dict) else str(e) for e in top_errors] | |
| else: | |
| messages = [str(top_errors)] | |
| raise ToolkitAPIError(f"GraphQL mutation failed: {humanize_collection(messages)}") |
References
- Error Handling: Graceful handling: Provide meaningful error messages. (link)
| if upsert.errors: | ||
| messages = [e.message for e in upsert.errors if e.message] | ||
| raise ToolkitAPIError(f"DML validation failed: {humanize_collection(messages)}") |
There was a problem hiding this comment.
If upsert.errors is populated but none of the errors contain a message (e.g., they only have kind and hint), the list comprehension will produce an empty list, resulting in an unhelpful empty error message (DML validation failed: ). We should fall back to formatting the kind and hint fields if message is absent.
| if upsert.errors: | |
| messages = [e.message for e in upsert.errors if e.message] | |
| raise ToolkitAPIError(f"DML validation failed: {humanize_collection(messages)}") | |
| if upsert.errors: | |
| messages = [ | |
| e.message or f"{e.kind or 'Error'}(hint={e.hint})" | |
| for e in upsert.errors | |
| ] | |
| raise ToolkitAPIError(f"DML validation failed: {humanize_collection(messages)}") |
References
- Error Handling: Graceful handling: Provide meaningful error messages. (link)
☂️ Code Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3088 +/- ##
=======================================
Coverage 86.04% 86.04%
=======================================
Files 475 475
Lines 43828 43847 +19
=======================================
+ Hits 37712 37729 +17
- Misses 6116 6118 +2
🚀 New features to boost your workflow:
|
1 participant
Description
When the CDF DML API rejects a GraphQL mutation (e.g. unknown fields, schema
validation failures, auth issues) it returns:
{"data": {"upsertGraphQlDmlVersion": null}, "errors": [{"message": "...real reason..."}]}The previous client called
GraphQLResponse.model_validate_json()first — Pydanticcrashed on the
nullbefore the code ever reached theerrorscheck. The customer saw:instead of the actual API rejection reason, making the failure impossible to self-diagnose.
Changes:
errorsbefore Pydantic validation so the real message is always surfacedUpsertResponseData.errorscorrected fromdict→list[DMLError](matches the API contract)UpsertResponseData.resultmade nullable (API returnsnullwhen DML validation fails)GraphQLUpsertResponse.upsert_graph_ql_dml_versionmade nullable (correct per API contract)Bump
Changelog
Fixed
cdf deployfor GraphQL data models: the real API rejection reason is now surfacedinstead of an opaque Pydantic
"Input should be an object"error whenupsertGraphQlDmlVersionreturns null.