Skip to content
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

fix: use correct case for authsource mongo #841

Merged
merged 1 commit into from May 29, 2023

Conversation

johanseto
Copy link
Contributor

@johanseto johanseto commented May 24, 2023

Description

Update authSource keyword argument to authsource

Authsource was configurated in camelcase.
https://github.com/overhangio/tutor/blob/master/tutor/templates/apps/openedx/settings/partials/common_all.py#L16 Something I think is ok because pymongo uses it in camelcase. 2023-04-03_10-52

However, the error was presented because edx-platform use another variable when credentials are provided (LOWERCASE). https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L72

Looking more , the auth_source is configured with another key name: authsource (LOWERCASE) from the kwargs. https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L34

This won't change the **kwargs behavior of the camelCase in pymongo MongoClient because pymongo has a caseInsentive method. https://github.com/mongodb/mongo-python-driver/blob/3.12.3/pymongo/mongo_client.py#L651
it's probably OK to pass authsource instead of authSource as a keyword argument.

issues related

#823

@johanseto johanseto force-pushed the jlc/update-authsource-mongo branch from 016b2af to f7316b0 Compare May 24, 2023 23:54
@@ -13,7 +13,7 @@
"password": {% if MONGODB_PASSWORD %}"{{ MONGODB_PASSWORD }}"{% else %}None{% endif %},
# Connection/Authentication
"ssl": {{ MONGODB_USE_SSL }},
"authSource": "{{ MONGODB_AUTH_SOURCE }}",
"authsource": "{{ MONGODB_AUTH_SOURCE }}",
"replicaSet": {% if MONGODB_REPLICA_SET %}"{{ MONGODB_REPLICA_SET }}"{% else %}None{% endif %},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"replicaSet": {% if MONGODB_REPLICA_SET %}"{{ MONGODB_REPLICA_SET }}"{% else %}None{% endif %},
"replicaset": {% if MONGODB_REPLICA_SET %}"{{ MONGODB_REPLICA_SET }}"{% else %}None{% endif %},

Looking more if pymongo use insensitive dict https://github.com/mongodb/mongo-python-driver/blob/3.12.3/pymongo/mongo_client.py#L651 we can test changing also replicaSet to replicaset in order to have mongo_db_parameters in standard way kwargs using lowercase.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It ain't broken (yet) so let's not try to fix this.

@johanseto
Copy link
Contributor Author

@regisb @keithgg I have created the PR to continue. Regards,

Copy link
Contributor

@keithgg keithgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to merge this @johanv26. I checked why we never ran into this issue and it's because we add the username and password to MONGODB_HOST and don't specify them separately.

  • I read through the code
  • I tested this locally to make sure the migration split_modulestore_django.0002_data_migration passes for mongodb+srv urls if the username and password is encoded in the MONGODB_HOST.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Could you please add a changelog entry? https://docs.tutor.overhang.io/tutor.html#contributing

@johanseto johanseto force-pushed the jlc/update-authsource-mongo branch from f7316b0 to a3966de Compare May 26, 2023 20:43
Update `authSource`  keyword argument to  `authsource`

Authsource was configurated in camelcase.
https://github.com/overhangio/tutor/blob/master/tutor/templates/apps/openedx/settings/partials/common_all.py#L16
Something I think is ok because pymongo uses it in camelcase.
https://pymongo.readthedocs.io/en/3.10.1/api/pymongo/mongo_client.html

However, the error was presented because edx-platform use another variable when credentials are provided **(LOWERCASE)**.
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L72

Looking more ,  the `auth_source `is configured with another key name:  `authsource` **(LOWERCASE)** from the kwargs.
https://github.com/openedx/edx-platform/blob/master/xmodule/mongo_utils.py#L34

This won't change the **kwargs behavior of the camelCase in pymongo `MongoClient` because pymongo has a caseInsentive method.
https://github.com/mongodb/mongo-python-driver/blob/3.12.3/pymongo/mongo_client.py#L651
 ``it's  OK to pass `authsource` instead of authSource as a keyword argument.``
@johanseto johanseto force-pushed the jlc/update-authsource-mongo branch from a3966de to 0780d24 Compare May 26, 2023 20:45
@johanseto
Copy link
Contributor Author

Looking good! Could you please add a changelog entry? https://docs.tutor.overhang.io/tutor.html#contributing

Changelog added, regards.

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks again for the PR!

@regisb regisb merged commit 4bb8671 into overhangio:master May 29, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants