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

use quote_plus instead of quote #122

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Conversation

ranchodeluxe
Copy link
Contributor

asyncpg doesn't work well for all US-ASCII characters, we need to use quote_plus instead

Related Issue(s):

MagicStack/asyncpg#1159

Description:

See PR and related ticket above

PR Checklist:

  • pre-commit hooks pass locally
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable, and docs build successfully (run make docs)
  • Changes are added to the CHANGELOG.

`asyncpg` doesn't work well for all US-ASCII characters, we need to use `quote_plus` instead

MagicStack/asyncpg#1159
@vincentsarago
Copy link
Member

vincentsarago commented Jun 13, 2024

@ranchodeluxe I'm trying to reproduce the issue using a password like a2Vw:yk=)CdSis[fek]tW= (and not applying your fix) but I don't see any thing failing. Are there any other particular things to get errors?

FYI: I'm changing https://github.com/stac-utils/stac-fastapi-pgstac/blob/main/tests/conftest.py#L56 to a2Vw:yk=)CdSis[fek]tW=

@ranchodeluxe
Copy link
Contributor Author

a2Vw:yk=)CdSis[fek]tW=

Sorry for the late reply @vincentsarago. I couldn't run tests here a few weeks ago gave up.

The / was the problem in the other repo. Try the password a2Vw:yk=)CdSis[fek]tW=/o.

For what it's worth still can't run the test docker image locally. It's failing to pip install things correctly in site-packages for the image user "newuser". Haven't spent much time on it but can try later

@ranchodeluxe
Copy link
Contributor Author

Okay, just approve the workflow please and we should see tests fail 😉

@@ -1,7 +1,7 @@
"""Postgres API configuration."""

from typing import List, Type
from urllib.parse import quote
from urllib.parse import quote_plus as quote
Copy link
Member

Choose a reason for hiding this comment

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

re-added this after confirming locally that the test were failing!

connection = (
f"postgresql://{jan.user}:{jan.password}@{jan.host}:{jan.port}/{jan.dbname}"
)
connection = f"postgresql://{jan.user}:{quote(jan.password)}@{jan.host}:{jan.port}/{jan.dbname}"
Copy link
Member

Choose a reason for hiding this comment

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

in the test we also needed to quote the password.

@vincentsarago
Copy link
Member

For what it's worth still can't run the test docker image locally. It's failing to pip install things correctly in site-packages for the image user "newuser". Haven't spent much time on it but can try later

Yeah sorry about that, I though I fixed this but it doesn't seems to be the case.

for what is worth we've made it easier to just run the test locally (in a virtual env) because now it doesn't touch your local postgres instance.

Once all the test are ✅ I'll merge and update the changelog.

thanks @ranchodeluxe 🙏

@vincentsarago vincentsarago self-requested a review June 27, 2024 07:19
@vincentsarago vincentsarago merged commit 8b61a87 into stac-utils:main Jun 27, 2024
7 checks 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

2 participants