Make project ID mandatory for get calls and fix dtos for apps#1410
Open
Make project ID mandatory for get calls and fix dtos for apps#1410
Conversation
Contributor
WalkthroughMake Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Service
participant Repository
participant Database
Client->>Handler: GET /applications?projectId=<uuid>&limit=20&offset=0
Handler->>Handler: Trim & validate projectId (required)\nParse & clamp limit/offset
Handler->>Service: GetApplicationsByOrganization(orgID, projectID, limit, offset)
Service->>Repository: GetApplicationsByProjectIDPaginated(projectID, orgID, limit, offset)
Repository->>Database: SELECT ... WHERE project_uuid=? AND organization_uuid=?\nORDER BY created_at DESC LIMIT ? OFFSET ?
Database-->>Repository: rows
Repository-->>Service: []*model.Application
Service->>Repository: CountApplicationsByProjectID(projectID, orgID)
Repository->>Database: SELECT COUNT(*) WHERE project_uuid=? AND organization_uuid=?
Database-->>Repository: totalCount
Repository-->>Service: totalCount
Service->>Handler: api.ApplicationListResponse {items, pagination:{total, offset, limit}}
Handler-->>Client: 200 OK + paginated results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 3
🧹 Nitpick comments (3)
platform-api/src/internal/handler/application.go (1)
202-219: Consider aligning pagination parsing style withListApplications.This pagination parsing uses a different style (inline conditionals with
TrimSpace) compared toListApplications(lines 114-128) which usesc.DefaultQuery. Both work correctly, but the inconsistency within the same file may cause minor confusion.♻️ Optional: Use consistent DefaultQuery pattern
- limit := 20 - if limitStr := strings.TrimSpace(c.Query("limit")); limitStr != "" { - parsedLimit, err := strconv.Atoi(limitStr) - if err == nil && parsedLimit > 0 { - if parsedLimit > 100 { - parsedLimit = 100 - } - limit = parsedLimit - } - } - - offset := 0 - if offsetStr := strings.TrimSpace(c.Query("offset")); offsetStr != "" { - parsedOffset, err := strconv.Atoi(offsetStr) - if err == nil && parsedOffset >= 0 { - offset = parsedOffset - } - } + limitStr := c.DefaultQuery("limit", "20") + offsetStr := c.DefaultQuery("offset", "0") + + limit, err := strconv.Atoi(limitStr) + if err != nil || limit <= 0 { + limit = 20 + } + if limit > 100 { + limit = 100 + } + + offset, err := strconv.Atoi(offsetStr) + if err != nil || offset < 0 { + offset = 0 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/handler/application.go` around lines 202 - 219, The pagination parsing for limit and offset is written with TrimSpace and inline conditionals; update it to match the ListApplications style by using c.DefaultQuery to fetch "limit" and "offset", then parse with strconv.Atoi and apply the same bounds (default limit 20, max 100, offset >= 0). Modify the code that sets variables limit and offset (the existing parsedLimit/parsedOffset logic) to follow the ListApplications pattern so the parsing/validation is consistent across handlers.platform-api/src/internal/service/application.go (2)
451-492: In-memory pagination for mapped API keys.The implementation fetches all keys then slices in-memory. This is acceptable for typical use cases but could become a concern for applications with many mapped keys.
One edge case to note: when
limit <= 0(used bybuildMappedAPIKeyList), the code returns all keys but doesn't apply the offset. This is correct sincebuildMappedAPIKeyListalways passesoffset=0, but if the paginated path ever passeslimit <= 0with a non-zero offset, the offset would be ignored while still being reported in the response pagination.Optional: Add defensive handling for the limit <= 0 case
pagedKeys := keys effectiveLimit := len(keys) if limit > 0 { effectiveLimit = limit end := offset + limit if end > total { end = total } pagedKeys = keys[offset:end] +} else if offset > 0 && offset < total { + // When no limit but offset specified, return from offset to end + pagedKeys = keys[offset:] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` around lines 451 - 492, The current buildMappedAPIKeyListPaginated fetches all keys and when limit <= 0 sets pagedKeys = keys without applying offset, which can misreport results if a non-zero offset is passed; update buildMappedAPIKeyListPaginated to apply the offset even when limit <= 0 (e.g., compute start := max(0, offset); pagedKeys = keys[start:total]) and ensure Pagination.Offset and Count reflect the actual slice used (use total and len(pagedKeys) accordingly) while keeping existing behavior when offset > total.
162-176: Remove unusedGetApplicationsByOrganizationIDPaginatedmethod.The repository defines
GetApplicationsByOrganizationIDPaginated, but it is not called anywhere in the codebase. SinceprojectIdis now mandatory and the service uses onlyGetApplicationsByProjectIDPaginated, the organization-level paginated method should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` around lines 162 - 176, Remove the unused organization-level paginated method: delete GetApplicationsByOrganizationIDPaginated from the repository interface and all concrete implementations (e.g., methods on the struct that implement projectRepo or applicationRepo), and remove any associated helper functions, mocks, and tests; ensure callers use GetApplicationsByProjectIDPaginated (the service now requires projectId) and run the build/tests to update any remaining references (search for GetApplicationsByOrganizationIDPaginated to locate all occurrences).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/internal/service/application_test.go`:
- Around line 63-108: The mock repository methods
GetApplicationsByProjectIDPaginated and GetApplicationsByOrganizationIDPaginated
ignore their projectID/orgID inputs and return the same slice, so tests can't
verify the project-scoped path; update the mock (mockApplicationRepository) to
filter m.applications by the provided projectID and orgID (or at minimum record
which method and which IDs were called) and return only matching records, and
change the test fixture to use distinct values for Application.Id (UUID) and
Application.Handle so the test can assert the correct method/filter was used;
ensure CountApplicationsByProjectID and CountApplicationsByOrganizationID
similarly consider the ID input or record the call for assertion.
- Around line 301-329: The test TestListMappedAPIKeys_LimitZeroReturnsAll uses
limit=0 which conflicts with the API contract (minimum: 1); update the test and
expectations to call svc.ListMappedAPIKeys with a legal limit (e.g., 1 or
another positive value) and adjust assertions accordingly (change the call in
the test from ListMappedAPIKeys("my-app", "org-1", 0, 0) to use a positive limit
and assert Count/List/Pagination.Total match that behavior) or alternatively
modify the implementation of ApplicationService.ListMappedAPIKeys to explicitly
accept 0 as a “return all” sentinel and document that in the OpenAPI spec—choose
one consistent approach and update either the test
(TestListMappedAPIKeys_LimitZeroReturnsAll) or the OpenAPI documentation to
match.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 7949-7958: The new query parameter component projectId-Q is marked
required but cannot be reused for GET /mcp-proxies because
components.schemas.MCPProxy allows projectId to be null
(mcp_proxies.project_uuid is nullable) so the collection must support
project-less proxies; fix by introducing a separate optional query parameter
(e.g., projectId-Q-optional or projectId) and use that for GET /mcp-proxies (or
make the GET /mcp-proxies parameter not required), updating the OpenAPI
references so components.schemas.MCPProxy and the GET /mcp-proxies operation
reference the optional parameter instead of projectId-Q.
---
Nitpick comments:
In `@platform-api/src/internal/handler/application.go`:
- Around line 202-219: The pagination parsing for limit and offset is written
with TrimSpace and inline conditionals; update it to match the ListApplications
style by using c.DefaultQuery to fetch "limit" and "offset", then parse with
strconv.Atoi and apply the same bounds (default limit 20, max 100, offset >= 0).
Modify the code that sets variables limit and offset (the existing
parsedLimit/parsedOffset logic) to follow the ListApplications pattern so the
parsing/validation is consistent across handlers.
In `@platform-api/src/internal/service/application.go`:
- Around line 451-492: The current buildMappedAPIKeyListPaginated fetches all
keys and when limit <= 0 sets pagedKeys = keys without applying offset, which
can misreport results if a non-zero offset is passed; update
buildMappedAPIKeyListPaginated to apply the offset even when limit <= 0 (e.g.,
compute start := max(0, offset); pagedKeys = keys[start:total]) and ensure
Pagination.Offset and Count reflect the actual slice used (use total and
len(pagedKeys) accordingly) while keeping existing behavior when offset > total.
- Around line 162-176: Remove the unused organization-level paginated method:
delete GetApplicationsByOrganizationIDPaginated from the repository interface
and all concrete implementations (e.g., methods on the struct that implement
projectRepo or applicationRepo), and remove any associated helper functions,
mocks, and tests; ensure callers use GetApplicationsByProjectIDPaginated (the
service now requires projectId) and run the build/tests to update any remaining
references (search for GetApplicationsByOrganizationIDPaginated to locate all
occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 169bea8f-d006-4383-831b-ad7e4265ab80
📒 Files selected for processing (16)
platform-api/src/api/generated.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/dto/application.goplatform-api/src/internal/handler/api.goplatform-api/src/internal/handler/application.goplatform-api/src/internal/handler/llm.goplatform-api/src/internal/handler/mcp.goplatform-api/src/internal/repository/application.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/service/application.goplatform-api/src/internal/service/application_test.goplatform-api/src/internal/service/llm_template_seeder.goplatform-api/src/internal/service/llm_template_seeder_test.goplatform-api/src/resources/openapi.yaml
💤 Files with no reviewable changes (1)
- platform-api/src/internal/dto/application.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
platform-api/src/internal/repository/application.go (1)
146-161: Paginated method signature accepts but ignores pagination parameters.The method signature includes
limit, offset intto satisfy theApplicationRepositoryinterface, but the implementation ignores them (using_, _). While the TODO explains the rationale, callers may incorrectly assume DB-level pagination is occurring. Consider either:
- Documenting this explicitly in the interface
- Removing the unused parameters from the interface until DB-level pagination is implemented
The current approach is functional since the service layer performs in-memory pagination, but it's an API contract mismatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/repository/application.go` around lines 146 - 161, The method GetApplicationsByProjectIDPaginated currently accepts pagination parameters (limit, offset) but ignores them (named as _, _) causing an API contract mismatch with the ApplicationRepository interface; update the codebase to either (A) propagate and use limit and offset in GetApplicationsByProjectIDPaginated (modify the SQL in ApplicationRepo.GetApplicationsByProjectIDPaginated to accept and apply limit/offset via r.db.Rebind and parameters) or (B) remove the unused limit and offset from the ApplicationRepository interface and all implementations (including ApplicationRepo) so the signature no longer advertises DB-level pagination; pick one approach and update the function signature and any interface(s) or callers (referencing GetApplicationsByProjectIDPaginated and the ApplicationRepository interface) to keep the contract consistent.platform-api/src/internal/service/application.go (1)
178-198: In-memory pagination fetches all records first.
GetApplicationsByProjectID(non-paginated) is called instead ofGetApplicationsByProjectIDPaginated. While this is consistent with the repository's current behavior (both return all rows per the TODO), it means:
- All applications are loaded into memory before slicing
- For large datasets, this could be inefficient
This is acceptable as an interim solution given the TODO comments in the repository layer, but should be addressed when DB-level pagination is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` around lines 178 - 198, The current code performs in-memory pagination by calling s.appRepo.GetApplicationsByProjectID and slicing the returned apps (variables: apps, offset, limit, pagedApps), which loads all rows into memory; change this to use a repository-level paginated call when available: replace the call with s.appRepo.GetApplicationsByProjectIDPaginated(projectID, orgID, offset, limit) (or similar signature), consume the repository's returned slice and totalCount (use the repo's total for pagination logic instead of len(apps)), and handle the fallback to the existing non-paginated GetApplicationsByProjectID if the paginated method is not yet implemented (keeping the current in-memory slicing until DB-level pagination is added).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@platform-api/src/internal/repository/application.go`:
- Around line 146-161: The method GetApplicationsByProjectIDPaginated currently
accepts pagination parameters (limit, offset) but ignores them (named as _, _)
causing an API contract mismatch with the ApplicationRepository interface;
update the codebase to either (A) propagate and use limit and offset in
GetApplicationsByProjectIDPaginated (modify the SQL in
ApplicationRepo.GetApplicationsByProjectIDPaginated to accept and apply
limit/offset via r.db.Rebind and parameters) or (B) remove the unused limit and
offset from the ApplicationRepository interface and all implementations
(including ApplicationRepo) so the signature no longer advertises DB-level
pagination; pick one approach and update the function signature and any
interface(s) or callers (referencing GetApplicationsByProjectIDPaginated and the
ApplicationRepository interface) to keep the contract consistent.
In `@platform-api/src/internal/service/application.go`:
- Around line 178-198: The current code performs in-memory pagination by calling
s.appRepo.GetApplicationsByProjectID and slicing the returned apps (variables:
apps, offset, limit, pagedApps), which loads all rows into memory; change this
to use a repository-level paginated call when available: replace the call with
s.appRepo.GetApplicationsByProjectIDPaginated(projectID, orgID, offset, limit)
(or similar signature), consume the repository's returned slice and totalCount
(use the repo's total for pagination logic instead of len(apps)), and handle the
fallback to the existing non-paginated GetApplicationsByProjectID if the
paginated method is not yet implemented (keeping the current in-memory slicing
until DB-level pagination is added).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a769945-adb0-4e5b-8a19-615136833964
📒 Files selected for processing (3)
platform-api/src/internal/repository/application.goplatform-api/src/internal/service/application.goplatform-api/src/internal/service/application_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/database/schema.sql (1)
41-55:⚠️ Potential issue | 🟠 MajorAdd an explicit migration to backfill
applications.project_uuidbefore enforcing the NOT NULL constraint.The base schema definition changes
project_uuidto NOT NULL, butCREATE TABLE IF NOT EXISTSonly affects fresh installs. Existing databases retain the old nullable column. Since the repository now scans this column into a plain string, any legacy rows with NULL values will fail on read after upgrade.Add an explicit ALTER TABLE and backfill statement under
platform-api/src/internal/databaseto:
- Backfill NULL
project_uuidvalues with a valid fallback or remove such rows- Add the NOT NULL constraint once data is clean
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/database/schema.sql` around lines 41 - 55, Add a new explicit SQL migration under platform-api/src/internal/database that first finds and fixes/removes rows in the applications table where project_uuid IS NULL (either by backfilling a valid fallback project UUID or deleting those legacy rows), then runs an ALTER TABLE applications statement to set project_uuid NOT NULL; ensure this migration runs before the new schema is applied so reads that map applications.project_uuid into a plain string won't encounter NULLs. Use the table name "applications" and column "project_uuid" in the migration and include a clear rollback or safety check to avoid accidental data loss.
♻️ Duplicate comments (1)
platform-api/src/internal/service/application_test.go (1)
77-123:⚠️ Potential issue | 🟡 MinorThe pagination mock still doesn’t prove the project-scoped path was used.
Both paginated methods and count helpers ignore
projectID/orgID, and the fixture still uses the same value forUUIDandHandle. This test can pass even if the service falls back to the org-wide repo method or mapsapi.Application.Idfrom the wrong model field. Filter the mock by the provided IDs (or record which method was called), and make UUID/handle distinct in the fixture.Also applies to: 354-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application_test.go` around lines 77 - 123, Update the mock implementations and fixture so the tests actually verify project-scoped vs org-scoped behavior: in mockApplicationRepository methods GetApplicationsByProjectIDPaginated, GetApplicationsByOrganizationIDPaginated, CountApplicationsByProjectID and CountApplicationsByOrganizationID, filter m.applications by the provided projectID/orgID (or set a flag on the mock when each specific method is invoked) instead of returning the whole slice, and update the test fixture so Application.UUID and Application.Handle are distinct values so a wrong-field mapping would fail the assertion.
🧹 Nitpick comments (3)
platform-api/src/internal/service/application.go (3)
519-519: Consider validating type before casting to enum.The direct cast
api.ApplicationType(app.Type)assumesapp.Typealways contains a valid enum value. WhilenormalizeApplicationTypevalidates on creation, data from the database could theoretically contain unexpected values (e.g., from migrations or direct DB edits).🛡️ Defensive alternative
func modelToApplicationType(appType string) api.ApplicationType { switch strings.ToLower(appType) { case "genai": return api.ApplicationTypeGenai default: return api.ApplicationType(appType) // fallback } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` at line 519, The direct cast at Type: api.ApplicationType(app.Type) can produce invalid enum values from unexpected DB data; replace it by introducing a validator/mapper (e.g., modelToApplicationType) that inspects the string (case-insensitive) and returns known api.ApplicationType constants (e.g., api.ApplicationTypeGenai) for recognized values and a safe fallback (e.g., api.ApplicationType(app.Type) or a default) otherwise, then use modelToApplicationType(app.Type) wherever the direct cast was used (refer to normalizeApplicationType for creation-time validation semantics).
183-196: Dead code:limit > 0is always true after clamping.Since
limitis clamped to[1..100]at lines 157-162, the conditionif limit > 0at line 190 is always true. The assignments at lines 188-189 are never used.♻️ Suggested simplification
totalCount := len(apps) if offset > totalCount { offset = totalCount } - end := totalCount - effectiveLimit := totalCount - if limit > 0 { - effectiveLimit = limit - end = offset + limit - if end > totalCount { - end = totalCount - } + end := offset + limit + if end > totalCount { + end = totalCount } - pagedApps := apps[offset:end] + pagedApps := apps[offset:end] response := &api.ApplicationListResponse{ Count: len(pagedApps), List: make([]api.Application, 0, len(pagedApps)), Pagination: api.Pagination{ Total: totalCount, Offset: offset, - Limit: effectiveLimit, + Limit: limit, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` around lines 183 - 196, The branch checking `if limit > 0` is dead because `limit` is clamped to [1..100] earlier; simplify by removing that conditional and compute `effectiveLimit` and `end` directly from the clamped `limit` (e.g., set effectiveLimit = limit, end = min(offset+limit, totalCount) after clamping offset to totalCount). Update the logic around the variables totalCount, offset, limit, effectiveLimit, and end to use the simplified computation and remove the unused assignments.
315-322: Minor: Duplicate database query for mapped API keys.Both
buildMappedAPIKeyList(line 315) andlistMappedAPIKeysForBroadcast(line 319) calls.appRepo.ListMappedAPIKeys(applicationUUID). Consider reusing the query result to avoid the redundant database call.♻️ Suggested optimization
- keys, err := s.buildMappedAPIKeyList(app.UUID) - if err != nil { - return nil, err - } - broadcastKeys, err := s.listMappedAPIKeysForBroadcast(app.UUID) + broadcastKeys, err := s.listMappedAPIKeysForBroadcast(app.UUID) if err != nil { return nil, err } + keys := buildMappedAPIKeyListFromKeys(broadcastKeys)This would require extracting the response-building logic into a helper that accepts the raw keys slice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/application.go` around lines 315 - 322, Both methods currently call s.appRepo.ListMappedAPIKeys(applicationUUID) twice causing a duplicate DB query; refactor to call ListMappedAPIKeys once and reuse the result by extracting the response-building logic into a helper that accepts the raw []MappedAPIKey slice. Specifically, replace the second call in the application flow by passing the already-fetched keys from buildMappedAPIKeyList to a new helper (e.g., buildMappedAPIKeyResponses or buildBroadcastKeyViews) that returns the two different response shapes used by buildMappedAPIKeyList and listMappedAPIKeysForBroadcast, or update those functions to accept the raw keys slice so the repository query happens only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/internal/handler/api.go`:
- Around line 213-219: The handler currently only checks projectId for non-empty
but should validate it's a UUID before calling
h.apiService.GetAPIsByOrganization; update the handler around the projectId
variable to parse/validate the UUID (e.g. using uuid.Parse or equivalent) and if
parsing fails return a 400 using utils.NewErrorResponse with a clear "invalid
projectId" message, otherwise pass the validated UUID (or the original string if
you keep type) into GetAPIsByOrganization so malformed IDs are rejected at the
handler boundary.
In `@platform-api/src/internal/repository/application.go`:
- Around line 146-178: Both Paginated methods ignore the limit/offset parameters
and fetch the full result set; update GetApplicationsByProjectIDPaginated and
GetApplicationsByOrganizationIDPaginated to accept named limit and offset ints
(replace the unnamed placeholders) and include "LIMIT ? OFFSET ?" in the SQL
(use r.db.Rebind to adapt placeholders) and pass limit then offset into
r.db.Query so the DB returns a paginated subset before scanApplications is
called.
---
Outside diff comments:
In `@platform-api/src/internal/database/schema.sql`:
- Around line 41-55: Add a new explicit SQL migration under
platform-api/src/internal/database that first finds and fixes/removes rows in
the applications table where project_uuid IS NULL (either by backfilling a valid
fallback project UUID or deleting those legacy rows), then runs an ALTER TABLE
applications statement to set project_uuid NOT NULL; ensure this migration runs
before the new schema is applied so reads that map applications.project_uuid
into a plain string won't encounter NULLs. Use the table name "applications" and
column "project_uuid" in the migration and include a clear rollback or safety
check to avoid accidental data loss.
---
Duplicate comments:
In `@platform-api/src/internal/service/application_test.go`:
- Around line 77-123: Update the mock implementations and fixture so the tests
actually verify project-scoped vs org-scoped behavior: in
mockApplicationRepository methods GetApplicationsByProjectIDPaginated,
GetApplicationsByOrganizationIDPaginated, CountApplicationsByProjectID and
CountApplicationsByOrganizationID, filter m.applications by the provided
projectID/orgID (or set a flag on the mock when each specific method is invoked)
instead of returning the whole slice, and update the test fixture so
Application.UUID and Application.Handle are distinct values so a wrong-field
mapping would fail the assertion.
---
Nitpick comments:
In `@platform-api/src/internal/service/application.go`:
- Line 519: The direct cast at Type: api.ApplicationType(app.Type) can produce
invalid enum values from unexpected DB data; replace it by introducing a
validator/mapper (e.g., modelToApplicationType) that inspects the string
(case-insensitive) and returns known api.ApplicationType constants (e.g.,
api.ApplicationTypeGenai) for recognized values and a safe fallback (e.g.,
api.ApplicationType(app.Type) or a default) otherwise, then use
modelToApplicationType(app.Type) wherever the direct cast was used (refer to
normalizeApplicationType for creation-time validation semantics).
- Around line 183-196: The branch checking `if limit > 0` is dead because
`limit` is clamped to [1..100] earlier; simplify by removing that conditional
and compute `effectiveLimit` and `end` directly from the clamped `limit` (e.g.,
set effectiveLimit = limit, end = min(offset+limit, totalCount) after clamping
offset to totalCount). Update the logic around the variables totalCount, offset,
limit, effectiveLimit, and end to use the simplified computation and remove the
unused assignments.
- Around line 315-322: Both methods currently call
s.appRepo.ListMappedAPIKeys(applicationUUID) twice causing a duplicate DB query;
refactor to call ListMappedAPIKeys once and reuse the result by extracting the
response-building logic into a helper that accepts the raw []MappedAPIKey slice.
Specifically, replace the second call in the application flow by passing the
already-fetched keys from buildMappedAPIKeyList to a new helper (e.g.,
buildMappedAPIKeyResponses or buildBroadcastKeyViews) that returns the two
different response shapes used by buildMappedAPIKeyList and
listMappedAPIKeysForBroadcast, or update those functions to accept the raw keys
slice so the repository query happens only once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c488bd4-f5aa-4702-9f09-697a4209e058
📒 Files selected for processing (16)
platform-api/src/api/generated.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/dto/application.goplatform-api/src/internal/handler/api.goplatform-api/src/internal/handler/application.goplatform-api/src/internal/handler/llm.goplatform-api/src/internal/handler/mcp.goplatform-api/src/internal/repository/application.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/service/application.goplatform-api/src/internal/service/application_test.goplatform-api/src/internal/service/llm_template_seeder.goplatform-api/src/internal/service/llm_template_seeder_test.goplatform-api/src/resources/openapi.yaml
💤 Files with no reviewable changes (1)
- platform-api/src/internal/dto/application.go
✅ Files skipped from review due to trivial changes (2)
- platform-api/src/internal/service/llm_template_seeder_test.go
- platform-api/src/internal/handler/application.go
🚧 Files skipped from review as they are similar to previous changes (7)
- platform-api/src/internal/handler/mcp.go
- platform-api/src/internal/service/llm_template_seeder.go
- platform-api/src/internal/database/schema.sqlite.sql
- platform-api/src/internal/database/schema.postgres.sql
- platform-api/src/internal/handler/llm.go
- platform-api/src/resources/openapi.yaml
- platform-api/src/internal/repository/interfaces.go
Comment on lines
+213
to
219
| projectId := strings.TrimSpace(c.Query("projectId")) | ||
| if projectId == "" { | ||
| c.JSON(http.StatusBadRequest, utils.NewErrorResponse(400, "Bad Request", "projectId query parameter is required")) | ||
| return | ||
| } | ||
|
|
||
| var projectId string | ||
| if params.ProjectId != nil { | ||
| projectId = string(*params.ProjectId) | ||
| } | ||
|
|
||
| apis, err := h.apiService.GetAPIsByOrganization(orgId, projectId) |
Contributor
There was a problem hiding this comment.
Validate projectId as a UUID here, not just as a non-empty string.
This makes the query mandatory, but it also drops the UUID-format validation from the endpoint contract. A value like projectId=foo will now reach GetAPIsByOrganization and usually surface as “project not found” instead of a 400. Parse/reject malformed UUIDs at the handler boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/internal/handler/api.go` around lines 213 - 219, The handler
currently only checks projectId for non-empty but should validate it's a UUID
before calling h.apiService.GetAPIsByOrganization; update the handler around the
projectId variable to parse/validate the UUID (e.g. using uuid.Parse or
equivalent) and if parsing fails return a 400 using utils.NewErrorResponse with
a clear "invalid projectId" message, otherwise pass the validated UUID (or the
original string if you keep type) into GetAPIsByOrganization so malformed IDs
are rejected at the handler boundary.
Comment on lines
+146
to
+178
| func (r *ApplicationRepo) GetApplicationsByProjectIDPaginated(projectID, orgID string, _, _ int) ([]*model.Application, error) { | ||
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | ||
| // across all supported database drivers. | ||
| rows, err := r.db.Query(r.db.Rebind(` | ||
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | ||
| FROM applications | ||
| WHERE project_uuid = ? AND organization_uuid = ? | ||
| ORDER BY created_at DESC, name ASC | ||
| `), projectID, orgID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| return scanApplications(rows) | ||
| } | ||
|
|
||
| func (r *ApplicationRepo) GetApplicationsByOrganizationIDPaginated(orgID string, _, _ int) ([]*model.Application, error) { | ||
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | ||
| // across all supported database drivers. | ||
| rows, err := r.db.Query(r.db.Rebind(` | ||
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | ||
| FROM applications | ||
| WHERE organization_uuid = ? | ||
| ORDER BY created_at DESC, name ASC | ||
| `), orgID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| return scanApplications(rows) | ||
| } |
Contributor
There was a problem hiding this comment.
These Paginated repository methods still fetch the full result set.
limit and offset are ignored in both queries, so every page request still reads all matching applications and relies on in-memory slicing later. That defeats pagination and will get expensive quickly as application counts grow.
🧭 Suggested direction
-func (r *ApplicationRepo) GetApplicationsByProjectIDPaginated(projectID, orgID string, _, _ int) ([]*model.Application, error) {
+func (r *ApplicationRepo) GetApplicationsByProjectIDPaginated(projectID, orgID string, limit, offset int) ([]*model.Application, error) {
rows, err := r.db.Query(r.db.Rebind(`
SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at
FROM applications
WHERE project_uuid = ? AND organization_uuid = ?
ORDER BY created_at DESC, name ASC
- `), projectID, orgID)
+ LIMIT ? OFFSET ?
+ `), projectID, orgID, limit, offset)Apply the same change to GetApplicationsByOrganizationIDPaginated.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *ApplicationRepo) GetApplicationsByProjectIDPaginated(projectID, orgID string, _, _ int) ([]*model.Application, error) { | |
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | |
| // across all supported database drivers. | |
| rows, err := r.db.Query(r.db.Rebind(` | |
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | |
| FROM applications | |
| WHERE project_uuid = ? AND organization_uuid = ? | |
| ORDER BY created_at DESC, name ASC | |
| `), projectID, orgID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| return scanApplications(rows) | |
| } | |
| func (r *ApplicationRepo) GetApplicationsByOrganizationIDPaginated(orgID string, _, _ int) ([]*model.Application, error) { | |
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | |
| // across all supported database drivers. | |
| rows, err := r.db.Query(r.db.Rebind(` | |
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | |
| FROM applications | |
| WHERE organization_uuid = ? | |
| ORDER BY created_at DESC, name ASC | |
| `), orgID) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| return scanApplications(rows) | |
| } | |
| func (r *ApplicationRepo) GetApplicationsByProjectIDPaginated(projectID, orgID string, limit, offset int) ([]*model.Application, error) { | |
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | |
| // across all supported database drivers. | |
| rows, err := r.db.Query(r.db.Rebind(` | |
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | |
| FROM applications | |
| WHERE project_uuid = ? AND organization_uuid = ? | |
| ORDER BY created_at DESC, name ASC | |
| LIMIT ? OFFSET ? | |
| `), projectID, orgID, limit, offset) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| return scanApplications(rows) | |
| } | |
| func (r *ApplicationRepo) GetApplicationsByOrganizationIDPaginated(orgID string, limit, offset int) ([]*model.Application, error) { | |
| // TODO: Re-enable DB-level pagination when query placeholders and syntax are verified | |
| // across all supported database drivers. | |
| rows, err := r.db.Query(r.db.Rebind(` | |
| SELECT uuid, handle, project_uuid, organization_uuid, created_by, name, description, type, created_at, updated_at | |
| FROM applications | |
| WHERE organization_uuid = ? | |
| ORDER BY created_at DESC, name ASC | |
| LIMIT ? OFFSET ? | |
| `), orgID, limit, offset) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| return scanApplications(rows) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform-api/src/internal/repository/application.go` around lines 146 - 178,
Both Paginated methods ignore the limit/offset parameters and fetch the full
result set; update GetApplicationsByProjectIDPaginated and
GetApplicationsByOrganizationIDPaginated to accept named limit and offset ints
(replace the unnamed placeholders) and include "LIMIT ? OFFSET ?" in the SQL
(use r.db.Rebind to adapt placeholders) and pass limit then offset into
r.db.Query so the DB returns a paginated subset before scanApplications is
called.
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.
Summary by CodeRabbit