chore(dao): Replace save/overwrite with create/update respectively #24467

john-bodley · 2023-06-21T03:19:00Z

SUMMARY

Per SIP-92 Proposal for restructuring the Python code base #24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.

This PR (one of many smaller bitesized PRs) replaces the save and overwrite methods with create and update methods respectively which removes a bunch of copypasta.

Note this PR required a fair amount of yak shaving as the create and update methods needed to be updated to support a combination of a instance and/or the attributes representing the instance.

Future fast follows intend to:

Remove the ability to defining properties/attributes when creating or updating.
Consolidating the create and update into an upsert method.
Provide a mechanism for bulk upsert.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

CI.

ADDITIONAL INFORMATION

Has associated issue: [SIP-92] Proposal for restructuring the Python code base #20630
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-06-21T22:02:36Z

Codecov Report

Merging #24467 (333b126) into master (0328dd2) will decrease coverage by 0.01%.
The diff coverage is 94.73%.

❗ Current head 333b126 differs from pull request most recent head d84ea7e. Consider uploading reports for the commit d84ea7e to get more accurate results

@@            Coverage Diff             @@
##           master   #24467      +/-   ##
==========================================
- Coverage   68.97%   68.96%   -0.01%     
==========================================
  Files        1901     1901              
  Lines       74008    73964      -44     
  Branches     8183     8183              
==========================================
- Hits        51047    51010      -37     
+ Misses      20840    20833       -7     
  Partials     2121     2121

