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.

Sign up for GitHub

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

Jump to bottom

add uri encoding for mongodb connector password🐛 #31371

Closed
wants to merge 6 commits into from
Closed

add uri encoding for mongodb connector password🐛 #31371

wants to merge 6 commits into from
+7 −4

Conversation

aglucky
Copy link

@aglucky aglucky commented Oct 13, 2023

What

Fixes #10569

MongoDB passwords need to be encoded with URI encoding to be valid. This is shown in the below image.

image

How

I encoded an encoding step when creating the config instance variable for the connector using the Ruby URI module.

Testing

There were no existing integration tests and I am unsure how to add them in this situation.

@vercel
Copy link

vercel bot commented Oct 13, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2023 4:02pm

@CLAassistant
Copy link

CLAassistant commented Oct 13, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link
Contributor

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
  • If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@aglucky aglucky changed the title add uri encoding for mongodb connector password add uri encoding for mongodb connector password🐛 Oct 13, 2023
@marcosmarxm marcosmarxm added needs-triage team/db-dw-sources Backlog for Database and Data Warehouse Sources team labels Oct 20, 2023
@marcosmarxm
Copy link
Member

Hello @aglucky thanks for the contribution. I added to the source team backlog to review it. At the moment there are quite a lot of development been made for the Mongo destination and source connector and maybe your change will be contemplated for this refactor.

rodireich
rodireich reviewed Oct 20, 2023
View reviewed changes
airbyte-integrations/connectors/source-mongodb-v2/metadata.yaml Outdated Show resolved Hide resolved
...ce-mongodb-v2/src/main/java/io/airbyte/integrations/source/mongodb/MongoConnectionUtils.java Outdated
@@ -40,7 +42,7 @@ public static MongoClient createMongoClient(final MongoDbSourceConfig config) {
if (config.hasAuthCredentials()) {
final String authSource = config.getAuthSource();
final String user = config.getUsername();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does user need encoding as well?

Copy link
Author

Choose a reason for hiding this comment

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

It appears so. I didn't realize usernames were allowed to have special characters in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like any of the class that generate the client connection string take care of encoding.
great stuff!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, just addressed the feedback!

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Oct 20, 2023
@rodireich
Copy link
Contributor

@aglucky If you don't mind, please update dockerImageTag to 1.0.4
in metadata.yaml under source-mongodb-v2.

I have no permission to update myself (I can merge to master though)
thanks

@aglucky
Copy link
Author

aglucky commented Oct 24, 2023

@rodireich Just pushed the update to dockerImageTag

@rodireich rodireich enabled auto-merge (squash) October 24, 2023 15:59
@rodireich rodireich mentioned this pull request Oct 31, 2023
Merged
@aglucky aglucky closed this Nov 7, 2023
auto-merge was automatically disabled November 7, 2023 22:10

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rodireich rodireich rodireich approved these changes

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/mongodb connectors/source/mongodb-v2 needs-triage team/db-dw-sources Backlog for Database and Data Warehouse Sources team
Projects
No open projects
Community Contributed PRs
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source MongoDB: connector fail with password which contains special character
5 participants
@aglucky @CLAassistant @marcosmarxm @rodireich @octavia-squidington-iii