feat: CourseKeyField can now be optionally case-sensitive, has default max_length [FC-0117]

[bradenmacdonald]

Our CourseKeyField and other opaque key fields by default are case-sensitive on SQLite but case-insensitive on MySQL. This opposite behavior can result in surprising bugs if you only test on SQLite.

This PR:

• Changes CourseKeyField so that it's case-insensitive by default on both SQLite and MySQL, matching the previous MySQL default behavior
  • This is technically a breaking change for SQLite, but not a breaking change for MySQL users, which is every production use case.
  • This will not result in any additional/changed migrations. As a result, it does not apply retroactively on any long-running SQLite databases; I don't think we have any though.
  • I suspect this may trigger a change in collation of some columns if you create a future migration that modifies some other aspect of these columns, such as blank or max_length ? e.g. if your database was still using utf8_general_ci by default and you make some unrelated change that results in an ALTER TABLE migration that updates one of these columns, it may change the collation to utf8mb4_unicode_ci. Since opaque keys are restricted to mostly ASCII characters anyways, I don't think this will matter, if it even happens. (?)
• Allows apps to opt-in to case-sensitivity with CourseKeyField(case_sensitive=True), which is highly recommended, and will apply to both SQLite and MySQL consistently.
• Specifies a default value for max_length so you don't have to pointlessly specify the same max length over and over every time you store an opaque key field.
• Does not yet support PostgreSQL - the case_sensitive=True argument will be ignored. But I think that PostgreSQL will always be case sensitive by default, and doesn't ship with any case-insensitive collations, although you can manually create them. (ref)

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[openedx-webhooks]

Thanks for the pull request, @bradenmacdonald!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

• If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
  • This process (including the steps you'll need to take) is documented here.
• If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

• Dependencies

  This PR must be merged before / after / at the same time as ...

• Blockers

  This PR is waiting for OEP-1234 to be accepted.

• Timeline information

  This PR must be merged by XX date because ...

• Partner information

  This is for a course on edx.org.

• Supporting documentation
• Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Details

---

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

• The size and impact of the changes that it introduces
• The need for product review
• Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.13%. Comparing base (542bdeb) to head (b7937f7).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #426      +/-   ##
==========================================
+ Coverage   93.93%   94.13%   +0.19%     
==========================================
  Files          31       31              
  Lines        3036     3068      +32     
  Branches      191      191              
==========================================
+ Hits         2852     2888      +36     
+ Misses        157      155       -2     
+ Partials       27       25       -2     

Flag	Coverage Δ
unittests	94.13% <100.00%> (+0.19%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kdmccormick]

Thanks for this, I think it's a great improvement. I didn't see any issues other than my postgres comment. I would want @ormsbee to take a look at this before merging, though.

[kdmccormick]

I keep seeing PRs about adding/fixing postgres support to the platform, so I wonder if it's worth making the case-insensitivity work on postgres too. At the very least, could you add your postgres note from the PR description here?

[bradenmacdonald]

From my research, getting case-insensitivity to work on PostgreSQL is not possible without a migration to create a case-insensitive collation (once, for the whole database), which we could then use. Since this library has no migrations, I don't think we should do that.

I can definitely add the note here though.

[bradenmacdonald]

I have a PR here to validate this before merging, but it's held up by some docker rate limit issues.

openedx/openedx-platform#38044

[bradenmacdonald]

OK, the validation PR caught another test where the tests were passing on SQLite but would have failed on MySQL due to different sorting of course IDs (lms/djangoapps/course_api/tests/test_views.py::CourseListSearchViewTest::test_too_many_courses)

So I think that's a good sign. Otherwise, I don't see any signs of unexpected side effects, so I think this PR is good to merge.

[kdmccormick]

Could you add that note about postgres to the code? With that added, merge away 🚀

[bradenmacdonald]

@kdmccormick I did already? 293058a

[kdmccormick]

Ah, sorry, I was looking for the comment in the db_params docstring, I didn't look closely enough at your commits. LGTM.