✨ API updates for platform awareness. #874
Conversation
Signed-off-by: Jeff Ortel <[email protected]>
WalkthroughAdds server-side filtering by platform.id to the Applications List endpoint, exposes application manifests in the List response, and adds Platform to the TaskDashboard API representation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Applications API (List)
participant Filter as qf Filter Builder
participant DB as Database
participant Manifest as Manifests join
Client->>API: GET /applications?filter=platform.id=18
API->>Filter: parse/build filter (platform.id → PlatformId literal)
Filter-->>API: filter.Where(db) clause
API->>DB: Query with applied where-clause and join manifests
DB-->>API: Filtered application rows + manifest IDs
API->>Manifest: build manifest refs from rows
API-->>Client: 200 OK (JSON list with manifests)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/application.go (1)
149-151: Document the filter query parameter in the swagger comments.You list supported filters, but the query parameter itself isn’t declared. Add a @param line so the filter shows up in generated API docs and clients.
Consider adding this line near the other params for the List endpoint:
// @param filter query string false "Filter expression. Example: platform.id=18"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/application.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/application.go (3)
api/filter/filter.go (3)
New(16-27)Assert(371-375)Field(223-225)api/filter/lexer.go (1)
LITERAL(23-23)api/filter/error.go (1)
Error(6-8)
🔇 Additional comments (2)
api/application.go (2)
10-10: Import of filter package (qf) looks correct and scoped.Aliasing the filter package as qf is clear and avoids naming collisions with the local variable named filter later. No issues.
243-243: Filter applied to the composed query — placement is fine.Applying the filter after joins and before pagination is correct. Once the rename maps to a real column/expression, this should work as intended.
After fixing the rename target, verify behavior quickly with a minimal manual check (assuming a dev DB exists):
- GET /applications?filter=platform.id=1 returns only apps with PlatformID=1
- GET /applications?filter=platform.id=999999 returns empty list
If you want, I can draft an integration test stub for this endpoint.
Signed-off-by: Jeff Ortel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
api/task.go (1)
103-106: Bug: wrong error is returned when injectFiles failsOn failure, the handler returns result.Error instead of the local err from injectFiles, masking the real cause.
Apply this diff:
- err := r.injectFiles(h.DB(ctx)) - if err != nil { - _ = ctx.Error(result.Error) - return - } + if err := r.injectFiles(h.DB(ctx)); err != nil { + _ = ctx.Error(err) + return + }
🧹 Nitpick comments (3)
api/task.go (3)
239-239: Swagger route for Queued report is incorrectThe handler is registered at TasksReportQueueRoot => /tasks/report/queue, but the doc says /tasks.
Apply this diff:
-// @router /tasks [get] +// @router /tasks/report/queue [get]
309-309: Swagger route for Dashboard report is incorrectThe handler is registered at TasksReportDashboardRoot => /tasks/report/dashboard, but the doc says /tasks.
Apply this diff:
-// @router /tasks [get] +// @router /tasks/report/dashboard [get]
3-11: Use os.ReadFile instead of deprecated io/ioutil.ReadFileio/ioutil is deprecated; prefer os.ReadFile.
Apply these diffs:
-import ( - "fmt" - "io/ioutil" - "net/http" +import ( + "fmt" + "net/http" + "os"- b, nErr := ioutil.ReadFile(m.Path) + b, nErr := os.ReadFile(m.Path)Also applies to: 915-919
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/task.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/task.go (6)
addon/adapter.go (1)
Platform(64-64)migration/v18/model/platform.go (1)
Platform(15-24)model/pkg.go (2)
Platform(37-37)Ref(57-57)migration/json/fields.go (1)
Ref(9-12)api/base.go (1)
Ref(340-343)migration/v13/model/core.go (1)
Ref(178-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-unit
- GitHub Check: build
- GitHub Check: vet
🔇 Additional comments (2)
api/task.go (2)
1009-1013: Add Platform to TaskDashboard: LGTMThe new Platform field mirrors Task.Platform and uses the same JSON/YAML tags. This keeps the dashboard representation consistent with Task and enables clients to access platform metadata directly.
1015-1024: Populate TaskDashboard.Platform consistently: LGTMUsing r.refPtr(m.PlatformID, m.Platform) matches how Application is handled and aligns with existing Task.With() behavior. Upstream DB query already joins Platform, so name resolution will work as with Application.
Signed-off-by: Jeff Ortel <[email protected]>
jortel
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
api/application.go (4)
149-151: Clarify filter usage in swagger doc with an inline exampleAdd a minimal example so users know the exact querystring format.
-// @description filters: -// @description - platform.id +// @description filters: +// @description - platform.id (e.g. GET /applications?filter=platform.id=1)
179-188: SQLite-specific alias in WHERE: document intent to prevent future confusionUsing SELECT aliases in WHERE is valid in SQLite (non-standard elsewhere). Given the repo uses SQLite, this is acceptable. Recommend adding an explanatory comment to avoid “fixes” that break it later.
filter, err := qf.New(ctx, []qf.Assert{ {Field: "platform.id", Kind: qf.LITERAL}, }) if err != nil { _ = ctx.Error(err) return } - filter = filter.Renamed("platform.id", "PlatformId") + // Note: SQLite supports using SELECT column aliases in WHERE clauses (non-standard). + // Mapping "platform.id" -> "PlatformId" relies on that behavior. + filter = filter.Renamed("platform.id", "PlatformId")
245-246: Minor: Apply filters before ORDER for readabilityFunctionally equivalent in GORM, but placing WHERE before ORDER improves readability and avoids surprises if future logic adds GROUP BY/HAVING.
- db = db.Order("a.ID") - db = filter.Where(db) + // Apply filters before ordering for clarity. + db = filter.Where(db) + db = db.Order("a.ID")
1411-1414: Optional: Sort manifests for stable API outputMap iteration order is non-deterministic; sort by ID for consistent JSON. sort is already imported.
r.Manifests = []Ref{} for _, mf := range m.Manifest { r.Manifests = append(r.Manifests, Ref{ID: mf.ID}) } +// Ensure stable ordering. +sort.Slice(r.Manifests, func(i, j int) bool { return r.Manifests[i].ID < r.Manifests[j].ID })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/application.go(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-09T11:55:52.155Z
Learnt from: jortel
PR: konveyor/tackle2-hub#849
File: test/api/application/manifest_test.go:33-33
Timestamp: 2025-07-09T11:55:52.155Z
Learning: The test `TestAppManifestGet` in test/api/application/manifest_test.go is intentionally designed to test the application-scoped manifest API functionality. It deliberately uses `Application.Manifest(application.ID)` for operations that are supported (Create, Get) while using `RichClient.Manifest.Delete` for cleanup operations that are not supported by the application-scoped API.
Applied to files:
api/application.go
📚 Learning: 2025-07-09T11:54:43.934Z
Learnt from: jortel
PR: konveyor/tackle2-hub#849
File: test/api/application/manifest_test.go:33-33
Timestamp: 2025-07-09T11:54:43.934Z
Learning: In test/api/application/manifest_test.go, the application-scoped manifest API (created via Application.Manifest(application.ID)) does not support delete operations. Delete operations must use the global RichClient.Manifest.Delete method. This explains why cleanup code uses RichClient.Manifest.Delete(r.ID) instead of the application-scoped Manifest variable.
Applied to files:
api/application.go
📚 Learning: 2025-08-12T17:51:09.657Z
Learnt from: jortel
PR: konveyor/tackle2-hub#874
File: api/application.go:179-188
Timestamp: 2025-08-12T17:51:09.657Z
Learning: SQLite supports using SELECT column aliases in WHERE clauses as a non-standard extension, unlike most other database systems (PostgreSQL, MySQL, SQL Server, Oracle). This means filter.Renamed() mappings to SELECT aliases are valid in SQLite-based applications like tackle2-hub.
Applied to files:
api/application.go
📚 Learning: 2025-08-12T17:51:09.657Z
Learnt from: jortel
PR: konveyor/tackle2-hub#874
File: api/application.go:179-188
Timestamp: 2025-08-12T17:51:09.657Z
Learning: SQLite supports using SELECT column aliases in WHERE clauses, which is a non-standard SQL extension that differs from most other database systems like MySQL, PostgreSQL, etc. This means filter.Renamed() mappings to SELECT aliases are valid in SQLite-based applications.
Applied to files:
api/application.go
🧬 Code Graph Analysis (1)
api/application.go (7)
api/filter/filter.go (3)
New(16-27)Assert(371-375)Field(223-225)api/filter/lexer.go (1)
LITERAL(23-23)api/filter/error.go (1)
Error(6-8)api/manifest.go (1)
Manifest(334-339)migration/v18/model/platform.go (1)
Manifest(7-13)model/pkg.go (2)
Manifest(34-34)Ref(57-57)api/base.go (1)
Ref(340-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test-unit
- GitHub Check: build
- GitHub Check: vet
🔇 Additional comments (8)
api/application.go (8)
10-10: Import qf alias: LGTMBrings in the filter package with an alias that matches subsequent usage.
205-205: M.ManifestId: LGTMStruct field aligns with the SELECT alias and downstream usage.
227-227: Projection of mf.ID as ManifestId: LGTMAlias matches M.ManifestId and supports the builder aggregation.
243-243: LEFT JOIN Manifest: LGTMJoin matches the projection and aggregation. The maps in the builder mitigate row multiplication from joins.
257-257: Manifests aggregation map: LGTMPattern mirrors identities/contributors/assessments aggregation.
303-307: Collect manifest refs: LGTMCorrectly guards against zero IDs and dedupes via map.
1340-1340: API field Manifests []Ref: LGTMMatches the new list behavior and uses the standard Ref type for related resources.
324-326: Verifymodel.Applicationfield name for manifests
I wasn’t able to locate theApplicationstruct in themodelpackage to confirm whether the slice field is namedManifestorManifests. Please ensure that:
model.Applicationdefines a field(singular) if you intend to useManifest []model.Manifestapp.Manifest,
or(plural) if you need to update the loop toManifests []model.Manifestapp.Manifests = append(app.Manifests, m).
Filter apps on platform id: GET /applications?filter=platform.id=1
Closes #872
Add platform to the task dashboard.
Closes #875
Add list of manifest (refs) to application.
Closes #876
Summary by CodeRabbit
New Features
Documentation