Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[feature] Added constraint support in migrations #108

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 50 additions & 4 deletions lib/ecto/adapters/clickhouse/migration.ex
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,37 @@ defmodule Ecto.Adapters.ClickHouse.Migration do
[[drop | @conn.quote_table(index.prefix, index.name)]]
end

def execute_ddl({command, %Constraint{} = _constraint})
def execute_ddl({command, %Constraint{} = constraint})
when command in [:create, :create_if_not_exists] do
raise "TODO"
table_name = @conn.quote_table(constraint.prefix, constraint.table)

add =
case command do
:create -> " ADD CONSTRAINT "
:create_if_not_exists -> " ADD CONSTRAINT IF NOT EXISTS "
end

constraint_expr = constraint |> constraint_expr() |> Enum.join("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to Enum.join since we are returning an iolist anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed Enum.join as removed constraint_expr/1


[["ALTER TABLE ", table_name, add, constraint_expr]]
end

def execute_ddl({command, %Constraint{} = _constraint, _mode})
def execute_ddl({command, %Constraint{} = constraint, _mode})
when command in [:drop, :drop_if_exists] do
raise "TODO"
drop =
case command do
:drop -> " DROP CONSTRAINT "
:drop_if_exists -> " DROP CONSTRAINT IF EXISTS "
end

[
[
"ALTER TABLE ",
@conn.quote_table(constraint.prefix, constraint.table),
drop,
@conn.quote_name(constraint.name)
]
]
end

def execute_ddl({:rename, %Table{} = current_table, %Table{} = new_table}) do
Expand Down Expand Up @@ -305,6 +328,29 @@ defmodule Ecto.Adapters.ClickHouse.Migration do
defp index_expr(literal) when is_binary(literal), do: literal
defp index_expr(literal), do: @conn.quote_name(literal)

defp constraint_expr(%Constraint{check: check, validate: true, comment: nil} = constraint)
when is_binary(check) do
[
@conn.quote_name(constraint.name),
" CHECK (",
check,
")"
]
end

defp constraint_expr(%Constraint{check: check, validate: true, comment: comment})
when is_binary(check) and is_binary(comment) do
raise "Clickhouse adapter does not support comments on check constraints"
Copy link
Contributor

@ruslandoga ruslandoga Aug 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably put all these checks into a single function clause like this

defp constraint_expr(%Constraint{} = constraint) do
  if constraint.comment do
    raise "Clickhouse adapter does not support comments on check constraints"
  end
  
  unless constraint.validate do
    raise ...
  end
  
  # etc. checks for other unsupported fields / options
  
  [@conn.quote_name(constraint.name), " CHECK (", constraint.check, ?)]
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case maybe it's more appropriate to move them inside execute_ddl/1

end

defp constraint_expr(%Constraint{check: check, validate: false}) when is_binary(check) do
raise "Clickhouse adapter does not support check constraints without validation on creation"
end

defp constraint_expr(%Constraint{exclude: exclude}) when is_binary(exclude) do
raise "Clickhouse adapter does not support exclude constraints"
end

defp options_expr(nil), do: []

defp options_expr(options) when is_list(options) do
Expand Down
37 changes: 20 additions & 17 deletions test/ecto/adapters/clickhouse/connection_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -2057,21 +2057,21 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
assert execute_ddl(drop) == [~s|DROP TABLE "foo"."posts"|]
end

@tag skip: true
test "drop constraint" do
drop = {:drop, constraint(:products, "price_must_be_positive", prefix: :foo), :restrict}

assert execute_ddl(drop) == [
~s{DROP CONSTRAINT}
~s|ALTER TABLE "foo"."products" DROP CONSTRAINT "price_must_be_positive"|
]
end

@tag skip: true
test "drop_if_exists constraint" do
drop =
{:drop_if_exists, constraint(:products, "price_must_be_positive", prefix: :foo), :restrict}

assert execute_ddl(drop) == []
assert execute_ddl(drop) == [
~s|ALTER TABLE "foo"."products" DROP CONSTRAINT IF EXISTS "price_must_be_positive"|
]
end

test "alter table" do
Expand Down Expand Up @@ -2355,16 +2355,6 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
assert execute_ddl(drop) == [~s|DROP INDEX "posts$main"|]
end

@tag skip: true
test "create constraint" do
assert_raise ArgumentError,
"ClickHouse does not support ALTER TABLE ADD CONSTRAINT",
fn ->
{:create, constraint(:products, "price_must_be_positive", check: "price > 0")}
|> execute_ddl()
end
end

test "drop index with prefix" do
drop = {:drop, index(:posts, [:id], name: "posts$main", prefix: :foo), :restrict}
assert execute_ddl(drop) == [~s|DROP INDEX "foo"."posts$main"|]
Expand All @@ -2382,10 +2372,12 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
assert execute_ddl(drop) == [~s|DROP INDEX "posts$main"|]
end

@tag skip: true
test "create check constraint" do
create = {:create, constraint(:products, "price_must_be_positive", check: "price > 0")}
assert execute_ddl(create) == []

assert execute_ddl(create) == [
~s|ALTER TABLE "products" ADD CONSTRAINT "price_must_be_positive" CHECK (price > 0)|
]

create =
{:create,
Expand All @@ -2394,7 +2386,18 @@ defmodule Ecto.Adapters.ClickHouse.ConnectionTest do
prefix: "foo"
)}

assert execute_ddl(create) == []
assert execute_ddl(create) == [
~s|ALTER TABLE "foo"."products" ADD CONSTRAINT "price_must_be_positive" CHECK (price > 0)|
]
end

test "create check constraint if not exists" do
create =
{:create_if_not_exists, constraint(:products, "price_must_be_positive", check: "price > 0")}

assert execute_ddl(create) == [
~s|ALTER TABLE "products" ADD CONSTRAINT IF NOT EXISTS "price_must_be_positive" CHECK (price > 0)|
]
end

@tag skip: true
Expand Down