-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(dao): Condense delete/bulk-delete operations #24466
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,8 +17,6 @@ | |
import logging | ||
from typing import Optional | ||
|
||
from flask_appbuilder.models.sqla import Model | ||
|
||
from superset.annotation_layers.annotations.commands.exceptions import ( | ||
AnnotationDeleteFailedError, | ||
AnnotationNotFoundError, | ||
|
@@ -36,16 +34,15 @@ def __init__(self, model_id: int): | |
self._model_id = model_id | ||
self._model: Optional[Annotation] = None | ||
|
||
def run(self) -> Model: | ||
def run(self) -> None: | ||
self.validate() | ||
assert self._model | ||
|
||
try: | ||
annotation = AnnotationDAO.delete(self._model) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to return the object we're deleting. |
||
AnnotationDAO.delete(self._model) | ||
except DAODeleteFailedError as ex: | ||
logger.exception(ex.exception) | ||
raise AnnotationDeleteFailedError() from ex | ||
return annotation | ||
|
||
def validate(self) -> None: | ||
# Validate/populate model exists | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,30 +17,14 @@ | |
import logging | ||
from typing import Optional, Union | ||
|
||
from sqlalchemy.exc import SQLAlchemyError | ||
|
||
from superset.daos.base import BaseDAO | ||
from superset.daos.exceptions import DAODeleteFailedError | ||
from superset.extensions import db | ||
from superset.models.annotations import Annotation, AnnotationLayer | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class AnnotationDAO(BaseDAO[Annotation]): | ||
@staticmethod | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copypasta except the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comments are very helpful while reviewing. Thank you. |
||
def bulk_delete(models: Optional[list[Annotation]], commit: bool = True) -> None: | ||
item_ids = [model.id for model in models] if models else [] | ||
try: | ||
db.session.query(Annotation).filter(Annotation.id.in_(item_ids)).delete( | ||
synchronize_session="fetch" | ||
) | ||
if commit: | ||
db.session.commit() | ||
except SQLAlchemyError as ex: | ||
db.session.rollback() | ||
raise DAODeleteFailedError() from ex | ||
|
||
@staticmethod | ||
def validate_update_uniqueness( | ||
layer_id: int, short_descr: str, annotation_id: Optional[int] = None | ||
|
@@ -63,21 +47,6 @@ def validate_update_uniqueness( | |
|
||
|
||
class AnnotationLayerDAO(BaseDAO[AnnotationLayer]): | ||
@staticmethod | ||
def bulk_delete( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copypasta except the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice cleanup! |
||
models: Optional[list[AnnotationLayer]], commit: bool = True | ||
) -> None: | ||
item_ids = [model.id for model in models] if models else [] | ||
try: | ||
db.session.query(AnnotationLayer).filter( | ||
AnnotationLayer.id.in_(item_ids) | ||
).delete(synchronize_session="fetch") | ||
if commit: | ||
db.session.commit() | ||
except SQLAlchemyError as ex: | ||
db.session.rollback() | ||
raise DAODeleteFailedError() from ex | ||
|
||
@staticmethod | ||
def has_annotations(model_id: Union[int, list[int]]) -> bool: | ||
if isinstance(model_id, list): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,30 +14,9 @@ | |
# KIND, either express or implied. See the License for the | ||
# specific language governing permissions and limitations | ||
# under the License. | ||
import logging | ||
from typing import Optional | ||
|
||
from sqlalchemy.exc import SQLAlchemyError | ||
|
||
from superset.daos.base import BaseDAO | ||
from superset.daos.exceptions import DAODeleteFailedError | ||
from superset.extensions import db | ||
from superset.models.core import CssTemplate | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
||
class CssTemplateDAO(BaseDAO[CssTemplate]): | ||
@staticmethod | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copypasta except the |
||
def bulk_delete(models: Optional[list[CssTemplate]], commit: bool = True) -> None: | ||
item_ids = [model.id for model in models] if models else [] | ||
try: | ||
db.session.query(CssTemplate).filter(CssTemplate.id.in_(item_ids)).delete( | ||
synchronize_session="fetch" | ||
) | ||
if commit: | ||
db.session.commit() | ||
except SQLAlchemyError as ex: | ||
if commit: | ||
db.session.rollback() | ||
raise DAODeleteFailedError() from ex | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is temporary. It should be cleaned up when we refactor the commands and enforce that
validate
doesn't augment any properties.