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]

Thank you for the PR. I'm wondering if we have an issue here too? https://github.com/elixir-ecto/ecto_sql/blob/373845629a6a66aabbcc5db4c1a201ccbf075d35/lib/ecto/adapters/postgres/connection.ex#L1561

[greg-rychlewski]

Here as well: https://github.com/elixir-ecto/ecto_sql/blob/373845629a6a66aabbcc5db4c1a201ccbf075d35/lib/ecto/adapters/postgres/connection.ex#L1550

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_columntype 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.

[dkuku]

I updated the pr with the code from last comment. We should decide either to merge it or revert the commit that caused the issue because master is broken.

[josevalim]

I am thinking a revert is saner. Thoughts @greg-rychlewski?

[greg-rychlewski]

That is my preference as well. I think we will have less issues if we make a separate function for modifying constraints.