Skip to content

Commit 661484f

Browse files
gaultierory-bot
authored andcommitted
chore: improve error reporting to help diagnose flaky test
GitOrigin-RevId: d08aa2c570dd17df5a5e14c2975cebe80aeb2e66
1 parent 9301ce9 commit 661484f

File tree

6 files changed

+56
-51
lines changed

6 files changed

+56
-51
lines changed

oryx/popx/match.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,10 @@
44
package popx
55

66
import (
7-
"fmt"
87
"regexp"
98

9+
"github.com/pkg/errors"
10+
1011
"github.com/ory/pop/v6"
1112
)
1213

@@ -43,18 +44,18 @@ func parseMigrationFilename(filename string) (*match, error) {
4344
} else {
4445
dbType = pop.CanonicalDialect(m[3][1:])
4546
if !pop.DialectSupported(dbType) {
46-
return nil, fmt.Errorf("unsupported dialect %s", dbType)
47+
return nil, errors.Errorf("unsupported dialect %s", dbType)
4748
}
4849
}
4950

5051
if m[6] == "fizz" && dbType != "all" {
51-
return nil, fmt.Errorf("invalid database type %q, expected \"all\" because fizz is database type independent", dbType)
52+
return nil, errors.Errorf("invalid database type %q, expected \"all\" because fizz is database type independent", dbType)
5253
}
5354

5455
if m[4] == ".autocommit" {
5556
autocommit = true
5657
} else if m[4] != "" {
57-
return nil, fmt.Errorf("invalid autocommit flag %q", m[4])
58+
return nil, errors.Errorf("invalid autocommit flag %q", m[4])
5859
}
5960

