Updated Postgre module to use pgx instead of pq

[Technerder]

Removed Postgre Close test since the pgx implementation of Close() doesn't return an error, Removed Storage.Close()'s return method since the pgx implementation of Close() doesn't return an error, Removed Config.maxIdleConns since pgx implementation doesn't support setting a maximum number of idle connections

[hi019]

Thanks for the PR! There are quiet a few breaking changes here, so we'll need to release a new major version of the postgres implementation.

[Technerder]

Is there anything I could change or do to help?

[ReneWerner87]

@technerder

I asked the developer of pgx as to whether or not there was a way to find out if pgxpool.Pool.Close() fails and they said that it simply blocks until it succeeds like I originally thought. I only removed the Test_Postgres_Close test because I thought it was redundant since the close method can't seem to fail. I also only modified the signature for the storage's Close method for the same reason

please add the test again to test the error free process

I'd be more than happy to change the signature back and make it return nil for the error but I'm not sure if that'd be considered best practice.

https://github.com/gofiber/fiber/blob/master/app.go#L66 we have several storages, it is important that they all use the same interface, otherwise you cannot use them as intended, please just change the return type

[Technerder]

we have several storages, it is important that they all use the same interface, otherwise you cannot use them as intended, please just change the return type

So I assume adding something like the following would be frowned down upon unless I also added it to all other storage implementations?

func (s *Storage) DB() *pgxpool.Pool {
	return s.db
}

[ReneWerner87]

No would be okay

[hi019]

Thanks for all the work you've done here. If we're going to release a new major version, what would you think of rewriting the config to just accept a pgx connection, as you wrote in your issue? We would be able to remove a lot of code.

[Technerder]

I'll start working on changing the module to accept a pgx object instead.

[Technerder]

Thanks for all the work you've done here. If we're going to release a new major version, what would you think of rewriting the config to just accept a pgx connection, as you wrote in your issue? We would be able to remove a lot of code.

Should I remove the postgres config struct altogether? I think it would make sense to do so.

[hi019]

Yeah, we should remove the config keys that correspond to the connection configuration.

[hi019]

But before you make changes, @ReneWerner87 what do you think of the idea?

[ReneWerner87]

passing the connection would be okay for me

[Technerder]

I'll start implementing this right now. I'm thinking of modifying the Config to look somewhat like this

// Config defines the config for storage.
type Config struct {
	// DBPool pgxpool.Pool object
	//
	// Required
	DBPool pgxpool.Pool

	// Table name
	//
	// Optional. Default is "fiber_storage"
	Table string

	// Reset clears any existing keys in existing Table
	//
	// Optional. Default is false
	Reset bool

	// Time before deleting expired keys
	//
	// Optional. Default is 10 * time.Second
	GCInterval time.Duration
}

But I'm unsure of how to go about modifying the defaultConfig, should it just panic out if no DBPool object is passed in or should I remove the defaultConfig altogether?

[hi019]

@Technerder Yes, panicking should be fine here

[Technerder]

Latest commit should allow people to directly pass in a pgxpool.Pool object as opposed to the database host, port, etc

[ReneWerner87]

@Technerder please check and

[Technerder]

@ReneWerner87 I asked the developer of pgx about the dependency (related issue) and this is what they told me

My first though was: why does a database driver have a dependency on a JWT library? As it turns out, there isn't any production dependency. It is there because of how modules include all the dependencies including tests in go.sum. It appears it is coming from go-kit. 48f3934 trimmed the go-kit dependency so I think this pseudo-dependency should drop entirely at some point.

Its only being used for some test, not in the actual library code.

[ReneWerner87]

@Technerder

and the snyk check failed

[hi019]

One more thing, can you put the new code in a v2 folder (so the path will be postgres/v2/)? That way we can issue a new major version with a /v2 import

[Technerder]

@Technerder

and the snyk check failed

Would you be able to confirm if this is related to CVE-2021-3538?

[Technerder]

One more thing, can you put the new code in a v2 folder (so the path will be postgres/v2/)? That way we can issue a new major version with a /v2 import

Should I keep the v1 code there in the base folder?

[hi019]

Yep

[Technerder]

Verification for "Tech hashtag 0067" in joeyxyz's dms

[rishavs]

Hey folks, checking back on this PR. PQ is in maintenance mode now and its maintainers even recommend pgx. https://github.com/lib/pq#status

If this PR is ready, it will be good to merge it as soon as possible, so that we can start using a postgres driver which is in active development.

[efectn]

@Technerder pgx v5 seems to be ready for production. Can you test it? The problem should be fixed

[Technerder]

@efectn I can test the code usage and functionality by tomorrow but I believe I lack the perms to run a Snyk security check/audit (unless I'm mistaken) and had to ask @ReneWerner87 the last time.

[efectn]

Satori uuid seems to be removed. Maybe we need to open an issue for these alerts @Technerder

[Technerder]

@efectn would you be able to provide a link to that page? I'm also a little confused about why yaml.v3 is being flagged since the go.mod on the main pgx repo shows the current version being used to be v3.0.1 as shown here which should remove the vulnerability according to the screenshot.

[efectn]

@efectn would you be able to provide a link to that page? I'm also a little confused about why yaml.v3 is being flagged since the go.mod on the main pgx repo shows the current version being used to be v3.0.1 as shown here which should remove the vulnerability according to the screenshot.

• github.com/gofiber/storage/[email protected] › github.com/jackc/[email protected] › golang.org/x/[email protected] › golang.org/x/[email protected]
• github.com/gofiber/storage/[email protected] › github.com/jackc/[email protected] › github.com/jackc/[email protected] › github.com/stretchr/[email protected] › github.com/stretchr/[email protected] › gopkg.in/[email protected]

🤔🤔

[efectn]

Created an issue --> https://github.com/jackc/pgx/issues/1352

[sixcolors]

From a dependancy CVE standpoint I think we should move forward with this PR review.

Per my comments here https://github.com/jackc/pgx/issues/1352 Snyk go.mod scanning issues are false positives. Checked with govulncheck.

~/Documents/GitHub/storage/postgres/v2 (pr/153) » govulncheck .                                                                                            1 ↵ sixcolors@Jasons-MacBook-Pro
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Scanning for dependencies with known vulnerabilities...
No vulnerabilities found.

And checking which packages are acutally used in v2

~/Documents/GitHub/storage/postgres/v2 (pr/153) » go list -deps -f '{{define "M"}}{{.Path}}@{{.Version}}{{end}}{{with .Module}}{{if not .Main}}{{if .Replace}}{{template "M" .Replace}}{{else}}{{template "M" .}}{{end}}{{end}}{{end}}' | sort -u
github.com/jackc/[email protected]
github.com/jackc/[email protected]
github.com/jackc/pgx/[email protected]
github.com/jackc/puddle/[email protected]
golang.org/x/[email protected]
golang.org/x/[email protected]