search

3YOURMIND / django-add-default-value

This django Migration Operation can be used to transfer a Fields default value to the database scheme.
Apache License 2.0
138 stars 19 forks source link

Add CockroachDB support #33

Closed mlazowik closed 3 years ago

mlazowik commented 3 years ago

Fixes #32

Tested by running: docker run -it -p 26257:26257 cockroachdb/cockroach:v20.2.8 start-single-node --insecure in one terminal, and tox . -e 'py38-django{22,30,31,32}-crdb' in another.

mlazowik commented 3 years ago 
(django-add-default-value) michal@whoooosh ~/p/q/django-add-default-value> tox . -e 'py38-django{22,30,31,32}-crdb'                                                                                                                                                      2 crdb!
GLOB sdist-make: /Users/michal/projects/qed/django-add-default-value/setup.py
py38-django22-crdb inst-nodeps: /Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip
py38-django22-crdb installed: Django==2.2.20,django-add-default-value @ file:///Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip,django-cockroachdb==2.2.3,psycopg2-binary==2.8.6,pytz==2021.1,sqlparse==0.4.1
py38-django22-crdb run-test-pre: PYTHONHASHSEED='2839153836'
py38-django22-crdb run-test: commands[0] | /Users/michal/projects/qed/django-add-default-value/.tox/py38-django22-crdb/bin/python manage.py test tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
/Users/michal/projects/qed/django-add-default-value/.tox/py38-django22-crdb/lib/python3.8/site-packages/django_add_default_value/add_default_value.py:77: UserWarning: AddDefaultValue cannot be applied on a non-supported vendor.
  warnings.warn(
......s.ssssssssssssssssssssssss
----------------------------------------------------------------------
Ran 32 tests in 0.252s

OK (skipped=25)
Destroying test database for alias 'default'...
py38-django30-crdb create: /Users/michal/projects/qed/django-add-default-value/.tox/py38-django30-crdb
py38-django30-crdb installdeps: Django>=3.0,<3.0.99, psycopg2-binary, django-cockroachdb>=3.0,<3.0.99
py38-django30-crdb inst: /Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip
py38-django30-crdb installed: asgiref==3.3.4,Django==3.0.14,django-add-default-value @ file:///Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip,django-cockroachdb==3.0.3,psycopg2-binary==2.8.6,pytz==2021.1,sqlparse==0.4.1
py38-django30-crdb run-test-pre: PYTHONHASHSEED='2839153836'
py38-django30-crdb run-test: commands[0] | /Users/michal/projects/qed/django-add-default-value/.tox/py38-django30-crdb/bin/python manage.py test tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
/Users/michal/projects/qed/django-add-default-value/.tox/py38-django30-crdb/lib/python3.8/site-packages/django_add_default_value/add_default_value.py:77: UserWarning: AddDefaultValue cannot be applied on a non-supported vendor.
  warnings.warn(
......s.ssssssssssssssssssssssss
----------------------------------------------------------------------
Ran 32 tests in 0.392s

OK (skipped=25)
Destroying test database for alias 'default'...
py38-django31-crdb create: /Users/michal/projects/qed/django-add-default-value/.tox/py38-django31-crdb
py38-django31-crdb installdeps: Django>=3.1,<3.1.99, psycopg2-binary, django-cockroachdb>=3.1,<3.1.99
py38-django31-crdb inst: /Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip
py38-django31-crdb installed: asgiref==3.3.4,Django==3.1.8,django-add-default-value @ file:///Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip,django-cockroachdb==3.1.3,psycopg2-binary==2.8.6,pytz==2021.1,sqlparse==0.4.1
py38-django31-crdb run-test-pre: PYTHONHASHSEED='2839153836'
py38-django31-crdb run-test: commands[0] | /Users/michal/projects/qed/django-add-default-value/.tox/py38-django31-crdb/bin/python manage.py test tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
/Users/michal/projects/qed/django-add-default-value/.tox/py38-django31-crdb/lib/python3.8/site-packages/django_add_default_value/add_default_value.py:77: UserWarning: AddDefaultValue cannot be applied on a non-supported vendor.
  warnings.warn(
......s.ssssssssssssssssssssssss
----------------------------------------------------------------------
Ran 32 tests in 0.305s

OK (skipped=25)
Destroying test database for alias 'default'...
py38-django32-crdb inst-nodeps: /Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip
py38-django32-crdb installed: asgiref==3.3.4,Django==3.2,django-add-default-value @ file:///Users/michal/projects/qed/django-add-default-value/.tox/.tmp/package/1/django-add-default-value-0.8.0.zip,django-cockroachdb==3.2,psycopg2-binary==2.8.6,pytz==2021.1,sqlparse==0.4.1
py38-django32-crdb run-test-pre: PYTHONHASHSEED='2839153836'
py38-django32-crdb run-test: commands[0] | /Users/michal/projects/qed/django-add-default-value/.tox/py38-django32-crdb/bin/python manage.py test tests
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
/Users/michal/projects/qed/django-add-default-value/.tox/py38-django32-crdb/lib/python3.8/site-packages/django_add_default_value/add_default_value.py:77: UserWarning: AddDefaultValue cannot be applied on a non-supported vendor.
  warnings.warn(
......s.ssssssssssssssssssssssss
----------------------------------------------------------------------
Ran 32 tests in 0.377s

OK (skipped=25)
Destroying test database for alias 'default'...
____________________________________________________________________________________________________________________________________ summary ____________________________________________________________________________________________________________________________________
  py38-django22-crdb: commands succeeded
  py38-django30-crdb: commands succeeded
  py38-django31-crdb: commands succeeded
  py38-django32-crdb: commands succeeded
  congratulations :)
flixx commented 3 years ago

Hello @mlazowik,

thanks a lot for your contribution. I'll give it a review later this week. On the first sight it looks good :)

Felix

flixx commented 3 years ago

Very nice @mlazowik! Thanks work working into the project logic :)

I have only one remark: While testing I'll see this warning:

add_default_value.py:90: UserWarning: You requested a default for a field / database combination that does not allow one. The default will not be set on: TestTextDefault.description.

It confuses me a bit that I don't see that warning in the test output you posted above. Also it confuses me a bit that the test for the TestTextDefault.description. field are passing.

I would guess that CockroachDB does support adding default values (However I don't know CockroachDB, so I am not sure). So I would assume that it is fine to add the vendor in the first vendor check in can_have_default_for_text()

What do you think?

Felix

mlazowik commented 3 years ago

Oh, huh, that is very strange indeed, I did not even notice that can_have_default_for_text() existed. Can you post the whole testing command you're using? I'll see if I can replicate.

I'll of course add CockroachDB to can_have_default_for_text either way.

mlazowik commented 3 years ago

Correction – there is an even "bigger" warning present in the test output I posted, but it's off screen, you need to scroll to see it. I somehow did not notice it too. Still puzzling why tests passed.

mlazowik commented 3 years ago

Hm, I'm looking into other places in the codebase where there are conditions on vendor (for example clean_value) and it looks like there might be more changes needed.

mlazowik commented 3 years ago

Ok, it looks like cockroach is doing some normalization, so even if it gets SET DEFAULT '0' it still prints the default value False.

Still, I'll add is_postgresql_like and make both PostgreSQL and CockroachDB pass that.

mlazowik commented 3 years ago

OK, ready :)

@flixx btw I think I know how to get rid of skipIf in the MigrateMixin – replace all TestCases with TransactionTestCase. Lmk if you want this bundled here or in a separate PR.

mlazowik commented 3 years ago

Ok, no, hold, still trying to figure out what's going on with tests.

mlazowik commented 3 years ago

Sorry for the spam. Somehow I'm getting the output of describe instead of the SQL 🤦

mlazowik commented 3 years ago

Phew, ok, fixed. I've misread the original output differences as expected, and committed them to tests. But what I committed was comments outputted by sqlmigrate. Now that cockroachdb is treated the same way as postgresql there is no need for custom overrides, a simple pass only.

Btw after this fix in tests, they do fail without adding crdb to can_have_default_for_text

flixx commented 3 years ago

Haha, thanks for ~the spam~ the fast response :D I'll give it another review early next week!

If you want to make further improvements, please on a seperate PR! Thanks already :)

Felix