Skip to content

Update to go 1.26, and other various backend updates#1364

Open
tankerkiller125 wants to merge 3 commits intomainfrom
mk/go-updates
Open

Update to go 1.26, and other various backend updates#1364
tankerkiller125 wants to merge 3 commits intomainfrom
mk/go-updates

Conversation

@tankerkiller125
Copy link
Contributor

@tankerkiller125 tankerkiller125 commented Mar 11, 2026

What type of PR is this?

  • cleanup

What this PR does / why we need it:

This pull request upgrades the backend to use Go 1.26 and switches the PostgreSQL driver from pq to pgx. It also updates related dependencies, modifies database connection logic, and fixes several code patterns for improved reliability and maintainability. The CI workflows and documentation are updated to reflect these changes.

Backend and Dependency Upgrades:

  • Upgraded Go version to 1.26 across the codebase, including backend/go.mod, workflow files, and documentation. [1] [2] [3] [4] [5] [6] [7] [8] [9]
  • Replaced github.com/lib/pq with github.com/jackc/pgx/v5 as the PostgreSQL driver, updated imports and dependency list, and added related indirect dependencies. [1] [2] [3]
  • Updated various dependencies to their latest versions for improved stability and compatibility. [1] [2] [3] [4] [5]

Database Connection and Migration Handling:

  • Modified database connection logic in backend/app/api/main.go to use pgx for PostgreSQL, and added proper connection closing with error logging. [1] [2]
  • Updated migration code to use pointer allocation for column name scanning, improving migration reliability.

Testing and Code Quality Improvements:

  • Updated rate limiter tests to use the new function signature with an explicit boolean parameter, ensuring test accuracy. [1] [2] [3] [4] [5] [6]
  • Refactored code to use pointer allocation patterns for improved correctness, including item patching and CSV generation. [1] [2] [3] [4]

Workflow and Documentation Updates:

  • Updated CI workflows to use Go 1.26 and clarified triggering conditions for copilot setup. [1] [2] [3] [4] [5] [6] [7] [8]
  • Updated .github/instructions/code.instructions.md to reflect the new backend Go version requirement.

Miscellaneous:

  • Changed SQLite driver import in tests to use the project’s custom package for consistency.

Summary by CodeRabbit

  • Chores
    • Updated Go requirement to 1.26+ and CI workflows accordingly.
    • Upgraded database driver and multiple dependencies for compatibility and security.
  • Refactor
    • Improved backend DB initialization and shutdown for more reliable startup/cleanup.
    • Simplified internal pointer/allocation patterns to reduce temporary variables.
  • Tests
    • Updated test suite to reflect internal API and allocation changes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 11, 2026

Walkthrough

Bumps Go toolchain to 1.26 across module and CI; swaps Postgres driver to pgx/v5 and adjusts Ent DB-open flow to use sql.DB + entsql.OpenDB; replaces mattn/go-sqlite3 with a cgofree sqlite driver in tests; many pointer initializations changed to use new(...) allocations; rate-limiter API/tests updated.

Changes

