Pertempto commented

(Migrated from github.com)

Summary

Implements database connectivity to Aptora (read-only) and Extensions (read-write) SQL Server databases
Adds resilient connection manager with automatic retry logic and health monitoring
Provides REST API endpoints for querying employees and invoices with proper error handling

Key Features

Database Manager: Connection pooling, 30-second retry logic, graceful startup when databases unavailable
Health Endpoint (GET /health): Reports database connectivity status
Employees Endpoint (GET /api/employees): Returns employee ID and name
Invoices Endpoint (GET /api/invoices): Query invoices by date range and employee with 500-result limit

## Summary - Implements database connectivity to Aptora (read-only) and Extensions (read-write) SQL Server databases - Adds resilient connection manager with automatic retry logic and health monitoring - Provides REST API endpoints for querying employees and invoices with proper error handling ### Key Features - **Database Manager**: Connection pooling, 30-second retry logic, graceful startup when databases unavailable - **Health Endpoint** (`GET /health`): Reports database connectivity status - **Employees Endpoint** (`GET /api/employees`): Returns employee ID and name - **Invoices Endpoint** (`GET /api/invoices`): Query invoices by date range and employee with 500-result limit

github-actions[bot] commented

2025-11-21 10:42:59 -05:00

(Migrated from github.com)

File: backend/internal/database/database.go

Issue: background reconnect goroutine never stops and Close() does not signal it. This can race with connection/closure and leak goroutines when the server shuts down.

Request:

Make NewManager accept a context.Context or add an internal stop channel so connectLoop listens for cancellation (select { case <-ctx.Done(): return }).
Ensure Close() cancels that context / closes the stop channel and waits for the goroutine to exit (e.g., via sync.WaitGroup or a done channel) before returning.

Rationale: without this, connectLoop can reopen connections while shutdown is in progress and leave goroutines running after Close() returns.

File: `backend/internal/database/database.go` Issue: background reconnect goroutine never stops and `Close()` does not signal it. This can race with connection/closure and leak goroutines when the server shuts down. Request: - Make `NewManager` accept a `context.Context` or add an internal stop channel so `connectLoop` listens for cancellation (`select { case <-ctx.Done(): return }`). - Ensure `Close()` cancels that context / closes the stop channel and waits for the goroutine to exit (e.g., via `sync.WaitGroup` or a done channel) before returning. Rationale: without this, `connectLoop` can reopen connections while shutdown is in progress and leave goroutines running after `Close()` returns.

github-actions[bot] commented

2025-11-21 10:43:14 -05:00

(Migrated from github.com)

File: backend/internal/database/database.go

Issue: initializeSchema always inserts a new row into health_check on each successful connection attempt. This will grow the table indefinitely with one row per reconnect.

Request:

Change the initialization behavior to either:
- Insert only if table is empty, or
- Use MERGE/UPSERT to update a single health row, or
- Use DELETE + INSERT or TRUNCATE if you explicitly want a single latest row.

Rationale: uncontrolled growth may cause the table to balloon over time and affect backups/maintenance.

File: `backend/internal/database/database.go` Issue: `initializeSchema` always inserts a new row into `health_check` on each successful connection attempt. This will grow the table indefinitely with one row per reconnect. Request: - Change the initialization behavior to either: - Insert only if table is empty, or - Use `MERGE`/`UPSERT` to update a single health row, or - Use `DELETE` + `INSERT` or `TRUNCATE` if you explicitly want a single latest row. Rationale: uncontrolled growth may cause the table to balloon over time and affect backups/maintenance.

github-actions[bot] commented

2025-11-21 10:43:31 -05:00

(Migrated from github.com)

File: backend/internal/database/database.go

Issue: Connection strings use encrypt=disable. That may be okay for internal networks but should be explicit and configurable.

Request:

Make encryption behavior configurable (e.g., Encrypt bool / EncryptMode string in Config) and default to secure settings.

Rationale: Disabling encryption by default is a security risk if this ever runs outside a trusted network.

File: `backend/internal/database/database.go` Issue: Connection strings use `encrypt=disable`. That may be okay for internal networks but should be explicit and configurable. Request: - Make encryption behavior configurable (e.g., `Encrypt bool` / `EncryptMode string` in `Config`) and default to secure settings. Rationale: Disabling encryption by default is a security risk if this ever runs outside a trusted network.

github-actions[bot] commented

2025-11-21 10:43:58 -05:00

(Migrated from github.com)

Changes Requested

Please address the following high-impact items before merging:

