Skip to content

Field Occurrence.breadcrumbs is broken for MariaDB #150

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
phihos opened this issue May 4, 2025 · 8 comments · May be fixed by #151
Open

Field Occurrence.breadcrumbs is broken for MariaDB #150

phihos opened this issue May 4, 2025 · 8 comments · May be fixed by #151
Assignees
Labels
bug Something isn't working

Comments

@phihos
Copy link
Contributor

phihos commented May 4, 2025

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.

@phihos phihos added the bug Something isn't working label May 4, 2025
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.Array`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
@phihos phihos linked a pull request May 4, 2025 that will close this issue
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.Array`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breakcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when storing and retrieving arrays. Therefore it works very similarly to the `:array` type but without crashing on retrieval when using MySQL/MariaDB.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when retrieving arrays with MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to encode and decode arrays when retrieving arrays with MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 4, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
@odarriba odarriba assigned odarriba and phihos and unassigned odarriba May 11, 2025
@odarriba
Copy link
Contributor

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:

Image

and the entry is on the database:

Image

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

Image

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
Copy link
Contributor Author

phihos commented May 11, 2025

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 added a commit to phihos/error-tracker that referenced this issue May 18, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 18, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
phihos added a commit to phihos/error-tracker that referenced this issue May 18, 2025
Instead of using `{:array, :string}` for the `:breadcrumbs` field in `ErrorTracker.Occurrence` we use a new custom field type `ErrorTracker.Types.StringArray`.
It uses `Jason` to decode arrays when retrieving data from MySQL/MariaDB. Therefore it works very similarly to the `:array` type but without crashing on retrieval.
This fix does not make a schema migration necessary. An additional test module `ErrorTracker.StoreFetchTest` was implemented to avoid regreessions in the future.

Fixes elixir-error-tracker#150.
@phihos
Copy link
Contributor Author

phihos commented May 18, 2025

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?

@phihos phihos changed the title Field Occurrence.breadcrumbs is broken for MySQL/MariaDB Field Occurrence.breadcrumbs is broken for MariaDB May 18, 2025
@odarriba
Copy link
Contributor

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
Copy link
Contributor Author

phihos commented May 19, 2025

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
Copy link
Contributor

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 added a commit to phihos/error-tracker that referenced this issue May 20, 2025
phihos added a commit to phihos/error-tracker that referenced this issue May 20, 2025
@phihos
Copy link
Contributor Author

phihos commented May 20, 2025

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 elixir-ecto/ecto_sql#668. We are closer to fix this!

That is great news! Thank you! 🥇

@odarriba
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants