update: shopify-app-session-storage-[mysql,postgresql] to respect SSL DB url param

[chr33s]

WHY are these changes introduced?

Fixes #230

WHAT is this pull request doing?

This updates the shopify-app-session-storage-mysql and shopify-app-session-storage-postgresql packages to respect SSL DB url param e.g. mysql://root:password@127.0.0.1:3306?ssl=false.

Type of change

• Patch: Bug (non-breaking change which fixes an issue)
• Minor: New feature (non-breaking change which adds functionality)
• Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
• I have added/updated tests for this change
• I have documented new APIs/updated the documentation for modified APIs (for public APIs)

[bitkidd]

Sorry, but I'd say that it is a bad PR, you completely ignore all the possible options that pg-connection-string can have, link.

The best option here is not to parse connection string and leave it be and then pass it into pool as connectionString.

[chr33s]

@bitkidd agreed its not ideal, I simply implemented the missing functionality using the fewest lines of code possible by reusing the existing architecture. More than willing to refactor if that's whats preferred.

NOTE: even the node-pg-pool docs use URL https://github.com/brianc/node-postgres/tree/master/packages/pg-pool

[maruffahmed]

DATABASE_URL=mysql://root:password@localhost:3306/db?ssl={"rejectUnauthorized":true}

(this.dbUrl.searchParams.get('ssl') !== 'false').toString()

Considering the above database connection URL this.dbUrl.searchParams.get('ssl') will return either '{"rejectUnauthorized":true}' as a string or null if not found. I think the !=='false' check will not work here.

I would suggest the following solution :

private async init(): Promise<void> {
    const sslConfig = this.dbUrl.searchParams.get('ssl');
    this.pool = await mysql.createPool({
      connectionLimit: this.connectionPoolLimit,
      host: this.dbUrl.hostname,
      user: decodeURIComponent(this.dbUrl.username),
      password: decodeURIComponent(this.dbUrl.password),
      database: this.getDatabase(),
      port: Number(this.dbUrl.port),
      ssl : sslConfig ? JSON.parse(sslConfig) : undefined
    });
  }

Ref of ssl types:

ssl?: string | SslOptions;

[chr33s]

@maruffahmed good catch, will update the pull request.

[maruffahmed]

I think it'll work now. Looking for a review from the code owner.

[maruffahmed]

I would just do this:

return ssl ? JSON.parse(ssl) : undefined;  

[chr33s]

@maruffahmed agreed my preference is also shorthand, updated.

[bitkidd]

@chr33s no, I don't get it, why even bother parsing connection string when the package that is used to connect to DB does this for you (link)?

Especially in a case when you do not respect all the options that can be passed down.

Of course this is not your fault that the initial design makes what it makes, but why not fix it by leaving connection string as is and pass it to the client, and then work with the second method that explicitly says that it was created for credentials to be passed separately.

It is pretty strange that this second method is designed as it is, but that is another story.

[chr33s]

@bitkidd this pull request is scoped to the ssl param, as that was my (& other's usecase, see linked issue). With the updated JSON.parse approach as suggested by @maruffahmed this pull request now satisfies the original scope.

I think changing the initial design change warrants a new pull request?

(updated: ref: Single Responsibility Principle of a Pull Request)

[maruffahmed]

@paulomarg please take a look at this PR.

[scraper095]

Any updates or workarounds? I am facing the same issue

[bitkidd]

@scraper095 copy their implementation and create yours based out of it.

[lizkenyon]

Thanks for this contribution! If you can add one more test case, we'll get this merged!

[lizkenyon]

@chr33s can you please sign the CLA?

[lizkenyon]

@chr33s Could you add a short explanation for this in the readmes for mysql and postgres. Then we should be good to go.

[chr33s]

@lizkenyon updated the readme's as requested and accepted the CLA previously with CLA test passing.

[chr33s]

@lizkenyon I noticed the workflow run failed, I toggled the ssl param back to the default ?ssl={"rejectUnauthorized":true}, which should resolve the issue.

[johnmcguin]

Hello, any updates on this PR (it seems stagnant) or is best path self patching?

[lizkenyon]

Hi there 👋

If @chr33s is able to get the tests passing. We should be able to merge and release this change.

[reinisdy]

Hello, any updates on this issue?

[chr33s]

@lizkenyon tests should now be passing, I did notice some DrizzleSessionStoragePostgres tests where failing but that's outside the scope of this PR.

[lizkenyon]

Hi there 👋

It looks like setting up the tests is trickier than we thought. The reason why your postgres test is passing is because we are not actually testing with SSL. And they were initially failing because the the docker server doesn't support SSL by default. We spent a good amount of trying to figure out how to make this work, but unfortunately we could not come up with an easy solution.

Looking at the code, we think we want to refactor it to either allow the adapters to use the connection string as we get it, or expose the actual config we're passing in when we create the clients. We as well need to ensure our tests work with both SSL and self signed certificates. And we have a ticket in our backlog to do this.

In the mean time you may want to explore using an environment variable to set if SSL is required, like this community member did.