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

Ensure url encoded characters are handled correctly #10

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lkrystkowiak-snowplow
Copy link
Collaborator

@lkrystkowiak-snowplow lkrystkowiak-snowplow commented Nov 20, 2023

  • I couldn't make xo/dburl work with url encoded characters so decided to drop it entirely and use drivers directly
  • DSN is passed to a driver "as is" with no changes. Caller is responsible for url encoding or any other formatting.
  • This approach has its downsides, all future drivers will need to be implemented explicitly, but big benefit (which imo outweighs downsides) is that you don't need to learn xo/dburl and can use drivers docs directly
  • It's a breaking change, Terraform modules need to be adjusted accordingly

Example usage

U=lukkry
P=$(echo -n 'abc%123' | jq -sRr @uri)     

# Use correct password                                                                                                                                                                                
./conntest check --driver snowflake --dsn "$U:$P@XXX.eu-central-1.snowflakecomputing.com/snowflake" --tags=jobId=job-123 -r 3 | jq
{
  "id": "89fa45a7-5c1a-46ff-8d73-07d168e10e75",
  "name": "fabric:warehouse-connection-check",
  "version": 1,
  "emittedBy": "conntest",
  "timestamp": "2023-11-20T10:34:40.891406+01:00",
  "data": {
    "complete": true,
    "messages": [],
    "tags": {
      "jobId": "job-123"
    },
    "attempts": 3
  }
}

# Use incorrect password
./conntest check --driver snowflake --dsn "$U:[email protected]/snowflake" --tags=jobId=job-123 -r 3 | jq
ERRO[0000]log.go:242 gosnowflake.(*defaultLogger).Errorln Authentication FAILED
10:36:08.625779 0 attempt: can't query snowflake: 390100 (08004): Incorrect username or password was specified.
ERRO[0000]log.go:242 gosnowflake.(*defaultLogger).Errorln Authentication FAILED
10:36:08.954373 1 attempt: can't query snowflake: 390100 (08004): Incorrect username or password was specified.
ERRO[0001]log.go:242 gosnowflake.(*defaultLogger).Errorln Authentication FAILED
10:36:09.392586 2 attempt: can't query snowflake: 390100 (08004): Incorrect username or password was specified.
{
  "id": "f172975e-52cd-44e4-8281-92941a534b30",
  "name": "fabric:warehouse-connection-check",
  "version": 1,
  "emittedBy": "conntest",
  "timestamp": "2023-11-20T10:36:09.392648+01:00",
  "data": {
    "complete": false,
    "messages": [
      "can't query snowflake: 390100 (08004): Incorrect username or password was specified."
    ],
    "tags": {
      "jobId": "job-123"
    },
    "attempts": 3
  }
}

@lkrystkowiak-snowplow lkrystkowiak-snowplow changed the title Ensure special characters are handled Ensure url encoded characters are handled correctly Nov 20, 2023
@lkrystkowiak-snowplow lkrystkowiak-snowplow marked this pull request as ready for review November 20, 2023 09:45
@@ -42,3 +42,44 @@ func TestMarshall(t *testing.T) {
t.Fail()
}
}

func TestCheckPostgres(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by the test results in the job output. All of the tests seem to produce errors but the tests pass.

=== RUN   TestCheckPostgres
09:29:11.664948 0 attempt: can't parse Postgres DSN: cannot parse `***localhost:5432/db`: failed to parse as DSN (invalid dsn)
--- PASS: TestCheckPostgres (0.00s)
=== RUN   TestCheckSnowflake
09:29:11.665012 0 attempt: can't parse Snowflake DSN: 260002: password is empty
--- PASS: TestCheckSnowflake (0.00s)
=== RUN   TestCheckDatabricks
09:29:11.665112 0 attempt: can't open a connection to databricks: scheme db not recognized
--- PASS: TestCheckDatabricks (0.00s)
=== RUN   TestCheckUnknownDriver
09:29:11.665152 0 attempt: unknown driver: unknown
--- PASS: TestCheckUnknownDriver (0.00s)

Copy link

Choose a reason for hiding this comment

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

I'm guessing that's because the tests are expecting to fail, so the failure messages get returned when performing the check? The tests fail if the result doesn't contain those errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pretty much what @ungn said, those unit tests ensure invalid DSNs are handled correctly. There is 1 integration tests which ensures Check works with Postgres (happy path)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for confirming ! Always seems wrong with just negative tests but we can't do positive tests for Databricks/Snowflake/Redshift.

Copy link
Contributor

@Andy-Hay Andy-Hay left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants