fix: make database function compatible with postgresql and mysql both #260

qasimgulzar · 2024-11-18T09:41:01Z

Summary

This pull request includes updates to the StringAgg class in openedx_tagging/core/tagging/models/utils.py to enhance its functionality and compatibility with different database backends. The most important changes include adding support for PostgreSQL, MySQL, and SQLite, and implementing abstract methods from the Combinable class.

Enhancements to StringAgg class:

Added support for PostgreSQL by using STRING_AGG with a specified separator and ensuring expressions are cast to TEXT.
Added support for MySQL and SQLite by using GROUP_CONCAT with a specified separator.
Implemented abstract methods from the Combinable class to allow logical operations (AND, OR, XOR).

Additional changes:

Imported db_connection from django.db to check the database backend.
Updated the StringAgg class to accept a delimiter parameter and handle the distinct option and output type accordingly.

This PR is subsequent of openedx/edx-platform#35762

openedx-webhooks · 2024-11-18T09:41:05Z

Thanks for the pull request, @qasimgulzar!

This repository is currently maintained by @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:

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.

mphilbrick211 · 2024-12-09T18:10:47Z

Hi @qasimgulzar! Just flagging some failing checks for you.

qasimgulzar · 2024-12-09T18:13:00Z

Thank you I will be looking into it.

qasimgulzar · 2024-12-12T11:10:38Z

@mphilbrick211 please approve the actions

qasimgulzar · 2025-01-14T14:12:08Z

I am going to take care of these broken tests

mphilbrick211 · 2025-02-24T22:33:25Z

Hi @qasimgulzar! Just flagging there's a couple failing checks here.

qasimgulzar · 2025-02-25T07:21:46Z

It is on my radar for this week.

qasimgulzar · 2025-03-06T08:12:08Z

@mphilbrick211 could you please approve the workflows

mphilbrick211 · 2025-03-06T14:00:25Z

All set, @qasimgulzar!

mphilbrick211 · 2025-03-12T15:12:52Z

@openedx/axim-engineering - this is ready for review. Thanks!

ormsbee

@pomegranited, @navinkarkera, @bradenmacdonald: Do any of you folks want to take a look at this before I merge?

openedx_tagging/core/tagging/models/utils.py

pomegranited

I'm not going to block merging this, so please consider my comments as suggestions :)

openedx_tagging/core/tagging/models/utils.py

pomegranited · 2025-04-08T07:41:26Z

openedx_tagging/core/tagging/models/utils.py

+        if 'postgresql' in db_connection.vendor.lower():
+            self.function = 'STRING_AGG'
+            self.template = '%(function)s(%(distinct)s%(expressions)s, %(delimiter)s)'
+            extra.update({"delimiter": delimiter})


If delimiter cannot be customized for MySQL or SQLite, then I don't think it should be exposed as an argument to the constructor here.

Can we just hardcode it to ',' for postgres?

Suggested change

extra.update({"delimiter": delimiter})

extra.update({"delimiter": ","})

And is it possible to avoid overriding as_sql() below by providing the same output_field that postgres StringAgg does?

Suggested change

extra.update({"delimiter": delimiter})

extra.update({

"delimiter": ",",

"output_field": TextField(),

})

I think that's a great idea

@pomegranited I just updated the PR as per your recommendations, thank you for the great suggestion

pomegranited

👍 Oh good, I'm glad my suggestions were helpful @qasimgulzar . Thank you for doing this, and all the other parts of this massive effort!

I tested this with MySQL under my usual devstack (not with Postgres or SQLite)
I read through the code
Includes documentation

bradenmacdonald · 2025-04-08T15:22:13Z

openedx_tagging/core/tagging/models/utils.py

    ' > ',  # Used in the search index and Instantsearch frontend to separate tag levels
            # e.g. tags_level3="Earth > North America > Mexico > Mexico City"
-    ';',   # Used in CSV exports to separate multiple tags from the same taxonomy
+    ';',   # Used in CSV exports to separate multiple tags from the same taxonomy…


Suggested change

';', # Used in CSV exports to separate multiple tags from the same taxonomy…

';', # Used in CSV exports to separate multiple tags from the same taxonomy

Not sure why this ellipsis character was added here.

openedx_tagging/core/tagging/models/utils.py

mphilbrick211 · 2025-05-12T18:42:45Z

@ormsbee are you able to merge this if it's all set?

Lord-KalEl · 2025-05-17T01:26:33Z

🎉

qasimgulzar · 2025-07-01T07:35:23Z

@ormsbee @mphilbrick211 could you please merge this PR?

mphilbrick211 · 2025-07-16T18:51:34Z

Bumping for when @ormsbee is back from vacation.

qasimgulzar · 2025-08-01T11:19:42Z

@ormsbee can we merge this one, I need to push for edx-platform after this one

Lord-KalEl · 2025-08-04T13:09:07Z

Hi, I'm glad to read this … but 1 question to this process:
How can I see, which release version it is? I use podman compose to deploy applications, but in this case, I dont find any version, release or something else …

I hope, it is not too offtopic …I'm sorry …

ormsbee · 2025-08-21T15:55:07Z

@bradenmacdonald: Do you consider your review to be blocking?

bradenmacdonald

@ormsbee I don't think we should merge a function that accepts a delimiter parameter but doesn't use it, and instead always hard-codes ,. But I could be missing something about how that is used. So I would like to see that changed or a comment added to explain before this merges. Otherwise no blockers.

bradenmacdonald · 2025-08-21T19:43:23Z

