Field Occurrence.breadcrumbs is broken for MariaDB

[phihos]

Describe the bug

Hey folks, I recently upgraded from v0.3.0 to v0.6.0 and the detail page of any error crashed with

** (ArgumentError) cannot load `"[]"` as type {:array, :string} for field :breadcrumbs in %ErrorTracker.Occurrence{__meta__: #Ecto.Schema.Metadata<:loaded, "error_tracker_occurrences">, id: nil, reason: nil, context: nil, breadcrumbs: nil, stacktrace: nil, error_id: nil, error: #Ecto.Association.NotLoaded<association :error is not loaded>, inserted_at: nil}

I already debugged this and it seems that the Ecto adapter Ecto.Adapters.MyXQL correctly stores arrays (like "[]" for an empty array), but crashes on retrieval. I think there are two hints that we can not make this work just by configuration:

1. I was not able to find documentation on how to make type :array work with MyXQL. The closest thing I found was this entry in the docs that describes how to store type :map in a JSON column. But no word about :array.
2. When looking at the MyXQL source we see no implementation for def loaders(:array, type)....

In order to fix this I propose implementing a custom type. I already implemented a proposal that I will show in a PR soon.

To Reproduce

Steps to reproduce the behavior:

1. Configure a MariaDB db as storage backend.
2. Generate some error.
3. Try to navigate to the error detail page (like /errors/1).
4. Internal server error.

Expected behavior

Detail page should not crash.

[odarriba]

Hi!

I'm trying to reproduce the issue, but I had no luck so far.

I configured a MySQL docker inatance with this command:

$ docker run -d --name mysql -p 3306:3306 -e MYSQL_ROOT_PASSWORD=password mysql

and then used you regression test - it passed without changing the field type.

I also used the dev.exs script that we use for local development (with the changes required to use the MySQL server instad of SQLite), and I got it working on main:

and the entry is on the database:

---

I also checked the schema and it is stored correctly as a JSON:

Can you check your schema to see if it matches with this one? Which specific version of MySQL/MariaDB and EctoSQL are you using?

[phihos]

Interesting. In my tests I was using MariaDB v11.5.2 and myxql v0.6.4.

The table schema looks like this:

MariaDB [error_tracker_test]> SHOW COLUMNS FROM error_tracker_occurrences;
+-------------+---------------------+------+-----+---------+----------------+
| Field       | Type                | Null | Key | Default | Extra          |
+-------------+---------------------+------+-----+---------+----------------+
| id          | bigint(20) unsigned | NO   | PRI | NULL    | auto_increment |
| context     | longtext            | NO   |     | NULL    |                |
| reason      | text                | NO   |     | NULL    |                |
| stacktrace  | longtext            | NO   |     | NULL    |                |
| error_id    | bigint(20) unsigned | NO   | MUL | NULL    |                |
| inserted_at | datetime(6)         | NO   |     | NULL    |                |
| breadcrumbs | longtext            | YES  |     | NULL    |                |
+-------------+---------------------+------+-----+---------+----------------+

According to the MariaDB docs json is an alias for longtext. Maybe this is the important difference here?

[phihos]

Hi, @odarriba I added a separate CI test run for mysql and added the old field type {:array, :string} to :breadcrumbs. The error was reproducible with mariadb. See this test log. I reverted that back to the new field type and ran the tests again. Now they pass. Apparently, separate mariadb and mysql tests are necessary to detect this kind of issue.

I know that the real bug is in the database driver, but would you accept this workaround regardless?

[odarriba]

Hi @phihos!

First of all, thanks for your efforts on trying to solve this issue and to add the CI pipeline - it is really appreciated. Also, I'm sorry for my slow responses. Many changes are happening these days on my life and I could not be as quick as I would like to.

I checked the issue on my local environment, and it looks like the issue is what you pointed: Maria DB is storing JSON as longtext.

The issue here is that MySQL/MariaDB does not have a column type for an array of strings, so we used a JSON column because, well, an arrray of strings is a valid JSON after all. However, using a :map type in Ecto was not possible, as an array is not a map, so we ended up with this mixed solution.

I was able to path Ecto SQL on my local environment and check that it worked, but I'm not sure about the implications of the change from a global point of view, so I'm keen towards opening a issue on the EctoSQL repo and see if they think it is a good addition. If we can fix the issue in the library, I prefer it rather than implementing a custom type. It is technically a good solution, but after talking about this with @crbelaus we botyh agree that not having to maintain a custom type is better (if possible).

I will open an issue in a few moments and lets see what are the thoughts of the EctoSQL team.

[phihos]

Hi @odarriba,

Also, I'm sorry for my slow responses. Many changes are happening these days on my life and I could not be as quick as I would like to.

No worries. I know how it is.

It is technically a good solution, but after talking about this with @crbelaus we botyh agree that not having to maintain a custom type is better (if possible).

Understandable. Thank you very much for opening the report upstream! Could we salvage the separate MariaDB and MySQL test runs and the StoreFetchTest? You could merge my PR as soon as the new EctoSQL release is out and the tests pass.

[odarriba]

Could we salvage the separate MariaDB and MySQL test runs and the StoreFetchTest? You could merge my PR as soon as the new EctoSQL release is out and the tests pass.

Of course! In fact, I think many other tests should fail even without that specific StoreFetch test.

To see how many tests fail, can you please update the PR to remove the custom field and leave the CI update and the new test?

Btw, the Ecto SQL merge request has been merged today. We are closer to fix this!

[phihos]

Of course! In fact, I think many other tests should fail even without that specific StoreFetch test.

No, I think we need that test. I originally ran the tests on MariaDB and nothing failed. I think none of the existing tests load a record of Occurrence after storing it.

To see how many tests fail, can you please update the PR to remove the custom field and leave the CI update and the new test?

Done. As you can see only the new test fails.

Btw, the Ecto SQL merge request https://github.com/elixir-ecto/ecto_sql/pull/668. We are closer to fix this!

That is great news! Thank you! 🥇

[odarriba]

Great! Lets maintain that new test and, once EctoSQL gerts a new release, we can merge it.

[phihos]

Hi @odarriba, ecto_sql v3.13.0 or later should fix this issue. What would be the preferred way to ensure others do not accidentally use MySQL with an older version? The deps could force it for all users of course, but this is not strictly necessary for postgres users for example. Maybe a compile/run time error in the MySQL init code is the way to go here?

[odarriba]

Hello!

I think we can just require the ecto_sql version and that's all.

If anyone can't update it (most updates are backwards compatible AFAIK) they can always go wild and override the dependency if needed :)

My general advice is to think about the average user, which can update ecto_sql without issues and expects error_tracker to behave as expected :D

We can also add a note on the changelog when we generate the next version.