chore: move datapipe status endpoint BED-8715#2906
Open
superlinkx wants to merge 27 commits into
Open
Conversation
- Create server/appcfg/ with appcfg.go Register entry point - Add internal/appdb, internal/handlers, internal/routes, internal/services - Add go:generate directives for mockgen in services and handlers - Stub out package interfaces following sliced onion architecture Part of migrating /api/v2/datapipe/status to new architecture. Step 1 of implementation checklist.
- Create appcfg_e2e_test.go with integration test - Test uses old v2.Resources.GetDatapipeStatus handler - Validates complete HTTP contract per OpenAPI spec - Tests 200 OK response with all five datapipe status fields - Fix unused context imports in services and handlers Green baseline established before migration to new architecture. Step 2 of implementation checklist.
- Add DatapipeStatus domain types in services layer - Implement Store.GetDatapipeStatus using sqlbuilder.PostgreSQL - Add datapipeStatusRow struct with db tags for pgx scanning - Map pgx.ErrNoRows to services.ErrNotFound sentinel - Add comprehensive unit tests using pgxmock - Change NewStore to accept pgxQuerier interface for testability All unit tests pass. E2e test validates old handler still works. Step 3 of implementation checklist.
- Service.GetDatapipeStatus delegates to Database interface - Domain types already defined in Step 3 - Generate MockDatabase using go.uber.org/mock/mockgen - Add comprehensive unit tests with gomock - Test success path, ErrNotFound, and error propagation All unit tests pass. Step 4 of implementation checklist.
- Service.GetDatapipeStatus delegates to Database interface - Domain types already defined in Step 3 - Generate MockDatabase using go tool mockery (testify/mock) - Add comprehensive unit tests with testify mock expectations - Test success path, ErrNotFound, and error propagation All unit tests pass. Step 4 of implementation checklist.
- Test all three datapipe status states: idle, ingesting, analyzing - Test timestamp updates after analysis start/complete - Verify all five fields in response envelope including nullable next_scheduled_analysis_at - Use SetDatapipeStatus, SetLastAnalysisStartTime, UpdateLastAnalysisCompleteTime to test different states Covers more of the OpenAPI contract as per spec. Enhancement to Step 2.
- Test 401 Unauthorized when no auth token is provided - Test 401 Unauthorized with invalid auth token - Test 200 OK with valid JWT token - Set up full router with RegisterFossGlobalMiddleware - Use api.NewAuthExtensions and api.NewAuthenticator - Mint JWT tokens using auther.CreateSession with real user/session Tests full request lifecycle including routing, authentication, rate limiting, and all other global middleware. Enhancement to Step 2.
- Replace manual route wiring with registration.NewV2API() - Use v2.NewResources() to create the actual Resources struct - Set up cache.NewCache() with proper config - This ensures tests validate the REAL route wiring from production - If routes are wired incorrectly in registration.go, tests will catch it The e2e test now validates: - Actual production route registration - Correct middleware application (.RequireAuth()) - Full authentication and authorization flow - All global middleware (auth, rate limit, logging, metrics, CORS, etc.) Enhancement to Step 2.
- Create DatapipeStatusView with json tags matching OpenAPI spec - Add BuildDatapipeStatusView() to project domain → view - Implement JSONView() to satisfy responses.JSONViewer - Decouples wire format from domain model Step 5 of implementation checklist.
- Define Appcfg service interface in handlers package - Implement GetDatapipeStatus handler using responses.WriteBasic - Add handleAppcfgError to map sentinels to HTTP status codes - Generate MockAppcfg using mockery - Add comprehensive unit tests covering all error paths All unit tests pass. Step 6 of implementation checklist.
- Implement routes.Register() to attach GET /api/v2/datapipe/status - Apply RequireAuth() middleware to the route - Add route registration test to verify route matches - Add authentication middleware test to verify 401 on unauthenticated requests All route tests pass. Step 7 of implementation checklist.
- Import appcfg package in server/modules/modules.go - Call appcfg.Register in modules.Register - Appcfg module is now wired into the server startup Step 9 of implementation checklist.
- Update TestGetDatapipeStatus to use modules.Register - Add full authentication setup with JWT tokens - Use production routing and middleware - Fix null.Time handling for nullable timestamp fields - Update all unit tests to use null.Time correctly All tests pass including e2e test. Step 10 of implementation checklist.
- Delete cmd/api/src/api/v2/datapipe.go - Remove route registration from registration/v2.go - Delete old TestGetDatapipeStatus_WithRouting test - Remove newDatapipeStatusHandler helper - Clean up unused imports Build succeeds, all tests pass. Step 11 of implementation checklist.
Generated by prepare-for-codereview.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR migrates Changesappcfg feature module implementation
Analysis e2e refactor and developer documentation
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over Client,datapipe_status: GET /api/v2/datapipe/status
end
participant Client
participant Router as router.Router
participant RequireAuth as RequireAuth middleware
participant Handlers as handlers.GetDatapipeStatus
participant Service as services.Service
participant Store as appdb.Store
participant datapipe_status as datapipe_status table
Client->>Router: GET /api/v2/datapipe/status + Bearer token
Router->>RequireAuth: validate JWT
alt invalid/missing token
RequireAuth-->>Client: 401 Unauthorized
else valid token
RequireAuth->>Handlers: forward request
Handlers->>Service: GetDatapipeStatus(ctx)
Service->>Store: GetDatapipeStatus(ctx)
Store->>datapipe_status: SELECT … LIMIT 1
datapipe_status-->>Store: row
Store-->>Service: DatapipeStatus
Service-->>Handlers: DatapipeStatus
Handlers->>Handlers: BuildDatapipeStatusView(status)
Handlers-->>Client: 200 OK + DatapipeStatusView JSON
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Expand implementation_checklist.md with complete patterns and examples - Add detailed code examples for each step directly in checklist - Include production routing pattern for e2e tests in Step 2 - Add comprehensive Step 10 explaining module registry swap - Add Step 12 details about prepare-for-codereview - Update README.md to link to checklist as primary guide - Keep detailed code examples in README as reference material - Single source of truth: checklist for process, README for concepts Resolves documentation duplication between README and checklist.
- Restructure checklist to separate actionable items from code examples - Keep Steps 1-12 concise with checkboxes only - Move all code patterns to 'Code Patterns Reference' section at end - Add emoji icons (📖) linking checklist items to relevant patterns - Use collapsible details for directory structure in Step 1 - Preserve all code examples but organize them as reference material - Improves readability: checklist flow not interrupted by code blocks This makes the checklist easier to follow while keeping all the detailed patterns accessible when needed.
- Add license headers to all code examples (matches actual code) - Update package documentation to match analysis/appcfg modules - Use correct import path for responses package (packages/go/responses) - Add detailed struct/function comments matching real implementation - Update comments to reference '<feature>' consistently - Ensure all examples follow actual working code from analysis module Code patterns now accurately reflect production code standards.
- Display expected directory structure upfront for immediate visibility - Remove collapsible details to show structure by default - Move checklist items below the structure for better flow - Developers see what to create before starting the task
Critical fixes: - Add authentication tests to E2E suite (401 for missing/invalid tokens, 400 for malformed auth header) - Add comprehensive integration tests for appdb layer (validates database contract with real PostgreSQL) Code quality improvements: - Simplify interface naming (Appcfg -> Service, handleAppcfgError -> handleServiceError) - Simplify function naming (NewHandlersContainer -> NewHandlers) - Clean up parameter names (databaseInterface -> db, appcfg -> service) - Remove redundant comments (keep only essential ones) - Remove empty lines after license headers - Apply goimports formatting (tabs in license headers) - Regenerate mocks after interface renaming Test coverage: - E2E: 7 test cases (3 auth + 4 functional) - Integration: 7 test cases (validates all enum values, NULL handling, singleton constraint) - Unit: Full coverage across all layers - All tests pass successfully Files added: - server/appcfg/internal/appdb/appdb_integration_test.go (new integration tests) Files modified: - server/appcfg/appcfg_e2e_test.go (added 3 auth tests) - server/appcfg/appcfg.go (updated function names) - server/appcfg/internal/services/services.go (cleaned up comments and naming) - server/appcfg/internal/handlers/handlers.go (simplified naming and comments) - server/appcfg/internal/handlers/views.go (cleaned up formatting) - server/appcfg/internal/handlers/handlers_test.go (updated to use new names) - server/appcfg/internal/routes/routes_test.go (updated to use new names) - server/appcfg/internal/handlers/mocks/service.go (regenerated)
- Swap Step 3 (appdb) and Step 4 (services) to correct order - Services layer must be implemented first because it defines domain types - Appdb depends on services.DatapipeStatus and services.ErrNotFound - Add explanatory notes to both steps explaining the dependency The correct order is: 1. Scaffold directory structure 2. Write E2E test 3. Define domain types in services (NEW position) 4. Implement persistence in appdb (NEW position) 5. Define JSON views 6. Implement handlers 7. Register routes 8. Wire up module 9. Register in modules registry
Add authentication validation tests to match the level of detail in appcfg E2E tests: Authentication tests added (9 total): - GET /api/v2/analysis/status: 3 tests (401 no token, 401 invalid token, 400 missing Bearer prefix) - PUT /api/v2/analysis: 3 tests (401 no token, 401 invalid token, 400 missing Bearer prefix) - DELETE /api/v2/analysis: 3 tests (401 no token, 401 invalid token, 400 missing Bearer prefix) Test infrastructure improvements: - Use production routing with modules.Register and registration.RegisterFossGlobalMiddleware - Add mintJWT helper to create authenticated test users with proper roles - Remove unused injectAuthMiddleware and manual wiring helpers - Use real JWT auth middleware instead of mock auth injection Test results: ✅ All 9 authentication tests pass ✅ Unit tests pass ✅ Integration tests pass⚠️ Functional E2E tests blocked by permission configuration issue (403 Forbidden) The authentication contract is now fully validated at the same level of detail as the appcfg module. Note: Functional tests currently fail with 403 due to role/permission setup in test database. This is a pre-existing issue with the analysis module requiring specific permissions (GraphDBRead, AppWriteApplicationConfiguration) that need proper test fixture setup.
Authentication tests: ✅ ALL PASS (9/9) - GET /api/v2/analysis/status: 401 no token, 401 invalid token, 400 missing Bearer - PUT /api/v2/analysis: 401 no token, 401 invalid token, 400 missing Bearer - DELETE /api/v2/analysis: 401 no token, 401 invalid token, 400 missing Bearer Functional tests:⚠️ BLOCKED by permission loading issue - User is created with Administrator role (confirmed: 27 permissions loaded) - Session is created successfully - Middleware validation returns 403 Forbidden instead of allowing access - Root cause: GORM session preloading may not be preserving user.Roles.Permissions properly This is a pre-existing infrastructure issue, not related to test quality. The authentication contract is fully validated at the same detail level as appcfg.
…ecks The auth middleware sets PermissionOverrides when EULAAccepted is false, which restricts permissions and causes 403 Forbidden responses even for users with the Administrator role. Setting EULAAccepted: true allows the full permission set to be used. All E2E tests now pass: ✅ GET /api/v2/analysis/status (5 tests including 3 auth tests) ✅ PUT /api/v2/analysis (6 tests including 3 auth tests) ✅ DELETE /api/v2/analysis (6 tests including 3 auth tests) Total: 17 E2E tests passing, all auth failure modes validated.
coderabbitai
Bot
added
api
coderabbitai
Bot
added
go
…is E2E tests Removes legacy helper functions that were replaced with production routing: - injectAuthMiddleware: No longer needed, using real auth middleware - newAnalysisGetHandler: Replaced by modules.Register - newCreateAnalysisHandler: Replaced by modules.Register - newCancelAnalysisHandler: Replaced by modules.Register Also removes unused imports: - github.com/gofrs/uuid - bhctx - appdb - services These were only used by the legacy helper functions and are no longer needed with the production routing approach.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
server/implementation_checklist.md (1)
18-35: ⚡ Quick winAdd language specifier to fenced code block.
The directory tree code block is missing a language specifier. For consistency with markdown linting standards, add an empty language identifier or
text.📝 Proposed fix
-``` +```text server/<feature>/ ├── <feature>.go # Register() entry point └── internal/ # Internal implementation packages🤖 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 `@server/implementation_checklist.md` around lines 18 - 35, The fenced code block in the implementation_checklist.md file is missing a language specifier after the opening triple backticks. Add a language identifier (such as `text`) immediately after the opening ``` to make the code block markdown-compliant. For example, change the opening from ``` to ```text before the server/<feature>/ directory tree structure.server/README.md (1)
168-186: ⚡ Quick winAdd language specifier to fenced code block.
The directory tree code block is missing a language specifier. Markdown linters require explicit language identifiers for fenced code blocks. For directory tree structures, use an empty language identifier or
text.📝 Proposed fix
-``` +```text server/myfeature/ ├── myfeature.go # Register entry point └── internal/ # Internal implementation packages🤖 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 `@server/README.md` around lines 168 - 186, The fenced code block containing the directory tree structure for server/myfeature/ is missing a language specifier after the opening triple backticks. Add a language identifier to the opening fence of the code block - use either `text` or leave it empty (just add backticks with the language specifier like ```text) to satisfy markdown linter requirements for explicit language identifiers on fenced code blocks.server/appcfg/internal/appdb/appdb.go (1)
95-98: ⚡ Quick winWrap query error with context for consistency.
The query error is returned without wrapping, but the scan error at line 105 is wrapped with
fmt.Errorf("reading rows: %w", err). For consistency and better debuggability, wrap the query error with context.♻️ Proposed fix
rows, err = s.db.Query(ctx, sqlQuery, args...) if err != nil { - return services.DatapipeStatus{}, err + return services.DatapipeStatus{}, fmt.Errorf("querying datapipe_status: %w", err) }🤖 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 `@server/appcfg/internal/appdb/appdb.go` around lines 95 - 98, The query error returned after the s.db.Query call is not wrapped with context information, but the scan error at line 105 is wrapped with fmt.Errorf for better debuggability. For consistency, wrap the query error in the error handling block following the s.db.Query invocation by replacing the direct return of err with fmt.Errorf using a descriptive message (such as "querying datapipe status") and the %w verb to preserve the error chain.
🤖 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 `@server/analysis/analysis_e2e_test.go`:
- Around line 162-198: The mintJWT function has variable declarations scattered
throughout the function body at multiple points. Consolidate all variable
declarations into a single var block at the top of the function (after the
t.Helper() call) to follow Go coding guidelines. Move declarations like
allRoles/err, adminRole/found, authSecret, and dbUser/err to the top, then
update the code to use assignments instead of declaration statements where
appropriate throughout the function body.
---
Nitpick comments:
In `@server/appcfg/internal/appdb/appdb.go`:
- Around line 95-98: The query error returned after the s.db.Query call is not
wrapped with context information, but the scan error at line 105 is wrapped with
fmt.Errorf for better debuggability. For consistency, wrap the query error in
the error handling block following the s.db.Query invocation by replacing the
direct return of err with fmt.Errorf using a descriptive message (such as
"querying datapipe status") and the %w verb to preserve the error chain.
In `@server/implementation_checklist.md`:
- Around line 18-35: The fenced code block in the implementation_checklist.md
file is missing a language specifier after the opening triple backticks. Add a
language identifier (such as `text`) immediately after the opening ``` to make
the code block markdown-compliant. For example, change the opening from ``` to
```text before the server/<feature>/ directory tree structure.
In `@server/README.md`:
- Around line 168-186: The fenced code block containing the directory tree
structure for server/myfeature/ is missing a language specifier after the
opening triple backticks. Add a language identifier to the opening fence of the
code block - use either `text` or leave it empty (just add backticks with the
language specifier like ```text) to satisfy markdown linter requirements for
explicit language identifiers on fenced code blocks.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1bbe3f45-b5b0-43c6-8825-99800564e140
📒 Files selected for processing (21)
cmd/api/src/api/registration/v2.goserver/README.mdserver/analysis/analysis_e2e_test.goserver/appcfg/appcfg.goserver/appcfg/appcfg_e2e_test.goserver/appcfg/internal/appdb/appdb.goserver/appcfg/internal/appdb/appdb_integration_test.goserver/appcfg/internal/appdb/appdb_test.goserver/appcfg/internal/handlers/handlers.goserver/appcfg/internal/handlers/handlers_test.goserver/appcfg/internal/handlers/mocks/appcfg.goserver/appcfg/internal/handlers/mocks/service.goserver/appcfg/internal/handlers/views.goserver/appcfg/internal/routes/routes.goserver/appcfg/internal/routes/routes_test.goserver/appcfg/internal/services/mocks/database.goserver/appcfg/internal/services/services.goserver/appcfg/internal/services/services_test.goserver/docs/architecture/model/bhce.c4server/implementation_checklist.mdserver/modules/modules.go
💤 Files with no reviewable changes (1)
- cmd/api/src/api/registration/v2.go
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
1 participant
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.
Description
Moves the /api/v2/datapipe/status endpoint to the new server architecture
Motivation and Context
Resolves BED-8715
Moves the required endpoint into the new architecture. Some adjustments to the checklist were made after running into some issues during the live demo. Also brought all e2e tests up to the same level of quality.
How Has This Been Tested?
Extensive testing, including an e2e test that was generated on the old handler and updated to point at the new handler to ensure contract equivalence.
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
GET /api/v2/datapipe/status) to return the current state and timestamp fields (including last completed and next scheduled analysis times).Bug Fixes
401when tokens are missing/invalid, and400when theAuthorizationheader is malformed).Documentation