-
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 31 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(): | ||
|
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 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 commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure it is.
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. @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. 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. @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).
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. Could you share the stacktrace 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. 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. Using this as the
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. @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. |
||
| 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) | ||

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.
Okay, maybe I'm just being dense here, but won't this still be a problem if I have an existing site and want to later add a foreign key that references this model? Because my existing site will have this primary key in the database as type "unsigned bigint". But the code here says it's actually an "bigint" now. So then when I make a new model that has a foreign key to
PersistentSubsectionGrade, won't the migration try to generate that foreign key as type "bigint"? Which will work fine on new installs, but blow up if I have an older site?FYI: @feanil, @bmedx
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.
I know you went over this before in an earlier thread, but I think there's something I'm just not understanding properly.
Thank you for your patience.
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.
@ormsbee That seems correct to me, but I haven't tested this kind of thing in years.
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.
@ormsbee I have tested it with mysql and it is not treating it as type change, I also have updated the underlaying migrations to use this type so that it doesn't cause any discrepancy for new installs. for existing users these migrations also not going to trigger even so it seems safe.
If you have any particular scenario please let me know.