References inside a prefixed table cannot reference an unprefixed table

[jdewar]

References inside a prefixed table cannot reference an unprefixed table

[josevalim]

I have looked into this and unfortunately I don't think there is an easy fix. Perhaps you can set it to prefix: prefix(), which is the default prefix?

[josevalim]

Nah, prefix() may return nil too. In order to fix this, we would need to change the .prefix field in index, table, reference and other structs to be {:ok, prefix} | :error or something of sorts. But this would be backwards incompatible.

[jdewar]

nothing super elegant, but ..

• one backwards compatible option would be to have a no_prefix: true (remove_prefix, clear_prefix, etc) option that raised if you tried to set it and :prefix in the same opts.
• another would be to allow prefix: to take something more complex than an atom/string, like {:clear, :prefix}
• similar to previous idea, maybe it's okay to special case the false atom as clearing the prefix (you could still use "false" string if that was a desired prefix) oh right, hmm, not backwards compatible, this.

[SteffenDE]

The default schema on postgres databases is "public". So you should be able to write add(:group_id, references("group", prefix: "public")) in this case.

[greg-rychlewski]

The default schema on postgres databases is "public". So you should be able to write add(:group_id, references("group", prefix: "public")) in this case.

This is good advice. Maybe we should just document something like this as the solution. As far as I know you should always be able to write unqualified table names as qualified table names. In Postgres every table is inside of a schema. In MySQL every table is inside of a database.

It sounds better to me than introducing a new conflicting option or a backwards incompatible change.

[josevalim]

Yeah, it is just the behaviour is inconsistent with the other uses of prefixes, but there isn't much we can do I think.

[greg-rychlewski]

Is there any precedent to create a new function that is basically the old one but with the correct fix? And then deprecate the old one.

[josevalim]

From my exploration the issue is really the struct. So we need to change the struct which will break the adapters. It is not a major pain we can do it. And if we change it to {:ok, ...} | :error, adapters can easily handle the different types.

[greg-rychlewski]

Oh I think I see what you mean. The issue is here right:

quote_table(ref.prefix || table.prefix, ref.table)

So if ref.prefix is :error then we use table.prefix but if it is {:ok, nil} then we use the unprefixed quote_table function?

That looks good to me. I wasn't sure what the policy was about changing the behaviour of existing migrations or other adapters like SQLite, Clickhouse, etc. But if it's normal procedure then +1 from me.

[josevalim]

Typically we add new callbacks on major versions, which often breaks the adapter anyway, but it is very unoften that we change behaviour. That's why adding a new field to the struct, as mentioned by @jdewar, may be the best option? We can call it options, keep it as a keyword list, and shove :prefix there? So the code above becomes:

quote_table(Keyword.get(ref.options, :prefix, table.prefix), ref.table)

And we deprecate .prefix?

[greg-rychlewski]

That sounds like the best option to me. And the docs are pretty clear about he behaviour:

:prefix - The prefix for the reference. Defaults to the prefix defined by the block's table/2 struct (the "products" table in the example above), or nil.

So if someone was using prefix: nil to be the table's prefix then they were using it in error anyway.

[jdewar]

We were able to side step the problem by creating a new prefix, and using "public" would've been our solve (wouldn't've created this issue even, had I known). Thanks much for this change! It's nice to set prefix to 'nil' in a way that matches my mental model.

:prefix - The prefix for the reference. Defaults to the prefix defined by the block's table/2 struct (the "products" table in the example above), or nil.

So if someone was using prefix: nil to be the table's prefix then they were using it in error anyway.

@greg-rychlewski I did want to comment on this part; one might have the Application config define a :migration_default_prefix, and then a table's prefix: nil could make sense, eg:

config :myapp, Repo, migration_default_prefix: "my_prefix"

...

create table("user", prefix: nil) do
  add(:prefixed_group_id, references("prefixed_group", prefix: "my_prefix"))
end

[greg-rychlewski]

Yes sorry. I meant it's ok for us to "break" users who are relying on our mistake to have the migrations behave in an undocumented way.