Database inspection capability - fixes #13 #90

MattFisher · 2021-12-20T04:43:50Z

Subject: Get inspectdb working

Feature or Bugfix

Bugfix

Purpose

Implement database inspection methods necessary for inspectdb management command to work.

Redshift is based on PostgreSQL 8.x, but Django supports PostgreSQL >= 9.4, so some of the more modern features of PostgreSQL such as unnest and array_agg are not available in Redshift. Django uses some of these in the postgres backend introspection methods, so different implementations were needed to retrieve the same information from a Redshift database.

Allow use with django-sql-explorer, which uses get_table_description to provide its schema view.

This has been broken since Django 3.2 introduced support for collations in the postgres backend. Relevant commit

Detail

Implementation of the changes needed for get_table_description was straightforward, and involved removing the join to the pg_collation table from the SQL.

The changes required to get_constraints, used by inspectdb, were more complex. The postgres version uses unnest, but there is no good substitute for this function in Redshift.
Rather than attempting to build an overly complex query to achieve the same output as the postgres backend, a second query is made to the pg_attribute table and the join made in python. This is done twice within the get_constraints method, once for constraints and again for indexes.

These changes are difficult to write tests for, as the current test suite does not assume a connection to a Redshift cluster, and using a database cursor in a test requires a working database (pytest will enforce use of a db fixture).
The tests as written mock the cursor, but this has the disadvantage of having to hard code expected SQL and return values, which leaves the tests very brittle.

I have not attempted to implement the orders, type, definition, or options fields within get_constraints. I don't know where these are used or if they're necessary.

Relates

manage.py inspectdb: All redshift tables fail with unable to inspect table - function unnest(int2vector) does not exist #13

shimizukawa · 2021-12-20T10:24:14Z

Thank you for your PR!
I'll take a look in a week (or around new year holiday)

codecov · 2021-12-20T19:55:44Z

Codecov Report

Merging #90 (a6e1015) into master (4fd73cd) will increase coverage by 2.08%.
The diff coverage is 92.30%.

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

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
+ Coverage   47.01%   49.09%   +2.08%     
==========================================
  Files           3        3              
  Lines         251      277      +26     
  Branches       62       79      +17     
==========================================
+ Hits          118      136      +18     
- Misses        123      130       +7     
- Partials       10       11       +1

