codeant-ai-for-open-source[bot] commented on PR #36934: URL: https://github.com/apache/superset/pull/36934#issuecomment-3721318320
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36934/files#diff-d3bf055ecf6a74aa0acbb0650d176b6c251aea7796543f77a2df3fd8b7e4c4b4R182-R186'><strong>Attribute availability</strong></a><br>The code returns `cache.queried_dttm` in the payload. Ensure the cache object always has this attribute (including for older cache entries or for cache implementations that don't set it). If the attribute is missing an AttributeError could be raised at runtime.<br> - [ ] <a href='https://github.com/apache/superset/pull/36934/files#diff-c8c1f6238c2b1e56fb96b47c3dec1ae7a944154cd45f85ca2f81c2b6300d86c6R114-R114'><strong>Timezone lost</strong></a><br>The code sets `queried_dttm` using `datetime.now(tz=timezone.utc).isoformat().split(".")[0]`, which strips fractional seconds and (importantly) the timezone offset (e.g. "+00:00"). As a result the stored timestamp can lose timezone information and become ambiguous. Prefer preserving timezone information in the stored ISO timestamp.<br> - [ ] <a href='https://github.com/apache/superset/pull/36934/files#diff-e56721f66184487dcf5beeac3e2fc56016bff3c9615cc7226a99b2fd598301dfR1467-R1473'><strong>Backwards compatibility risk</strong></a><br>Adding a required response field may break clients or server code paths that don't populate `queried_dttm` yet (older code paths, cached responses, async flows). Verify all code paths producing ChartDataResponseResult populate this field before marking it required.<br> - [ ] <a href='https://github.com/apache/superset/pull/36934/files#diff-e56721f66184487dcf5beeac3e2fc56016bff3c9615cc7226a99b2fd598301dfR1471-R1473'><strong>Required vs nullable semantics</strong></a><br>The field is declared `required=True` while also `allow_none=True`. This ambiguous combination may cause schema validation surprises (e.g. failing when the code doesn't set it, or accepting null when clients expect a valid timestamp). Confirm intended contract: always present non-null, optional, or present but allowed null, and make schema match that behavior.<br> - [ ] <a href='https://github.com/apache/superset/pull/36934/files#diff-e56721f66184487dcf5beeac3e2fc56016bff3c9615cc7226a99b2fd598301dfR1467-R1469'><strong>Data type choice</strong></a><br>The new `queried_dttm` field is declared as `fields.String` but represents an ISO-8601 UTC timestamp. Using a plain string loses automatic parsing/validation that `fields.DateTime(format="iso")` would provide and may allow invalid timestamp values to pass validation. Consider using a datetime field to enforce format and timezone semantics.<br> </td></tr> </table> -- 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]
