some ddl callback clauses are duplicated

[ruslandoga]

These two ranges seem to be duplicated. The second one seems "more complete", it has exceptions on constraints and uses prefix in table renaming. Except, what are table prefixes in SQLite?

ecto_sqlite3/lib/ecto/adapters/sqlite3/connection.ex

Lines 460 to 562 in 6dcc0b2

	@impl true
	def execute_ddl({:create, %Index{} = index}) do
	fields = intersperse_map(index.columns, ", ", &index_expr/1)
	[
	[
	"CREATE ",
	if_do(index.unique, "UNIQUE "),
	"INDEX ",
	quote_name(index.name),
	" ON ",
	quote_table(index.prefix, index.table),
	" (",
	fields,
	?),
	if_do(index.where, [" WHERE ", to_string(index.where)])
	]
	]
	end
	@impl true
	def execute_ddl({:create_if_not_exists, %Index{} = index}) do
	fields = intersperse_map(index.columns, ", ", &index_expr/1)
	[
	[
	"CREATE ",
	if_do(index.unique, "UNIQUE "),
	"INDEX IF NOT EXISTS ",
	quote_name(index.name),
	" ON ",
	quote_table(index.prefix, index.table),
	" (",
	fields,
	?),
	if_do(index.where, [" WHERE ", to_string(index.where)])
	]
	]
	end
	@impl true
	def execute_ddl({:drop, %Index{} = index}) do
	[
	[
	"DROP INDEX ",
	quote_table(index.prefix, index.name)
	]
	]
	end
	@impl true
	def execute_ddl({:drop, %Index{} = index, _mode}) do
	execute_ddl({:drop, index})
	end
	@impl true
	def execute_ddl({:drop_if_exists, %Index{} = index}) do
	[
	[
	"DROP INDEX IF EXISTS ",
	quote_table(index.prefix, index.name)
	]
	]
	end
	@impl true
	def execute_ddl({:drop_if_exists, %Index{} = index, _mode}) do
	execute_ddl({:drop_if_exists, index})
	end
	@impl true
	def execute_ddl({:rename, %Table{} = current_table, %Table{} = new_table}) do
	[
	[
	"ALTER TABLE ",
	quote_table(current_table.prefix, current_table.name),
	" RENAME TO ",
	quote_table(nil, new_table.name)
	]
	]
	end
	@impl true
	def execute_ddl({:rename, %Table{} = current_table, old_col, new_col}) do
	[
	[
	"ALTER TABLE ",
	quote_table(current_table.prefix, current_table.name),
	" RENAME COLUMN ",
	quote_name(old_col),
	" TO ",
	quote_name(new_col)
	]
	]
	end
	@impl true
	def execute_ddl(string) when is_binary(string), do: [string]
	@impl true
	def execute_ddl(keyword) when is_list(keyword) do
	raise ArgumentError, "SQLite3 adapter does not support keyword lists in execute"
	end

and

ecto_sqlite3/lib/ecto/adapters/sqlite3/connection.ex