Flag	Coverage Δ
hive	`54.17% <28.94%> (+0.05%)`	⬆️
mysql	`79.35% <94.73%> (-0.01%)`	⬇️
postgres	`79.44% <94.73%> (-0.01%)`	⬇️
presto	`54.06% <28.94%> (+0.05%)`	⬆️
python	`83.45% <94.73%> (-0.01%)`	⬇️
sqlite	`78.02% <78.94%> (-0.02%)`	⬇️
unit	`54.88% <39.47%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/daos/chart.py	`91.11% <ø> (-1.62%)`	⬇️
superset/datasets/commands/update.py	`94.11% <ø> (ø)`
superset/daos/dataset.py	`91.24% <75.00%> (+0.06%)`	⬆️
superset/daos/report.py	`86.17% <92.85%> (+2.24%)`	⬆️
superset/daos/base.py	`94.44% <93.75%> (+0.76%)`	⬆️
...t/annotation_layers/annotations/commands/create.py	`84.61% <100.00%> (-0.39%)`	⬇️
superset/annotation_layers/commands/create.py	`88.46% <100.00%> (-0.43%)`	⬇️
superset/charts/commands/create.py	`92.15% <100.00%> (-0.16%)`	⬇️
superset/daos/dashboard.py	`97.07% <100.00%> (+0.39%)`	⬆️
superset/daos/database.py	`100.00% <100.00%> (ø)`
... and 11 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-06-22T06:38:32Z

superset/daos/base.py

-        :raises: DAOCreateFailedError
+    def create(
+        cls,
+        item: T | None = None,


To accommodate the save functionality I extended the create method to accept a predefined object.

Would be possible to set item as T | dict[str, Any] | None and remove the attributes parameter?

@michael-s-molina that's an interesting idea, though I wonder if we're overloading item here. I was likely just trying to replicate the same behavior (familiarity) as the update method which needs to have either/or both the item and attributes—well technically you're just updating an existing item, but it seems cleaner (from a DRY perspective) to have the base update method perform said logic.

Technically I can call create with both item and attributes set as None. It feels really weird for me.

@michael-s-molina sorry for the delay in getting back to you on this. You mention you could do something of the form,

chart = ChartDAO.create()

which would try to create a new chart and persist it to the database. This operation would succeed if the model attributes can be nullable. I realize this isn't a typical workflow, but I'm not entirely sure it's wrong per se.

BTW I totally agree there's room for improvement here—I was even toiling with the idea that maybe create and update should be handled by a single upsert method—however I do sense this PR helps ensure that the code is more consistent, i.e., the save and override methods have been removed and both the create and update methods have the same function signature.

john-bodley · 2023-06-22T06:39:05Z

superset/daos/base.py

        """
-        Generic update a model
-        :raises: DAOCreateFailedError


Previously this comment was wrong.

john-bodley · 2023-06-22T06:42:59Z

superset/daos/base.py

-    def update(cls, model: T, properties: dict[str, Any], commit: bool = True) -> T:
+    def update(
+        cls,
+        item: T | None = None,


The item and/or attributes are now optional which provides more flexibility.

john-bodley · 2023-06-22T06:46:09Z

superset/daos/base.py

@@ -38,12 +39,12 @@ class BaseDAO(Generic[T]):
    Base DAO, implement base CRUD sqlalchemy operations
    """

-    model_cls: Optional[type[Model]] = None
+    model_cls: type[Model] | None = None


This was automatically updated via pyupgrade.

john-bodley · 2023-07-07T16:00:45Z

superset/daos/base.py

+    def create(
+        cls,
+        item: T | None = None,
+        attributes: dict[str, Any] | None = None,


I used the term "attributes" rather than "properties" as this is the lingo that SQLAlchemy uses—combined with the fact you use the setattr method for setting them.

john-bodley · 2023-07-21T20:04:09Z

@michael-s-molina I've hopefully addressed/answered your questions? Would it be possible to (re)review the change? Once merged I was hoping to make some inroads in terms of how we commit/rollback changes as well as grouping all the commands together. Ideally these tasks will be easier to manage after once this PR is merged otherwise the merge conflict could be rather convoluted.

michael-s-molina

LGTM

…pache#24467)

pull-request-size bot added the size/M label Jun 21, 2023

john-bodley changed the title ~~chore(dao): Replace save/override with create/update respectively~~ chore(dao): Replace save/overwrite with create/update respectively Jun 21, 2023

john-bodley requested review from villebro, dpgaspar and michael-s-molina June 21, 2023 03:20

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch from 8d1fa15 to b399955 Compare June 21, 2023 21:03

pull-request-size bot added size/L and removed size/M labels Jun 21, 2023

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch from b399955 to 6d67571 Compare June 21, 2023 21:40

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch 2 times, most recently from c4b0f1b to 84394ec Compare June 21, 2023 22:33

john-bodley commented Jun 22, 2023

View reviewed changes

superset/daos/base.py

"""

Generic update a model

:raises: DAOCreateFailedError

Copy link

Member Author

john-bodley Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this comment was wrong.

john-bodley commented Jun 22, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch 2 times, most recently from ee8d86d to 0b8c785 Compare June 23, 2023 05:00

john-bodley requested a review from betodealmeida June 27, 2023 03:56

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch 3 times, most recently from 66cdeed to dd656b1 Compare July 7, 2023 04:45

john-bodley commented Jul 7, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch 2 times, most recently from 3b06454 to 610edf5 Compare August 10, 2023 23:16

chore(dao): Replace save/override with create/update respectively

6110e6b

john-bodley force-pushed the john-bodley--dao-remove-save-overwrite-etc branch from 610edf5 to 6110e6b Compare August 11, 2023 01:01

michael-s-molina approved these changes Aug 11, 2023

View reviewed changes

john-bodley merged commit ed0d288 into apache:master Aug 11, 2023
31 checks passed

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore(dao): Replace save/overwrite with create/update respectively (a…

f520954

…pache#24467)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(dao): Replace save/overwrite with create/update respectively #24467

chore(dao): Replace save/overwrite with create/update respectively #24467

john-bodley commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

john-bodley Jun 22, 2023 •

edited

Loading

michael-s-molina Jul 11, 2023

john-bodley Jul 15, 2023 •

edited

Loading

michael-s-molina Jul 24, 2023

john-bodley Aug 10, 2023 •

edited

Loading

john-bodley Jun 22, 2023

john-bodley Jun 22, 2023 •

edited

Loading

john-bodley Jun 22, 2023

john-bodley Jul 7, 2023

john-bodley commented Jul 21, 2023

michael-s-molina left a comment

chore(dao): Replace save/overwrite with create/update respectively #24467

chore(dao): Replace save/overwrite with create/update respectively #24467

Conversation

john-bodley commented Jun 21, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

john-bodley Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jul 11, 2023

Choose a reason for hiding this comment

john-bodley Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Jul 24, 2023

Choose a reason for hiding this comment

john-bodley Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

john-bodley Jun 22, 2023

Choose a reason for hiding this comment

john-bodley Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

john-bodley Jun 22, 2023

Choose a reason for hiding this comment

john-bodley Jul 7, 2023

Choose a reason for hiding this comment

john-bodley commented Jul 21, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

john-bodley commented Jun 21, 2023 •

edited

Loading

codecov bot commented Jun 21, 2023 •

edited

Loading

john-bodley Jun 22, 2023 •

edited

Loading

john-bodley Jul 15, 2023 •

edited

Loading

john-bodley Aug 10, 2023 •

edited

Loading

john-bodley Jun 22, 2023 •

edited

Loading