Initial MySQL support (early draft)

[Joshun]

Hi, I might be able to provide a contribution for Hordak to support MySQL. We are thinking of potentially using django-hordak for some internal tools, but are heavily coupled to MySQL. I've had to make a few big changes though so I thought I'd outline them here - I'm not expecting this to be accepted in it's current form:

• Rewrote some the migrations to have additional MySQL procedures - I've had to add some if statements checking connection.vendor and running either the MySQL or Postgres version depending on the db. Obviously it's not ideal changing migrations but I don't really see any way around this as in it's current form, the migrations would try to run and MySQL and fail. The Postgres triggers / procedures have been kept intact so Postgres users shouldn't notice any difference
• On Leg and Account I've had to add extra behaviour to the save method to trigger MySQL stored procedures if the connection type is mysql. This is because MySQL does not support deferred transactional triggers (only before or after every row insert/update/delete) so triggers like check_leg would not work as triggers alone
• Implemented a custom HordakMysqlArrayField, which behaves like a Postgres array but serializes and deserializes as JSON.

Let me know your thoughts. I'm not expecting this to be accepted but feedback would be useful and any thoughts on how to integrate the two database systems cleanly. All tests passing on my local machine at time of opening this on both Postgres and MySQL.

Note your CI tests probably won't cover this fully, I'm assuming they are just set up to test Postgres and not MySQL.

[nitsujri]

I'm not a maintainer but have been contributing, so all decisions rest with @PetrDlouhy. I am impressed with the ambition of the PR.

Recommendations

• Use JSONField instead of ArrayField.
  • The HordakArrayField, while the initial concept of it is impressive, would create a long term maintainability problem as the specs would need to be incredibly strong even if just for the MySQL side. Because hordak is only using this field as a string array, JSONField works as a drop in replacement. A lot of the complexity of maintaining a custom field falls away and gets pushed to Django core.
  • Migrations then should have 2 parts.
    • Historical simple ArrayField => JSONField
    • Additional migration if ArrayField => make duplicate column of copied data in JSONField => drop old currencies col => rename duplicate column to currencies
• Add CI spec to have at least 1 version of MySQL that runs through the migrations.

Thoughts?

[PetrDlouhy]

Hi @Joshun,

thank you very much for this great work and such helpful enhancement of this application.

HordakArrayField

I agree with @nitsujri that maintaing whole new field type just for MySQL would be complicated. Although I am not sure what challenges would be needed to overcome if we would transform the field to JSONField for all DB engines and make it in backward compatible way. For example I have in the code many occurrences of things like:

baker.make("Account", code="BHA", currencies=["USD"])

Can the new field handle such code? Also, will the account with currencies=["USD", "EUR"] be compatible with another with currencies=["EUR", "USD"] after the change to JSONField? Maybe the users are using some queries that are filtering by the ArrayField (e.g. Account.objects.filter(currencies__contains=["EUR"]))

If we would want to make bigger change it is also worth thinking about alternatives. The current model with ArrayField could be sometimes difficult to work with when somebody wants to link to account with defined currency. For example I was implementing running totals in https://github.com/adamcharnock/django-hordak/pull/76/ and I had to add whole RunningTotal model while if there would be CurrencyAccount for every currency I could just add the running_total field to it which would make the things much easier.

This all said, I am not sure if this is not out of scope of this PR. I would willing to accept this even with HordakArrayField if you find the other alternatives too difficult and exceeding complexity of this task.

Triggers

Regarding the triggers, I think that it is quite OK to have implemented them in the migrations the same way as triggers for PostgreSQL. Although I am a bit dissatisfied with how this works and maybe even with triggers as such, I think solving this bigger issue might be a topic for another PR. @Joshun But if you are willing to make at least a small improvement in this PR, maybe we could move the triggers creation into separate function which would be called from the migrations as well as it could be called through some management command such as refresh_triggers.

CI tests

As @nitsujri wrote, in order to accept this PR it would be needed to modify the CI specs to run all tests also on MySQL as well as adding tests for any added new feature.

[nitsujri]

Can the new field handle such code? Also, will the account with currencies=["USD", "EUR"] be compatible with another with currencies=["EUR", "USD"] after the change to JSONField? Maybe the users are using some queries that are filtering by the ArrayField (e.g. Account.objects.filter(currencies__contains=["EUR"]))

@PetrDlouhy very fair question - I claimed it was a drop in replacement but is it truly so? Almost is the proper answer. Changes are:

• The DB trigger changes to handle jsonb.
• Form input needs to be proper JSON.
  • Previously: EUR, USD
  • Now: ["EUR", "USD"]

I also added the specs to cover the __contains which works as expected.

So, if we decide to accept MySQL - I do recommend pushing through to JSONField even with the Form changes.

Thoughts?

[Joshun]

Thanks for the kind feedback @nitsujri and @PetrDlouhy - I think the first step is probably for me to add the MySQL CI tests, and then if we do decide to opt for further changes to JSONField etc. we can verify if they still pass. Do you have a preference for MariaDB or MySQL in the CI? Both are supported by Django officially. We use MariaDB, but most of the functionality is interchangeable - could be worth adding a check for both even.

[Joshun]

Just marked it as ready for review because I thought it would trigger the workflows but looks like that isn't the case, I'll setup the workflows in my own repo and see if I can get them working

[Joshun]

All tests passing in MariaDB apart from one of the lint tests

Meant to ask - I'm sure there was a very good reason, but why was an ArrayField used for currencies instead of a join table? (though this is someone coming from a MySQL background)

[codecov-commenter]

Codecov Report

Patch coverage: 76.69% and project coverage change: -0.54 :warning:

Comparison is base (50d6247) 92.45% compared to head (32ffbe1) 91.91%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #89 +/- ## ========================================== - Coverage 92.45% 91.91% -0.54% ========================================== Files 57 57 Lines 3513 3613 +100 Branches 222 243 +21 ========================================== + Hits 3248 3321 +73 - Misses 223 241 +18 - Partials 42 51 +9 ``` | [Impacted Files](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/89?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock) | Coverage Δ | | |---|---|---| | [hordak/models/core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/89?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL21vZGVscy9jb3JlLnB5) | `91.02% <76.69%> (-7.09%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/89/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[nitsujri]

one of the lint tests

For the linters you can run precommit. Basically does everything for you.

why was an ArrayField used for currencies instead of a join table?

Hrm, probably have to ask @adamcharnock, but I'll flip the question, what would be gained by normalizing the currency column?

[PetrDlouhy]

| but why was an ArrayField used for currencies instead of a join table?

I don't have any idea too and as I have already written, join table would be much easier to work with when implementing running totals (PR #76).

If there isn't any strong reason for using ArrayField I see join table to be much cleaner option. Although I am not sure how much work would be to make the transition done in backward-compatible deprecation style.

[Joshun]

@PetrDlouhy looks like you're having the same issue on the CI for MariaDB that we've had here in the past couple of days - I think MariaDB have changed a parameter in their image that has caused the healthchecks to stop working. For now I'd suggest changing the image: under MariaDB to image: mariadb:10.5.21. This is purely to do with Github actions and not the underlying code. I can add this in a separate mini-PR if needed.

[Joshun]

@PetrDlouhy looks like you're having the same issue on the CI for MariaDB that we've had here in the past couple of days - I think MariaDB have changed a parameter in their image that has caused the healthchecks to stop working. For now I'd suggest changing the image: under MariaDB to image: mariadb:10.5.21. This is purely to do with Github actions and not the underlying code. I can add this in a separate mini-PR if needed.

Oops wrong PR

[Joshun]

Closed as was superseded by #91