Make the Manager lifecycle cancellable and deterministic: ensure the background reconnect goroutine can be stopped and Close() waits for it to exit (use a context the manager owns or an internal stop channel + sync.WaitGroup).
Prevent unbounded inserts into health_check: change initializeExtensionsSchema to upsert/update a single health row (or insert-only-if-empty / truncate) instead of inserting on every reconnect.
Make DB encryption explicit/configurable: remove hardcoded encrypt=disable; add config fields (e.g., EncryptMode/Encrypt + TrustServerCertificate) and update .env.example.
Make verifyReadOnly deterministic: perform the write-check on a single connection (use db.BeginTx or db.Conn) so create/drop happen on the same physical connection and are rolled back.
Fix invoices handler query handling:
- Parse start_date/end_date into time.Time and pass typed values to the driver (not raw strings).
- Build COUNT and main queries so parameter placeholders and the args slice order always match (avoid ad-hoc @p1/@p3 mismatch).
Clean up backend/go.mod duplicate require blocks and run go mod tidy.

Summary of Changes

Added DB manager with retry loop, schema init, and read-only verification: backend/internal/database/database.go.
Wired manager into server startup: backend/cmd/server/main.go now builds database.Config and passes database.Manager to server.
Added HTTP endpoints and API routing: GET /health, GET /api/employees, GET /api/invoices in backend/internal/server/server.go.
Updated .env.example, README.md, justfile, and specs/000-proof-of-concept.md for DB setup and dev flow.
Modified backend/go.mod/go.sum to add DB driver and dependencies.

Overall Feedback

What needs work (negative):

Manager lifecycle and health-check logic are unsafe for shutdown and production: the reconnect goroutine is not cancellable and Close() does not wait, initializeExtensionsSchema always inserts a new row (table will grow), and verifyReadOnly uses pooled ops that can be nondeterministic and leave objects behind.
The invoices handler builds queries using string placeholders while passing an args slice that can get out of sync and sends raw date strings to the DB driver; this risks wrong results and SQL errors.
backend/go.mod contains duplicate require blocks and should be tidied.

What I liked (positive):

Good separation between DB manager and HTTP server; handlers use sensible timeouts and consistent JSON error responses.
Clear incremental approach: starting the server even when DBs are unavailable and reporting health is pragmatic.

Small, targeted fixes in the manager and the invoices handler will make this production-ready. Ping me when you’ve addressed these and I’ll re-review quickly — nice progress! 44d680

# Changes Requested Please address the following high-impact items before merging: - Make the `Manager` lifecycle cancellable and deterministic: ensure the background reconnect goroutine can be stopped and `Close()` waits for it to exit (use a context the manager owns or an internal stop channel + `sync.WaitGroup`). - Prevent unbounded inserts into `health_check`: change `initializeExtensionsSchema` to upsert/update a single health row (or insert-only-if-empty / truncate) instead of inserting on every reconnect. - Make DB encryption explicit/configurable: remove hardcoded `encrypt=disable`; add config fields (e.g., `EncryptMode`/`Encrypt` + `TrustServerCertificate`) and update `.env.example`. - Make `verifyReadOnly` deterministic: perform the write-check on a single connection (use `db.BeginTx` or `db.Conn`) so create/drop happen on the same physical connection and are rolled back. - Fix invoices handler query handling: - Parse `start_date`/`end_date` into `time.Time` and pass typed values to the driver (not raw strings). - Build COUNT and main queries so parameter placeholders and the `args` slice order always match (avoid ad-hoc `@p1/@p3` mismatch). - Clean up `backend/go.mod` duplicate `require` blocks and run `go mod tidy`. ## Summary of Changes - Added DB manager with retry loop, schema init, and read-only verification: `backend/internal/database/database.go`. - Wired manager into server startup: `backend/cmd/server/main.go` now builds `database.Config` and passes `database.Manager` to server. - Added HTTP endpoints and API routing: `GET /health`, `GET /api/employees`, `GET /api/invoices` in `backend/internal/server/server.go`. - Updated `.env.example`, `README.md`, `justfile`, and `specs/000-proof-of-concept.md` for DB setup and dev flow. - Modified `backend/go.mod`/`go.sum` to add DB driver and dependencies. ## Overall Feedback What needs work (negative): - Manager lifecycle and health-check logic are unsafe for shutdown and production: the reconnect goroutine is not cancellable and `Close()` does not wait, `initializeExtensionsSchema` always inserts a new row (table will grow), and `verifyReadOnly` uses pooled ops that can be nondeterministic and leave objects behind. - The invoices handler builds queries using string placeholders while passing an `args` slice that can get out of sync and sends raw date strings to the DB driver; this risks wrong results and SQL errors. - `backend/go.mod` contains duplicate `require` blocks and should be tidied. What I liked (positive): - Good separation between DB manager and HTTP server; handlers use sensible timeouts and consistent JSON error responses. - Clear incremental approach: starting the server even when DBs are unavailable and reporting health is pragmatic. Small, targeted fixes in the manager and the invoices handler will make this production-ready. Ping me when you’ve addressed these and I’ll re-review quickly — nice progress! 44d680

github-actions[bot] commented

2025-11-21 11:05:43 -05:00

(Migrated from github.com)

File: backend/internal/database/database.go