Impacted Files	Coverage Δ
django_redshift_backend/base.py	`48.27% <92.30%> (+4.87%)`	⬆️
django_redshift_backend/__init__.py	`50.00% <0.00%> (-50.00%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fd73cd...a85bd90. Read the comment docs.

MattFisher · 2021-12-21T02:09:35Z

It might also be worth adding supports_collation_on_charfield = False and supports_collation_on_textfield = False to the DatabaseFeatures class, but I haven't looked into this enough to discover what it affects or how to test it.

From https://docs.djangoproject.com/en/4.0/releases/3.2/#database-backend-api

Third-party database backends must implement support for column database collations on CharFields and TextFields or set DatabaseFeatures.supports_collation_on_charfield and DatabaseFeatures.supports_collation_on_textfield to False. If non-deterministic collations are not supported, set supports_non_deterministic_collations to False.

shimizukawa

Thanks, it worked on my Redshift environment!

shimizukawa · 2022-02-14T20:55:14Z

django_redshift_backend/base.py

@@ -553,6 +555,162 @@ class DatabaseCreation(BasePGDatabaseCreation):
    pass


+class DatabaseIntrospection(BasePGDatabaseIntrospection):
+    pass


nits: I'll remove this pass line after merging

shimizukawa · 2022-02-14T20:56:50Z

django_redshift_backend/base.py

+        # contain details of materialized views.
+
+        # This function is based on the version from the Django postgres backend
+        # from before support for collations were introduced in Django 3.2


nits: I'll add a URL to original django source after merging:

Suggested change

# from before support for collations were introduced in Django 3.2

# from before support for collations were introduced in Django 3.2

# https://github.com/django/django/blob/3.1.14/django/db/backends/

# postgresql/introspection.py#L66-L94

shimizukawa · 2022-02-14T20:57:20Z

django_redshift_backend/base.py

+        one or more columns. Also retrieve the definition of expression-based
+        indexes.
+        """
+        # Based on code from Django 3.2


nits: I'll add a URL to original django source after merging:

Suggested change

# Based on code from Django 3.2

# Based on code from Django 3.2

# https://github.com/django/django/blob/3.2.12/django/db/backends/

# postgresql/introspection.py#L148-L182

shimizukawa · 2022-02-14T20:57:49Z

django_redshift_backend/base.py

+            }
+
+        # Now get indexes
+        # Based on code from Django 1.7


nits: I'll add a URL to original django source after merging:

Suggested change

# Based on code from Django 1.7

# Based on code from Django 1.7

# https://github.com/django/django/blob/1.7.11/django/db/backends/

# postgresql_psycopg2/introspection.py#L182-L207

shimizukawa · 2022-02-14T21:33:51Z

These changes are difficult to write tests for, as the current test suite does not assume a connection to a Redshift cluster, and using a database cursor in a test requires a working database (pytest will enforce use of a db fixture).
The tests as written mock the cursor, but this has the disadvantage of having to hard code expected SQL and return values, which leaves the tests very brittle.

Yes, it is currently difficult to provide a connection to Redshift for automated testing.
As an alternative, I have set up postgres on GitHub Actions so that we can do a simple test on #93.
I don't think it works for this case yet, but I hope to make it work with postgres in the future.

Anyway, thanks to this PR, we can now use inspectdb with django-redshift-backend, and I've prepared a project showcase in the examples directory, so I'll provide django-sql-explorer case later.

Thanks for your awesome contribution!

shimizukawa · 2022-02-16T02:24:14Z

As an alternative, I have set up postgres on GitHub Actions so that we can do a simple test on #93.
I don't think it works for this case yet, but I hope to make it work with postgres in the future.

done by #95 ;)

shimizukawa · 2022-02-27T01:35:50Z

It has been released https://pypi.org/project/django-redshift-backend/3.0.0/

MattFisher added 8 commits December 20, 2021 11:24

Subclass DatabaseIntrospection to implement get_table_description

c148720

Implement Redshift-compatible get_constraints

0a90836

IntrospectionTest

54fe69e

Better variable naming

d16e9a3

Extract get_attribute_number_to_name_map_for_table

b25e73f

Tests for get_constraints

6cf3908

Tidy

9a8e794

Revert accidental change to tox.ini

a6e1015

MattFisher added 5 commits December 21, 2021 09:15

Tweak norm_sql function to retain single spaces

cbe82e7

More reliable handling of mock calls in tests

5dfc3ed

Remove trailing whitespace

2126c44

Flake8

72dbbf3

More Flake8

a85bd90

shimizukawa approved these changes Feb 14, 2022

View reviewed changes

shimizukawa merged commit e2aa6cd into jazzband:master Feb 14, 2022

shimizukawa added a commit that referenced this pull request Feb 14, 2022

update CHANGES for #13, #90

58981ac

shimizukawa mentioned this pull request Feb 14, 2022

manage.py inspectdb: All redshift tables fail with unable to inspect table - function unnest(int2vector) does not exist #13

Closed

shimizukawa mentioned this pull request Feb 16, 2022

add inspectdb test #95

Merged

shimizukawa mentioned this pull request Feb 26, 2022

sqlmigrate for drop table cause exception #8

Closed

shimizukawa mentioned this pull request Mar 14, 2022

add django-sql-explorer for examples #99

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Database inspection capability - fixes #13 #90

Database inspection capability - fixes #13 #90

MattFisher commented Dec 20, 2021 •

edited

Loading

shimizukawa commented Dec 20, 2021

codecov bot commented Dec 20, 2021 •

edited

Loading

MattFisher commented Dec 21, 2021

shimizukawa left a comment

shimizukawa Feb 14, 2022

shimizukawa Feb 14, 2022

shimizukawa Feb 14, 2022

shimizukawa Feb 14, 2022

shimizukawa commented Feb 14, 2022

shimizukawa commented Feb 16, 2022

shimizukawa commented Feb 27, 2022

Database inspection capability - fixes #13 #90

Database inspection capability - fixes #13 #90

Conversation

MattFisher commented Dec 20, 2021 • edited Loading

Feature or Bugfix

Purpose

Detail

Relates

shimizukawa commented Dec 20, 2021

codecov bot commented Dec 20, 2021 • edited Loading

Codecov Report

MattFisher commented Dec 21, 2021

shimizukawa left a comment

Choose a reason for hiding this comment

shimizukawa Feb 14, 2022

Choose a reason for hiding this comment

shimizukawa Feb 14, 2022

Choose a reason for hiding this comment

shimizukawa Feb 14, 2022

Choose a reason for hiding this comment

shimizukawa Feb 14, 2022

Choose a reason for hiding this comment

shimizukawa commented Feb 14, 2022

shimizukawa commented Feb 16, 2022

shimizukawa commented Feb 27, 2022

MattFisher commented Dec 20, 2021 •

edited

Loading

codecov bot commented Dec 20, 2021 •

edited

Loading