Misc improvements from mochaa/main

CHANGELOG.md

@@ -7,6 +7,9 @@ project adheres to [Semantic Versioning][semver].
 
 ## Unreleased
 
+- changed: Raise on incorrect use of drop mode.
+- added: Support rendering `:modifiers` in `CREATE TABLE` commands.
+- added: Support check constraints via [`Ecto.Migration.constraint/3`](https://hexdocs.pm/ecto_sql/Ecto.Migration.html#constraint/3).
 - changed: Reimplement querying prefix names.
 - added: Raise on cross-database foreign keys.
 - changed: Made `quote_name/1` private.

integration_test/prefix_test.exs

@@ -1,7 +1,7 @@
 defmodule Ecto.Integration.PrefixTest do
   use Ecto.Integration.Case, async: true
 
-  import Ecto.Query, only: [from: 2]
+  import Ecto.Query, only: [from: 1, from: 2]
 
   alias Ecto.Integration.Post
   alias Ecto.Integration.TestRepo
@@ -17,5 +17,11 @@ defmodule Ecto.Integration.PrefixTest do
       |> TestRepo.all()
 
     assert [%Post{id: 1}] = results
+
+    results =
+      from(Post)
+      |> TestRepo.all()
+
+    assert [] = results
   end
 end

lib/ecto/adapters/sqlite3.ex

@@ -160,22 +160,21 @@ defmodule Ecto.Adapters.SQLite3 do
   ### Check constraints
 
   SQLite3 supports specifying check constraints on the table or on the column definition.
-  We currently only support adding a check constraint via a column definition, since the
-  table definition approach only works at table-creation time and cannot be added at
-  table-alter time. You can see more information in the SQLite3
-  [CREATE TABLE documentation](https://sqlite.org/lang_createtable.html).
+  Table-alter time check constraints are supported since SQLite 3.53.0.
 
-  Because of this, you cannot add a constraint via the normal `Ecto.Migration.constraint/3`
-  method, as that operates via `ALTER TABLE ADD CONSTRAINT`, and this type of `ALTER TABLE`
-  operation SQLite3 does not support. You can however get the full functionality by
-  adding a constraint at the column level, specifying the name and expression. Per the
-  SQLite3 documentation, there is no _functional_ difference between a column or table
-  constraint.
+  Adding a constraint via `Ecto.Migration.constraint/3` is recommended for new projects.
+  Note that SQLite's `ADD CONSTRAINTS` verifies existing rows with `WHERE (check) IS NOT TRUE`,
+  so make sure your check is NULL-safe.
 
-  Thus, adding a check constraint for a new column is as simple as:
+  As an adapter-specific extension, the old way of adding column-level constraints is still
+  available:
 
       add :email, :string, check: %{name: "test_constraint", expr: "email != 'test@example.com'"}
 
+  Note that column-level constraints are slightly different than table-level constraints,
+  since `DROP COLUMN` can drop column-level constraints but not table-level constraints.
+  Drop the constraint first before dropping the column in those cases.
+
   ### Handling foreign key constraints in changesets
 
   Unfortunately, unlike other databases, SQLite3 does not provide the precise name of

lib/ecto/adapters/sqlite3/connection.ex

@@ -390,38 +390,30 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   ## DDL
   ##
 
+  @creates [:create, :create_if_not_exists]
+  @drops [:drop, :drop_if_exists]
+
   @impl true
   def execute_ddl({_command, %Table{options: options}, _}) when is_list(options) do
     raise ArgumentError, "SQLite3 adapter does not support keyword lists in :options"
   end
 
-  def execute_ddl({:create, %Table{} = table, columns}) do
+  def execute_ddl({command, %Table{} = table, columns}) when command in @creates do
     {table, composite_pk_def} = composite_pk_definition(table, columns)
     composite_fk_defs = composite_fk_definitions(table, columns)
 
-    [
-      [
-        "CREATE TABLE ",
-        quote_name(table.prefix, table.name),
-        ?\s,
-        ?(,
-        column_definitions(table, columns),
-        composite_pk_def,
-        composite_fk_defs,
-        ?),
-        options_expr(table.options)
-      ]
-    ]
-  end
+    table_name = quote_name(table.prefix, table.name)
 
-  def execute_ddl({:create_if_not_exists, %Table{} = table, columns}) do
-    {table, composite_pk_def} = composite_pk_definition(table, columns)
-    composite_fk_defs = composite_fk_definitions(table, columns)
+    modifiers = modifiers_expr(table.modifiers)
+    assert_unqual_name(modifiers, table.prefix)
 
     [
       [
-        "CREATE TABLE IF NOT EXISTS ",
-        quote_name(table.prefix, table.name),
+        "CREATE ",
+        modifiers,
+        "TABLE ",
+        if_do(command == :create_if_not_exists, "IF NOT EXISTS "),
+        table_name,
         ?\s,
         ?(,
         column_definitions(table, columns),
@@ -433,32 +425,17 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     ]
   end
 
-  def execute_ddl({:drop, %Table{} = table}) do
+  def execute_ddl({command, %Table{} = table, mode}) when command in @drops do
     [
       [
         "DROP TABLE ",
-        quote_name(table.prefix, table.name)
-      ]
-    ]
-  end
-
-  def execute_ddl({:drop, %Table{} = table, _mode}) do
-    execute_ddl({:drop, table})
-  end
-
-  def execute_ddl({:drop_if_exists, %Table{} = table}) do
-    [
-      [
-        "DROP TABLE IF EXISTS ",
-        quote_name(table.prefix, table.name)
+        if_do(command == :drop_if_exists, "IF EXISTS "),
+        quote_name(table.prefix, table.name),
+        drop_mode(mode)
       ]
     ]
   end
 
-  def execute_ddl({:drop_if_exists, %Table{} = table, _mode}) do
-    execute_ddl({:drop_if_exists, table})
-  end
-
   def execute_ddl({:alter, %Table{} = table, changes}) do
     Enum.map(changes, fn change ->
       [
@@ -490,14 +467,15 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     raise ArgumentError, "`nulls_distinct` is not supported with SQLite3"
   end
 
-  def execute_ddl({:create, %Index{} = index}) do
+  def execute_ddl({command, %Index{} = index}) when command in @creates do
     fields = Enum.map_intersperse(index.columns, ", ", &index_expr/1)
 
     [
       [
         "CREATE ",
         if_do(index.unique, "UNIQUE "),
         "INDEX ",
+        if_do(command == :create_if_not_exists, "IF NOT EXISTS "),
         quote_name(index.prefix, index.name),
         " ON ",
         quote_name(index.table),
@@ -509,51 +487,17 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     ]
   end
 
-  def execute_ddl({:create_if_not_exists, %Index{} = index}) do
-    fields = Enum.map_intersperse(index.columns, ", ", &index_expr/1)
-
-    [
-      [
-        "CREATE ",
-        if_do(index.unique, "UNIQUE "),
-        "INDEX IF NOT EXISTS ",
-        quote_name(index.prefix, index.name),
-        " ON ",
-        quote_name(index.table),
-        " (",
-        fields,
-        ?),
-        if_do(index.where, [" WHERE ", to_string(index.where)])
-      ]
-    ]
-  end
-
-  def execute_ddl({:drop, %Index{} = index}) do
+  def execute_ddl({command, %Index{} = index, mode}) when command in @drops do
     [
       [
         "DROP INDEX ",
-        quote_name(index.prefix, index.name)
-      ]
-    ]
-  end
-
-  def execute_ddl({:drop, %Index{} = index, _mode}) do
-    execute_ddl({:drop, index})
-  end
-
-  def execute_ddl({:drop_if_exists, %Index{} = index}) do
-    [
-      [
-        "DROP INDEX IF EXISTS ",
-        quote_name(index.prefix, index.name)
+        if_do(command == :drop_if_exists, "IF EXISTS "),
+        quote_name(index.prefix, index.name),
+        drop_mode(mode)
       ]
     ]
   end
 
-  def execute_ddl({:drop_if_exists, %Index{} = index, _mode}) do
-    execute_ddl({:drop_if_exists, index})
-  end
-
   def execute_ddl({:rename, %Table{} = current_table, %Table{} = new_table}) do
     [
       [
@@ -584,21 +528,41 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     raise ArgumentError, "SQLite3 adapter does not support keyword lists in execute"
   end
 
-  def execute_ddl({:create, %Constraint{}}) do
-    raise ArgumentError, "SQLite3 does not support ALTER TABLE ADD CONSTRAINT."
+  def execute_ddl({:create, %Constraint{check: check} = constraint})
+      when is_binary(check) do
+    [
+      [
+        "ALTER TABLE ",
+        quote_name(constraint.prefix, constraint.table),
+        " ADD",
+        new_constraint_expr(constraint)
+      ]
+    ]
+  end
+
+  def execute_ddl({:create, %Constraint{exclude: exclude}}) when is_binary(exclude) do
+    raise ArgumentError, "SQLite3 does not support exclusion constraints"
   end
 
-  def execute_ddl({:drop, %Constraint{}, _mode}) do
-    raise ArgumentError, "SQLite3 does not support ALTER TABLE DROP CONSTRAINT."
+  def execute_ddl({:drop, %Constraint{} = constraint, mode}) do
+    [
+      [
+        "ALTER TABLE ",
+        quote_name(constraint.prefix, constraint.table),
+        " DROP CONSTRAINT ",
+        quote_name(constraint.name),
+        drop_mode(mode)
+      ]
+    ]
   end
 
   def execute_ddl({:drop_if_exists, %Constraint{}, _mode}) do
-    raise ArgumentError, "SQLite3 does not support ALTER TABLE DROP CONSTRAINT."
+    raise ArgumentError, "SQLite3 does not support `drop_if_exists` for constraints"
   end
 
   def execute_ddl({:rename, %Index{} = index, new_index}) do
     [
-      execute_ddl({:drop, index}),
+      execute_ddl({:drop, index, :restrict}),
       execute_ddl({:create, %Index{index | name: new_index}})
     ]
   end
@@ -611,6 +575,14 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     {"SELECT name FROM sqlite_master WHERE type='table' AND name=? LIMIT 1", [table]}
   end
 
+  defp drop_mode(:cascade) do
+    raise ArgumentError, "SQLite3 does not support `CASCADE` in this command"
+  end
+
+  # SQLite does not support RESTRICT either, but since `Ecto.Migration.drop/2`
+  # defaults to `:restrict` we have to silently ignore it
+  defp drop_mode(:restrict), do: []
+
   ##
   ## Query generation
   ##
@@ -1671,6 +1643,13 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   defp null_expr(true), do: " NULL"
   defp null_expr(_), do: []
 
+  defp new_constraint_expr(%Constraint{validate: validate}) when validate == false do
+    raise ArgumentError, "SQLite3 does not support `validate: false` in constraints"
+  end
+
+  defp new_constraint_expr(%Constraint{check: check} = constraint)
+       when is_binary(check), do: check_expr(%{name: constraint.name, expr: check})
+
   defp default_expr({:ok, nil}) do
     " DEFAULT NULL"
   end
@@ -1705,6 +1684,22 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   defp pk_expr(true, _), do: " PRIMARY KEY"
   defp pk_expr(_, _), do: []
 
+  defp modifiers_expr(nil), do: []
+
+  defp modifiers_expr(modifiers) when is_binary(modifiers) do
+    if temporary?(modifiers) do
+      [modifiers, ?\s]
+    else
+      raise ArgumentError,
+            ~s|SQLite3 adapter expects :modifiers to be one of [nil, "TEMP", "TEMPORARY"], got #{inspect(modifiers)}|
+    end
+  end
+
+  defp modifiers_expr(other) do
+    raise ArgumentError,
+          "SQLite3 adapter expects :modifiers to be a string or nil, got #{inspect(other)}"
+  end
+
   defp options_expr(nil), do: []
 
   defp options_expr(options) when is_list(options) do
@@ -1717,7 +1712,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   defp reference_expr(%Reference{with: [_]}, _table, _name), do: []
 
   defp reference_expr(%Reference{} = ref, table, name) do
-    assert_same_database(table.prefix, ref.prefix)
+    assert_same_database(table, ref)
 
     [
       " CONSTRAINT ",
@@ -1828,7 +1823,7 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   end
 
   defp composite_fk_definition(table, {_op, name, ref, _opts}) do
-    assert_same_database(table.prefix, ref.prefix)
+    assert_same_database(table, ref)
 
     {current_columns, reference_columns} = Enum.unzip([{name, ref.column} | ref.with])
 
@@ -1919,8 +1914,6 @@ defmodule Ecto.Adapters.SQLite3.Connection do
   # We know this holds since exqlite does not export sqlite3_db_config from Sqlite3NIF,
   # thus nobody can call sqlite3_db_config(db, SQLITE_DBCONFIG_MAINDBNAME, ...)
   defp normalize_database_name(nil) do
-    # TODO: handle modifier-selected temp database somehow?
-    # src/parse.y: `temp(A) ::= TEMP.  {A = pParse->db->init.busy==0;}`
     "main"
   end
 
@@ -1932,10 +1925,34 @@ defmodule Ecto.Adapters.SQLite3.Connection do
     String.downcase(name, :ascii)
   end
 
-  defp assert_same_database(table_prefix, ref_prefix) do
-    if normalize_database_name(ref_prefix || table_prefix) !=
-         normalize_database_name(table_prefix) do
+  defp temporary?(modifiers) when is_binary(modifiers),
+    do:
+      (modifiers
+       |> String.trim()
+       |> String.upcase(:ascii)) in ["TEMP", "TEMPORARY"]
+
+  defp temporary?(_), do: false
+
+  defp table_database(%Table{} = table) do
+    if temporary?(table.modifiers) do
+      "temp"
+    else
+      normalize_database_name(table.prefix)
+    end
+  end
+
+  defp assert_same_database(%Table{} = table, %Reference{} = ref) do
+    table_db = table_database(table)
+    ref_db = if ref.prefix, do: normalize_database_name(ref.prefix), else: table_db
+
+    if ref_db != table_db do
       raise ArgumentError, "SQLite3 does not support cross-database foreign keys"
     end
   end
+
+  defp assert_unqual_name(modifiers, prefix) do
+    if modifiers != [] and prefix != nil do
+      raise ArgumentError, "SQLite3 does not support a prefix on a temporary table"
+    end
+  end
 end

test/ecto/adapters/sqlite3/connection/cte_test.exs

@@ -176,7 +176,7 @@ defmodule Ecto.Adapters.SQLite3.Connection.CteTest do
       |> plan()
 
     assert_raise Ecto.QueryError,
-                 ~r/join `:left_lateral` not supported by SQLite3/,
+                 ~r/^join `:left_lateral` not supported by SQLite3/,
                  fn ->
                    all(query)
                  end