refactor(storage): move from lib/pq to pgx

[erka]

closes #2982

[codecov]

Codecov Report

Attention: Patch coverage is 97.05882% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 72.57%. Comparing base (f997fb9) to head (a9a3a03).
Report is 251 commits behind head on main.

❗ Current head a9a3a03 differs from pull request most recent head ebb37d6. Consider uploading reports for the commit ebb37d6 to get more accurate results

Files	Patch %	Lines
internal/storage/sql/migrator.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2996      +/-   ##
==========================================
+ Coverage   70.78%   72.57%   +1.78%     
==========================================
  Files          91       95       +4     
  Lines        8729     7270    -1459     
==========================================
- Hits         6179     5276     -903     
+ Misses       2165     1604     -561     
- Partials      385      390       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[erka]

@GeorgeMac I am stuck a bit with this and need some help. pgx has special ConnectError which contains host and credentials to database. The thing is that the error message goes to the client without any preprocessing.

I see that Flipt has driver.AdaptError func but it isn't used for all sql storage. Any reason for that? Any advice how to prevent credentials leakage to the client?

[GeorgeMac]

Sorry for the slow reply @erka !

I just did a little walkthrough as it has been a while since I looked at all this.

AdaptError came about when we added the authn and oplock packages.
Previously, with core storage/sql package an implementation of the storage.Store was created per driver, to support error mapping. This was getting a little verbose and repetetive, so I didn't want to perpetuate that same pattern into these new storage layers. Instead, I created AdaptError to be dropped in to the authn package where needed, so we could have a single sql storage implementation in these places.

Obviously, now we have two patterns, which is a shame:

• AdaptError for authn and oplock.
• Wrapping implementations for core storage/sql (see: postgres decorator).

I think the quick win for your PR would be to add support for this ConnectError to the storage/sql/postgres package. Perhaps a longer term fix would be to lean into AdaptError everywhere more. What do you think?

Sadly it is all a bit fiddly to implement. One thing I considered, but would be quite a subtle and fiddly thing to implement, would be to create an implementation of driver.Driver and intercept these errors there. That would at-least stop the error mapping from having to creep throughout the entire codebase, but still would probably have some subtle and weird edge-cases.

[erka]

@GeorgeMac driver.Driver is a great idea. It fixes the problem with minimal impact on codebase and simplifies maintenance in future. Thank you!

[GeorgeMac]

Change looks great. Glad the driver.Driver approach worked out!
I think the change looks good. I put one take it or leave it on there.

[markphelps]

looks great to me! thank you @erka !!