Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/codeql.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ jobs:
# For more details on CodeQL's query packs, refer to: https://docs.github.com/en/code-security/code-scanning/automatically-scanning-your-code-for-vulnerabilities-and-errors/configuring-code-scanning#using-queries-in-ql-packs
# queries: security-extended,security-and-quality

# If the analyze step fails for one of the languages you are analyzing with
# If the "analyze" step fails for one of the languages you are analyzing with
# "We were unable to automatically build your code", modify the matrix above
# to set the build mode to "manual" for that language. Then modify this step
# to build your code.
Expand Down
93 changes: 78 additions & 15 deletions .golangci.yaml
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
# Copyright the dmorph contributors.
# SPDX-License-Identifier: MPL-2.0

# Configuration file for golangci-lint
# See https://golangci-lint.run/usage/configuration/ for more information

Expand All @@ -9,25 +12,85 @@ run:

linters:
default: all
#enable:
# # Default linters
# - errcheck
# - govet
# - ineffassign
# - staticcheck
# - unused
# # Additional linters
# - gosec
# - misspell
# - revive
# - bodyclose
# - noctx

disable:
- exhaustruct
- forbidigo
- noinlineerr
- nonamedreturns
- wsl
- wsl_v5

Comment on lines 14 to 23
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Invalid key: use linters.enable-all instead of linters.default

golangci-lint doesn’t support linters.default: all. To enable all and selectively disable, use enable-all: true.

Apply this diff:

 linters:
-    default: all
+    enable-all: true
     disable:
         - exhaustruct
         - forbidigo
         - noinlineerr
         - nonamedreturns
         - wsl
         - wsl_v5
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
default: all
#enable:
# # Default linters
# - errcheck
# - govet
# - ineffassign
# - staticcheck
# - unused
# # Additional linters
# - gosec
# - misspell
# - revive
# - bodyclose
# - noctx
disable:
- exhaustruct
- forbidigo
- noinlineerr
- nonamedreturns
- wsl
- wsl_v5
linters:
enable-all: true
disable:
- exhaustruct
- forbidigo
- noinlineerr
- nonamedreturns
- wsl
- wsl_v5
🤖 Prompt for AI Agents
In the .golangci.yaml file between lines 14 and 23, replace the key 'default:
all' with 'enable-all: true' under the linters section to correctly enable all
linters. Keep the 'disable' list as is to selectively disable specific linters.
This change aligns with golangci-lint's configuration syntax requirements.

exclusions:
warn-unused: true

rules:
- path: main_test.go
- path: _test\.go
linters:
- gosec
- cyclop
- dupl
- dupword
- err113
- funlen
- gocognit
- maintidx
- nestif

- path: examples/
linters:
- err113

Comment on lines 24 to +42
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Misplaced and non-standard: move “exclusions” under issues.exclude-rules

The “exclusions” block and “warn-unused” aren’t valid under linters. Path- and linter-specific rules belong under issues.exclude-rules. “warn-unused” is not a recognized top-level option.

Proposed structure:

-    exclusions:
-        warn-unused: true
-
-        rules:
-            - path: _test\.go
-              linters:
-                  - cyclop
-                  - dupl
-                  - dupword
-                  - err113
-                  - funlen
-                  - gocognit
-                  - maintidx
-                  - nestif
-
-            - path: examples/
-              linters:
-                  - err113
+issues:
+    max-issues-per-linter: 0
+    max-same-issues: 0
+    exclude-rules:
+        # Example: ignore gochecknoglobals in internal test-exports file
+        - path: ^exports_internal_test\.go$
+          linters:
+              - gochecknoglobals
+        # Example: restrict heavy complexity checks to tests only by excluding them elsewhere
+        - path: ^(?!.*_test\.go).*$  # non-test files
+          linters:
+              - cyclop
+              - dupl
+              - dupword
+              - funlen
+              - gocognit
+              - maintidx
+              - nestif
+        # Only run err113 in examples by excluding it elsewhere, or vice versa per intent
+        - path: ^examples/
+          linters:
+              - $all

