fix_regression_on_modify_column

[dkuku]

I did not analyzed the code much, it's just the minimal change that fixes the #647 and it's compatible with existing code.

[greg-rychlewski]

Suggested change

	defp extract_opts(opts) do
	with {:ok, from} <- Keyword.fetch(opts, :from),
	{_type, from_opts} <- from do
	from_opts
	else
	_ -> opts
	end
	end
	defp extract_from_opts({_type, opts}), do: opts
	defp extract_from_opts(_), do: []

I think we might need to do something like this. Otherwise if someone supplies something like this it will think they are equal:

modify(:field, :string, size: 100, from: :string)

[greg-rychlewski]

Thank you for the PR. I'm wondering if we have an issue here too?

ecto_sql/lib/ecto/adapters/postgres/connection.ex

Line 1561 in 3738456

if reference_column_type == reference_column_type(from_column_type, opts) do

[greg-rychlewski]

Here as well:

ecto_sql/lib/ecto/adapters/postgres/connection.ex

Line 1550 in 3738456

from_column_type = extract_column_type(opts[:from])

if we are allowing from: {%Reference{}, opts}

[josevalim]

Yeah, I am afraid this is a bit too tricky to the point a separate helper may be better after all. :(

[dkuku]

Thanks @greg-rychlewski for the review. The 2 places you marked are in the same function. I think it should be good there now

if we are allowing from: {%Reference{}, opts}

I'm not sure if this one will not crash, because the reference column type is handled differently, when wrapped in tuple then instead of returning type it will return the reference struct.

  1852     defp extract_column_type({type, _}) when is_atom(type), do: type                                                                   
  1853     defp extract_column_type(type) when is_atom(type), do: type                                                                        
  1854     defp extract_column_type(%Reference{type: type}), do: type      

Feel free to update the pr. We should test the type system on this piece of code :D

[greg-rychlewski]

Sorry what I meant was

defp column_change(table, {:modify, name, %Reference{} = ref, opts}) do
      reference_column_type = reference_column_type(ref.type, opts)
      from_column_type = extract_column_type(opts[:from])
      from_opts = extract_opts(opts[:from])

If opts is from: {%Reference{}, from_opts} then extract_opts(opts[:from]) will return nil here, from what i can see

defp extract_column_type({type, _}) when is_atom(type), do: type
defp extract_column_type(type) when is_atom(type), do: type
defp extract_column_type(%Reference{type: type}), do: type
defp extract_column_type(_), do: nil

But I also agree with Jose this is kind of getting hairy. It's also making me wonder if it people will use it wrong without realizing

[dkuku]

If opts is from: {%Reference{}, from_opts} then extract_opts(opts[:from]) will return nil here, from what i can see

But in this case extract_column_type will also return nil because no other pattern matches, the first one will not match because {%Reference{} = reference, _} where is_atom(reference).
Maybe using something like that is cleaner?

defp extract_column_type_and_opts({type, opts}) when is_atom(type), do: {type, opts}
defp extract_column_type_and_opts({%Reference{type: type}, opts}), do: {type, opts}
defp extract_column_type_and_opts(type) when is_atom(type), do: {type, []}
defp extract_column_type_and_opts(%Reference{type: type}), do: {type, []}
defp extract_column_type_and_opts(_), do: {nil, []}

[greg-rychlewski]

That's a good point. I think you uncovered another potential issue.

So then the question is do we want to continue trying to make it work this way or would it be simpler to take the functionality out of modify.