Cohort / File(s) Summary
Go toolchain & CI
\.github/instructions/code.instructions.md, .github/workflows/binaries-publish.yaml, .github/workflows/copilot-setup-steps.yml, .github/workflows/e2e-partial.yaml, .github/workflows/partial-backend.yaml, .github/workflows/partial-frontend.yaml, backend/go.mod
Bumped Go to 1.26 in go.mod and CI; updated various dependencies and removed a push trigger from copilot workflow.
DB driver & startup flow
backend/app/api/main.go, backend/go.mod
Replaced lib/pq with pgx/v5 and stdlib adapter; startup now resolves driver, opens sql.DB, wraps with ent via entsql.OpenDB, runs migrations, and defers explicit closers.
SQLite driver swap (tests)
backend/internal/core/services/main_test.go, backend/internal/data/repo/main_test.go
Replaced runtime sqlite import with internal cgofree sqlite driver and adjusted related imports.
Pointer-allocation refactor (multiple files)
backend/internal/data/... (repo_.go, repo__test.go), backend/internal/core/services/main_test.go, backend/internal/core/services/reporting/bill_of_materials.go, backend/pkgs/labelmaker/labelmaker_test.go, backend/internal/data/migrations/.../sync_children.go
Replaced many &localVar pointer initializations with new(...) allocations; combined map+marshal in BOM; one change in repo_item_templates.go appears to allocate a zero UUID instead of referencing the original field ID — requires review.
Rate limiter & middleware
backend/app/api/middleware_ratelimit_test.go, backend/app/api/middleware.go
Tests updated for newSimpleRateLimiter(..., trustProxy bool); removed authRateLimiter.clientIP method and use extractClientIP inline in key derivation.
Misc dependency updates
backend/go.mod
Broad dependency bumps (OpenTelemetry, googleapis, gRPC, golang.org/x/*, jackc/pgx family, and various indirects).

Sequence Diagram(s)

sequenceDiagram
    participant Startup as Startup (main.go)
    participant Config as Config
    participant DB as sql.DB (pgx/sqlite)
    participant Ent as ent.Client
    participant Migrator as Migrator

    Startup->>Config: read DB_DRIVER, DATABASE_URL
    Startup->>DB: sql.Open(driver, DATABASE_URL)
    DB-->>Startup: *sql.DB
    Startup->>Ent: entsql.OpenDB(dialect, DB) / ent.OpenDB(dialect, DB)
    Ent-->>Startup: *ent.Client
    Startup->>Migrator: run migrations via ent driver
    Migrator-->>Startup: migrations result
    Startup->>Startup: defer close(ent client) and DB.Close()
    Startup->>Startup: start HTTP server
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • katosdev
  • tonyaellie

Security Recommendations

  • Validate DB connection/config: confirm TLS, authentication, and pooling settings when migrating to pgx; verify connection string parsing equivalence.
  • Migration safety: run migrations against a staging DB with pgx to confirm behavior matches previous driver (index/constraint SQL compatibility).
  • Pointer changes: audit places where new(...) replaced &localVar for secrets (passwords) and IDs — ensure no zero-value secrets or incorrect IDs are introduced (notably repo_item_templates.go).
  • Dependency audit: run vulnerability scans on updated deps (jackc/pgx, OpenTelemetry, googleapis, gRPC).

Poem

🌱 Go 1.26 steps on the stage,
pgx and ent rewrite the page,
Pointers born from brand-new springs,
Migrations hum as CI sings,
Review the UUID — check tiny things.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: Go 1.26 upgrade and PostgreSQL driver migration with dependency updates, though it could be more specific about the driver change.
Description check ✅ Passed The description covers all required sections with comprehensive details about changes, including backend upgrades, database modifications, testing improvements, and workflow updates with relevant citations.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mk/go-updates

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@tankerkiller125 tankerkiller125 changed the title chore: update to go 1.26, use pgx for postgres, and remove direct dep… Update to go 1.26, and other various backend updates Mar 11, 2026
@coderabbitai coderabbitai bot requested review from katosdev and tonyaellie March 11, 2026 01:09
@coderabbitai coderabbitai bot added ⬆️ enhancement New feature or request go Pull requests that update Go code labels Mar 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.github/instructions/code.instructions.md (1)

114-114: ⚠️ Potential issue | 🟡 Minor

Documentation inconsistency: CI/CD section still references Go 1.24.

Line 114 states "Backend: Go 1.24" but the workflows have been updated to use Go 1.26. This inconsistency could confuse contributors about the actual Go version requirements.

📝 Proposed fix
-1. **Backend**: Go 1.24, golangci-lint, `task go:build`, `task go:coverage`
+1. **Backend**: Go 1.26, golangci-lint, `task go:build`, `task go:coverage`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/instructions/code.instructions.md at line 114, Update the
documentation line that currently reads "Backend: Go 1.24, golangci-lint, `task
go:build`, `task go:coverage`" to reference Go 1.26 so it matches the workflows;
ensure the phrase "Backend: Go 1.26, golangci-lint, `task go:build`, `task
go:coverage`" (or equivalent wording) is used and scan the CI/CD/README sections
for any other mentions of Go 1.24 to replace with 1.26 for consistency.
backend/internal/data/repo/repo_tokens.go (1)

66-70: ⚠️ Potential issue | 🔴 Critical

GetRoles has a compile-time error on line 70.

set.New() returns a Set[string] value, but Go's builtin new() only accepts types as arguments. Create a temporary variable and return a pointer to it instead:

Suggested fix
-	return new(set.New(roleStrings...)), nil
+	roleSet := set.New(roleStrings...)
+	return &roleSet, nil

This blocks the code from compiling and must be addressed before merging.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/internal/data/repo/repo_tokens.go` around lines 66 - 70, GetRoles is
using new(set.New(roleStrings...)) which is invalid because new() expects a
type; replace this by creating the set value first (e.g., s :=
set.New(roleStrings...)) and then return its pointer (return &s, nil). Update
references to the temporary variable created from set.New(roleStrings...) and
remove the erroneous use of new() so the function returns *set.Set[string]
correctly.
🧹 Nitpick comments (1)
backend/go.mod (1)

22-22: Please re-check Postgres TLS settings as part of the pgx migration.

This driver swap is a good time to confirm production DSNs still set sslmode and any CA/cert options explicitly, so the new client stack does not inherit weaker transport defaults from environment-specific config.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/go.mod` at line 22, Verify and enforce Postgres TLS settings after
migrating to github.com/jackc/pgx/v5 by locating where the DB connection/DSN is
built (e.g., functions like NewDB, OpenDB, or any use of
pgxpool.Connect/pgx.ParseConfig) and ensure sslmode is explicitly set (prefer
"verify-full" or "require") and that CA/client cert/key options
(sslrootcert/sslcert/sslkey or TLSConfig with RootCAs/Certificates) are supplied
from secure config; if DSNs are read from env, validate and override
missing/weak sslmode values and ensure TLSConfig is constructed and passed into
pgx connection config rather than relying on defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/app/api/main.go`:
- Around line 123-128: The code currently remaps postgres to "pgx" and calls
ent.Open(sqlDriver, databaseURL), which fails because
backend/internal/data/ent/client.go only supports "mysql", "postgres", and
"sqlite3"; stop remapping postgres to "pgx" and instead use Ent's "postgres"
dialect and create a *sql.DB using the "pgx" driver (e.g. sql.Open("pgx",
databaseURL)) and pass that DB into Ent (via ent.Open/ent.NewClient or the
OpenDB helper) so Ent sees the "postgres" dialect while pgx is the underlying
driver; also ensure you never log the connection string during driver
negotiation and validate the dialect mapping at initialization (e.g., check
sqlDriver is one of the supported values) to surface configuration errors early.

In `@backend/app/api/middleware_ratelimit_test.go`:
- Line 13: Add test cases that exercise the proxy-trusting branch by
constructing the rate limiter with trustProxy=true via newSimpleRateLimiter(3,
10*time.Second, true) and sending requests that include X-Real-IP and
X-Forwarded-For headers; assert that throttling is applied per the proxied
client IP (not the reverse proxy IP). Update the existing test instances that
call newSimpleRateLimiter(..., false) to include at least one parallel subtest
or table entry using trustProxy=true and build requests with those headers (use
the same test helper/request builder used in middleware_ratelimit_test.go) to
ensure header parsing logic and per-IP limits do not regress.

In `@backend/internal/core/services/main_test.go`:
- Around line 43-47: The test is using invalid syntax new(fk.Str(10)) when
constructing repo.UserCreate in the tRepos.Users.Create call; change Password to
take the address of a temporary string variable instead (e.g. create pw :=
fk.Str(10) and set Password: &pw) so Password is a *string; update the
repo.UserCreate construction in the test (the block around tRepos.Users.Create
and the Password field) accordingly.

In `@backend/internal/data/migrations/sqlite3/20241226183416_sync_children.go`:
- Line 22: The call to tx.QueryRowContext(ctx,
query).Scan(new("sync_child_items_locations")) uses new with a string literal;
replace it with a proper string pointer such as new(string) or a named variable
(e.g., var name string; tx.QueryRowContext(ctx, query).Scan(&name)) so Scan
writes into a valid *string; update the Scan invocation accordingly (refer to
the tx.QueryRowContext(...).Scan call in this file).

In `@backend/internal/data/repo/repo_group_test.go`:
- Line 29: The Password field in the test (Password: new("password123")) uses
invalid Go syntax; replace it with a pointer to a string value — e.g., declare a
local variable password := "password123" and set Password: &password — or, if lo
is available, use lo.ToPtr("password123"); apply the same fix for the Password
usage in repo_group_test.go and the other tests (repo_item_templates_test.go,
labelmaker_test.go) where new("...") is used.

In `@backend/internal/data/repo/repo_item_templates_test.go`:
- Around line 17-24: The test uses invalid calls like new(1), new(fk.Str(20)),
and new("") to initialize pointer fields (e.g., DefaultQuantity, DefaultName,
DefaultDescription, DefaultManufacturer, DefaultModelNumber,
DefaultWarrantyDetails) in repo_item_templates_test.go; fix by creating actual
variables or a small generic helper (e.g., ptr[T any](v T) *T) and replace
occurrences of new(value) with either &var or ptr(value), and update any other
occurrences noted (template.DefaultQuantity etc.) so the test compiles under
go:test.

In `@backend/internal/data/repo/repo_items.go`:
- Around line 195-196: The code incorrectly calls new(...) with values (e.g.,
new(mapLocationSummary(item.Edges.Location))) which uses Go's builtin new that
only accepts types; replace these with an address-of temporary variable instead:
create a local variable (e.g., loc := mapLocationSummary(item.Edges.Location))
and then assign its pointer (&loc) to the target. Do the same for the other
occurrence noted (the second new(...) around line 269) so you pass a pointer to
the struct rather than attempting to call new with a value.

In `@backend/internal/data/repo/repo_locations.go`:
- Around line 71-74: The code incorrectly uses
new(mapLocationSummary(location.Edges.Parent)) — new requires a type and you
cannot call new on a function return; instead capture the value returned by
mapLocationSummary(location.Edges.Parent) into a temporary and take its address,
e.g. tmp := mapLocationSummary(location.Edges.Parent); parent = &tmp,
referencing the parent variable, mapLocationSummary function, and
LocationSummary type.

In `@backend/internal/data/repo/repo_users_test.go`:
- Around line 12-17: In userFactory() the Password field incorrectly uses
new(fk.Str(10)) (invalid syntax); generate the password string into a local
variable (e.g., pwd := fk.Str(10)) and assign its address to Password (Password:
&pwd) so Password is a *string; ensure you use fk.Str(10) for the value and
follow the same pointer-handling pattern used in production fixtures for
password fields in UserCreate.

In `@backend/pkgs/labelmaker/labelmaker_test.go`:
- Line 58: The test currently uses invalid pointer construction new("...") for
RegularFontPath; replace these with a proper pointer to a string (e.g., declare
a local fontPath variable and use &fontPath) or use a small generic helper
ptr[T](v T) *T and pass ptr("/non/existent/path/font.ttf") so RegularFontPath is
a *string; update the occurrences around RegularFontPath (and the other similar
literal pointer usages in this file) to use the chosen approach.

---

Outside diff comments:
In @.github/instructions/code.instructions.md:
- Line 114: Update the documentation line that currently reads "Backend: Go
1.24, golangci-lint, `task go:build`, `task go:coverage`" to reference Go 1.26
so it matches the workflows; ensure the phrase "Backend: Go 1.26, golangci-lint,
`task go:build`, `task go:coverage`" (or equivalent wording) is used and scan
the CI/CD/README sections for any other mentions of Go 1.24 to replace with 1.26
for consistency.

In `@backend/internal/data/repo/repo_tokens.go`:
- Around line 66-70: GetRoles is using new(set.New(roleStrings...)) which is
invalid because new() expects a type; replace this by creating the set value
first (e.g., s := set.New(roleStrings...)) and then return its pointer (return
&s, nil). Update references to the temporary variable created from
set.New(roleStrings...) and remove the erroneous use of new() so the function
returns *set.Set[string] correctly.

---

Nitpick comments:
In `@backend/go.mod`:
- Line 22: Verify and enforce Postgres TLS settings after migrating to
github.com/jackc/pgx/v5 by locating where the DB connection/DSN is built (e.g.,
functions like NewDB, OpenDB, or any use of pgxpool.Connect/pgx.ParseConfig) and
ensure sslmode is explicitly set (prefer "verify-full" or "require") and that
CA/client cert/key options (sslrootcert/sslcert/sslkey or TLSConfig with
RootCAs/Certificates) are supplied from secure config; if DSNs are read from
env, validate and override missing/weak sslmode values and ensure TLSConfig is
constructed and passed into pgx connection config rather than relying on
defaults.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4fbab1ee-ede7-4f43-b09a-d0e7821dab30

📥 Commits

Reviewing files that changed from the base of the PR and between 90b6912 and 2cc666d.

⛔ Files ignored due to path filters (1)
  • backend/go.sum is excluded by !**/*.sum
📒 Files selected for processing (23)
  • .github/instructions/code.instructions.md
  • .github/workflows/binaries-publish.yaml
  • .github/workflows/copilot-setup-steps.yml
  • .github/workflows/e2e-partial.yaml
  • .github/workflows/partial-backend.yaml
  • .github/workflows/partial-frontend.yaml
  • backend/app/api/main.go
  • backend/app/api/middleware_ratelimit_test.go
  • backend/go.mod
  • backend/internal/core/services/main_test.go
  • backend/internal/core/services/reporting/bill_of_materials.go
  • backend/internal/core/services/service_items.go
  • backend/internal/data/migrations/postgres/20250112202302_sync_children.go
  • backend/internal/data/migrations/sqlite3/20241226183416_sync_children.go
  • backend/internal/data/repo/main_test.go
  • backend/internal/data/repo/repo_group_test.go
  • backend/internal/data/repo/repo_item_templates.go
  • backend/internal/data/repo/repo_item_templates_test.go
  • backend/internal/data/repo/repo_items.go
  • backend/internal/data/repo/repo_locations.go
  • backend/internal/data/repo/repo_tokens.go
  • backend/internal/data/repo/repo_users_test.go
  • backend/pkgs/labelmaker/labelmaker_test.go

@coderabbitai coderabbitai bot added the review needed A review is needed on this PR or Issue label Mar 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/app/api/main.go`:
- Around line 157-177: The code currently defers closing db, drv, and c
(variables db, drv created via entsql.OpenDB and ent.NewClient), which causes
redundant Close() calls and false-positive shutdown errors; remove the defer
wrappers that call db.Close() and drv.Close() and keep only defer c.Close()
(logging errors on c.Close() as before), relying on the Client→Driver→*sql.DB
ownership cascade; do not add DSN or sensitive data to log messages.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 533c4fd8-0855-4fc4-9d2e-9632f0d7f99b

📥 Commits

Reviewing files that changed from the base of the PR and between 2cc666d and 48f34ea.

📒 Files selected for processing (4)
  • .github/workflows/copilot-setup-steps.yml
  • backend/app/api/main.go
  • backend/app/api/middleware.go
  • backend/internal/data/repo/repo_items.go
💤 Files with no reviewable changes (1)
  • backend/app/api/middleware.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • backend/internal/data/repo/repo_items.go
  • .github/workflows/copilot-setup-steps.yml

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (2)
backend/app/api/main.go (2)

140-156: Consider configuring connection pool parameters for production resilience.

The sql.Open call creates a connection pool but uses default settings. For production workloads, explicitly configuring pool parameters helps prevent connection exhaustion and improves reliability:

♻️ Suggested pool configuration
 	db, err := sql.Open(driverName, databaseURL)
 	if err != nil {
 		log.Error().
 			Err(err).
 			Str("driver", strings.ToLower(cfg.Database.Driver)).
 			Str("host", cfg.Database.Host).
 			Str("port", cfg.Database.Port).
 			Str("database", cfg.Database.Database).
 			Msg("failed opening connection to {driver} database at {host}:{port}/{database}")
 		return fmt.Errorf("failed opening connection to %s database at %s:%s/%s: %w",
 			strings.ToLower(cfg.Database.Driver),
 			cfg.Database.Host,
 			cfg.Database.Port,
 			cfg.Database.Database,
 			err,
 		)
 	}
+
+	// Configure connection pool for production reliability
+	db.SetMaxOpenConns(25)
+	db.SetMaxIdleConns(5)
+	db.SetConnMaxLifetime(5 * time.Minute)

These values should ideally be configurable via cfg.Database rather than hardcoded. Consider adding MaxOpenConns, MaxIdleConns, and ConnMaxLifetime fields to the database config.

Security note: Good practice here—the error logging correctly omits the password and full connection string, logging only the driver, host, port, and database name.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/app/api/main.go` around lines 140 - 156, The DB connection created by
sql.Open (db) needs explicit pool configuration: add configurable fields to
cfg.Database (e.g., MaxOpenConns int, MaxIdleConns int, ConnMaxLifetime
string/int seconds or time.Duration) and after successful sql.Open set
db.SetMaxOpenConns(cfg.Database.MaxOpenConns),
db.SetMaxIdleConns(cfg.Database.MaxIdleConns) and
db.SetConnMaxLifetime(parsedDuration) (parse ConnMaxLifetime into
time.Duration); ensure you handle zero/omitted values with sensible defaults and
keep these changes near the sql.Open block so the connection pool is configured
before use.

127-138: Consider removing the unreachable "sqlite" variant.

The "sqlite" case on line 133 is defensive but unreachable: setupDatabaseURL() (called at line 122) only accepts config.DriverSqlite3 ("sqlite3") or config.DriverPostgres ("postgres"), and returns an error for anything else. Any config value of "sqlite" would fail before reaching this switch.

♻️ Optional cleanup
-	case config.DriverSqlite3, "sqlite":
+	case config.DriverSqlite3:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/app/api/main.go` around lines 127 - 138, The switch in main.go
handling sqlDriver includes an unreachable "sqlite" case; remove the `"sqlite"`
alternative from the switch so the cases match the accepted drivers from
setupDatabaseURL() (use only config.DriverSqlite3 and config.DriverPostgres),
keeping driverName assignment for sqlite3 as "sqlite3" and sqlDriver =
dialect.SQLite in the case for config.DriverSqlite3; ensure the switch default
error remains unchanged and no other logic depends on the removed `"sqlite"`
literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/app/api/middleware_ratelimit_test.go`:
- Around line 20-42: Add a test case to the tests slice that verifies spoofed
proxy headers are ignored when trustProxy is false: create a testCase with name
like "SpoofedHeader", trustProxy: false, and a setupReq that sets r.RemoteAddr =
ip + ":1234" and also sets r.Header.Set("X-Real-IP", "<different-ip>") and
r.Header.Set("X-Forwarded-For", "<different-ip>, 10.0.0.2"); ensure the test
asserts the middleware/rate limiter (the code exercised by this table using the
tests variable and setupReq) uses r.RemoteAddr for client IP resolution rather
than the spoofed headers so the headers are effectively ignored when trustProxy
is false.
- Line 30: Replace the repeated literal "10.0.0.1:1234" in
middleware_ratelimit_test.go with a single package-level constant (e.g.
proxyRemoteAddr) declared near the top of the test file, then update all places
that set r.RemoteAddr = "10.0.0.1:1234" to use that constant; this removes the
duplicate literal (goconst) and keeps tests using the same proxy RemoteAddr
fixture.
- Line 47: The subtests create a limiter via newSimpleRateLimiter(3,
10*time.Second, tc.trustProxy) which launches a background cleanup goroutine but
never stops it; immediately after constructing limiter register a cleanup to
shut it down (e.g., t.Cleanup(func(){ limiter.Stop() })) so each test stops the
limiter goroutine before returning—apply this to the three occurrences that call
newSimpleRateLimiter in middleware_ratelimit_test.go.

---

Nitpick comments:
In `@backend/app/api/main.go`:
- Around line 140-156: The DB connection created by sql.Open (db) needs explicit
pool configuration: add configurable fields to cfg.Database (e.g., MaxOpenConns
int, MaxIdleConns int, ConnMaxLifetime string/int seconds or time.Duration) and
after successful sql.Open set db.SetMaxOpenConns(cfg.Database.MaxOpenConns),
db.SetMaxIdleConns(cfg.Database.MaxIdleConns) and
db.SetConnMaxLifetime(parsedDuration) (parse ConnMaxLifetime into
time.Duration); ensure you handle zero/omitted values with sensible defaults and
keep these changes near the sql.Open block so the connection pool is configured
before use.
- Around line 127-138: The switch in main.go handling sqlDriver includes an
unreachable "sqlite" case; remove the `"sqlite"` alternative from the switch so
the cases match the accepted drivers from setupDatabaseURL() (use only
config.DriverSqlite3 and config.DriverPostgres), keeping driverName assignment
for sqlite3 as "sqlite3" and sqlDriver = dialect.SQLite in the case for
config.DriverSqlite3; ensure the switch default error remains unchanged and no
other logic depends on the removed `"sqlite"` literal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 7c7f9822-733b-4ebd-876f-48864c6080fe

📥 Commits

Reviewing files that changed from the base of the PR and between 48f34ea and 2d0fc22.

📒 Files selected for processing (2)
  • backend/app/api/main.go
  • backend/app/api/middleware_ratelimit_test.go

name: "ProxyXRealIP",
trustProxy: true,
setupReq: func(r *http.Request, ip string) {
r.RemoteAddr = "10.0.0.1:1234" // Proxy IP
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Pull the proxy RemoteAddr fixture into a shared constant.

"10.0.0.1:1234" is repeated six times and is already tripping goconst, so this file will keep failing lint until it is centralized.

♻️ Minimal fix
+const proxyRemoteAddr = "10.0.0.1:1234"
+
 ...
-				r.RemoteAddr = "10.0.0.1:1234" // Proxy IP
+				r.RemoteAddr = proxyRemoteAddr // Proxy IP
As per coding guidelines `backend/**/*.go`: Backend Go code must pass golangci-lint validation with no errors (6-minute timeout in CI).

Also applies to: 38-38, 104-104, 164-164, 228-228, 297-297

🧰 Tools
🪛 GitHub Check: Backend Server Tests / Go

[failure] 30-30:
string 10.0.0.1:1234 has 6 occurrences, make it a constant (goconst)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/app/api/middleware_ratelimit_test.go` at line 30, Replace the
repeated literal "10.0.0.1:1234" in middleware_ratelimit_test.go with a single
package-level constant (e.g. proxyRemoteAddr) declared near the top of the test
file, then update all places that set r.RemoteAddr = "10.0.0.1:1234" to use that
constant; this removes the duplicate literal (goconst) and keeps tests using the
same proxy RemoteAddr fixture.

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// Create a rate limiter that allows 3 requests per 10 seconds
limiter := newSimpleRateLimiter(3, 10*time.Second, tc.trustProxy)
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Stop each test limiter before the subtest returns.

newSimpleRateLimiter starts a background cleanup goroutine, but these three subtests never shut it down. That leaves goroutines behind for the rest of the test process; please register cleanup right after construction.

♻️ Minimal fix
 limiter := newSimpleRateLimiter(3, 10*time.Second, tc.trustProxy)
+t.Cleanup(limiter.Stop)

Also applies to: 113-113, 172-172

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/app/api/middleware_ratelimit_test.go` at line 47, The subtests create
a limiter via newSimpleRateLimiter(3, 10*time.Second, tc.trustProxy) which
launches a background cleanup goroutine but never stops it; immediately after
constructing limiter register a cleanup to shut it down (e.g., t.Cleanup(func(){
limiter.Stop() })) so each test stops the limiter goroutine before
returning—apply this to the three occurrences that call newSimpleRateLimiter in
middleware_ratelimit_test.go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⬆️ enhancement New feature or request go Pull requests that update Go code review needed A review is needed on this PR or Issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant