Parse NOT NULL for MySQL generated columns

Fixed issue where NULL/NOT NULL would not be properly reflected from a
MySQL column that also specified the VIRTUAL or STORED directives.  Pull
request courtesy Georg Wicke-Arndt.

Fixes: #10850
Closes: #10851
Pull-request: https://github.com/sqlalchemy/sqlalchemy/pull/10851
Pull-request-sha: fb9a81020c393231ca90a1e88342b11cf64414a1

Change-Id: I9a80d0db722c15682e18f0390a7b58e5979e73a1

diff --git a/doc/build/changelog/unreleased_20/10850.rst b/doc/build/changelog/unreleased_20/10850.rst
new file mode 100644 (file)
index 0000000..6b6b323
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/10850.rst
@@ -0,0 +1,7 @@
+.. change::
+    :tags: bug, mysql
+    :tickets: 10850
+
+    Fixed issue where NULL/NOT NULL would not be properly reflected from a
+    MySQL column that also specified the VIRTUAL or STORED directives.  Pull
+    request courtesy Georg Wicke-Arndt.

diff --git a/lib/sqlalchemy/dialects/mysql/reflection.py b/lib/sqlalchemy/dialects/mysql/reflection.py
index 74c60f07b58c274e20a5e96ae784f6bcc5927715..c764e8ccc7fd36e98810d2cd571bc319596e521f 100644 (file)
--- a/lib/sqlalchemy/dialects/mysql/reflection.py
+++ b/lib/sqlalchemy/dialects/mysql/reflection.py
@@ -290,6 +290,9 @@ class MySQLTableDefinitionParser:
         # this can be "NULL" in the case of TIMESTAMP
         if spec.get("notnull", False) == "NOT NULL":
             col_kw["nullable"] = False
+        # For generated columns, the nullability is marked in a different place
+        if spec.get("notnull_generated", False) == "NOT NULL":
+            col_kw["nullable"] = False
 
         # AUTO_INCREMENT
         if spec.get("autoincr", False):
@@ -452,7 +455,9 @@ class MySQLTableDefinitionParser:
             r"(?: +ON UPDATE [\-\w\.\(\)]+)?)"
             r"))?"
             r"(?: +(?:GENERATED ALWAYS)? ?AS +(?P<generated>\("
-            r".*\))? ?(?P<persistence>VIRTUAL|STORED)?)?"
+            r".*\))? ?(?P<persistence>VIRTUAL|STORED)?"
+            r"(?: +(?P<notnull_generated>(?:NOT )?NULL))?"
+            r")?"
             r"(?: +(?P<autoincr>AUTO_INCREMENT))?"
             r"(?: +COMMENT +'(?P<comment>(?:''|[^'])*)')?"
             r"(?: +COLUMN_FORMAT +(?P<colfmt>\w+))?"

diff --git a/test/dialect/mysql/test_reflection.py b/test/dialect/mysql/test_reflection.py
index f3d1f34599b00c3a5ed68afaf41af824787f04f7..79e7198ef3d94e33a28fbe76e2818697bad686d0 100644 (file)
--- a/test/dialect/mysql/test_reflection.py
+++ b/test/dialect/mysql/test_reflection.py
@@ -764,103 +764,152 @@ class ReflectionTest(fixtures.TestBase, AssertsCompiledSQL):
         view_names = dialect.get_view_names(connection, "information_schema")
         self.assert_("TABLES" in view_names)
 
