codeant-ai-for-open-source[bot] commented on PR #36958: URL: https://github.com/apache/superset/pull/36958#issuecomment-3720974619
## 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/36958/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR200-R210'><strong>Backwards compatibility</strong></a><br>Existing database rows may have NULL/absent `configuration_method` (server_default applies only to new rows). Verify export, import and the UI handle NULL values gracefully and that the exported YAML doesn't produce unexpected errors when `configuration_method` is absent.<br> - [ ] <a href='https://github.com/apache/superset/pull/36958/files#diff-462c756528595a2be4a0a3c3063077f984384c7858ecdb5bd3e9603163c3678bR200-R210'><strong>Import validation</strong></a><br>The PR adds `configuration_method` to export/import fields. Make sure imports handle missing or invalid values (older rows, malformed YAML). The string is stored as free-form text; importing unknown values could lead to inconsistent UI behaviour. Consider coercing to a safe default or validating against `ConfigurationMethod`.<br> - [ ] <a href='https://github.com/apache/superset/pull/36958/files#diff-3dd3f2419d1263ecdd17242eed1721da7a3e028fa2ed8e17893d9dcd31910289R891-R893'><strong>Inconsistent representation</strong></a><br>The new `configuration_method` Enum is added to the Import V1 schema, but other schemas and serialized representations may treat `configuration_method` as a plain string (or have different defaults). Verify imports/exports and other schemas (and the frontend) expect the same representation and default when the field is missing to avoid subtle mismatches on import/export.<br> - [ ] <a href='https://github.com/apache/superset/pull/36958/files#diff-023d03cc917d159e443cf0e947dda5e28cfdf7c5a26c592860e1d419fc5b317eR360-R378'><strong>Missing round-trip coverage</strong></a><br>The change ensures `configuration_method` is exported, but there is no test verifying that an export containing `configuration_method` is correctly imported and persisted/handled (or gracefully ignored) by the importer. Add a round-trip test (export -> import) or a targeted import test to validate importer behavior for databases that require `configuration_method` (e.g., BigQuery service account flow).<br> - [ ] <a href='https://github.com/apache/superset/pull/36958/files#diff-5694d787037e6535ae595aa351e46cc55121a7f56692ae05d97cd99fcbe9a779R300-R300'><strong>Implicit default reliance</strong></a><br>The test expects `configuration_method: sqlalchemy_form` but the created `Database` instance isn't explicitly assigned a `configuration_method`. This relies on an implicit default in the model/exporter — the test should explicitly set or verify where the default comes from to avoid false positives if defaults change.<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]
