[schema] Updating the tables schema

UPDATING.md

@@ -36,6 +36,14 @@ using `ENABLE_PROXY_FIX = True`, review the newly-introducted variable,
 backend serialization. To disable set `RESULTS_BACKEND_USE_MSGPACK = False`
 in your configuration.
 
+* [5449](https://github.com/apache/incubator-superset/pull/5449): a change which
+adds a uniqueness criterion to the tables table. Depending on the integrity of
+the data, manual intervention may be required.
+
+* [8332](https://github.com/apache/incubator-superset/pull/8332): makes
+`tables.table_name`, `dbs.database_name`, and `clusters.cluster_name` non-nullable.
+Depending on the integrity of the data, manual intervention may be required.
+
 ## 0.34.0
 
 * [7848](https://github.com/apache/incubator-superset/pull/7848): If you are

superset/connectors/sqla/models.py

@@ -41,9 +41,11 @@
     Table,
     Text,
 )
-from sqlalchemy.exc import CompileError
+from sqlalchemy.engine.base import Connection
+from sqlalchemy.exc import CompileError, SQLAlchemyError
 from sqlalchemy.orm import backref, Query, relationship, RelationshipProperty, Session
 from sqlalchemy.orm.exc import NoResultFound
+from sqlalchemy.orm.mapper import Mapper
 from sqlalchemy.schema import UniqueConstraint
 from sqlalchemy.sql import column, ColumnElement, literal_column, table, text
 from sqlalchemy.sql.expression import Label, Select, TextAsFrom
@@ -304,7 +306,11 @@ class SqlaTable(Model, BaseDatasource):
     owner_class = security_manager.user_model
 
     __tablename__ = "tables"
-    __table_args__ = (UniqueConstraint("database_id", "table_name"),)
+
+    # Note this unqiuness constraint is not part of the physicalschema, i.e., it doesn't
+    # exist in the migrations, but is required by `import_from_dict` to ensure the
+    # correct filters are applied in order to identify uniqueness.
+    __table_args__ = (UniqueConstraint("database_id", "schema", "table_name"),)
 
     table_name = Column(String(250))
     main_dttm_col = Column(String(250))
@@ -1112,6 +1118,82 @@ def get_extra_cache_keys(self, query_obj: Dict) -> List[Any]:
             return extra_cache_keys
         return []
 
+    @staticmethod
+    def before_insert(
+        mapper: Mapper, connection: Connection, target: "SqlaTable"
+    ) -> None:
+        """
+        Check whether before insert if the target table already exists.
+
+        :param mapper: The table mappper
+        :param connection: The DB-API connection
+        :param target: The mapped instance being persisted
+        :raises Exception: If the target table is not unique
+        """
+
+        from superset.views.base import get_datasource_exist_error_msg
+
+        if SqlaTable.exists(target):
+            raise SQLAlchemyError(get_datasource_exist_error_msg(target.full_name))
+
+    @staticmethod
+    def before_update(
+        mapper: Mapper, connection: Connection, target: "SqlaTable"
+    ) -> None:
+        """
+        Check whether before update if the target table already exists.
+
+        Note this listener is called when any fields are being updated and thus it is
+        necessary to first check whether the reference table is being updated.
+
+        :param mapper: The table mapper
+        :param connection: The DB-API connection
+        :param target: The mapped instance being persisted
+        :raises Exception: If the target table is not unique
+        """
+
+        from superset.views.base import get_datasource_exist_error_msg
+
+        # Check whether the relevant attributes have changed.
+        state = db.inspect(target)  # pylint: disable=no-member
+
+        for attr in ["database_id", "schema", "table_name"]:
+            history = state.get_history(attr, True)
+
+            if history.has_changes():
+                break
+        else:
+            return None
+
+        if SqlaTable.exists(target):
+            raise SQLAlchemyError(get_datasource_exist_error_msg(target.full_name))
+
+    @staticmethod
+    def exists(record: "SqlaTable") -> bool:
+        """
+        Return True if the table exists, False otherwise.
+
+        A table is deemed to already exist based on the uniqueness of the database,
+        schema, and name.
+
+        :param record: The table record
+        :returns: Whether the record exists
+        """
+
+        count = (
+            db.session.query(SqlaTable)
+            .filter_by(
+                database_id=record.database_id,
+                schema=record.schema,
+                table_name=record.table_name,
+            )
+            .count()
+        )
+
+        return count != 0
+
 
 sa.event.listen(SqlaTable, "after_insert", security_manager.set_perm)
 sa.event.listen(SqlaTable, "after_update", security_manager.set_perm)
+sa.event.listen(SqlaTable, "before_insert", SqlaTable.before_insert)
+sa.event.listen(SqlaTable, "before_update", SqlaTable.before_update)

superset/connectors/sqla/views.py

@@ -27,14 +27,14 @@
 from flask_babel import gettext as __
 from flask_babel import lazy_gettext as _
 from wtforms.ext.sqlalchemy.fields import QuerySelectField
+from wtforms.validators import Regexp
 
 from superset import appbuilder, db, security_manager
 from superset.connectors.base.views import DatasourceModelView
 from superset.utils import core as utils
 from superset.views.base import (
     DatasourceFilter,
     DeleteMixin,
-    get_datasource_exist_error_msg,
     ListWidgetWithCheckboxes,
     SupersetModelView,
     YamlExportMixin,
@@ -303,6 +303,11 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin):  # noqa
         "template_params": _("Template parameters"),
         "modified": _("Modified"),
     }
+    validators_columns = {
+        "table_name": [
+            Regexp(r"^[^\.]+$", message=_("Table name cannot contain a schema"))
+        ]
+    }
 
     edit_form_extra_fields = {
         "database": QuerySelectField(
@@ -313,14 +318,10 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin):  # noqa
     }
 
     def pre_add(self, table):
-        with db.session.no_autoflush:
-            table_query = db.session.query(models.SqlaTable).filter(
-                models.SqlaTable.table_name == table.table_name,
-                models.SqlaTable.schema == table.schema,
-                models.SqlaTable.database_id == table.database.id,
-            )
-            if db.session.query(table_query.exists()).scalar():
-                raise Exception(get_datasource_exist_error_msg(table.full_name))
+
+        # Although the listener exists this is necessary to ensure that FAB flashes the
+        # specific message as opposed to "General Error <class.Exception>".
+        models.SqlaTable.before_insert(None, None, table)
 
         # Fail before adding if the table can't be found
         try:

superset/migrations/versions/1d73b15530e7_update_tables.py

@@ -0,0 +1,57 @@
+# 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.
+"""update tables
+
+Revision ID: 1d73b15530e7
+Revises: b6fa807eac07
+Create Date: 2018-07-20 11:36:04.535859
+
+"""
+from alembic import op
+from sqlalchemy import engine
+from sqlalchemy.exc import OperationalError, ProgrammingError
+
+from superset.utils.core import generic_find_uq_constraint_name
+
+# revision identifiers, used by Alembic.
+revision = "1d73b15530e7"
+down_revision = "b6fa807eac07"
+
+conv = {"uq": "uq_%(table_name)s_%(column_0_name)s"}
+
+
+def upgrade():
+    bind = op.get_bind()
+    insp = engine.reflection.Inspector.from_engine(bind)
+
+    # Drop the uniqueness constraint if it exists.
+    try:
+        with op.batch_alter_table("tables", naming_convention=conv) as batch_op:
+            batch_op.drop_constraint(
+                generic_find_uq_constraint_name("tables", {"table_name"}, insp)
+                or "uq_tables_table_name",
+                type_="unique",
+            )
+    except (ProgrammingError, OperationalError):
+        pass
+
+
+def downgrade():
+
+    # Re-add the uniqueness constraint.
+    with op.batch_alter_table("tables", naming_convention=conv) as batch_op:
+        batch_op.create_unique_constraint("uq_tables_table_name", ["table_name"])