john-bodley opened a new issue, #25830: URL: https://github.com/apache/superset/issues/25830
## [SIP-99D] Proposal for consolidating Command/DAO create, update, and upsert operations This SIP is part of the [[SIP-99] Proposal for correctly handling business logic](https://github.com/apache/superset/issues/25048) series. Specifically, it proposes consolidating the Command/DAO create, update, and upsert* operations in a single upsert operation to adhere to the DRY principle which should significantly reduce the code footprint/complexity. \* upsert is a _portmanteau_ via the combination of the words "update" and "insert". These operations are already supported per the [`UpsertKeyValueCommand`](https://github.com/apache/superset/blob/1e37f0b41782d35d68f657bfa87aeb9055d1e6e7/superset/key_value/commands/upsert.py) (which interestingly references only the `CreateKeyValueCommand` command and not the `UpdateKeyValueCommand` command) and the [`EmbeddedDashboardDAO.upsert()`](https://github.com/apache/superset/blob/1e37f0b41782d35d68f657bfa87aeb9055d1e6e7/superset/daos/dashboard.py#L373C1-L385) method. ### Primer [[SIP-35] Proposal for Improving Superset’s Python Code Organization](https://github.com/apache/superset/issues/9077) introduced the concept of a Commands and Data Access Objects (DAOs) and suggested that CRUD operations would be represented via create, update, and delete Commands and DAOs. The challenge however is the create and update operations are rather similar and typically mostly differ on the absence or presence of a primary key respectively. #### Examples - The [`CreateChartCommand`](https://github.com/apache/superset/blob/1e37f0b41782d35d68f657bfa87aeb9055d1e6e7/superset/charts/commands/create.py) and [`UpdateChartCommand`](https://github.com/apache/superset/blob/1e37f0b41782d35d68f657bfa87aeb9055d1e6e7/superset/charts/commands/update.py) classes are (as expected) rather similar (per the FileMerge screenshot) yet—beyond the obvious differences between creating and updating—there exists some subtle (and unexplained) differences which has the potential to lead to various inconsistencies: 1. The `CreateChartCommand.validate()` method checks that the user is an owner of associated dashboards whereas the `UpdateChartCommand.validate()` method does not. 2. Both the `CreateChartCommand.validate()` and `UpdateChartCommand.validate()` methods fetch the dashboards associated with said chart, however only the later skips using the base filters. It’s unclear as to why this is. 3. The`UpdateChartCommand.validate()` [# Validate/Populate dashboards only if it's a list](https://github.com/apache/superset/blob/1e37f0b41782d35d68f657bfa87aeb9055d1e6e7/superset/charts/commands/update.py#L108) comment is misleading or outdated, i.e., there’s no check that the referenced dashboards is a list. No similar check exists for the `CreateChartCommand.validate()` method. <img width="1440" alt="Screenshot 2023-10-31 at 4 15 06 PM" src="https://github.com/apache/superset/assets/4567245/ebaafd31-5d93-4a32-a481-667ca7e30dcd";> ### Proposed Change - The existing create and update Commands and DAOs should be combined to include a single upsert operation to reduce complexity by ensuring that we adhere to the DRY principle. - The Commands and DAOs should support bulk upserting—akin to previous efforts where the `BaseDAO.bulk_delete` and `BaseDAO.delete` operations were condensed per https://github.com/apache/superset/pull/24466. ### New or Changed Public Interfaces None. ### New Dependencies None. ### Migration Plan and Compatibility None. ### Rejected Alternatives None. -- 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]
