Phoenix: Invalid migration generated by gen.model and run-time errors after omitting `_id` postfix in association column name

[afronski]

Environment

• Phoenix version: 1.1.4, _master_
• Ecto version: 1.1.4
  

  Steps to reproduce

I've created two models with those commands:

• mix phoenix.gen.html User users foo:string - _not important_, just for clarity.
• mix phoenix.gen.html Room rooms author:references:users

Second command is crucial, because I forgot to add _id postfix - it should look more like this one:

• mix phoenix.gen.html Room rooms author_id:references:users

Actual behavior

So the fragment of generated migration looks like the following:

  # ...
    create table(:rooms) do
      # It should be `author_id`...
      add :author, references(:users, on_delete: :nothing)

  # ... and similar issue for column name for index creation :(

This would not be a problem - migrations will be applied successfully, app will compile without any errors - however it will crash during the run-time execution.

It happens because belongs_to will automatically add _id postfix to the column name and it will look for a non-existing column e.g. doing Repo.all(Room):

It is not a big deal - it can be fixed manually, by changing column name in migration. However _IMHO_ it can lead to confusion and if you're using generators you'd expect to have working state immediately after using a command.

Expected behavior

Created migration could look like this:

defmodule InteractiveTetris.Repo.Migrations.CreateRoom do
  use Ecto.Migration

  def change do
    create table(:rooms) do
      add :author_id, references(:users, on_delete: :nothing)

      timestamps
    end
    create index(:rooms, [:author_id])

  end
end

Or at least it should trigger _a warning_ that we're expecting certain naming convention for column names in references - it looks like it trims postfix automatically here.

This can be also reformulated to a different question:

Is _gen.model_'s association syntax compliant with both formats - the reference name (author) and column name (author_id) - or just with column name in certain format (author_id)?

If you'll find this _"bug"_ worth fixing - I'd like to provide a fix for that. :neckbeard:
If not - feel free to close it. :wink:

[chrismccord]

Hey @afronski,

Thanks the report. I can see how this can be confusing, but I'd prefer to wait until it becomes clearer that it's a common issue. The problem with either warning or implicitly adding "_id" is you may have a key that doesn't abide by the _id postfix, which is why we require it to prevent confusion in the other direction. I may revisit this if it becomes a bigger issue, and I'll be sure to ping you for the help :) Thanks!

[josevalim]

Exactly. The issue with automatically guessing and fixing things is that we may end-up "fixing" cases that should not be fixed because they user another "convention". :)

[michalmuskala]

The main issue is that the generated code is wrong and does not work. If we want to allow passing filed names without _id we should probably change the belongs_to declaration in the model, so that it works correctly.

[josevalim]

I see what you mean or the generated code is correct or we should raise. Today is none of those. Ideas how to fix this then? Should we enforce _id?

[allyraza]

IMO we should not be guessing the column names in some cases we have users that are working with legacy systems that can't follow these conventions I would suggest we should change the generators and reference the column name based on user given input

[pfitz]

Is anyone writing a pullrequest for this? If not: I could takle this issue I guess.

[chrismccord]

@pfitz please go ahead!

[pfitz]

Ok. Will do a pr. But first how do you guys want to tackle this?

The options so far in this thread seems to be:

1. trigger a warning that we're expecting certain naming convention for column names in references
2. change the belongs_to declaration in the model, so that it works correctly.
3. I see what you mean or the generated code is correct or we should raise.

Personally I think one should be free to choose database column names. Like @allyraza said it is a possibilty to use legacy databases where one does not have a choice to conform to the „phoenix convention of doing things“. Therefore I suggest to use option 2 and change the belongs_to declaration.

[josevalim]

@pfitz if we can make 2 work, that sounds good to me!

[josevalim]

Thanks for all the work so far!

[pfitz]

I started working on the issue and realized so far the following.
I created a new test project and issued mix phoenix.gen.html Room rooms author:references:users.
The created model is :

defmodule TestId.Room do
  use TestId.Web, :model

  schema "rooms" do
    belongs_to :author, TestId.Author

    timestamps
  end

  @required_fields ~w()
  @optional_fields ~w()

  @doc """
  Creates a changeset based on the `model` and `params`.

  If no params are provided, an invalid changeset is returned
  with no validation performed.
  """
  def changeset(model, params \\ :empty) do
    model
    |> cast(params, @required_fields, @optional_fields)
  end
end

and the generated migration is:

defmodule TestId.Repo.Migrations.CreateRoom do
  use Ecto.Migration

  def change do
    create table(:rooms) do
      add :author, references(:users, on_delete: :nothing)

      timestamps
    end
    create index(:rooms, [:author])

  end
end

So when one do not want to force column names to end with _id this seems to be all correct. Or do I have some error in my thoughts.

[allyraza]

I was messing with this but nothing concrete

[josevalim]

@pfitz trying to list or create a room should fail. If it doesn't, make sure you drop the database and start a new one from scratch, specially if you are like me and use the same application name for different temporary projects :)

[allyraza]

has this been finalized? or should I just do a PR with what I have

[pfitz]

I am very sorry that I could not contribute yet. Since my time problems seems not to improve I im „stepping down“ from this task. Sorry for the delay I caused :(

[EduardoBautista]

I can take this issue. However, I am confused on how we should handle this issue. How should "belongs_to" work? Should it expect an option to specify a different column name?

[GriffinMB]

I made a PR for this a few days ago. It seemed like we wanted to allow the flexibility of having columns without "_id", so I avoided throwing an error. It also seemed like consensus was that we didn't like the idea of silently appending "_id" to the column name.

The check for "_id" happens within ecto though, so it seemed like adding options to the belongs_to method was the best bet. Let me know if you guys have any thoughts, or if you think a different approach would be better. I'd be happy to take another look at it if that's what we think is best.

[GriffinMB]

@josevalim Any thoughts on the implementation?

[shhavel]

Maybe when there is a collision between the name of the association
and the name of the field (foreign key doesn't end with _id),
the field can be defined without association
and the comment about collision can be generated, e.g:

# This is a foreign key-reference to another table.
# Association could not be generated automatically
# due to the collision of its name with the name of the foreign key.
# You can define the association instead, if you change its name:
#
#     belongs_to :author, MyApp.User, foreign_key: :author
# 
field :author, :integer

Also a warning can be generated:

warning: foreign key-reference field doesn't end with `_id`

[wademauger]

Does this still need addressed? I'm just getting started with things, so if it's something that needs doing, I could try.

[yaBliznyk]

We have foreign key end with __id. And name starts with capitalize word. Hotel__id, Resort__id and so on.

[kelvinst]

@shhavel if you are generating the warning on the model, why not generate the code where most of the cases are the intended code, which in my opinion would be:

belongs_to :author, MyApp.User, foreign_key: :author

And about the warning, I guess that, if we don't want to force _id fields for references because we want to be agnostic to it, we should not warn the user if they are not like that. After all, if the generated code is the intended code in most cases, the user will not need to be aware of a "unexpected behaviour", because it's following it's natural course.

What do you think?