Added support for Postgresql database #412
Conversation
PR Verification Succeeded: Coverage >= |
…includes breaking changes. 1. Updated db example, added example for translating unique constraint violation. 2. Refactored db package (breaking change): a. Introduced postgresql package. b. The cockroach package is moved under the postgresql package as a postgresql extension. c. Removed database specific packages from the data package. Code that uses db specific driver packages are moved into sub packages for that database. i.e. types/pqx. d. The db connection property is moved to data.db from data.cockroach 3. Updated tenancy and opa tests to have coverage for both postgresql and cockroach db
| var Module = &bootstrap.Module{ | ||
| Name: "cockroach", | ||
| Precedence: bootstrap.DatabasePrecedence, | ||
| Options: []fx.Option{ | ||
| fx.Provide(newAnnotatedGormDbCreator()), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Is it possible to use Modules field instead of having postgresql.Module in Use()? the difference is scenarios that Module is directly used, e.g. in tests
pkg/data/context.go
Outdated
| GormConfigurerGroup = "gorm_config" | ||
| DatabaseCreatorGroup = "db_creator" |
There was a problem hiding this comment.
We usually keep one fx group per top-level package. Can we use gorm_config grpi[ to inject DB creators?
|
|
||
| //nolint:errorlint | ||
| func (t WebDataErrorTranslator) dataIntegrityErrorWithStatusCode(_ context.Context, err error, sc int) error { | ||
| switch err.(DataError).RootCause().(type) { | ||
| case *pgconn.PgError, *pq.Error: | ||
| default: | ||
| return NewErrorWithStatusCode(err.(DataError), sc) | ||
| } | ||
| msg := "duplicate keys" | ||
| matches := dataIntegrityRegexp.FindStringSubmatch(err.Error()) | ||
| for i, name := range dataIntegrityRegexp.SubexpNames() { | ||
| if i >= len(matches) { | ||
| break | ||
| } | ||
| if name == "value" { | ||
| if matches[i] != "" { | ||
| msg = fmt.Sprintf("duplicate value: %s", matches[i]) | ||
| } | ||
| break | ||
| } | ||
| } | ||
| return NewErrorWithStatusCode(err.(DataError), sc).WithMessage(msg) | ||
| } |
There was a problem hiding this comment.
I see this is moved to example, which would require all services to include this code. Could we also move it to postgresql package?
If service want to override the behavior, it could add translator with higher order
pkg/data/errors.go
Outdated
| // Use WithCause because we want to modify message but preserve the cause | ||
| CodedError: e.CodedError.WithCause(e.CodedError.Cause(), msg, args...), | ||
| details: e.details, | ||
| } | ||
| } |
There was a problem hiding this comment.
this actually changed unwrapping behaviour, might causes behaviour changes on error translators.
Should we do this in errorutils.CodedError instead of here?
pkg/data/package.go
Outdated
| BindDataProperties, | ||
| BindDatabaseProperties, |
There was a problem hiding this comment.
those two properties are defined and provided in same package, can we make DatabaseProperties as a field of DataProperties? Is there any case where we need to bind one properties but not the other one, in tests?
pkg/data/postgresql/properties.go
Outdated
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package postgresql |
There was a problem hiding this comment.
empty file, should be deleted
| di := &testDI{} | ||
| test.RunTest(context.Background(), t, | ||
| apptest.Bootstrap(), | ||
| dbtest.WithDBPlayback("testdb"), | ||
| dbtest.WithDBPlayback("testdb", dbtest.DBPort(26257)), |
There was a problem hiding this comment.
looks like the copyist data need to be moved to this package too. (it's still under types/testdata now)
| if valMsg != "" { | ||
| msg = fmt.Sprintf("%s; %s", msg, valMsg) | ||
| } | ||
| return e.WithCause(e.Cause(), msg) |
There was a problem hiding this comment.
with the other change in DataError, could be e.WithMessage(msg) now
Breaking Change
Refactored data package to support postgresql and cockroachdb. This includes breaking changes.
postgresqlpackage.cockroachpackage is now a package under thepostgresqlpackage as a postgresql extension (cockroach db supports the postgresql wire protocol).datapackage. Code that uses db specific driver packages are moved into sub packages for that database. i.e.types/pqx.data.dbfromdata.cockroachOther Changes
Type of Change
Checklist