Initial MySQL support (early draft) - JSONField version

Joshun commented 1 year ago

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

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 :tada:

Comparison is base (322c264) 92.95% compared to head (88108e0) 92.97%.

: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 #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 ``` | [Impacted Files](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/91?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/91?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL21vZGVscy9jb3JlLnB5) | `98.31% <100.00%> (+0.08%)` | :arrow_up: | | [hordak/tests/models/test\_core.py](https://app.codecov.io/gh/adamcharnock/django-hordak/pull/91?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Adam+Charnock#diff-aG9yZGFrL3Rlc3RzL21vZGVscy90ZXN0X2NvcmUucHk=) | `100.00% <100.00%> (ø)` | |

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

Joshun commented 1 year ago

Hmm not actually sure why this is failing with exit code 1 on Django >= 4.0, struggling to reproduce this locally

nitsujri commented 1 year ago

@Joshun thanks for giving this a shot.

Looking at the error, it says it's looking for file 0038_alter_leg_amount_currency.py, but that file isn't anywhere in the PR.

I'm not even sure where it's getting the idea to try and run that. Any ideas?

Joshun commented 1 year ago

Looks like it's a result of ./manage.py makemigrations --check hordak presumably checking that there are no new model changes that haven't been incorporated into a migration, and failing because it thinks there are changes. I'll have a look and see if there's any migrations that I've somehow skipped

Joshun commented 1 year ago

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? @nitsujri @PetrDlouhy

nitsujri commented 1 year ago

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:

Rename anything postgres... to be more generic unless it is actually specific i.e. test_postgres_trigger... specs.
Add notes to CHANGES.txt
Add install notes/general documentation notes

@PetrDlouhy, anything else above? Separately, I recommend removing any specific DB dependency from requirements.txt:

Remove mysqlclient & psycopg2-binary from requirements.txt
Updating documentation to state manually installing for both using and contributing.
Update github/workflows to install the library it needs to run the spec set.

Thoughts?

PetrDlouhy commented 1 year ago

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:

First I would like to merge only the JSONField change. I am not sure if everything regarding that is already in #90 (maybe some documentation?). If yes, I would only request @nitsujri to rebase against current master and I will test it with my project and merge it.
Then if needed I would like to merge the small corrections independent on this PR. I see the transition to hordak.models.core.default_currencies in 0033_alter_account_currencies.py and 0034_alter_account_currencies_alter_leg_amount_currency.py which might be merged separately and @nitsujri is having issue with that in https://github.com/mobile-power/django-hordak/pull/1, so maybe it would be worth to clear that up in separate PR.
And finally I would merge the transition to MySQL