Issue: verifyReadOnly uses CREATE TABLE #read_only_test then attempts to DROP TABLE if the create succeeds. That check is unreliable when using database/sql connection pooling because the create and drop may run on different physical connections (or the temp table may persist on the connection), leading to false negatives/positives and residual objects.

Request:

Replace the current approach with a single-connection transactional check. For example:
- Use tx, err := db.BeginTx(ctx, nil) to get a dedicated connection, Exec the create within the tx, then tx.Rollback() so nothing is persisted. This ensures the create/drop happen on the same connection and are rolled back.
- Alternatively, drop the write-check entirely and document the behavior; rely on permissions and schema init safeguards instead.

Rationale: the existing check can behave inconsistently in production and can leave temp objects behind. Using a Tx (or another same-connection mechanism) makes the check deterministic and safe.

File: `backend/internal/database/database.go` Issue: `verifyReadOnly` uses `CREATE TABLE #read_only_test` then attempts to `DROP TABLE` if the create succeeds. That check is unreliable when using `database/sql` connection pooling because the create and drop may run on different physical connections (or the temp table may persist on the connection), leading to false negatives/positives and residual objects. Request: - Replace the current approach with a single-connection transactional check. For example: - Use `tx, err := db.BeginTx(ctx, nil)` to get a dedicated connection, `Exec` the create within the tx, then `tx.Rollback()` so nothing is persisted. This ensures the create/drop happen on the same connection and are rolled back. - Alternatively, drop the write-check entirely and document the behavior; rely on permissions and schema init safeguards instead. Rationale: the existing check can behave inconsistently in production and can leave temp objects behind. Using a Tx (or another same-connection mechanism) makes the check deterministic and safe.

github-actions[bot] commented

2025-11-21 11:07:13 -05:00

(Migrated from github.com)

File: backend/internal/database/database.go

Issue: Connection strings hardcode encrypt=disable which is insecure if deployed outside a trusted network.

Actionable change:

Add an Encrypt (bool) or EncryptMode (string) field to Config and use it when building connection strings instead of hardcoding encrypt=disable.
Default to secure settings (e.g., encrypt=true;trustservercertificate=false) and allow overriding via config.

Rationale: disabling encryption by default is a security risk. Make encryption explicit and configurable so deployments can enforce TLS.

File: `backend/internal/database/database.go` Issue: Connection strings hardcode `encrypt=disable` which is insecure if deployed outside a trusted network. Actionable change: - Add an `Encrypt` (bool) or `EncryptMode` (string) field to `Config` and use it when building connection strings instead of hardcoding `encrypt=disable`. - Default to secure settings (e.g., `encrypt=true;trustservercertificate=false`) and allow overriding via config. Rationale: disabling encryption by default is a security risk. Make encryption explicit and configurable so deployments can enforce TLS.

github-actions[bot] commented

2025-11-21 11:44:25 -05:00

(Migrated from github.com)

@Pertempto

initializeSchema always inserts a new row into health_check on each successful connection attempt.

Please stop unconditional INSERTs that will bloat the table. Actionable changes:

Use an upsert to maintain a single health row, e.g. MERGE on a known key, or
Insert only if the table is empty (check SELECT COUNT(*) first), or
Truncate before inserting if you intentionally want only the latest row.

I suggest adding a single-row key like name VARCHAR(64) UNIQUE and then MERGE/UPDATE the timestamp for key "manager_health". That avoids table growth while keeping a last-seen timestamp.

@Pertempto > `initializeSchema` always inserts a new row into `health_check` on each successful connection attempt. Please stop unconditional INSERTs that will bloat the table. Actionable changes: - Use an upsert to maintain a single health row, e.g. MERGE on a known key, or - Insert only if the table is empty (check `SELECT COUNT(*)` first), or - Truncate before inserting if you intentionally want only the latest row. I suggest adding a single-row key like `name VARCHAR(64) UNIQUE` and then `MERGE`/`UPDATE` the `timestamp` for key `"manager_health"`. That avoids table growth while keeping a last-seen timestamp.

github-actions[bot] commented

2025-11-21 12:04:06 -05:00

(Migrated from github.com)

README.md: The SQL for the Extensions DB grants db_ddladmin to the aptora_extensions user. That's highly permissive (can change schema). Please drop db_ddladmin and grant only the roles needed (e.g., db_datareader, db_datawriter). If schema changes are required, perform them with an admin account as part of deployment, not via the application user.

`README.md`: The SQL for the Extensions DB grants `db_ddladmin` to the `aptora_extensions` user. That's highly permissive (can change schema). Please drop `db_ddladmin` and grant only the roles needed (e.g., `db_datareader`, `db_datawriter`). If schema changes are required, perform them with an admin account as part of deployment, not via the application user.

Rows
Columns

feat: backend database connection #3

Summary

Key Features

Changes Requested

Summary of Changes

Overall Feedback