-    def test_nullable_reflection(self, metadata, connection):
-        """test reflection of NULL/NOT NULL, in particular with TIMESTAMP
-        defaults where MySQL is inconsistent in how it reports CREATE TABLE.
-
-        """
-        meta = metadata
-
-        # this is ideally one table, but older MySQL versions choke
-        # on the multiple TIMESTAMP columns
-        row = connection.exec_driver_sql(
-            "show variables like '%%explicit_defaults_for_timestamp%%'"
-        ).first()
-        explicit_defaults_for_timestamp = row[1].lower() in ("on", "1", "true")
-
-        reflected = []
-        for idx, cols in enumerate(
+    @testing.combinations(
+        (
             [
-                [
-                    "x INTEGER NULL",
-                    "y INTEGER NOT NULL",
-                    "z INTEGER",
-                    "q TIMESTAMP NULL",
-                ],
-                ["p TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP"],
-                ["r TIMESTAMP NOT NULL"],
-                ["s TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP"],
-                ["t TIMESTAMP"],
-                ["u TIMESTAMP DEFAULT CURRENT_TIMESTAMP"],
-            ]
-        ):
-            Table("nn_t%d" % idx, meta)  # to allow DROP
-
-            connection.exec_driver_sql(
-                """
-                    CREATE TABLE nn_t%d (
-                        %s
-                    )
-                """
-                % (idx, ", \n".join(cols))
-            )
-
-            reflected.extend(
-                {
-                    "name": d["name"],
-                    "nullable": d["nullable"],
-                    "default": d["default"],
-                }
-                for d in inspect(connection).get_columns("nn_t%d" % idx)
-            )
-
-        if connection.dialect._is_mariadb_102:
-            current_timestamp = "current_timestamp()"
-        else:
-            current_timestamp = "CURRENT_TIMESTAMP"
-
-        eq_(
-            reflected,
+                "x INTEGER NULL",
+                "y INTEGER NOT NULL",
+                "z INTEGER",
+                "q TIMESTAMP NULL",
+            ],
             [
                 {"name": "x", "nullable": True, "default": None},
                 {"name": "y", "nullable": False, "default": None},
                 {"name": "z", "nullable": True, "default": None},
                 {"name": "q", "nullable": True, "default": None},
-                {"name": "p", "nullable": True, "default": current_timestamp},
+            ],
+        ),
+        (
+            ["p TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP"],
+            [
+                {
+                    "name": "p",
+                    "nullable": True,
+                    "default": "CURRENT_TIMESTAMP",
+                }
+            ],
+        ),
+        (
+            ["r TIMESTAMP NOT NULL"],
+            [
                 {
                     "name": "r",
                     "nullable": False,
-                    "default": None
-                    if explicit_defaults_for_timestamp
-                    else (
-                        "%(current_timestamp)s "
-                        "ON UPDATE %(current_timestamp)s"
-                    )
-                    % {"current_timestamp": current_timestamp},
-                },
-                {"name": "s", "nullable": False, "default": current_timestamp},
+                    "default": None,
+                    "non_explicit_defaults_for_ts_default": (
+                        "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP"
+                    ),
+                }
+            ],
+        ),
+        (
+            ["s TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP"],
+            [
+                {
+                    "name": "s",
+                    "nullable": False,
+                    "default": "CURRENT_TIMESTAMP",
+                }
+            ],
+        ),
+        (
+            ["t TIMESTAMP"],
+            [
                 {
                     "name": "t",
-                    "nullable": True
-                    if explicit_defaults_for_timestamp
-                    else False,
-                    "default": None
-                    if explicit_defaults_for_timestamp
-                    else (
-                        "%(current_timestamp)s "
-                        "ON UPDATE %(current_timestamp)s"
-                    )
-                    % {"current_timestamp": current_timestamp},
-                },
+                    "nullable": True,
+                    "default": None,
+                    "non_explicit_defaults_for_ts_nullable": False,
+                    "non_explicit_defaults_for_ts_default": (
+                        "CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP"
+                    ),
+                }
+            ],
+        ),
+        (
+            ["u TIMESTAMP DEFAULT CURRENT_TIMESTAMP"],
+            [
                 {
                     "name": "u",
-                    "nullable": True
-                    if explicit_defaults_for_timestamp
-                    else False,
-                    "default": current_timestamp,
-                },
+                    "nullable": True,
+                    "non_explicit_defaults_for_ts_nullable": False,
+                    "default": "CURRENT_TIMESTAMP",
+                }
             ],
-        )
+        ),
+        (
+            ["v INTEGER GENERATED ALWAYS AS (4711) VIRTUAL NOT NULL"],
+            [
+                {
+                    "name": "v",
+                    "nullable": False,
+                    "default": None,
+                }
+            ],
+            testing.requires.mysql_notnull_generated_columns,
+        ),
+        argnames="ddl_columns,expected_reflected",
+    )
+    def test_nullable_reflection(
+        self, metadata, connection, ddl_columns, expected_reflected
+    ):
+        """test reflection of NULL/NOT NULL, in particular with TIMESTAMP
+        defaults where MySQL is inconsistent in how it reports CREATE TABLE.
+
+        """
+        row = connection.exec_driver_sql(
+            "show variables like '%%explicit_defaults_for_timestamp%%'"
+        ).first()
+        explicit_defaults_for_timestamp = row[1].lower() in ("on", "1", "true")
+
+        def get_expected_default(er):
+            if (
+                not explicit_defaults_for_timestamp
+                and "non_explicit_defaults_for_ts_default" in er
+            ):
+                default = er["non_explicit_defaults_for_ts_default"]
+            else:
+                default = er["default"]
+
+            if default is not None and connection.dialect._is_mariadb_102:
+                default = default.replace(
+                    "CURRENT_TIMESTAMP", "current_timestamp()"
+                )
+
+            return default
+
+        def get_expected_nullable(er):
+            if (
+                not explicit_defaults_for_timestamp
+                and "non_explicit_defaults_for_ts_nullable" in er
+            ):
+                return er["non_explicit_defaults_for_ts_nullable"]
+            else:
+                return er["nullable"]
+
+        expected_reflected = [
+            {
+                "name": er["name"],
+                "nullable": get_expected_nullable(er),
+                "default": get_expected_default(er),
+            }
+            for er in expected_reflected
+        ]
+
+        Table("nullable_refl", metadata)
+
+        cols_ddl = ", \n".join(ddl_columns)
+        connection.exec_driver_sql(f"CREATE TABLE nullable_refl ({cols_ddl})")
+
+        reflected = [
+            {
+                "name": d["name"],
+                "nullable": d["nullable"],
+                "default": d["default"],
+            }
+            for d in inspect(connection).get_columns("nullable_refl")
+        ]
+        eq_(reflected, expected_reflected)
 
     def test_reflection_with_unique_constraint(self, metadata, connection):
         insp = inspect(connection)

diff --git a/test/requirements.py b/test/requirements.py
index cb6ceeb2652bb72152221f6bffcd6c49469d2196..e5692a83f78646128a3f33c31ab41910711c4cb0 100644 (file)
--- a/test/requirements.py
+++ b/test/requirements.py
@@ -1694,6 +1694,10 @@ class DefaultRequirements(SuiteRequirements):
     def mysql_fsp(self):
         return only_if(["mysql >= 5.6.4", "mariadb"])
 
+    @property
+    def mysql_notnull_generated_columns(self):
+        return only_if(["mysql >= 5.7"])
+
     @property
     def mysql_fully_case_sensitive(self):
         return only_if(self._has_mysql_fully_case_sensitive)