Skip to content

add non-ascii connection string tests #63

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

Merged
merged 1 commit into from
Sep 16, 2022

Conversation

osaxma
Copy link
Contributor

@osaxma osaxma commented Sep 16, 2022

I was taking a look at #38 but I could not reproduce the issue (actually I remember facing issues with @ symbol a while back as well but I could not reproduce that either).

Anyway, I've already added tests for passwords using non-ASCII characters for the three supported auth methods by the package -- namely:

  • clear password
  • md5
  • scram-sha-256

All of them pass (won't work on the CI though as each test needs its own pg_hba.conf to be mounted).

They might be useful to keep if you think so. -- I believe they are necessary based on the next comment.

@osaxma
Copy link
Contributor Author

osaxma commented Sep 16, 2022

Actually, the error is reproducible if you change the pubspec.yaml deps to:

dependencies:
  buffer: ^1.1.0
  crypto: ^3.0.0
  collection: ^1.15.0
  sasl_scram: '0.1.0' # <~~~~ pin here

In this version, both md5 and scram-sha-256 tests would fail with the following error:

PostgreSQLSeverity.fatal 28P01: password authentication failed for user "abc@def"

In my local pubspec.lock, the version was 0.1.1 which had the issue fixed already..

When #38 was filed (and when I experienced issues with @ symbol) , 0.1.1 was not published yet.

See:

void main() {
/// This cannot be run on CI as each test requires different pg_hba.conf
/// to be mounted to the container.
if (Platform.environment.containsKey('GITHUB_ACTION')) {
Copy link
Owner

Choose a reason for hiding this comment

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

I haven't really checked yet, but IIRC Github Action is able to run docker run and other commands... have you tried it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I believe it's possible but not with the current setup (e.g. using services.postgres)... I'll give it another shot later and try a different setup.

Copy link
Owner

Choose a reason for hiding this comment

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

Note: a repository can have multiple workflows, and we can differentiate tests either via directory or tags.

Copy link
Owner

Choose a reason for hiding this comment

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

(Also: thank you for looking into this!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that what I was thinking -- to tag tests with a solo tag and exclude them from common tests and then run all solo tests with concurrency=1.

Copy link
Owner

@isoos isoos left a comment

Choose a reason for hiding this comment

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

Thank you for the continued work here! I'll just merge this and we can figure out how to run it on Github actions later.

@isoos isoos merged commit 5a80ca1 into isoos:master Sep 16, 2022
@osaxma osaxma deleted the support_non_acii_username_and_passwords branch September 16, 2022 16:54
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.

2 participants