-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: migrations to make postgresql compatible. #35762
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
base: master
Are you sure you want to change the base?
Changes from 39 commits
70bbd68
c222853
fe65206
2335e69
4fd6ca0
1c4295c
60c4830
0a9cd28
00b235a
2614432
8c21658
d096773
63875d9
78c2209
fc80ce2
c739e41
c0f0695
03b4b00
43938ac
1f5c9d0
499c852
558f5ad
22dc5f9
139b8a6
cdd8403
42f9b3c
5e73119
023e5b0
30811ef
6e11456
c44cd5d
d4049f3
7df2ae2
aaa8a71
c4f859c
edcad98
c1c287c
f48ad00
41555c9
4ac2812
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ | |
| Common initialization app for the LMS and CMS | ||
| """ | ||
|
|
||
|
|
||
| from django.apps import AppConfig | ||
| from django.db import connection | ||
|
|
||
|
|
||
| class CommonInitializationConfig(AppConfig): # lint-amnesty, pylint: disable=missing-class-docstring | ||
|
|
@@ -14,6 +14,7 @@ def ready(self): | |
| # Common settings validations for the LMS and CMS. | ||
| from . import checks # lint-amnesty, pylint: disable=unused-import | ||
| self._add_mimetypes() | ||
| self._add_required_adapters() | ||
|
|
||
| @staticmethod | ||
| def _add_mimetypes(): | ||
|
|
@@ -26,3 +27,21 @@ def _add_mimetypes(): | |
| mimetypes.add_type('application/x-font-opentype', '.otf') | ||
| mimetypes.add_type('application/x-font-ttf', '.ttf') | ||
| mimetypes.add_type('application/font-woff', '.woff') | ||
|
|
||
| @staticmethod | ||
| def _add_required_adapters(): | ||
| """ | ||
| Register CourseLocator in psycopg2 extensions | ||
| :return: | ||
| """ | ||
| if 'postgresql' in connection.vendor.lower(): | ||
| from opaque_keys.edx.locator import CourseLocator, LibraryLocator, BlockUsageLocator | ||
| from psycopg2.extensions import QuotedString, register_adapter | ||
|
|
||
| def adapt_course_locator(course_locator): | ||
| return QuotedString(course_locator._to_string()) # lint-amnesty, pylint: disable=protected-access | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Why does
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well it was breaking with postgresql<14 only for CourseLocator as there as special character involved. However now I have tested with postgresql14 and it is even working without this change. I am going to remove this change.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I mean why
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ormsbee because each locator class contains it's own implementation. |
||
|
|
||
| # Register the adapter | ||
| register_adapter(CourseLocator, adapt_course_locator) | ||
| register_adapter(LibraryLocator, adapt_course_locator) | ||
| register_adapter(BlockUsageLocator, adapt_course_locator) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,45 +1,42 @@ | ||
| from django.db import migrations, models, connection | ||
|
|
||
| def table_description(): | ||
| """Handle Mysql/Pg vs Sqlite""" | ||
| # django's mysql/pg introspection.get_table_description tries to select * | ||
| # from table and fails during initial migrations from scratch. | ||
| # sqlite does not have this failure, so we can use the API. | ||
| # For not-sqlite, query information-schema directly with code lifted | ||
| # from the internals of django.db.backends.mysql.introspection.py | ||
|
|
||
| """Handle MySQL/Postgres vs SQLite compatibility for table introspection""" | ||
| if connection.vendor == 'sqlite': | ||
| fields = connection.introspection.get_table_description(connection.cursor(), 'course_overviews_courseoverview') | ||
| return [f.name for f in fields] | ||
| else: | ||
| cursor = connection.cursor() | ||
| cursor.execute(""" | ||
| SELECT column_name | ||
| FROM information_schema.columns | ||
| WHERE table_name = 'course_overviews_courseoverview' AND table_schema = DATABASE()""") | ||
| if connection.vendor == 'mysql': | ||
| cursor.execute(""" | ||
| SELECT column_name | ||
| FROM information_schema.columns | ||
| WHERE table_name = 'course_overviews_courseoverview' AND table_schema = DATABASE() | ||
| """) | ||
| elif connection.vendor == 'postgresql': | ||
| cursor.execute(""" | ||
| SELECT column_name | ||
| FROM information_schema.columns | ||
| WHERE table_name = 'course_overviews_courseoverview' AND table_catalog = current_database() | ||
| """) | ||
| rows = cursor.fetchall() | ||
| return [r[0] for r in rows] | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('course_overviews', '0008_remove_courseoverview_facebook_url'), | ||
| ] | ||
|
|
||
| # An original version of 0008 removed the facebook_url field We need to | ||
| # handle the case where our noop 0008 ran AND the case where the original | ||
| # 0008 ran. We do that by using the standard information_schema to find out | ||
| # what columns exist. _meta is unavailable as the column has already been | ||
| # removed from the model | ||
| operations = [] | ||
| fields = table_description() | ||
|
|
||
| # during a migration from scratch, fields will be empty, but we do not want to add | ||
| # an additional facebook_url | ||
| # Ensure 'facebook_url' is added if it does not exist in the table | ||
| if fields and not any(f == 'facebook_url' for f in fields): | ||
| operations += migrations.AddField( | ||
| model_name='courseoverview', | ||
| name='facebook_url', | ||
| field=models.TextField(null=True), | ||
| ), | ||
| operations.append( | ||
| migrations.AddField( | ||
| model_name='courseoverview', | ||
| name='facebook_url', | ||
| field=models.TextField(null=True), | ||
| ) | ||
| ) | ||
|
Comment on lines
+36
to
+42
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is an unrelated bug (even if it's necessary for pg to work), I would put it in a separate, neutral, bugfix PR that should get merged before the new stuff. From the look of the code, you might argue that there is already pg-specific code and at some point in the past pg probably was supported and that support was never really properly dropped (or there wouldn't be any pg-specific code left). So this whole PR is actually just a bugfix, making pg work properly again. But I'm not sure the maintainers see it this way :-).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am going to take care of it in other PR, as it is not tightly coupled with this change. |
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why is this necessary for course locators and not other opaque key types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure it is.
LibraryLocatorat least also seems to require it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AntonOfTheWoods is it breaking for you? Because I have tested it with postgresql 14 and it working fine for me so I have reverted that change from CourseLocator.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@qasimgulzar It's not working fine for me on PG18 with psycopg3 when I try and import the demo course (manually in a pod, obviously not using tutor). Both are the officially recommended "best" versions for each project, and should be fine for Django.
Django says psycopg3 is the recommended and psycopg2 might be deprecated (soon?).
I had a play around trying to add the adapter where you were doing it but it wasn't getting picked up when running commands (was working fine when running a shell). Mr Sonnet (the 4.5th) suggested using a DB Wrapper instead, and that works. It also seemed like a good way to isolate pg-specific stuff, so I thought why not. It seems to get much further now, though there is still a slight issue trying to import the demo course (the demo library imports fine with that change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you share the stacktrace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this as the
engineseems to make that issue go awayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AntonOfTheWoods i believe once this PR is merged you can add this wrapper as a separate PR, or even feel free to create PR against this one.