Lines 564 to 685 in 6dcc0b2

	@impl true
	def execute_ddl({:create, %Index{} = index}) do
	fields = intersperse_map(index.columns, ", ", &index_expr/1)
	[
	[
	"CREATE ",
	if_do(index.unique, "UNIQUE "),
	"INDEX",
	?\s,
	quote_name(index.name),
	" ON ",
	quote_table(index.prefix, index.table),
	?\s,
	?(,
	fields,
	?),
	if_do(index.where, [" WHERE ", to_string(index.where)])
	]
	]
	end
	@impl true
	def execute_ddl({:create_if_not_exists, %Index{} = index}) do
	fields = intersperse_map(index.columns, ", ", &index_expr/1)
	[
	[
	"CREATE ",
	if_do(index.unique, "UNIQUE "),
	"INDEX IF NOT EXISTS",
	?\s,
	quote_name(index.name),
	" ON ",
	quote_table(index.prefix, index.table),
	?\s,
	?(,
	fields,
	?),
	if_do(index.where, [" WHERE ", to_string(index.where)])
	]
	]
	end
	@impl true
	def execute_ddl({:create, %Constraint{}}) do
	raise ArgumentError, "SQLite3 does not support ALTER TABLE ADD CONSTRAINT."
	end
	@impl true
	def execute_ddl({:drop, %Index{} = index}) do
	[
	[
	"DROP INDEX ",
	quote_table(index.prefix, index.name)
	]
	]
	end
	@impl true
	def execute_ddl({:drop, %Index{} = index, _mode}) do
	execute_ddl({:drop, index})
	end
	@impl true
	def execute_ddl({:drop_if_exists, %Index{} = index}) do
	[
	[
	"DROP INDEX IF EXISTS ",
	quote_table(index.prefix, index.name)
	]
	]
	end
	@impl true
	def execute_ddl({:drop_if_exists, %Index{} = index, _mode}) do
	execute_ddl({:drop_if_exists, index})
	end
	@impl true
	def execute_ddl({:drop, %Constraint{}, _mode}) do
	raise ArgumentError, "SQLite3 does not support ALTER TABLE DROP CONSTRAINT."
	end
	@impl true
	def execute_ddl({:drop_if_exists, %Constraint{}, _mode}) do
	raise ArgumentError, "SQLite3 does not support ALTER TABLE DROP CONSTRAINT."
	end
	@impl true
	def execute_ddl({:rename, %Table{} = current_table, %Table{} = new_table}) do
	[
	[
	"ALTER TABLE ",
	quote_table(current_table.prefix, current_table.name),
	" RENAME TO ",
	quote_table(new_table.prefix, new_table.name)
	]
	]
	end
	@impl true
	def execute_ddl({:rename, %Table{} = table, current_column, new_column}) do
	[
	[
	"ALTER TABLE ",
	quote_table(table.prefix, table.name),
	" RENAME COLUMN ",
	quote_name(current_column),
	" TO ",
	quote_name(new_column)
	]
	]
	end
	@impl true
	def execute_ddl(string) when is_binary(string), do: [string]
	@impl true
	def execute_ddl(keyword) when is_list(keyword) do
	raise ArgumentError, "SQLite3 adapter does not support keyword lists in execute"
	end

[warmwaffles]

Oh that's weird. I wonder why it wasn't caught by linting or the compiler?

[ruslandoga]

I wondered that too, so I had to double check that the clauses are in fact the same. Maybe @impl true has something to do with it? Or maybe the expressions in the clauses are "too complex".

Should we tag someone from the Elixir core team?

[warmwaffles]

It very well could be all the @impl trues. I think we should remove them to be set on the first defined method of the implementation.

[warmwaffles]

@ruslandoga table prefixes IIRC is a way to prefix table names via a config. https://hexdocs.pm/ecto_sql/Ecto.Migration.html#module-prefixes

We definitely need to support the table prefixing.

[ruslandoga]

But does sqlite support them?

sqlite> create table prefix.table(a integer);
Parse error: near "table": syntax error
  create table prefix.table(a integer);
                      ^--- error here

My understanding is, sqlite is one file, one schema, one database.

[warmwaffles]

It does not, but we can support it via <prefix>_<tablename>

[warmwaffles]

Or we could just throw our hands up in the air and raise an exception saying that is is not supported.

[ruslandoga]

I'd probably raise an exception. <prefix>_<tablename> does not really provide any data separation that is expected from separate schemas (postgresql) / databases (mysql).

[warmwaffles]

I'm okay with removing the prefix support and failing loudly. We'd need to make clear call out in the changelog that it is a breaking change and will need to be accounted for.

[ruslandoga]

I don't think anyone was using table prefixes

ecto_sqlite3/lib/ecto/adapters/sqlite3/connection.ex

Line 1802 in 6dcc0b2

defp quote_table(prefix, name), do: [quote_entity(prefix), ?., quote_entity(name)]

since that would've caused the sqlite parser to throw

sqlite> create table prefix.table(a integer);
Parse error: near "table": syntax error
  create table prefix.table(a integer);
                      ^--- error here

so it's going to be a safe change. I hope.

[ruslandoga]

If it's ok with you, I'd like to try and work on all this (duplicate ddl clauses, impl true, table prefix) tomorrow :)

[warmwaffles]

I'm all good with that.

I took a stab at adding dialyzer here too to see if it would catch the duplicate function definition. But that was a mess to deal with.