6061
return &match{

oryx/popx/migration_box.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ func WithTestdata(t *testing.T, testdata fs.FS) MigrationBoxOption {
8989
return func(m *MigrationBox) {
9090
require.NoError(t, fs.WalkDir(testdata, ".", func(path string, info fs.DirEntry, err error) error {
9191
if err != nil {
92-
return err
92+
return errors.WithStack(err)
9393
}
9494
if !info.Type().IsRegular() {
9595
t.Logf("skipping testdata entry that is not a file: %s", path)
@@ -120,13 +120,13 @@ func WithTestdata(t *testing.T, testdata fs.FS) MigrationBoxOption {
120120
Runner: func(m Migration, c *pop.Connection) error {
121121
b, err := fs.ReadFile(testdata, m.Path)
122122
if err != nil {
123-
return err
123+
return errors.WithStack(err)
124124
}
125125
if isMigrationEmpty(string(b)) {
126126
return nil
127127
}
128128
_, err = c.Store.SQLDB().Exec(string(b))
129-
return err
129+
return errors.WithStack(err)
130130
},
131131
})
132132

@@ -230,7 +230,7 @@ func (mb *MigrationBox) findMigrations(
230230
}
231231

232232
if details == nil {
233-
return errors.WithStack(fmt.Errorf("Found a migration file that does not match the file pattern: filename=%s pattern=%s", info.Name(), MigrationFileRegexp))
233+
return errors.Errorf("Found a migration file that does not match the file pattern: filename=%s pattern=%s", info.Name(), MigrationFileRegexp)
234234
}
235235

236236
content, err := fs.ReadFile(dir, p)
@@ -269,7 +269,7 @@ func (mb *MigrationBox) findMigrations(
269269
// Sort ascending.
270270
sort.Sort(mb.migrationsUp)
271271

272-
return err
272+
return errors.WithStack(err)
273273
}
274274

275275
// hasDownMigrationWithVersion checks if there is a migration with the given
@@ -293,12 +293,12 @@ func (mb *MigrationBox) check() error {
293293

294294
for _, n := range mb.migrationsUp {
295295
if err := n.Valid(); err != nil {
296-
return err
296+
return errors.WithStack(err)
297297
}
298298
}
299299
for _, n := range mb.migrationsDown {
300300
if err := n.Valid(); err != nil {
301-
return err
301+
return errors.WithStack(err)
302302
}
303303
}
304304
return nil

oryx/popx/migrator.go

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/ory/x/cmdx"
2323
"github.com/ory/x/logrusx"
2424
"github.com/ory/x/otelx"
25+
"github.com/ory/x/sqlcon"
2526
)
2627

2728
const (
@@ -37,7 +38,7 @@ func (mb *MigrationBox) shouldNotUseTransaction(m Migration) bool {
3738
// Up runs pending "up" migrations and applies them to the database.
3839
func (mb *MigrationBox) Up(ctx context.Context) error {
3940
_, err := mb.UpTo(ctx, 0)
40-
return err
41+
return errors.WithStack(err)
4142
}
4243

4344
// UpTo runs up to step "up" migrations and applies them to the database.
@@ -83,21 +84,22 @@ func (mb *MigrationBox) UpTo(ctx context.Context, step int) (applied int, err er
8384
err := conn.RawQuery(fmt.Sprintf("INSERT INTO %s (version) VALUES (?)", mtn), mi.Version).Exec()
8485
return errors.Wrapf(err, "problem inserting migration version %s", mi.Version)
8586
}); err != nil {
86-
return err
87+
return errors.WithStack(err)
8788
}
8889
continue
8990
}
9091

9192
l.Info("Migration has not yet been applied, running migration.")
9293

9394
if err := mi.Valid(); err != nil {
94-
return err
95+
return errors.WithStack(err)
9596
}
9697

9798
noTx := mb.shouldNotUseTransaction(mi)
9899
if noTx {
100+
l.Info("NOT running migrations inside a transaction")
99101
if err := mi.Runner(mi, c); err != nil {
100-
return err
102+
return errors.WithStack(err)
101103
}
102104

103105
// #nosec G201 - mtn is a system-wide const
@@ -107,7 +109,7 @@ func (mb *MigrationBox) UpTo(ctx context.Context, step int) (applied int, err er
107109
} else {
108110
if err := mb.isolatedTransaction(ctx, "up", func(conn *pop.Connection) error {
109111
if err := mi.Runner(mi, conn); err != nil {
110-
return err
112+
return errors.WithStack(err)
111113
}
112114

113115
// #nosec G201 - mtn is a system-wide const
@@ -116,7 +118,7 @@ func (mb *MigrationBox) UpTo(ctx context.Context, step int) (applied int, err er
116118
}
117119
return nil
118120
}); err != nil {
119-
return err
121+
return errors.WithStack(err)
120122
}
121123
}
122124

@@ -133,7 +135,7 @@ func (mb *MigrationBox) UpTo(ctx context.Context, step int) (applied int, err er
133135
}
134136
return nil
135137
})
136-
return
138+
return applied, errors.WithStack(err)
137139
}
138140

139141
// Down runs pending "down" migrations and rolls back the
@@ -148,7 +150,7 @@ func (mb *MigrationBox) Down(ctx context.Context, steps int) (err error) {
148150
}
149151

150152
c := mb.c.WithContext(ctx)
151-
return mb.exec(ctx, func() (err error) {
153+
return errors.WithStack(mb.exec(ctx, func() (err error) {
152154
mtn := sanitizedMigrationTableName(c)
153155
count, err := c.Count(mtn)
154156
if err != nil {
@@ -203,7 +205,7 @@ func (mb *MigrationBox) Down(ctx context.Context, steps int) (err error) {
203205
if mb.shouldNotUseTransaction(mi) {
204206
err := mi.Runner(mi, c)
205207
if err != nil {
206-
return err
208+
return errors.WithStack(err)
207209
}
208210

209211
// #nosec G201 - mtn is a system-wide const
@@ -232,7 +234,7 @@ func (mb *MigrationBox) Down(ctx context.Context, steps int) (err error) {
232234
reverted++
233235
}
234236
return nil
235-
})
237+
}))
236238
}
237239

238240
func (mb *MigrationBox) createTransactionalMigrationTable(ctx context.Context, c *pop.Connection, l *logrusx.Logger) error {
@@ -243,7 +245,7 @@ func (mb *MigrationBox) createTransactionalMigrationTable(ctx context.Context, c
243245
fmt.Sprintf(`CREATE UNIQUE INDEX %s_version_idx ON %s (version)`, mtn, mtn),
244246
fmt.Sprintf(`CREATE INDEX %s_version_self_idx ON %s (version_self)`, mtn, mtn),
245247
}); err != nil {
246-
return err
248+
return errors.WithStack(err)
247249
}
248250

249251
l.WithField("migration_table", mtn).Debug("Transactional migration table created successfully.")
@@ -277,7 +279,7 @@ func (mb *MigrationBox) migrateToTransactionalMigrationTable(ctx context.Context
277279
}
278280

279281
if err := mb.createMigrationStatusTableTransaction(ctx, workload...); err != nil {
280-
return err
282+
return errors.WithStack(err)
281283
}
282284

283285
l.WithField("migration_table", mtn).Debug("Successfully migrated legacy schema_migration to new transactional schema_migration table.")
@@ -296,7 +298,7 @@ func (mb *MigrationBox) isolatedTransaction(ctx context.Context, direction strin
296298
}
297299

298300
return Transaction(ctx, mb.c.WithContext(ctx), func(ctx context.Context, connection *pop.Connection) error {
299-
return fn(connection)
301+
return errors.WithStack(fn(connection))
300302
})
301303
}
302304

@@ -319,7 +321,7 @@ func (mb *MigrationBox) createMigrationStatusTableTransaction(ctx context.Contex
319321
}
320322
return nil
321323
}); err != nil {
322-
return err
324+
return errors.WithStack(err)
323325
}
324326
}
325327
}
@@ -341,14 +343,14 @@ func (mb *MigrationBox) CreateSchemaMigrations(ctx context.Context) error {
341343
if err != nil {
342344
mb.l.WithError(err).WithField("migration_table", mtn).Debug("An error occurred while checking for the legacy migration table, maybe it does not exist yet? Trying to create.")
343345
// This means that the legacy pop migrator has not yet been applied
344-
return mb.createTransactionalMigrationTable(ctx, c, mb.l)
346+
return errors.WithStack(mb.createTransactionalMigrationTable(ctx, c, mb.l))
345347
}
346348

347349
mb.l.WithField("migration_table", mtn).Debug("A migration table exists, checking if it is a transactional migration table.")
348350
_, err = c.Store.Exec(fmt.Sprintf("select version, version_self from %s", mtn))
349351
if err != nil {
350352
mb.l.WithError(err).WithField("migration_table", mtn).Debug("An error occurred while checking for the transactional migration table, maybe it does not exist yet? Trying to create.")
351-
return mb.migrateToTransactionalMigrationTable(ctx, c, mb.l)
353+
return errors.WithStack(mb.migrateToTransactionalMigrationTable(ctx, c, mb.l))
352354
}
353355

354356
mb.l.WithField("migration_table", mtn).Debug("Migration tables exist and are up to date.")
@@ -436,7 +438,7 @@ func (mb *MigrationBox) Status(ctx context.Context) (MigrationStatuses, error) {
436438
// It also means that we can ignore this state and act as if no migrations have been applied yet.
437439
} else {
438440
// On any other error, we fail.
439-
return nil, errors.Wrapf(err, "problem with migration")
441+
return nil, errors.Wrap(err, "problem with migration")
440442
}
441443
}
442444

@@ -471,12 +473,12 @@ func (mb *MigrationBox) DumpMigrationSchema(ctx context.Context) error {
471473
schema := "schema.sql"
472474
f, err := os.Create(schema) //#nosec:G304) //#nosec:G304
473475
if err != nil {
474-
return err
476+
return errors.WithStack(err)
475477
}
476478
err = c.Dialect.DumpSchema(f)
477479
if err != nil {
478480
_ = os.RemoveAll(schema)
479-
return err
481+
return errors.WithStack(err)
480482
}
481483
return nil
482484
}
@@ -501,24 +503,24 @@ func (mb *MigrationBox) exec(ctx context.Context, fn func() error) error {
501503

502504
if mb.c.Dialect.Name() == "sqlite3" {
503505
if err := mb.c.RawQuery("PRAGMA foreign_keys=OFF").Exec(); err != nil {
504-
return err
506+
return sqlcon.HandleError(err)
505507
}
506508
}
507509

508510
if mb.c.Dialect.Name() == "cockroach" {
509511
outer := fn
510512
fn = func() error {
511-
return crdb.Execute(outer)
513+
return errors.WithStack(crdb.Execute(outer))
512514
}
513515
}
514516

515517
if err := fn(); err != nil {
516-
return err
518+
return errors.WithStack(err)
517519
}
518520

519521
if mb.c.Dialect.Name() == "sqlite3" {
520522
if err := mb.c.RawQuery("PRAGMA foreign_keys=ON").Exec(); err != nil {
521-
return err
523+
return sqlcon.HandleError(err)
522524
}
523525
}
524526

oryx/popx/sql_template_funcs.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@
44
package popx
55

66
import (
7-
"fmt"
87
"regexp"
8+
9+
"github.com/pkg/errors"
910
)
1011

1112
var SQLTemplateFuncs = map[string]interface{}{
@@ -16,7 +17,7 @@ var identifierPattern = regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9_]*$")
1617

1718
func Identifier(i string) (string, error) {
1819
if !identifierPattern.MatchString(i) {
19-
return "", fmt.Errorf("invalid SQL identifier '%s'", i)
20+
return "", errors.Errorf("invalid SQL identifier '%s'", i)
2021
}
2122
return i, nil
2223
}

oryx/popx/transaction.go

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/cockroachdb/cockroach-go/v2/crdb"
1111
"github.com/jmoiron/sqlx"
12+
"github.com/pkg/errors"
1213
"github.com/prometheus/client_golang/prometheus"
1314

1415
"github.com/ory/pop/v6"
@@ -30,32 +31,32 @@ func Transaction(ctx context.Context, connection *pop.Connection, callback func(
3031
c := ctx.Value(transactionKey)
3132
if c != nil {
3233
if conn, ok := c.(*pop.Connection); ok {
33-
return callback(ctx, conn.WithContext(ctx))
34+
return errors.WithStack(callback(ctx, conn.WithContext(ctx)))
3435
}
3536
}
3637

3738
if connection.Dialect.Name() == "cockroach" {
3839
return connection.WithContext(ctx).Dialect.Lock(func() error {
3940
transaction, err := connection.NewTransaction()
4041
if err != nil {
41-
return err
42+
return errors.WithStack(err)
4243
}
4344

4445
attempt := 0
45-
return crdb.ExecuteInTx(ctx, sqlxTxAdapter{transaction.TX.Tx}, func() error {
46+
return errors.WithStack(crdb.ExecuteInTx(ctx, sqlxTxAdapter{transaction.TX.Tx}, func() error {
4647
attempt++
4748
if attempt > 1 {
4849
caller := caller()
4950
transactionRetries.WithLabelValues(caller).Inc()
5051
}
51-
return callback(WithTransaction(ctx, transaction), transaction)
52-
})
52+
return errors.WithStack(callback(WithTransaction(ctx, transaction), transaction))
53+
}))
5354
})
5455
}
5556

56-
return connection.WithContext(ctx).Transaction(func(tx *pop.Connection) error {
57-
return callback(WithTransaction(ctx, tx), tx)
58-
})
57+
return errors.WithStack(connection.WithContext(ctx).Transaction(func(tx *pop.Connection) error {
58+
return errors.WithStack(callback(WithTransaction(ctx, tx), tx))
59+
}))
5960
}
6061

6162
func GetConnection(ctx context.Context, connection *pop.Connection) *pop.Connection {
@@ -76,15 +77,15 @@ var _ crdb.Tx = sqlxTxAdapter{}
7677

7778
func (s sqlxTxAdapter) Exec(ctx context.Context, query string, args ...interface{}) error {
7879
_, err := s.Tx.ExecContext(ctx, query, args...)
79-
return err
80+
return errors.WithStack(err)
8081
}
8182

8283
func (s sqlxTxAdapter) Commit(ctx context.Context) error {
83-
return s.Tx.Commit()
84+
return errors.WithStack(s.Tx.Commit())
8485
}
8586

8687
func (s sqlxTxAdapter) Rollback(ctx context.Context) error {
87-
return s.Tx.Rollback()
88+
return errors.WithStack(s.Tx.Rollback())
8889
}
8990

9091
var (

0 commit comments

Comments
 (0)