Skip to content
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

Add support for Django 4.1 #111

Closed
wants to merge 3 commits into from
Closed

Add support for Django 4.1 #111

wants to merge 3 commits into from

Conversation

fizyk
Copy link
Contributor

@fizyk fizyk commented Nov 16, 2022

* Strip IDENTITY related suffixes auto fields are getting on Django 4.1.
  These suffixes are upsetting redshift.

Subject: Django 4.1 support

Feature or Bugfix

  • Feature

Purpose

  • Add support for Django 4.1

Detail

  • Clearing DatabaseWrapper.data_types_suffix which, on DJango 4.1, are responsible for adding "GENERATED BY DEFAULT AS IDENTITY" to auto fields, and which are upsetting Redshift. Actually makes impossible to create new table or django_migration table.

Relates

@fizyk
Copy link
Contributor Author

fizyk commented Nov 17, 2022

pre-commit is... miscofngured

@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #111 (2042b72) into master (3173766) will decrease coverage by 0.38%.
The diff coverage is 100.00%.

❗ Current head 2042b72 differs from pull request most recent head a7b1dc7. Consider uploading reports for the commit a7b1dc7 to get more accurate results

@@            Coverage Diff             @@
##           master     #111      +/-   ##
==========================================
- Coverage   62.38%   61.99%   -0.39%     
==========================================
  Files           3        3              
  Lines         420      421       +1     
  Branches      116      116              
==========================================
- Hits          262      261       -1     
- Misses        116      117       +1     
- Partials       42       43       +1     
Impacted Files Coverage Δ
django_redshift_backend/base.py 62.28% <100.00%> (-0.41%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

    * Strip IDENTITY related suffixes auto fields are getting on Django 4.1.
      These suffixes are upsetting redshift.
@fizyk
Copy link
Contributor Author

fizyk commented Nov 24, 2022

Updated documentation and setup.cfg as well

exclude:
- django-version: '4.0'
python-version: '3.7'
- django-version: '4.1'
python-version: '3.7'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memo: django 4.1 supports python 3.8 or later
https://docs.djangoproject.com/en/4.1/releases/4.1/

@@ -17,6 +17,7 @@ python =
DJANGO =
3.2: dj32
4.0: dj40
4.1: dj41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need dj41 in envlist (line 2) and deps (line 32)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@shimizukawa
Copy link
Member

IDENTITY related suffixes auto fields are getting on Django 4.1

The suffix related to IDENTITY appeared because Django 4.1 now creates AutoField with integer GENERATED BY DEFAULT AS IDENTITY available since postgres 10 instead of serial.
https://github.com/django/django/blob/bc3b8f152452ba0e41f28baa93c0bf8f39cddb09/django/db/backends/postgresql/schema.py#L44-L47

Therefore, the current PR will create tables with integer instead of serial, which is a different behavior than in django-4.0 and earlier.
I've been investigating for a couple of hours and haven't found a good fix yet. Please let me know if there is a way to fix it.

@fizyk
Copy link
Contributor Author

fizyk commented Nov 26, 2022

Hmmm... I thought the serial stayed since the tests checked the resulting SQL and without the suffixes it was the same as on earlier Django versions.

@fizyk
Copy link
Contributor Author

fizyk commented Dec 1, 2022

Where should I look? The AutoField data_type was already overriden by redshift-engine to integer identity(1, 1) so i guess somewhere is a part that also needs to be changed for postgresql to either vendor, or ditch the serial in favour of a modern GENERATED BY DEFAULT AS IDENTITY 🤔

@fizyk
Copy link
Contributor Author

fizyk commented Dec 1, 2022

@shimizukawa the way I see it is that the tests running directly on Postgres will become increasingly hard to maintain with each PostgreSQL/Django release (and redshift evolves too).

Maybe the best approach would be to fork the whole code from Django 4.0.x instead of inheriting from the Postgres engine from whatever Django is there underneath. Plus the Postgres implementation and redshift will become increasingly distinct 🤔

I tried to pinpoint what has changed between 4.0.x and 4.1.x but all I got was additional IntegerField generated with primary_key set to True. (copied get_field_type from 4.0.x)

@shimizukawa
Copy link
Member

Thank you @fizyk,
I apologize for the long delay. I was able to complete the implementation of django-4.2 support based on your proposal to vend django code base.
I'm closing this PR now that Django-4.2 support is complete.
Thank you for your great contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants