refactor: enable DI for controllers and split Adapter interface#968
Merged
Conversation
…e mockgen commands - Updated Dockerfiles and docker-compose-test.yml to use Go 1.26. - Modified go.mod to reflect the new Go version. - Adjusted GitHub workflows for build and linting to use Go 1.26. - Refactored mockgen commands in Makefile for better clarity. - Enhanced adapter interfaces and added new catalog and permissions handling in controllers. - Removed deprecated GetDatabases function and introduced a new CatalogHandler for database operations.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR bumps Go from 1.25 to 1.26 and the version number to 2.0.0, decomposes the monolithic ChangesGo 1.26 and CI/tooling updates
Dependency-injected handler refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant muxRouter as mux.Router
participant CRUDStack as CRUDStack middleware
participant CRUDHandler
participant PermissionsChecker
participant QueryExecutor
participant ResponseCacher
Client->>muxRouter: GET /db/schema/table
muxRouter->>CRUDStack: route through negroni chain
CRUDStack->>PermissionsChecker: TablePermissions(table, op, user)
PermissionsChecker-->>CRUDStack: allowed
CRUDStack->>CRUDHandler: Select(w, r)
CRUDHandler->>PermissionsChecker: FieldsPermissions(r, table, read, user)
PermissionsChecker-->>CRUDHandler: fields
CRUDHandler->>QueryExecutor: QueryCtx(ctx, sql, params...)
QueryExecutor-->>CRUDHandler: Scanner
CRUDHandler->>ResponseCacher: BuntSet(url, bytes)
CRUDHandler-->>Client: 200 OK + JSON bytes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
controllers/script.go (1)
70-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap propagated script errors with
%w.These errors are returned to callers, so preserve the cause chain while adding context.
Suggested fix
- err = fmt.Errorf("could not get script %s/%s, %v", queriesPath, script, err) + err = fmt.Errorf("could not get script %s/%s: %w", queriesPath, script, err) @@ - err = fmt.Errorf("could not parse script %s/%s, %v", queriesPath, script, err) + err = fmt.Errorf("could not parse script %s/%s: %w", queriesPath, script, err)As per coding guidelines, "Return wrapped errors with context using
%wwhen propagating errors."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/script.go` around lines 70 - 80, In controllers/script.go, the error propagation in the script loading/parsing flow is losing the original cause because the fmt.Errorf calls in the script retrieval and h.scripts.ParseScript paths use plain formatting instead of wrapped errors. Update the relevant error returns in the script-handling logic (the block around ParseScript and the earlier script lookup failure) to preserve the underlying error chain with wrapped context using the existing identifiers queriesPath, script, and h.scripts.ParseScript, so callers can inspect the root cause while still getting the added message.Source: Coding guidelines
router/router.go (1)
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
RegisterRoutesfully dependency-injected.This exported function still reads
config.PrestConf.AuthEnabled, so route registration depends on global config even when callers provide injected handlers. Pass the auth-enabled flag intoRegisterRoutesor include it in a small routing deps struct.Based on learnings, avoid accidental breaking changes in API/config surfaces and prefer dependency injection where patterns already exist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/router.go` around lines 17 - 18, RegisterRoutes still depends on global config by reading config.PrestConf.AuthEnabled, which breaks full dependency injection. Update RegisterRoutes to receive the auth-enabled flag via an injected parameter or a small routing deps struct, and use that value when deciding whether to register the /auth route. Keep the change aligned with the existing RegisterRoutes and h.Auth.Login wiring so callers control route registration without reaching into global config.Source: Learnings
middlewares/crud_stack.go (1)
44-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a defensive copy of the middleware slice.
[]negroni.Handleris mutable. Exposings.handlerslets callers reorder or replace auth/access-control middleware after construction, which makes this stack easier to misuse and harder to reason about. Returning a copy keeps the chain immutable outside this package. As per coding guidelines, "Do not weaken authentication or authorization logic" and "Preserve thread-safety assumptions in cache, middleware, and shared components."Proposed fix
func (s *CRUDStack) Handlers() []negroni.Handler { - return s.handlers + return append([]negroni.Handler(nil), s.handlers...) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@middlewares/crud_stack.go` around lines 44 - 45, CRUDStack.Handlers currently returns the internal s.handlers slice directly, which allows callers to mutate the middleware chain after construction. Update Handlers() to return a defensive copy of the slice instead of the original, using CRUDStack and s.handlers as the key locations to change, so external code cannot reorder, replace, or remove auth/access-control middleware.Source: Coding guidelines
controllers/healthcheck.go (1)
25-26: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winClone the checklist in the constructor.
CheckListis a slice, so storing it directly lets callers mutate a live handler's checks after construction. Copying it here keeps the health handler stable if the source slice is reused or modified later. As per coding guidelines, "Preserve thread-safety assumptions in cache, middleware, and shared components."Proposed fix
func NewHealthHandler(checks CheckList) *HealthHandler { - return &HealthHandler{checks: checks} + return &HealthHandler{checks: append(CheckList(nil), checks...)} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/healthcheck.go` around lines 25 - 26, The NewHealthHandler constructor currently stores the incoming CheckList slice directly, so later mutations to the caller’s slice can affect an existing HealthHandler. Update NewHealthHandler to clone the checks slice before assigning it to HealthHandler.checks, using the CheckList type and the HealthHandler constructor path to keep the handler’s internal state isolated from external changes.Source: Coding guidelines
controllers/health_db.go (1)
15-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the DB health-check failures with context.
Both error paths currently lose whether
/_healthfailed while acquiring the connection or executing the probe. Wrapping them makes the logs actionable without changing behavior. As per coding guidelines, "Return wrapped errors with context using%wwhen propagating errors."Proposed fix
import ( "context" + "fmt" "github.com/prest/prest/v2/adapters/postgres" ) @@ func CheckDBHealth(ctx context.Context) error { conn, err := postgres.Get() if err != nil { - return err + return fmt.Errorf("get postgres connection: %w", err) } _, err = conn.ExecContext(ctx, ";") - return err + if err != nil { + return fmt.Errorf("exec health probe: %w", err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/health_db.go` around lines 15 - 21, The CheckDBHealth function currently returns raw errors from postgres.Get and conn.ExecContext, which hides whether the failure happened during connection acquisition or the probe query. Update both error paths in CheckDBHealth to wrap the underlying error with contextual messages using %w so callers can tell which step failed while preserving the original error.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/helpers.go`:
- Around line 15-18: The requestContext helper is turning a missing
pctx.HTTPTimeoutKey into a zero timeout, which makes the returned context
immediately canceled; update requestContext to detect when the timeout value is
absent or invalid and preserve existing behavior instead of calling
context.WithTimeout with 0. Use the requestContext symbol and its
ctx.Value(pctx.HTTPTimeoutKey) lookup to keep the current
validation/sanitization flow, and only apply a timeout when a valid duration is
actually present.
In `@controllers/script.go`:
- Around line 48-50: The script request context is being wrapped with
context.WithTimeout in controllers/script.go even when pctx.HTTPTimeoutKey is
missing, which makes timeout default to 0 and cancels the request immediately
before ExecuteScriptsCtx runs. Update the timeout handling around the
ctx.Value(pctx.HTTPTimeoutKey) lookup so that you only create a timeout context
when the extracted timeout is a positive value; otherwise keep the original ctx
unchanged and proceed normally through ExecuteScriptsCtx.
In `@router/router.go`:
- Around line 38-41: The CRUD routes are currently mounted through a catch-all
PathPrefix("/") handler, which can intercept requests that should be handled by
mux with 405 responses. Update the router setup in router.go so the Negroni
stack is applied directly to each CRUD route/subrouter instead of wrapping all
CRUD traffic under a single unrestricted parent route. Keep the auth and other
non-CRUD routes registered normally, and use the existing crudStack, crudRoutes,
and router setup to locate and refactor the mounting logic.
---
Nitpick comments:
In `@controllers/health_db.go`:
- Around line 15-21: The CheckDBHealth function currently returns raw errors
from postgres.Get and conn.ExecContext, which hides whether the failure happened
during connection acquisition or the probe query. Update both error paths in
CheckDBHealth to wrap the underlying error with contextual messages using %w so
callers can tell which step failed while preserving the original error.
In `@controllers/healthcheck.go`:
- Around line 25-26: The NewHealthHandler constructor currently stores the
incoming CheckList slice directly, so later mutations to the caller’s slice can
affect an existing HealthHandler. Update NewHealthHandler to clone the checks
slice before assigning it to HealthHandler.checks, using the CheckList type and
the HealthHandler constructor path to keep the handler’s internal state isolated
from external changes.
In `@controllers/script.go`:
- Around line 70-80: In controllers/script.go, the error propagation in the
script loading/parsing flow is losing the original cause because the fmt.Errorf
calls in the script retrieval and h.scripts.ParseScript paths use plain
formatting instead of wrapped errors. Update the relevant error returns in the
script-handling logic (the block around ParseScript and the earlier script
lookup failure) to preserve the underlying error chain with wrapped context
using the existing identifiers queriesPath, script, and h.scripts.ParseScript,
so callers can inspect the root cause while still getting the added message.
In `@middlewares/crud_stack.go`:
- Around line 44-45: CRUDStack.Handlers currently returns the internal
s.handlers slice directly, which allows callers to mutate the middleware chain
after construction. Update Handlers() to return a defensive copy of the slice
instead of the original, using CRUDStack and s.handlers as the key locations to
change, so external code cannot reorder, replace, or remove auth/access-control
middleware.
In `@router/router.go`:
- Around line 17-18: RegisterRoutes still depends on global config by reading
config.PrestConf.AuthEnabled, which breaks full dependency injection. Update
RegisterRoutes to receive the auth-enabled flag via an injected parameter or a
small routing deps struct, and use that value when deciding whether to register
the /auth route. Keep the change aligned with the existing RegisterRoutes and
h.Auth.Login wiring so callers control route registration without reaching into
global config.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ebcd49c7-6d0a-4720-a0bf-4cc43dc58968
📒 Files selected for processing (42)
.github/copilot-instructions.md.github/workflows/build.yml.github/workflows/lint.ymlDockerfileDockerfile.nopluginsMakefileadapters/adapter.goadapters/catalog.goadapters/database_registry.goadapters/mockgen/adapter.goadapters/mockgen/scanner.goadapters/permissions.goadapters/query_executor.goadapters/request_query.goadapters/scripts.goadapters/sql_builder.gocontrollers/auth.gocontrollers/auth_test.gocontrollers/catalog.gocontrollers/crud.gocontrollers/databases.gocontrollers/databases_test.gocontrollers/deps.gocontrollers/handlers_test.gocontrollers/health_db.gocontrollers/healthcheck.gocontrollers/healthcheck_test.gocontrollers/helpers.gocontrollers/schemas.gocontrollers/schemas_test.gocontrollers/script.gocontrollers/sql_test.gocontrollers/table.gocontrollers/tables.gocontrollers/tables_test.godocker-compose-test.ymlgo.modmiddlewares/config_test.gomiddlewares/crud_stack.gomiddlewares/middlewares.gorouter/router.gotestdata/Dockerfile
💤 Files with no reviewable changes (3)
- controllers/databases.go
- controllers/schemas.go
- controllers/tables.go
* feat: Enhance testing framework with unit and integration tests - Updated Makefile to include separate targets for unit and integration tests. - Added GitHub workflows for running unit and integration tests on push and pull requests. - Introduced new mock generation for various adapters to improve test coverage. - Refactored existing tests to align with new testing structure and improve clarity. - Exposed certain functions in the Postgres adapter for integration testing. * fix code coverage setup * feat: Add code coverage reporting to GitHub Actions workflow - Introduced steps to archive code coverage results and upload them to Codecov. - Added a new job for generating coverage reports on pull requests, enhancing visibility of test coverage metrics. * fix tests * controllers: enhance test coverage * middlewares: enhance test coverage * cop-instructions: add unit test file naming rule * controllers: move unit test funcs * copilot: enhance hexagonal instructions * fix testdata arg * postgres: add unit tests for adapter - Introduced comprehensive unit tests for the Postgres adapter, covering query execution, insertion, deletion, and update operations. - Added SQL mock integration to simulate database interactions and validate expected behaviors. - Enhanced test coverage for error handling scenarios and context-aware queries. - Included helper functions for setting up test configurations and database connections. * controllers: fix unit test comments * copilot: add new testing instruction * CR: fix nitpicks * adpter: fix CR comment
- Introduced a new CodeQL configuration file to enhance security analysis for the pREST project. - Updated the CodeQL workflow to utilize the new configuration file. - Added comments in the Postgres adapter to clarify the SQL preparation process and its security considerations.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
integration/middlewares/config_test.go (1)
41-64: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegister cleanup right after mutating middleware globals.
These tests reset shared app state only at the tail. Any earlier
require/t.Fatalpath skips that reset and leavesmiddlewares.GetApp()/middlewares.MiddlewareStackpolluted for later cases.Suggested fix pattern
func TestGetAppWithReorderedMiddleware(t *testing.T) { middlewares.ResetForTest() + t.Cleanup(middlewares.ResetForTest) middlewares.MiddlewareStack = []negroni.Handler{ negroni.Handler(negroni.HandlerFunc(customMiddleware)), } ... - - middlewares.ResetForTest() }Also applies to: 66-79, 81-126, 255-263, 286-298
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration/middlewares/config_test.go` around lines 41 - 64, The tests that mutate shared middleware state leave cleanup until the end, so any failed require or fatal path can skip the reset and pollute later cases. In TestGetAppWithReorderedMiddleware and the other affected tests, call t.Cleanup immediately after middlewares.ResetForTest() and after assigning middlewares.MiddlewareStack so the cleanup always runs even if the test exits early. Keep using the existing ResetForTest helper to restore middlewares.GetApp() and middleware globals.config/config_test.go (1)
48-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThis subtest no longer verifies
PREST_CONFparsing.Setting
PREST_PG_DATABASEon Line 50 to the same value asserted on Line 55 makes the check pass even if../testdata/prest.tomlis never read. Unset the PG env for this case—or assert a fixture-only setting—so the test still proves file-based parsing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/config_test.go` around lines 48 - 56, The PREST_CONF subtest in config_test.go is currently masking whether file-based parsing happens because PREST_PG_DATABASE is set to the same expected value. Update the test around viperCfg and Parse so it verifies a value that only comes from ../testdata/prest.toml, or unset PREST_PG_DATABASE before calling Parse and assert the fixture-driven field(s) instead. Keep the check tied to Prest so the test still proves PREST_CONF is actually being read.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-integration.yml:
- Around line 19-21: The actions/setup-go step in this workflow is using its
default cache behavior in a pull_request-triggered job, which can let untrusted
code populate cache used by trusted runs. Update the setup-go configuration to
disable caching here, or only enable caching when the workflow is running on
trusted refs/events. Locate the change in the setup-go step within the
test-integration workflow and adjust its cache-related settings accordingly.
- Line 18: The integration workflow’s checkout step is leaving persisted Git
credentials enabled, which exposes the checkout token inside the mounted
workspace. Update the workflow’s checkout configuration for both checkout steps
to disable persisted credentials, using the existing actions/checkout usage in
the test-integration job so the repo code still checks out without writing the
token into .git/config.
In @.github/workflows/test-unit.yml:
- Around line 18-20: The checkout in the test-unit workflow should not leave
credentials behind, and Go caching should be disabled for this pull_request job.
Update the actions/checkout step in the workflow to explicitly turn off
persisted credentials, and adjust the actions/setup-go@v6 configuration used by
the job so its cache writes are disabled. Keep the change scoped to the workflow
that runs make test-unit and use the existing checkout/setup-go steps as the
anchors.
In `@adapters/postgres/postgres_exec_test.go`:
- Around line 16-30: The ctx-path tests currently do not verify that
pctx.DBNameKey actually overrides the global database because withSQLMock
hardcodes both the connection and injected URI to "test". Update the Postgres
test setup and the affected tests in postgres_exec_test.go and queries_test.go
to use distinct default and context database names, or separate mocked
connections, so TestQueryCtx_WithDBNameKey and related cases can fail if the
implementation ignores DBNameKey. Ensure the tests assert behavior through
Postgres methods that read the context and route to the correct database instead
of relying on the same global value.
In `@controllers/auth_test.go`:
- Around line 76-80: The current test in Test_encrypt_unknownAlgorithm is
locking in the empty-string fallback for unsupported AuthHandler.encrypt
behavior, which weakens auth handling. Update AuthHandler.encrypt (and any
caller path in AuthHandler that uses AuthConfig.Encrypt) to fail closed for
unknown algorithms instead of returning an empty value, and change the test to
assert the error/failure outcome for an invalid encrypt setting like PLAINTEXT
using the existing testAuthHandler and encrypt symbols.
- Around line 258-260: The TestToken setup is overwriting the global
config.PrestConf and always clearing it in cleanup, which drops any pre-existing
value and can make tests order-dependent. Update the test to save the current
config.PrestConf before assigning the temporary config, then restore that saved
pointer in the t.Cleanup callback. Keep the change localized to TestToken and
use the existing config.PrestConf symbol so the original global state is
preserved after the test.
In `@integration/adapters/postgres/queries_test.go`:
- Around line 63-65: The tests are constructing query script paths directly from
PREST_QUERIES_LOCATION, which can be empty and produce invalid absolute paths.
Update the affected cases in queries_test.go to use the queries directory
already loaded by config.Load() via config.PrestConf.Adapter instead of a bare
os.Getenv lookup, and keep the existing ParseScript calls intact while changing
only how the base path is resolved.
- Around line 79-80: The negative assertion around ParseScript is unsafe because
it calls err.Error() without checking for nil, so a nil error will panic and
mask the real failure. Update the test in queries_test.go to guard against nil
before calling Error()—for example by asserting err is not nil first or by using
the testing framework’s error assertions—so the check on ParseScript and
strings.Contains remains meaningful.
- Around line 1-12: Initialize config.PrestConf and its adapter/db setup in the
test package before any test uses it, since the current tests dereference
config.PrestConf.Adapter... before any bootstrap occurs and can panic. Add a
shared test setup/fixture for the postgres_test package so the tests that call
WriteSQL and ExecuteScripts run against a valid initialized pool, and make sure
the setup happens before any access in the test functions that reference
config.PrestConf.
In `@integration/controllers/auth_test.go`:
- Around line 16-21: The disabled-auth test currently depends on shared config
state instead of explicitly covering the disabled branch. In the auth test setup
around helpers.LoadTestConfig, NewHandlersFromConfig, and the AuthEnabled check,
force config.PrestConf.AuthEnabled to false for this test and restore the
original value afterward so the scenario stays stable regardless of fixture
changes.
In `@integration/controllers/health_test.go`:
- Around line 12-14: The current test bypasses the refactored HealthHandler by
calling controllers.CheckDBHealth directly, so it never verifies the HTTP
behavior in controllers/healthcheck.go. Update TestCheckDBHealth in
health_test.go to start the integration server and send a real request through
the health route, asserting the HTTP response from the public endpoint instead
of the internal helper. Use the existing controllers.HealthHandler/health route
wiring and keep the test exercising the end-to-end HTTP contract with Docker
Postgres.
In `@Makefile`:
- Line 3: The UNIT_PKGS assignment is being eagerly evaluated at parse time via
go list, which forces a host Go binary before targets like test-integration run.
Change the Makefile so package discovery is deferred or moved into the test-unit
target (or otherwise only evaluated when that target runs), and keep
test-integration from depending on UNIT_PKGS; use the UNIT_PKGS symbol and the
test-unit/test-integration targets to locate the change.
In `@middlewares/test_exports.go`:
- Around line 11-13: SetMiddlewareStackForTest only updates MiddlewareStack, so
an already-created app from GetApp() keeps serving the old Negroni handlers.
Update SetMiddlewareStackForTest to also reset the cached app state (the app
variable) so the next GetApp() rebuilds from the new test stack. Use the
existing SetMiddlewareStackForTest and GetApp symbols to make the cache
invalidation happen whenever the test middleware stack is swapped.
---
Outside diff comments:
In `@config/config_test.go`:
- Around line 48-56: The PREST_CONF subtest in config_test.go is currently
masking whether file-based parsing happens because PREST_PG_DATABASE is set to
the same expected value. Update the test around viperCfg and Parse so it
verifies a value that only comes from ../testdata/prest.toml, or unset
PREST_PG_DATABASE before calling Parse and assert the fixture-driven field(s)
instead. Keep the check tied to Prest so the test still proves PREST_CONF is
actually being read.
In `@integration/middlewares/config_test.go`:
- Around line 41-64: The tests that mutate shared middleware state leave cleanup
until the end, so any failed require or fatal path can skip the reset and
pollute later cases. In TestGetAppWithReorderedMiddleware and the other affected
tests, call t.Cleanup immediately after middlewares.ResetForTest() and after
assigning middlewares.MiddlewareStack so the cleanup always runs even if the
test exits early. Keep using the existing ResetForTest helper to restore
middlewares.GetApp() and middleware globals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8a7f6e7-e531-47ea-ae00-d956e8e70066
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (55)
.github/copilot-instructions.md.github/workflows/test-integration.yml.github/workflows/test-unit.yml.github/workflows/test.ymlMakefileadapters/mockgen/catalog_querier.goadapters/mockgen/database_registry.goadapters/mockgen/permissions_checker.goadapters/mockgen/query_executor.goadapters/mockgen/request_query_builder.goadapters/mockgen/script_runner.goadapters/mockgen/sql_builder.goadapters/postgres/internal/connection/test_exports.goadapters/postgres/postgres.goadapters/postgres/postgres_exec_test.goadapters/postgres/postgres_test.goadapters/postgres/queries_test.goadapters/postgres/test_exports.goconfig/config_test.gocontrollers/auth_test.gocontrollers/catalog_test.gocontrollers/crud_test.gocontrollers/databases_test.gocontrollers/deps_test.gocontrollers/errors_test.gocontrollers/healthcheck_test.gocontrollers/helpers_test.gocontrollers/script_test.gocontrollers/table_test.godocker-compose-test.ymlgo.modhandlerstest/deps.gohelpers/prest.gointegration/adapters/connection/conn_test.gointegration/adapters/postgres/postgres_test.gointegration/adapters/postgres/queries_test.gointegration/controllers/auth_test.gointegration/controllers/catalog_test.gointegration/controllers/crud_test.gointegration/controllers/health_test.gointegration/controllers/scripts_test.gointegration/helpers/setup.gointegration/middlewares/config_test.gointegration/plugins/handler_test.gointegration/plugins/middleware_test.gointegration/router/routes_test.gomiddlewares/cache_test.gomiddlewares/config.gomiddlewares/config_testhooks.gomiddlewares/crud_stack_test.gomiddlewares/middlewares_test.gomiddlewares/test_exports.gomiddlewares/utils_test.goplugins/middleware_test.gotestdata/runtest.sh
💤 Files with no reviewable changes (4)
- .github/workflows/test.yml
- docker-compose-test.yml
- plugins/middleware_test.go
- controllers/databases_test.go
✅ Files skipped from review due to trivial changes (9)
- helpers/prest.go
- adapters/mockgen/script_runner.go
- adapters/mockgen/database_registry.go
- go.mod
- adapters/mockgen/sql_builder.go
- adapters/mockgen/catalog_querier.go
- adapters/mockgen/permissions_checker.go
- adapters/mockgen/request_query_builder.go
- adapters/mockgen/query_executor.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controllers/healthcheck_test.go
- Updated Makefile to improve package listing for unit tests. - Modified GitHub workflows for integration and unit tests to include credential persistence and caching based on event type. - Refactored Postgres adapter tests to utilize new mock functions for better context handling and error checking. - Improved error handling in the AuthHandler's encryption method and added new error definitions. - Enhanced integration tests for health checks and script parsing with better setup and teardown procedures. - Updated middleware tests to reset application state and validate custom middleware behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/auth.go`:
- Around line 144-149: The AuthHandler.encrypt method still allows weak MD5/SHA1
digests, and the default Encrypt setting in config/config.go keeps new installs
on MD5. Update encrypt to use a password KDF such as bcrypt or argon2 for the
default path, and change the configuration default away from MD5; if legacy
hashes must remain readable, keep the MD5/SHA1 branches only for
backward-compatible verification or migration, not for new password generation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68a521a9-ae0d-4a4c-bb25-41c9aa2378df
📒 Files selected for processing (14)
.github/workflows/test-integration.yml.github/workflows/test-unit.ymlMakefileadapters/postgres/postgres_exec_test.goadapters/postgres/postgres_test.goadapters/postgres/queries_test.gocontrollers/auth.gocontrollers/auth_test.gocontrollers/errors.gointegration/adapters/postgres/queries_test.gointegration/controllers/auth_test.gointegration/controllers/health_test.gomiddlewares/middlewares_test.gomiddlewares/test_exports.go
🚧 Files skipped from review as they are similar to previous changes (6)
- middlewares/test_exports.go
- adapters/postgres/postgres_exec_test.go
- integration/controllers/auth_test.go
- Makefile
- adapters/postgres/queries_test.go
- integration/adapters/postgres/queries_test.go
- Replaced MD5 and SHA1 password encryption with bcrypt for enhanced security. - Refactored AuthHandler to support legacy password checks while transitioning to bcrypt. - Updated related tests to validate new password hashing and checking methods. - Improved configuration handling for password encryption algorithms.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/auth.go (1)
217-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap bcrypt hashing errors with context.
HashPasswordcurrently propagates the bcrypt error directly.Suggested fix
hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) if err != nil { - return "", err + return "", fmt.Errorf("hash password with bcrypt: %w", err) }As per coding guidelines, “Return wrapped errors with context using %w when propagating errors.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/auth.go` around lines 217 - 222, HashPassword currently returns the bcrypt error directly without context; update the error path in HashPassword to wrap the GenerateFromPassword failure using the function name and a short description, so callers can identify where it came from. Keep the existing signature and return flow, but change the err return in HashPassword to use wrapped error semantics with %w.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/codeql-analysis.yml:
- Around line 46-51: Remove the repo-wide CodeQL query-filters exclusions from
the workflow so security checks stay enabled for all Go files. Keep the global
`go/sql-injection` and `go/weak-sensitive-data-hashing` queries active in the
workflow file, and rely on the existing narrow suppression in
`controllers/auth.go` for the specific weak-hashing case. Use the workflow
config block in the CodeQL job to locate and update the `config`/`query-filters`
section.
In `@controllers/auth.go`:
- Around line 118-124: Preserve the legacy implicit-default login path in the
auth flow so existing MD5/SHA1 users do not break when h.cfg.Encrypt defaults to
bcrypt. Update the password verification logic in h.basicPasswordCheck (and the
bcrypt/legacy branches it calls, including basicPasswordCheckLegacy and
basicPasswordCheckBcrypt) to detect legacy-shaped stored digests after the
username lookup and fall back to legacy comparison when appropriate, rather than
always routing default bcrypt-configured accounts to CompareHashAndPassword.
Keep the existing public auth behavior unchanged for deployments that never set
auth.encrypt, and only change the verification path in a backward-compatible
way.
---
Nitpick comments:
In `@controllers/auth.go`:
- Around line 217-222: HashPassword currently returns the bcrypt error directly
without context; update the error path in HashPassword to wrap the
GenerateFromPassword failure using the function name and a short description, so
callers can identify where it came from. Keep the existing signature and return
flow, but change the err return in HashPassword to use wrapped error semantics
with %w.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63162a31-90b4-44bb-a5a2-a66521d26279
📒 Files selected for processing (7)
.github/workflows/codeql-analysis.ymlconfig/config.goconfig/config_test.gocontrollers/auth.gocontrollers/auth_test.gogo.modintegration/middlewares/config_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- integration/middlewares/config_test.go
- Introduced a new method to verify stored passwords, supporting bcrypt, MD5, and SHA1. - Updated the basicPasswordCheck method to utilize the new verification logic. - Added unit tests for legacy password checks with MD5 and SHA1 to ensure backward compatibility. - Cleaned up CodeQL configuration by removing unnecessary exclusions.
|
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
This was referenced Jul 2, 2026
This was referenced Jul 11, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upgrade Go version to 1.26 across all configurations and update mockgen commands
Summary by CodeRabbit
New Features
Bug Fixes
Chores