openedx_tagging/core/tagging/models/utils.py

+            self.function = 'STRING_AGG'
+            self.template = '%(function)s(%(distinct)s%(expressions)s, %(delimiter)s)'
+            extra.update({
+                "delimiter": ",",


Suggested change

"delimiter": ",",

"delimiter": delimiter,

Isn't this all that's needed to address my comment?

qasimgulzar · 2025-08-21T19:47:29Z

@ormsbee I don't think we should merge a function that accepts a delimiter parameter but doesn't use it, and instead always hard-codes ,. But I could be missing something about how that is used. So I would like to see that changed or a comment added to explain before this merges. Otherwise no blockers.

@bradenmacdonald I can update function to use delimiter parameter instead of hardcoding it, if that resolves your concerns.

bradenmacdonald · 2025-08-21T20:50:33Z

@qasimgulzar Yes that would resolve my concerns. Or remove the parameter, since we didn't have it before - I believe you added it but it's unused as far as I can tell.

qasimgulzar · 2025-08-23T12:17:51Z

@qasimgulzar Yes that would resolve my concerns. Or remove the parameter, since we didn't have it before - I believe you added it but it's unused as far as I can tell.

@bradenmacdonald I have updated the PR to use delimiter param to make it configurable.

bradenmacdonald · 2025-08-25T20:04:53Z

Thanks @qasimgulzar. That resolves my concern. @ormsbee This can move ahead as long as someone else has reviewed/approved it - I don't have any concerns remaining nor plan to review any further.

fix: make database function compatible with postgresql and mysql both

c594632

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Nov 18, 2024

This was referenced Nov 18, 2024

fix: migrations to make postgresql compatible. openedx/edx-platform#35762

Open

Add PostgreSQL support #255

Open

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Nov 21, 2024

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 9, 2024

fix: tests and quality issues

bc7a493

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Dec 12, 2024

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 15, 2025

fix: quality issues

1f13467

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 6, 2025

mphilbrick211 moved this from Waiting on Author to Needs Tests Run or CLA Signed in Contributions Mar 6, 2025

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Mar 6, 2025

mphilbrick211 moved this from Needs Tests Run or CLA Signed to Waiting on Author in Contributions Mar 6, 2025

fix: quality issues

48194a0

mphilbrick211 moved this from Waiting on Author to Ready for Review in Contributions Mar 12, 2025

ormsbee approved these changes Apr 7, 2025

View reviewed changes

pomegranited reviewed Apr 8, 2025

View reviewed changes

openedx_tagging/core/tagging/models/utils.py Show resolved Hide resolved

pomegranited reviewed Apr 8, 2025

View reviewed changes

fix: feedback accommodated

6042f4e

pomegranited approved these changes Apr 8, 2025

View reviewed changes

Merge branch 'openedx:main' into qasim/postgresql

fe0da59

bradenmacdonald reviewed Apr 8, 2025

View reviewed changes

mphilbrick211 moved this from Ready for Review to Ready to Merge in Contributions May 12, 2025

mphilbrick211 requested a review from ormsbee July 16, 2025 18:51

bradenmacdonald reviewed Aug 21, 2025

View reviewed changes

qasimgulzar added 2 commits August 23, 2025 17:15

fix: make delimiter configurable

71698c3

fix: remove ellipsis from comment

4731b8b

ormsbee approved these changes Sep 4, 2025

View reviewed changes

ormsbee merged commit a241755 into openedx:main Sep 4, 2025
11 checks passed

github-project-automation bot moved this from Ready to Merge to Done in Contributions Sep 4, 2025

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed

ormsbee mentioned this pull request Dec 30, 2025

WIP Proposal: Unified authoring app #454

Draft

	extra.update({"delimiter": delimiter})
	extra.update({"delimiter": ","})

	';', # Used in CSV exports to separate multiple tags from the same taxonomy…
	';', # Used in CSV exports to separate multiple tags from the same taxonomy

fix: make database function compatible with postgresql and mysql both #260

fix: make database function compatible with postgresql and mysql both #260

Uh oh!

Conversation

qasimgulzar commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

openedx-webhooks commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mphilbrick211 commented Dec 9, 2024

Uh oh!

qasimgulzar commented Dec 9, 2024

Uh oh!

qasimgulzar commented Dec 12, 2024

Uh oh!

qasimgulzar commented Jan 14, 2025

Uh oh!

mphilbrick211 commented Feb 24, 2025

Uh oh!

qasimgulzar commented Feb 25, 2025

Uh oh!

qasimgulzar commented Mar 6, 2025

Uh oh!

mphilbrick211 commented Mar 6, 2025

Uh oh!

mphilbrick211 commented Mar 12, 2025

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pomegranited Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

pomegranited Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

qasimgulzar Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

qasimgulzar Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

pomegranited left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

qasimgulzar Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mphilbrick211 commented May 12, 2025

Uh oh!

Lord-KalEl commented May 17, 2025

Uh oh!

qasimgulzar commented Jul 1, 2025

Uh oh!

mphilbrick211 commented Jul 16, 2025

Uh oh!

qasimgulzar commented Aug 1, 2025

Uh oh!

Lord-KalEl commented Aug 4, 2025

Uh oh!

ormsbee commented Aug 21, 2025

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Aug 21, 2025

Choose a reason for hiding this comment

Uh oh!

qasimgulzar commented Aug 21, 2025

qasimgulzar commented Nov 18, 2024 •

edited

Loading

openedx-webhooks commented Nov 18, 2024 •

edited

Loading