Note: golangci-lint doesn’t support “only run these linters for this path”. The approach is to exclude linters in paths where they shouldn’t run.

Please confirm against the official schema; if desired, I can tailor a precise exclude-rules matrix for your intent.


🌐 Web query:

Is "exclusions" a valid top-level option in GolangCI-Lint configuration? 
Does GolangCI-Lint support a top-level "warn-unused" setting? 
Should path- and linter-specific rules be placed under an "exclusions" block, or under "issues.exclude-rules" instead?

💡 Result:

"exclusions" is not a valid top-level option in GolangCI-Lint's configuration.
GolangCI-Lint does not have a top-level "warn-unused" setting.
Path- and linter-specific rules should be placed under "issues.exclude-rules", not an "exclusions" block.

Details and Evidence

  • No "exclusions" top-level block:
    The official GolangCI-Lint [documentation][7], open-source config references[4][9], and blog articles show the recognized top-level keys such as run, output, issues, linters, and linters-settings, but none refer to or support an exclusions key. Further, usage of exemptions is always specified within the issues block, under exclude or exclude-rules[4][9].

  • No "warn-unused" top-level setting:
    There is no mention of a warn-unused option in the config file or CLI switches[7]. Unused code warnings are handled via the unused linter and its specific settings, which are configured within linters-settings: unused: ...[8][10].

  • Where to place rules for excluding issues:

    • To exclude issues by path, linter, or code pattern, use the issues.exclude-rules list within your configuration:
      issues:
        exclude-rules:
          - path: _test\.go
            linters:
              - errcheck
            text: "error return value not checked"
      [3][4]
      This is supported directly in [documentation][3] and observed in community examples.
    • There is only one place for structured exclusion rules; issues.exclude is also supported, but for regex patterns only, not rule objects.

Additional context

  • The run.skip-dirs and run.skip-files options can be used at top-level (under run) for broad file/directory exclusions.
  • The confusion may arise from similar-sounding options in other linter tools or extensions.

Conclusion

For all issue/path/linter-specific exclusions, use issues.exclude-rules. "exclusions" and "warn-unused" are not valid top-level settings in GolangCI-Lint.[3][4][7][8][9]

Citations:


Move “exclusions” and “warn-unused” into issues.exclude-rules

GolangCI-Lint does not recognize a top-level exclusions block or warn-unused setting. All path- and linter-specific ignores must live under issues.exclude-rules.

Please update .golangci.yaml (lines 24–42) as follows:

-    exclusions:
-        warn-unused: true
-
-        rules:
-            - path: _test\.go
-              linters:
-                  - cyclop
-                  - dupl
-                  - dupword
-                  - err113
-                  - funlen
-                  - gocognit
-                  - maintidx
-                  - nestif
-
-            - path: examples/
-              linters:
-                  - err113
+issues:
+  # Prevent any unused warnings; configure via the “unused” linter instead
+  max-issues-per-linter: 0
+  max-same-issues: 0
+  exclude-rules:
+    # Don’t run complexity checks on non-test files
+    - path: ^(?!.*_test\.go).*$   # all files except *_test.go
+      linters:
+        - cyclop
+        - dupl
+        - dupword
+        - funlen
+        - gocognit
+        - maintidx
+        - nestif
+
+    # Only ignore err113 in examples/
+    - path: ^examples/
+      linters:
+        - err113

Tags:

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In .golangci.yaml around lines 24 to 42, the "exclusions" and "warn-unused"
settings are incorrectly placed at the top level. Move these settings under the
"issues.exclude-rules" section to comply with GolangCI-Lint configuration
requirements. Specifically, nest the existing path and linter-specific ignore
rules inside "issues.exclude-rules" and set "warn-unused" as a property within
that section.

settings:
cyclop:
max-complexity: 25

depguard:
rules:
main:
files:
- $all
- "!$test"
- "!**/examples/**/*"
allow:
- $gostd
test:
files:
- $test
- "**/examples/**/*"
allow:
- $gostd
- github.com/stretchr/testify/assert
- github.com/stretchr/testify/require
- github.com/AlphaOne1/dmorph
- modernc.org/sqlite

