codeant-ai-for-open-source[bot] commented on PR #36985: URL: https://github.com/apache/superset/pull/36985#issuecomment-3724574792
## 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/36985/files#diff-090abddb6d9306c432a5642a19da74b293f5fe31f6b3ac707bf0314d421693c8R1062-R1065'><strong>Possible Bug</strong></a><br>The new code unconditionally sets `adhoc_filter["subject"]` for filters whose `operator` is "TEMPORAL_RANGE". Some adhoc filters may be of expression type "SQL" (which use `sqlExpression` instead of `subject`) or may not have `subject` at all; overwriting or adding `subject` to these could produce incorrect or inconsistent filter behavior. Validate the filter's expression type or the presence of `subject` before mutating it.<br> - [ ] <a href='https://github.com/apache/superset/pull/36985/files#diff-288a899a3df22faccec12d1a49d0b8d1ae90eb2761fd431f587aa652803055f3R1120-R1158'><strong>Coverage gap</strong></a><br>There is no unit test exercising the branch where `extra_form_data` includes a `time_range` but no `granularity_sqla`. The implementation contains logic that sets TEMPORAL_RANGE `comparator` only when `form_data.get("time_range") and not form_data.get("granularity_sqla")`. Add a test for that scenario to prevent regressions.<br> - [ ] <a href='https://github.com/apache/superset/pull/36985/files#diff-288a899a3df22faccec12d1a49d0b8d1ae90eb2761fd431f587aa652803055f3R1093-R1116'><strong>Missing post-condition check</strong></a><br>merge_extra_form_data pops `extra_form_data` from the input (it calls `form_data.pop("extra_form_data", {})`). The tests do not assert that `extra_form_data` has been removed from `form_data` after the call. Adding an assertion will make the test verify the full contract/side-effects of the function.<br> - [ ] <a href='https://github.com/apache/superset/pull/36985/files#diff-53082ed52a75af5bd61d9c4cc7d9183fd02aab5e5d4e50abb44a779cbcc29c0cR299-R304'><strong>Cache pollution risk</strong></a><br>The test writes a cache entry with `cache_manager.explore_form_data_cache.set(test_form_data_key, entry)` but does not remove it afterward. If the cache is shared across tests, this can create flakiness or interfere with other tests. Ensure the cache key is cleaned up after the test.<br> - [ ] <a href='https://github.com/apache/superset/pull/36985/files#diff-53082ed52a75af5bd61d9c4cc7d9183fd02aab5e5d4e50abb44a779cbcc29c0cR316-R322'><strong>Fragile assertion</strong></a><br>The test only asserts the subject of the first TEMPORAL_RANGE filter (`temporal_range_filters[0]`) — if multiple TEMPORAL_RANGE filters are present only the first is validated, which can hide regressions where only some filters are updated. Consider asserting all matching filters are updated.<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]