Joshun commented 1 year ago

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:
* First I would like to merge only the JSONField change. I am not sure if everything regarding that is already in [Testing JSONField replacement requirement #90](https://github.com/adamcharnock/django-hordak/pull/90) (maybe some documentation?). If yes, I would only request @nitsujri to rebase against current master and I will test it with my project and merge it.

* Then if needed I would like to merge the small corrections independent on this PR. I see the transition to `hordak.models.core.default_currencies` in `0033_alter_account_currencies.py` and `0034_alter_account_currencies_alter_leg_amount_currency.py` which might be merged separately and @nitsujri is having issue with that in [Correction to Migration mobile-power/django-hordak#1](https://github.com/mobile-power/django-hordak/pull/1), so maybe it would be worth to clear that up in separate PR.

* And finally I would merge the transition to MySQL

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

nitsujri commented 1 year ago

Thanks guys. I updated https://github.com/adamcharnock/django-hordak/pull/90 and got it ready for merging w/ the fix included.

For the currencies join table, I agree it is beyond the scope of this PR. Admittedly, I am skeptical of the benefits of the change, but happy to work through it in another PR.

PetrDlouhy commented 1 year ago

I have merged the JSONField changes from @nitsujri now.

@Joshun Can you please rebase this on current master branch?

Joshun commented 1 year ago

I have merged the JSONField changes from @nitsujri now.

@Joshun Can you please rebase this on current master branch?

I'll have a go but I might need a hand because there have been so many changes since

Joshun commented 1 year ago

Looks like some of the procedures have changed further e.g. check_leg_and_account_currency_match so I need to modify the MySQL version also - is there anything else like that I need to be aware of that could catch me out? Ideally this change would've been left until after this had been merged in as rebasing is already a big job as it is

Joshun commented 1 year ago

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)

PetrDlouhy commented 1 year ago

@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 django-hordak with this change to run automated tests for my project. The first problem I bumped into is with the mysqlclient dependency. It is unnecessary for my project and it even breaks it:

#17 23.23   ChefBuildError
#17 23.23 
#17 23.23   Backend subprocess exited when trying to invoke get_requires_for_build_wheel
#17 23.23   
#17 23.23   /bin/sh: line 1: mysql_config: command not found
#17 23.23   /bin/sh: line 1: mariadb_config: command not found
#17 23.23   /bin/sh: line 1: mysql_config: command not found
#17 23.23   mysql_config --version
#17 23.23   mariadb_config --version
#17 23.23   mysql_config --libs
#17 23.23   Traceback (most recent call last):
#17 23.23     File "/usr/local/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
#17 23.23       main()
#17 23.23     File "/usr/local/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 335, in main
#17 23.23       json_out['return_val'] = hook(**hook_input['kwargs'])
#17 23.23                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#17 23.23     File "/usr/local/lib/python3.11/site-packages/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
#17 23.23       return hook(config_settings)
#17 23.23              ^^^^^^^^^^^^^^^^^^^^^
#17 23.23     File "/tmp/tmpppbn50dt/.venv/lib/python3.11/site-packages/setuptools/build_meta.py", line 341, in get_requires_for_build_wheel
#17 23.23       return self._get_build_requires(config_settings, requirements=['wheel'])
#17 23.23              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#17 23.23     File "/tmp/tmpppbn50dt/.venv/lib/python3.11/site-packages/setuptools/build_meta.py", line 323, in _get_build_requires
#17 23.23       self.run_setup()
#17 23.23     File "/tmp/tmpppbn50dt/.venv/lib/python3.11/site-packages/setuptools/build_meta.py", line 488, in run_setup
#17 23.23       self).run_setup(setup_script=setup_script)
#17 23.23             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#17 23.23     File "/tmp/tmpppbn50dt/.venv/lib/python3.11/site-packages/setuptools/build_meta.py", line 338, in run_setup
#17 23.23       exec(code, locals())
#17 23.23     File "<string>", line 15, in <module>
#17 23.23     File "/tmp/tmp8zm2ecub/mysqlclient-2.1.0/setup_posix.py", line 70, in get_config
#17 23.23       libs = mysql_config("libs")
#17 23.23              ^^^^^^^^^^^^^^^^^^^^
#17 23.23     File "/tmp/tmp8zm2ecub/mysqlclient-2.1.0/setup_posix.py", line 31, in mysql_config
#17 23.23       raise OSError("{} not found".format(_mysql_config_path))
#17 23.23   OSError: mysql_config not found
#17 23.23   
#17 23.23 
#17 23.23   at /usr/local/lib/python3.11/site-packages/poetry/installation/chef.py:147 in _prepare
#17 23.25       143│ 
#17 23.27       144│                 error = ChefBuildError("\n\n".join(message_parts))
#17 23.27       145│ 
#17 23.28       146│             if error is not None:
#17 23.28     → 147│                 raise error from None
#17 23.28       148│ 
#17 23.28       149│             return path
#17 23.28       150│ 
#17 23.28       151│     def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path:
#17 23.28 
#17 23.28 Note: This error originates from the build backend, and is likely not a problem with poetry but with mysqlclient (2.1.0) not supporting PEP 517 builds. You can verify this by running 'pip wheel --use-pep517 "mysqlclient (==2.1.0)"'.
#17 23.28

Can you please remove the dependency so I can proceed with the tests?

Joshun commented 1 year ago

@PetrDlouhy that error is because you need mysql-dev or mariadb-dev installed for this library. Would it be best to make it an optional dependency like pip install hordak[mysql]? Or remove it from requirements.txt and some documentation to say which dependencies are needed for whether you are planning on Postgres/MySQL?

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?

PetrDlouhy commented 1 year ago

@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 pip install hordak[mysql]. If django-hordak would need only mysqlclient, I would expect it would be enough to mention it in the documentation.

For the CI you could modify the Action.

PetrDlouhy commented 1 year ago

@Joshun You also wouldn't want to introduce postgresql dependencies to your project, so you can also remove the psycopg2-binary dependency in similar way.

Joshun commented 1 year ago

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.

PetrDlouhy commented 1 year ago

@Joshun Yeah, you can move the mysql and psycopg to the requirements-test.txt which is for the testing (GitHub Actions and local).

Joshun commented 1 year ago

@PetrDlouhy ok have just done this

PetrDlouhy commented 1 year ago

@Joshun All tests ran well, and everything seems OK. Only the psycopg2-binary dependency in requirements.txt. Don't you think it should be moved to requirements_test.txt?

Joshun commented 1 year ago

@Joshun All tests ran well, and everything seems OK. Only the psycopg2-binary dependency in requirements.txt. Don't you think it should be moved to requirements_test.txt?

Oops missed that, have just done that also

Joshun commented 1 year ago

Looks like pyscopg is needed for mypy CI to function?

PetrDlouhy commented 1 year ago

@Joshun Yes. You probably can install whole requirements_test.txt in that step.

PetrDlouhy commented 1 year ago

It seems very nice now. I will pull this PR and make new version.

PetrDlouhy commented 1 year ago

@Joshun I forgot about the documentation. Could you please make a new PR with update of CHANGELOG.txt and update of this page: https://django-hordak.readthedocs.io/en/latest/ To include info about the MySQL support?

Joshun commented 1 year ago

@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 commented 1 year ago

@Joshun I forgot about the documentation. Could you please make a new PR with update of CHANGELOG.txt and update of this page: https://django-hordak.readthedocs.io/en/latest/ To include info about the MySQL support?

Ok will do this shortly I might also add the fix for the CI here too

adamcharnock commented 1 week ago

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 transaction.on_commit). My (perhaps hard-nosed) attitude is that if people deploying Hordak want advanced database features then I would prefer they work towards using a database that supports these features.

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'.

adamcharnock / django-hordak

Initial MySQL support (early draft) - JSONField version #91

Codecov Report