exhaustive:
default-signifies-exhaustive: true

mnd:
ignored-numbers:
- "2"

paralleltest:
ignore-missing: true

perfsprint:
errorf: false

testpackage:
skip-regexp: internal_test\.go

varnamelen:
max-distance: 10
ignore-names:
- ctx
- db
- err
- tx

whitespace:
multi-if: true
multi-func: true

issues:
max-issues-per-linter: 0
Expand Down
32 changes: 21 additions & 11 deletions dialects.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,48 +4,58 @@
package dmorph

import (
"context"
"database/sql"
"errors"
"fmt"
)

// BaseDialect is a convenience type for databases that manage the necessary operations solely using
// queries. Defining the CreateTemplate, AppliedTemplate and RegisterTemplate enables the BaseDialect to
// perform all the necessary operation to fulfill the Dialect interface.
// perform all the necessary operations to fulfill the Dialect interface.
type BaseDialect struct {
CreateTemplate string // statement ensuring the existence of the migration table
AppliedTemplate string // statement getting applied migrations ordered by application date
RegisterTemplate string // statement registering a migration
}

// EnsureMigrationTableExists ensures that the migration table, saving the applied migrations ids, exists.
func (b BaseDialect) EnsureMigrationTableExists(db *sql.DB, tableName string) error {
tx, err := db.Begin()
func (b BaseDialect) EnsureMigrationTableExists(ctx context.Context, db *sql.DB, tableName string) error {
tx, err := db.BeginTx(ctx, nil)

if err != nil {
return err
}

defer func() { _ = tx.Rollback() }()

_, execErr := tx.Exec(fmt.Sprintf(b.CreateTemplate, tableName))
// Safety net for unexpected panics
defer func() {
if tx != nil {
_ = tx.Rollback()
}
}()

if execErr != nil {
if _, execErr := tx.ExecContext(ctx, fmt.Sprintf(b.CreateTemplate, tableName)); execErr != nil {
rollbackErr := tx.Rollback()
tx = nil

return errors.Join(execErr, rollbackErr)
}

if err := tx.Commit(); err != nil {
rollbackErr := tx.Rollback()
tx = nil

return errors.Join(err, rollbackErr)
}

tx = nil

return nil
}

// AppliedMigrations gets the already applied migrations from the database, ordered by application date.
func (b BaseDialect) AppliedMigrations(db *sql.DB, tableName string) ([]string, error) {
rows, rowsErr := db.Query(fmt.Sprintf(b.AppliedTemplate, tableName))
func (b BaseDialect) AppliedMigrations(ctx context.Context, db *sql.DB, tableName string) ([]string, error) {
rows, rowsErr := db.QueryContext(ctx, fmt.Sprintf(b.AppliedTemplate, tableName))

if rowsErr != nil {
return nil, rowsErr
Expand All @@ -67,8 +77,8 @@ func (b BaseDialect) AppliedMigrations(db *sql.DB, tableName string) ([]string,
}

// RegisterMigration registers a migration in the migration table.
func (b BaseDialect) RegisterMigration(tx *sql.Tx, id string, tableName string) error {
_, err := tx.Exec(fmt.Sprintf(b.RegisterTemplate, tableName),
func (b BaseDialect) RegisterMigration(ctx context.Context, tx *sql.Tx, id string, tableName string) error {
_, err := tx.ExecContext(ctx, fmt.Sprintf(b.RegisterTemplate, tableName),
sql.Named("id", id))

return err
Expand Down
97 changes: 26 additions & 71 deletions dialects_test.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
// Copyright the DMorph contributors.
// SPDX-License-Identifier: MPL-2.0

package dmorph
package dmorph_test

import (
"database/sql"
"os"
"regexp"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/AlphaOne1/dmorph"
)

// TestDialectStatements verifies that each database dialect has valid and
Expand All @@ -19,19 +19,17 @@ func TestDialectStatements(t *testing.T) {
// that the statements for the databases are somehow filled
tests := []struct {
name string
caller func() BaseDialect
caller func() dmorph.BaseDialect
}{
{name: "CSVQ", caller: DialectCSVQ},
{name: "DB2", caller: DialectDB2},
{name: "MSSQL", caller: DialectMSSQL},
{name: "MySQL", caller: DialectMySQL},
{name: "Oracle", caller: DialectOracle},
{name: "Postgres", caller: DialectPostgres},
{name: "SQLite", caller: DialectSQLite},
{name: "CSVQ", caller: dmorph.DialectCSVQ},
{name: "DB2", caller: dmorph.DialectDB2},
{name: "MSSQL", caller: dmorph.DialectMSSQL},
{name: "MySQL", caller: dmorph.DialectMySQL},
{name: "Oracle", caller: dmorph.DialectOracle},
{name: "Postgres", caller: dmorph.DialectPostgres},
{name: "SQLite", caller: dmorph.DialectSQLite},
}

re := regexp.MustCompile("%s")

for k, v := range tests {
d := v.caller()

Expand All @@ -40,83 +38,54 @@ func TestDialectStatements(t *testing.T) {
}
assert.Contains(t, d.CreateTemplate, "%s",
"no table name placeholder in create template for", v.name)
assert.Regexp(t, re, d.CreateTemplate)

if len(d.AppliedTemplate) < 10 {
t.Errorf("%v: applied template is too short for %v", k, v.name)
}
assert.Contains(t, d.AppliedTemplate, "%s",
"no table name placeholder in applied template for", v.name)
assert.Regexp(t, re, d.AppliedTemplate)

if len(d.RegisterTemplate) < 10 {
t.Errorf("%v: register template is too short for %v", k, v.name)
}
assert.Contains(t, d.RegisterTemplate, "%s",
"no table name placeholder in register template for", v.name)
assert.Regexp(t, re, d.RegisterTemplate)
}
}

// TestCallsOnClosedDB verifies that methods fail as expected when called on a closed database connection.
func TestCallsOnClosedDB(t *testing.T) {
dbFile, dbFileErr := prepareDB()

if dbFileErr != nil {
t.Errorf("DB file could not be created: %v", dbFileErr)
} else {
defer func() { _ = os.Remove(dbFile) }()
}

db, dbErr := sql.Open("sqlite", dbFile)

if dbErr != nil {
t.Errorf("DB file could not be created: %v", dbErr)
} else {
_ = db.Close()
}
db := openTempSQLite(t)
require.NoError(t, db.Close())

assert.Error(t,
DialectSQLite().EnsureMigrationTableExists(db, "irrelevant"),
dmorph.DialectSQLite().EnsureMigrationTableExists(t.Context(), db, "irrelevant"),
"expected error on closed database")

_, err := DialectSQLite().AppliedMigrations(db, "irrelevant")
_, err := dmorph.DialectSQLite().AppliedMigrations(t.Context(), db, "irrelevant")
assert.Error(t, err, "expected error on closed database")
}

// TestEnsureMigrationTableExistsSQLError tests the EnsureMigrationTableExists function
// for handling SQL errors during execution.
func TestEnsureMigrationTableExistsSQLError(t *testing.T) {
d := BaseDialect{
dialect := dmorph.BaseDialect{
CreateTemplate: `
CRATE TABLE test (
id VARCHAR(255) PRIMARY KEY,
create_ts TIMESTAMP DEFAULT CURRENT_TIMESTAMP
)`,
}

dbFile, dbFileErr := prepareDB()

if dbFileErr != nil {
t.Errorf("DB file could not be created: %v", dbFileErr)
} else {
defer func() { _ = os.Remove(dbFile) }()
}
db := openTempSQLite(t)

db, dbErr := sql.Open("sqlite", dbFile)

if dbErr != nil {
t.Errorf("DB file could not be created: %v", dbErr)
} else {
defer func() { _ = db.Close() }()
}

assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error")
assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error")
}

// TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists when a commit error occurs.
// TestEnsureMigrationTableExistsCommitError tests the behavior of EnsureMigrationTableExists
// when a commit error occurs.
func TestEnsureMigrationTableExistsCommitError(t *testing.T) {
d := BaseDialect{
dialect := dmorph.BaseDialect{
CreateTemplate: `
CREATE TABLE t0 (
id INTEGER PRIMARY KEY
Expand All @@ -134,25 +103,11 @@ func TestEnsureMigrationTableExistsCommitError(t *testing.T) {
DELETE FROM t0 WHERE id = 1;`,
}

dbFile, dbFileErr := prepareDB()

if dbFileErr != nil {
t.Errorf("DB file could not be created: %v", dbFileErr)
} else {
defer func() { _ = os.Remove(dbFile) }()
}

db, dbErr := sql.Open("sqlite", dbFile)

if dbErr != nil {
t.Errorf("DB file could not be created: %v", dbErr)
} else {
defer func() { _ = db.Close() }()
}
db := openTempSQLite(t)

_, execErr := db.Exec("PRAGMA foreign_keys = ON")
_, execErr := db.ExecContext(t.Context(), "PRAGMA foreign_keys = ON")

assert.NoError(t, execErr, "foreign keys checking could not be enabled")
require.NoError(t, execErr, "foreign keys checking could not be enabled")

assert.Error(t, d.EnsureMigrationTableExists(db, "test"), "expected error")
assert.Error(t, dialect.EnsureMigrationTableExists(t.Context(), db, "test"), "expected error")
}
18 changes: 18 additions & 0 deletions exports_internal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package dmorph

import (
"context"
"database/sql"
)

// The exported names in this file are only used for internal testing and are not part of the public API.

var (
TapplyStepsStream = applyStepsStream
TmigrationFromFileFS = migrationFromFileFS
TmigrationOrder = migrationOrder
)
Comment on lines +10 to +14
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace global var aliases with thin wrapper functions to satisfy gochecknoglobals

Global variables trigger gochecknoglobals. Use wrappers instead; behavior unchanged and constrained to _test.go files.

Apply this diff:

-package dmorph
-
-import "database/sql"
-
-// The exported names in this file are only used for internal testing and are not part of the public API.
-
-var (
-    TapplyStepsStream    = applyStepsStream
-    TmigrationFromFileFS = migrationFromFileFS
-    TmigrationOrder      = migrationOrder
-)
-
-func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error {
-    return m.applyMigrations(db, lastMigration)
-}
+package dmorph
+
+import (
+    "database/sql"
+    "io"
+    "io/fs"
+    "log/slog"
+)
+
+// The exported names in this file are only used for internal testing and are not part of the public API.
+
+func TapplyStepsStream(tx *sql.Tx, r io.Reader, id string, log *slog.Logger) error {
+    return applyStepsStream(tx, r, id, log)
+}
+
+func TmigrationFromFileFS(name string, dir fs.FS, log *slog.Logger) FileMigration {
+    return migrationFromFileFS(name, dir, log)
+}
+
+func TmigrationOrder(m, n Migration) int {
+    return migrationOrder(m, n)
+}
+
+func (m *Morpher) TapplyMigrations(db *sql.DB, lastMigration string) error {
+    return m.applyMigrations(db, lastMigration)
+}
🧰 Tools
🪛 golangci-lint (2.2.2)

8-8: TapplyStepsStream is a global variable

(gochecknoglobals)


9-9: TmigrationFromFileFS is a global variable

(gochecknoglobals)


10-10: TmigrationOrder is a global variable

(gochecknoglobals)

🤖 Prompt for AI Agents
In exports_internal_test.go around lines 7 to 11, replace the global variable
aliases with thin wrapper functions that return the original variables. This
avoids global variables triggering gochecknoglobals while preserving the same
behavior. Define functions named TapplyStepsStream, TmigrationFromFileFS, and
TmigrationOrder that return the respective original variables instead of using
var aliases.


func (m *Morpher) TapplyMigrations(ctx context.Context, db *sql.DB, lastMigration string) error {
return m.applyMigrations(ctx, db, lastMigration)
}
Loading
Loading