You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/10/09 18:18:14 UTC
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #11179: chore: simplify alerting data model to leverage a single class
dpgaspar commented on a change in pull request #11179:
URL: https://github.com/apache/incubator-superset/pull/11179#discussion_r502591556
##########
File path: superset/migrations/versions/af30ca79208f_collapse_alerting_models_into_a_single_.py
##########
@@ -0,0 +1,299 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Collapse alerting models into a single one
+
+Revision ID: af30ca79208f
+Revises: b56500de1855
+Create Date: 2020-10-05 18:10:52.272047
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.dialects.sqlite.base import SQLiteDialect
+from sqlalchemy.ext.declarative import declarative_base, declared_attr
+from sqlalchemy.orm import backref, relationship, RelationshipProperty
+
+from superset import db
+from superset.utils.core import generic_find_fk_constraint_name
+
+revision = "af30ca79208f"
+down_revision = "b56500de1855"
+
+
+Base = declarative_base()
+
+
+class Alert(Base):
+ __tablename__ = "alerts"
+ id = sa.Column(sa.Integer, primary_key=True)
+ sql = sa.Column(sa.Text, nullable=False)
+ validator_type = sa.Column(sa.String(100), nullable=False)
+ validator_config = sa.Column(sa.Text)
+ database_id = sa.Column(sa.Integer)
+
+
+class SQLObserver(Base):
+ __tablename__ = "sql_observers"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sql = sa.Column(sa.Text, nullable=False)
+ database_id = sa.Column(sa.Integer)
+
+ @declared_attr
+ def alert_id(self) -> int:
+ return sa.Column(sa.Integer, sa.ForeignKey("alerts.id"), nullable=False)
+
+ @declared_attr
+ def alert(self) -> RelationshipProperty:
+ return relationship(
+ "Alert",
+ foreign_keys=[self.alert_id],
+ backref=backref("sql_observer", cascade="all, delete-orphan"),
+ )
+
+
+class Validator(Base):
+ __tablename__ = "alert_validators"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ validator_type = sa.Column(sa.String(100), nullable=False)
+ config = sa.Column(sa.Text)
+
+ @declared_attr
+ def alert_id(self) -> int:
+ return sa.Column(sa.Integer, sa.ForeignKey("alerts.id"), nullable=False)
+
+ @declared_attr
+ def alert(self) -> RelationshipProperty:
+ return relationship(
+ "Alert",
+ foreign_keys=[self.alert_id],
+ backref=backref("validators", cascade="all, delete-orphan"),
+ )
+
+
+def upgrade():
+ bind = op.get_bind()
+ insp = sa.engine.reflection.Inspector.from_engine(bind)
+
+ if isinstance(bind.dialect, SQLiteDialect):
+ op.add_column(
+ "alerts",
+ sa.Column("validator_config", sa.Text(), server_default="", nullable=True),
+ )
+ op.add_column(
+ "alerts",
+ sa.Column("database_id", sa.Integer(), server_default="0", nullable=False),
+ )
+ op.add_column(
+ "alerts", sa.Column("sql", sa.Text(), server_default="", nullable=False)
+ )
+ op.add_column(
+ "alerts",
+ sa.Column(
+ "validator_type",
+ sa.String(length=100),
+ server_default="",
+ nullable=False,
+ ),
+ )
+ else: # mysql does not support server_default for text fields
+ op.add_column(
+ "alerts",
+ sa.Column("validator_config", sa.Text(), default="", nullable=True),
+ )
+ op.add_column(
+ "alerts", sa.Column("database_id", sa.Integer(), default=0, nullable=False),
+ )
+ op.add_column("alerts", sa.Column("sql", sa.Text(), default="", nullable=False))
+ op.add_column(
+ "alerts",
+ sa.Column(
+ "validator_type", sa.String(length=100), default="", nullable=False
+ ),
+ )
+ # Migrate data
+ session = db.Session(bind=bind)
+ alerts = session.query(Alert).all()
+ for a in alerts:
+ if a.sql_observer:
+ a.sql = a.sql_observer[0].sql
+ a.database_id = a.sql_observer[0].database_id
+ if a.validators:
+ a.validator_type = a.validators[0].validator_type
+ a.validator_config = a.validators[0].config
+ session.commit()
+
+ # op.alter_column('alerts', 'database_id', server_default=None)
Review comment:
remove?
##########
File path: superset/views/alerts.py
##########
@@ -248,5 +170,12 @@ def pre_add(self, item: "AlertModelView") -> None:
if not croniter.is_valid(item.crontab):
raise SupersetException("Invalid crontab format")
+ item.validator_type = item.validator_type.lower()
+ check_validator(item.validator_type, item.validator_config)
+
+ def pre_update(self, item: "AlertModelView") -> None:
+ item.validator_type = item.validator_type.lower()
+ check_validator(item.validator_type, item.validator_config)
Review comment:
It brings a better used experience to use Flask-WTF validators instead: https://wtforms.readthedocs.io/en/2.3.x/validators/#module-wtforms.validators. This way the user get's alerted about a validation fail while still on the form and not after submitting
##########
File path: tests/alerts_tests.py
##########
@@ -165,41 +148,38 @@ def test_alert_observer(setup_database):
WHERE first = 1
""",
)
- observe(alert8.id, dbsession)
- assert alert8.sql_observer[0].observations[-1].value == 1.0
- assert alert8.sql_observer[0].observations[-1].error_msg is None
+ observe(alert8.id, db_session)
+ assert alert8.observations[-1].value == 1.0
+ assert alert8.observations[-1].error_msg is None
# Test jinja
- alert9 = create_alert(dbsession, "SELECT {{ 2 }}")
- observe(alert9.id, dbsession)
- assert alert9.sql_observer[0].observations[-1].value == 2.0
- assert alert9.sql_observer[0].observations[-1].error_msg is None
+ alert9 = create_alert(db_session, "SELECT {{ 2 }}")
+ observe(alert9.id, db_session)
+ assert alert9.observations[-1].value == 2.0
+ assert alert9.observations[-1].error_msg is None
@patch("superset.tasks.schedules.deliver_alert")
def test_evaluate_alert(mock_deliver_alert, setup_database):
- dbsession = setup_database
+ db_session = setup_database
Review comment:
sort of related, but looking at `setup_database` does it make sense at the cleanup to `DROP TABLE test_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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org