Initial MySQL support (early draft) - JSONField version #91
Conversation
Codecov ReportPatch coverage:
❗ 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@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 92.95% 92.97% +0.02%
==========================================
Files 59 59
Lines 3817 3829 +12
Branches 245 250 +5
==========================================
+ Hits 3548 3560 +12
Misses 224 224
Partials 45 45
☔ View full report in Codecov by Sentry. |
|
Hmm not actually sure why this is failing with exit code 1 on Django >= 4.0, struggling to reproduce this locally |
|
@Joshun thanks for giving this a shot. Looking at the error, it says it's looking for file I'm not even sure where it's getting the idea to try and run that. Any ideas? |
|
Looks like it's a result of |
|
I've fixed the migration issue and also resolved conflicts with master; it's passing on our CI now. If it passes again here, is there much else you guys would need me to do for this to be accepted? At some point the way currencies is stored, whether as JSON / join table / something else may want to be revisited, but I feel that might be starting to expand beyond the scope of this PR... Thoughts? |
|
Overall this looks great. With the JSONField all the current specs pretty much cover the use cases including the DB triggers. To bring it to release ready:
@PetrDlouhy, anything else above? Separately, I recommend removing any specific DB dependency from requirements.txt:
Thoughts? |
|
Thank you @Joshun. At first glance everything looks very good. I aggree with @nitsujri on the improvements. If you both agree that easy transition to JSONField is OK for now even if there might be need for join table in the future, I am OK with that. I see the tests are running only for MariaDB. I don't know how big the differences with MySQL are, but maybe it would be beneficial to have MySQL also tested, at least in one Django/Python version combination. I would be happier with a bit cleaner commit history, but I can use Squash and merge, especially if we split this in 2 or 3 separate PRs (see below). I would like to merge this in few steps to simplify the revision process:
|
Thanks @PetrDlouhy that sounds like a good plan. Once you've merged the JSONField changes let me know if you need a hand with adding the MySQL changes |
|
Thanks guys. I updated #90 and got it ready for merging w/ the fix included. For the |
MariaDB testing MariaDB testing (2) custom healthcheck command custom healthcheck command (2) custom healthcheck command (3) custom healthcheck command (4) root password run as root run as root (2) run as root (3) mariadb-client mariadb-client (2)
|
Looks like some of the procedures have changed further e.g. |
Joshun
force-pushed
the
currencies-as-json
branch
from
db0b855 to
2e0de30
Compare
- get check_leg_and_account_currency_match working in MySQL again - resolve remaining currency import issues - escaping of % in procedures (needed in RunPython)
Joshun
force-pushed
the
currencies-as-json
branch
from
2e0de30 to
68c58f5
Compare
|
Ok I've done the rebase now, but will probably need checking over, I had to combine a few of the changes to the Postgres procedures and change the way the errors were handled in one of the tests (MySQL raises OperationalError when a stored procedure signals an error, so have to catch both that and InternalError) |
requirements.txt
Outdated
| @@ -10,3 +10,4 @@ django-money>=0.9.1 | |||
| django-import-export>=0.5.0 | |||
| babel>=2.9.1 | |||
| openpyxl<=2.6;python_version<"3.5" | |||
| mysqlclient==2.1.0 | |||
There was a problem hiding this comment.
@Joshun awesome stuff!
@PetrDlouhy let's:
- remove both psycopg2 and mysqlclient. add a requirements_pg.txt and requirements_mysql.txt for respective test infra.
- Change workflow to install client for specific test.
- Modify docs as part of the cold-start/install process to install the corresponding client.
Thoughts?
There was a problem hiding this comment.
Maybe we should let it be installable like pip install hordak[postgres] or pip install hordak[mysql]?
There was a problem hiding this comment.
Always possible.
I'm of the mind they'll already have it installed anyway.
There was a problem hiding this comment.
@nitsujri You are probably right. It would not make sense to have project without it.
|
@Joshun Thank you very much for this. I think the PR looks overall very good at first glance. I will try to do more detail check. First I wanted to try Can you please remove the dependency so I can proceed with the tests? |
|
@PetrDlouhy that error is because you need Also we'll need it in order to run the unit tests for MySQL in CI. But if you don't want it in the requirements.txt file, I could modify the Github Action to just install the package manually with pip? |
|
@Joshun Would the mysql require other packages than those that you would expect to be in every project with mysql setup? Would it be needed in the future? If the answer is yes, I would suggest the solution with For the CI you could modify the Action. |
|
@Joshun You also wouldn't want to introduce postgresql dependencies to your project, so you can also remove the |
|
Ok I'll remove it from the requirements.txt and update the action shortly. What are you guys planning on doing with requirements_test.txt as I noticed this has psycopg2 inside it? But I can always just leave that for now. |
|
@Joshun Yeah, you can move the mysql and psycopg to the |
|
@PetrDlouhy ok have just done this |
|
@Joshun All tests ran well, and everything seems OK. |
Oops missed that, have just done that also |
|
Looks like pyscopg is needed for mypy CI to function? |
|
@Joshun Yes. You probably can install whole |
|
It seems very nice now. I will pull this PR and make new version. |
|
@Joshun I forgot about the documentation. Could you please make a new PR with update of |
|
@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. |
Ok will do this shortly I might also add the fix for the CI here too |
| return super(Leg, self).save(*args, **kwargs) | ||
|
|
||
| leg = super(Leg, self).save(*args, **kwargs) | ||
| transaction.on_commit(lambda: _enforce_leg(transaction_id=self.transaction_id)) |
There was a problem hiding this comment.
Maybe I missed something, but is this check not executed after the transaction is committed?
From https://docs.djangoproject.com/en/4.2/topics/db/transactions/#performing-actions-after-commit
on_commit() allows you to register callbacks that will be executed after the open transaction is successfully committed
There was a problem hiding this comment.
hmm that is possible actually, it's a bit tricky though as if _enforce_leg is called before the transaction is committed, changes made won't be visible to the check_leg stored procedure. I wonder if a suitable solution would be to rollback the transaction if the procedure fails?
There was a problem hiding this comment.
Afaik you see the changes "in" a transaction even before it is committed.
I wonder if a suitable solution would be to rollback the transaction if the procedure fails?
I don't think that you can rollback a already committed transaction.
There was a problem hiding this comment.
Afaik you see the changes "in" a transaction even before it is committed.
I wonder if a suitable solution would be to rollback the transaction if the procedure fails?
I don't think that you can rollback a already committed transaction.
I will have a look into this. In the meantime would you mind opening this as a new issue? Commenting on an already-closed PR might mean this issue has poor visibility.
There was a problem hiding this comment.
After looking at this again, I think the reason I did this in on_commit was to emulate the behaviour of the original Postgres trigger:
https://github.com/adamcharnock/django-hordak/blob/master/hordak/migrations/0002_check_leg_trigger_20160903_1149.py
Postgres has deferred triggers, which Hordak uses to check the legs are equal to zero after a transaction has taken place (need to make sure both legs have been added first), but MySQL doesn't have this.
There was a problem hiding this comment.
Apologies for being pedantic. I believe it's still inside the transaction:
They can be fired either at the end of the statement causing the triggering event, or at the end of the containing transaction; in the latter case they are said to be deferred.
There was a problem hiding this comment.
It's inside the transaction, but after all the changes have been applied, which isn't something MySQL has, so we'd have to emulate this behaviour somehow. Seems to be something a lot of people are wanting [1] [2]
[1] https://stackoverflow.com/questions/5014700/in-mysql-can-i-defer-referential-integrity-checks-until-commit
[2] https://www.draconianoverlord.com/2010/07/22/dammit-mysql.html/
There was a problem hiding this comment.
I've just added a PR for a possible approach to improving on this behaviour:
#106
Currently it requires you to decorate the core functions with @enforce_atomic (replacing @db_transaction.atomic)
Some of the tests will fail currently as they don't use this decorator. Ideally there would be a way of enforcing this without the decorator.
|
I'm going to repeat a comment here that I left on #106, just so it is more visible: FWIW: I'm not keen on adding complexity to the core Hordak codebase in order to support MySQL's limitations (which includes the current use of Adding complexity and workarounds to Hordak's core feels like pushing in the wrong direction to me. I'm open to conversation on this, but I'm currently leaning towards Hordak having 'MySQL support with caveats'. |
Same as #89, but incorporate @nitsujri's changes in #90 with some modifications to only migrate over old data on Postgres, and only if there is data present