extra autoincrement and (maybe) contradictory error message in myxql adapter connection.ex

[petermueller]

Environment

• Elixir version: v1.11.2
• Database and version: MariaDB 10.1.48
• Ecto version: v3.6.1
• Database adapter and version: myxql, v0.5.1
• Operating system: macOS

Current behavior

Adding a :bigserial column to a table that already has a primary_key/autogenerated column throws an error.

# migration
create table(:some_audit_table, primary_key: false) do
  add(:audit_id, :bigserial, primary_key: true, null: false)
  add(:id, :bigserial, null: false) # id of some audited table, reference() wouldn't make sense here
  ...
end

$> mix ecto.migrate --log-sql
[debug] QUERY ERROR db=1.7ms queue=2.1ms idle=49.2ms CREATE TABLE `some_audit_table` (`audit_id` bigint unsigned not null auto_increment NOT NULL, `id` bigint unsigned not null auto_increment NOT NULL ..., PRIMARY KEY (`audit_id`)) ENGINE = INNODB
** (MyXQL.Error) (1075) Incorrect table definition; there can be only one auto column and it must be defined as a key

Expected behavior

For the migration to not quietly make a second autoincrement column when it's a regular :bigserial (i.e. not a reference())

I'll note though that is this is tricky, because I understand that SERIAL is an alias for BIGINT UNSIGNED NOT NULL AUTO_INCREMENT UNIQUE in MySQL/MariaDB, and this setting makes it easier to catch early, before you've accidentally added a primary key to a table you had intended to have some other kind of primary key.

After looking at the Ecto.Adapters.MyXQL.Connection and Ecto.Adapters.Postgres.Connection adapters I noticed that both adapters mention in an error for ecto_to_db/2 that the type can be an atom, string, or tuple, but when I set the type to be a string, e.g. add(:id, "bigint unsigned", null: false), I get the following error:

** (FunctionClauseError) no function clause matching in Ecto.Migration.validate_type!/1

    The following arguments were given to Ecto.Migration.validate_type!/1:

        # 1
        "bigint unsigned"

    Attempted function clauses (showing 5 out of 5):

        defp validate_type!(:datetime)
        defp validate_type!(type) when is_atom(type)
        defp validate_type!({type, subtype}) when is_atom(type) and is_atom(subtype)
        defp validate_type!({type, subtype}) when is_atom(type) and is_tuple(subtype)
        defp validate_type!(%Ecto.Migration.Reference{} = reference)

If I switch the type to a "stringy" atom, (:"bigint unsigned") the migration works as expected, but that seems like a bad idea in the long-run since it relies on the implementation of a private function.

With the error text from the adapters, I'd assume that being able to set the type using a string would be something that should've been supported but I wasn't sure.

If this would be better as a proposal discussion on the Google Forum I can post this there.

I'm also happy to make a PR or discuss whether this even is a bug. I'm not fluent in much of the data mapping logic, but I think I understand the migration/runner/connection logic a bit more now.

Thank you for your time 😃

[josevalim]

The type is meant to be atoms or tuples. Quoted atoms are also allowed. We need to improve validate_type to raise something readable instead of function clause error.

[petermueller]

Ok 👍🏻
Would it also make sense to change the adapters' ecto_to_db/2 defs?
They're currently:

    defp ecto_to_db(str, _query)  when is_binary(str), do: str
    defp ecto_to_db(type, _query) do
      raise ArgumentError,
            "unsupported type `#{inspect(type)}`. The type can either be an atom, a string " <>
              "or a tuple of the form `{:map, t}` where `t` itself follows the same conditions."
    end

Passing the str I could see making sense if you want to handle edge-cases higher up in the private functions, but should we remove the "string" part from the error message?

I don't wanna make unnecessary work, was just curious. I can totally live with just doing the quoted atom in my code.
If we'd like to update just the validate_type or all of the errors but you don't have time, I can also make a PR with a shot at some wording that would make sense