feat(api): add page-based pagination for deployments #804
Conversation
WalkthroughThis pull request introduces a comprehensive overhaul of pagination mechanisms across the GraphQL API and backend infrastructure. The changes replace cursor-based pagination with a more flexible page-based pagination approach, modifying GraphQL schemas, resolver methods, and repository implementations. The new pagination system introduces more structured pagination parameters, including page number, page size, ordering direction, and total count information. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQLResolver
participant Repository
participant Database
Client->>GraphQLResolver: Request with PageBasedPagination
GraphQLResolver->>Repository: Find with pagination parameters
Repository->>Database: Query with offset and limit
Database-->>Repository: Return paginated results
Repository-->>GraphQLResolver: Return results with PageInfo
GraphQLResolver-->>Client: Respond with paginated data
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-flow canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
api/internal/infrastructure/memory/deployment.go (1)
37-45: Validate sorting for consistent ordering.
The results are appended in whatever order they appear inr.data. If the order of returned deployments matters, consider sorting them before pagination to avoid nondeterministic order across requests.api/internal/infrastructure/mongo/deployment.go (2)
61-64: Return an empty slice instead ofnilwhenidsis empty.- return nil, nil + return []*deployment.Deployment{}, nil
79-83: Consider removing the unusedoperatorparam if not needed.api/internal/adapter/gql/generated.go (2)
212-218: Separate cursor-based and page-based fields inPageInfo.
Mixing fields likestartCursor/endCursorwith newly introducedcurrentPage/totalPagescan cause confusion and maintenance complexity. Consider splitting them into separate types or adding clear documentation to distinguish cursor-based from page-based pagination.
8674-8799: Resolvers for_PageInfo_totalCount,_PageInfo_currentPage,_PageInfo_totalPages
Implementation correctly distinguishes the required nature oftotalCountfrom the optional fields. IfcurrentPage/totalPagesshould never be null, consider returning default values.api/internal/usecase/repo/deployment.go (1)
Line range hint
28-57: Consider optimizing the iteration logicWhile the implementation is correct, consider these improvements:
- Extract the cursor pagination creation logic into a helper function
- Add early return when no deployments are found
func IterateDeploymentsByWorkspace(repo Deployment, ctx context.Context, tid accountdomain.WorkspaceID, batch int64, callback func([]*deployment.Deployment) error) error { - cursorPagination := usecasex.CursorPagination{ - First: lo.ToPtr(batch), - }.Wrap() + createCursorPagination := func(after *usecasex.Cursor) *usecasex.Pagination { + return usecasex.CursorPagination{ + First: lo.ToPtr(batch), + After: after, + }.Wrap() + } + + cursorPagination := createCursorPagination(nil) for { pagination := &interfaces.PaginationParam{ Cursor: cursorPagination, } deployments, info, err := repo.FindByWorkspace(ctx, tid, pagination) if err != nil { return err } if len(deployments) == 0 { return nil } if err := callback(deployments); err != nil { return err } if !info.HasNextPage { break } c := usecasex.Cursor(deployments[len(deployments)-1].ID().String()) - cursorPagination = usecasex.CursorPagination{ - First: lo.ToPtr(batch), - After: &c, - }.Wrap() + cursorPagination = createCursorPagination(&c) } return nilapi/internal/infrastructure/mongo/deployment_test.go (1)
58-61: Consider adding more test data variations.The test data could be enhanced with:
- Different updatedat timestamps for better sort testing
- Edge cases like same timestamp updates
api/gql/_shared.graphql (1)
30-32: Consider making totalCount nullable.While the new pagination fields are well-structured, making
totalCountnon-nullable (Int!) might be restrictive. There could be cases where calculating the total count is expensive or not feasible, especially with large datasets.- totalCount: Int! + totalCount: Int
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/gql/_shared.graphql(1 hunks)api/gql/deployment.graphql(1 hunks)api/internal/adapter/gql/generated.go(23 hunks)api/internal/adapter/gql/gqlmodel/convert.go(2 hunks)api/internal/adapter/gql/gqlmodel/models_gen.go(2 hunks)api/internal/adapter/gql/loader_deployment.go(1 hunks)api/internal/adapter/gql/resolver_query.go(1 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/container.go(2 hunks)api/internal/infrastructure/mongo/deployment.go(5 hunks)api/internal/infrastructure/mongo/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/mongodoc/deployment.go(3 hunks)api/internal/usecase/interactor/deployment.go(1 hunks)api/internal/usecase/interfaces/deployment.go(1 hunks)api/internal/usecase/repo/deployment.go(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
api/internal/infrastructure/memory/deployment_test.go
[error] 253-253: Test failure in TestDeployment_FindByWorkspace/page_based_pagination:_second_page: Expected and actual deployment objects do not match. Differences in ID and version fields.
api/internal/infrastructure/mongo/container.go
[error] 34-34: Unnecessary type assertion: NewDeployment(client) already has type repo.Deployment (gosimple S1040)
🪛 GitHub Check: ci-api / ci-api-lint
api/internal/infrastructure/mongo/container.go
[failure] 34-34:
S1040: type assertion to the same type: NewDeployment(client) already has type repo.Deployment (gosimple)
api/internal/infrastructure/mongo/deployment.go
[failure] 228-228:
composites: go.mongodb.org/mongo-driver/bson/primitive.E struct literal uses unkeyed fields (govet)
[failure] 231-231:
composites: go.mongodb.org/mongo-driver/bson/primitive.E struct literal uses unkeyed fields (govet)
🪛 golangci-lint (1.62.2)
api/internal/infrastructure/mongo/container.go
34-34: S1040: type assertion to the same type: NewDeployment(client) already has type repo.Deployment
(gosimple)
api/internal/infrastructure/mongo/deployment.go
228-228: composites: go.mongodb.org/mongo-driver/bson/primitive.E struct literal uses unkeyed fields
(govet)
231-231: composites: go.mongodb.org/mongo-driver/bson/primitive.E struct literal uses unkeyed fields
(govet)
🔇 Additional comments (76)
api/internal/infrastructure/mongo/container.go (1)
69-69: Confirm that the asserted type remains valid.
Castingr.Deploymentto*DeploymentAdapteris acceptable if we are certain no other types assign tor.Deployment. Consider adding safety checks if future code changes modifyr.Deployment's type.api/internal/infrastructure/memory/deployment.go (4)
8-8: Imports look good.
The newly importedinterfacespackage aligns with the updated function signatures referencingPaginationParam.
52-74: Page-based pagination logic appears correct.
The calculation ofskipandlimitis straightforward, and returning an empty slice ifskip >= len(result)aligns with standard paging approaches.
74-125: Cursor-based pagination approach is thorough.
The code correctly handlesFirstandAfterto slice the result. However, support forBeforeorLastis not implemented here. If partial backward pagination is needed, consider extending logic to handle those parameters.
128-130: Use consistent total counts.
Returning the entire filteredlen(result)asTotalCountfor both page-based and cursor-based approaches ensures consistent pagination info. Good job.api/internal/adapter/gql/loader_deployment.go (7)
44-47: Good use of typed ID conversion.
Converting fromwsIDto the strongly typedaccountdomain.Workspaceensures type safety for workspace references.
49-61: Page-based pagination parameter handling is clear.
This block properly packages the page-based pagination details intopaginationParam, aligning with the newinterfaces.PaginationParam.
62-71: Cursor-based pagination parameter is well-defined.
Falling back to cursor-based pagination when page-based parameters are unavailable is a clean approach.
74-96: Potential mismatch in total count.
You uselen(res)forTotalCounthere rather than referencingpageInfo.TotalCountfrom the usecase. If the repository layer slices results under the hood, you might want to ensure a true “total” is reported rather than the size of the current slice. Confirm the desired behavior.
98-110: Dedicated page-based method is neatly structured.
This method concisely creates aPaginationParamand calls the usecase, avoiding duplication of logic inFindByWorkspace. Great approach for clarity.
121-125: Consistent node and edge formation.
Converting deployments to GraphQL edges and nodes follows the established pattern and fosters easy integration with your front-end.
132-133: Same total count consideration.
Again, note thatlen(res)might reflect only the slice returned by the usecase. Confirm if you want the total items in the dataset versus the items in this slice.api/internal/infrastructure/mongo/deployment.go (26)
7-8: Imports look good.
12-12: Additional import is appropriate.
27-27: Switching from*mongox.ClientCollectionto*mongox.Collectionlooks fine.
31-34: IntroducingDeploymentAdapteris consistent with the adapter pattern.
35-39:NewDeploymentreturning aDeploymentAdapteraligns clearly with the new design.
46-51:Filteredmethod correctly merges workspace filters.
71-76: Query logic forFindByIDsis clear and correct.
85-85: No issues with adding theworkspacefield to the query filter.
89-91:FindByProjectmethod forwarding tofindOneis straightforward.
95-104:FindByVersioncorrectly constructs and passes the filter.
106-112:FindHeadlogic is consistent with retrieving the head deployment.
117-130:FindVersionsretrieves all matching deployments with no issues.
132-145:Createmethod workflow for new deployments looks good.
148-167:Updatemethod correctly retrieves and updates relevant fields before saving.
169-177:Executeis intentionally left unimplemented withErrNotImplemented.
178-183:Fetchfunction simply delegates toFindByIDs, which is a fine approach.
187-192:Saveproperly transforms a deployment into a MongoDB document and saves it.
195-198:Removefunction implements straightforward deletion logic.
300-303:FindByIDsinDeploymentAdapterconsistently proxies to the underlying implementation.
304-307:FindByIDpass-through is correct.
308-311:FindByWorkspaceadapter method aligns with the new pagination logic.
312-315:FindByVersionadapter method is consistent with the underlying implementation.
316-319:FindHeadpass-through is fine.
320-323:FindVersionspass-through is straightforward.
324-327:Savepass-through is correct.
328-330:Removepass-through is consistent.api/internal/adapter/gql/generated.go (21)
272-272: New complexity function forDeploymentsPage
This addition looks appropriate, aligning with the newly introduced page-based pagination.
423-423: Method signature forDeploymentsPageinQueryResolver
The new method to fetch deployments by page-based parameters is clearly defined. Ensure it’s consistently implemented and tested.
1234-1240: Complexity function forPageInfo.currentPage
The logic for handlingcurrentPagecomplexity appears consistent with other fields inPageInfo.
1269-1282: Complexity handlers fortotalCountandtotalPages
These fields are well integrated into the complexity layer. Validate that the resolvers always provide meaningful values under high-load conditions.
1553-1564: Complexity handling forDeploymentsPage
This extends the complexity checks to the new query, ensuring consistent GraphQL performance management.
1981-1981: Reference tounmarshalInputPageBasedPagination
The new unmarshal function is properly referenced. No concerns here.
2141-2155: Updated GraphQL schema
IntroducingtotalCount,currentPage,totalPages, and the newPageBasedPaginationinput type is coherent. The optional vs. required fields align with typical usage.
2337-2337:deploymentsPagefield in the Query
The schema addition fordeploymentsPagealigns with the page-based pagination paradigm.
3361-3383: Arguments forQuery.deploymentsPage
Type-safe argument parsing ensures strong binding toPageBasedPagination.
4321-4326: Extended switch cases forPageInfofields
The additions fortotalCount,currentPage, andtotalPagesmatch the existing approach for other fields.
5504-5509: Repeat ofPageInfofield handling
No issues. Code consistently manages the new fields.
6284-6289: Consistent field coverage inPageInfo
The expansions follow the same pattern as the other partial sections, maintaining uniformity.
10037-10042: AdditionalPageInfocases
The pattern for handling each new field remains consistent.
10520-10584: Resolver for_Query_deploymentsPage
Properly checks arguments and returns a non-nullDeploymentConnection. Looks good.
12431-12436: Refined switch forPageInfo
No issues. Matches the existing structure for field resolution.
15721-15767: Implementation ofunmarshalInputPageBasedPagination
Considers each pagination field in a strict order. Recommend validatingpageandpageSizeto ensure they are > 0, preventing invalid or nonsensical pagination requests.
15811-15838: ExtendedunmarshalInputPagination
The partial overlap between page-based and cursor-based pagination is well-handled.
17805-17813: Selections for new PageInfo fields
The selection logic fortotalCount,currentPage, andtotalPagesis consistent with the pattern used elsewhere.
18344-18365: Concurrent resolution approach fordeploymentsPage
Concurrently resolving thedeploymentsPagequery is commendable for performance.
20406-20409: Non-null unmarshal forPageBasedPagination
Ensures page-based pagination is always provided and validated.
21493-21508: Unmarshal function forOrderDirection
Properly returnsnilif unspecified. This matches typical GraphQL optional enum handling.api/internal/adapter/gql/gqlmodel/convert.go (2)
56-62: LGTM! Clean implementation of OrderDirectionToStringThe function properly handles nil input and follows good practices for string conversion.
33-33: Consider potential integer overflow when converting TotalCountConverting
int64tointcould lead to data loss if the total count exceeds the maximum value of a 32-bit integer.✅ Verification successful
Integer overflow risk is negligible in this context
The conversion from int64 to int for TotalCount is safe because:
- Deployments are scoped to workspaces
- GraphQL spec uses 32-bit integers by design
- Business context makes it impractical to exceed 2^31 deployments per workspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any deployments counts that could exceed int32 max value rg -l "deployment.*count|count.*deployment" | xargs rg "count|length|size" -A 5 -B 5Length of output: 22899
api/internal/usecase/repo/deployment.go (1)
18-18: LGTM! Clean interface update for paginationThe interface change properly accommodates both cursor-based and page-based pagination through the new
PaginationParamtype.api/internal/infrastructure/mongo/mongodoc/deployment.go (1)
33-41: LGTM! Improved error handlingThe changes properly handle nil values and empty IDs, returning appropriate errors. This makes the function more robust and easier to use safely.
api/internal/adapter/gql/resolver_query.go (1)
32-34: LGTM! Clean implementation of page-based pagination resolver.The implementation follows the established pattern and correctly delegates to the loader.
api/internal/infrastructure/mongo/deployment_test.go (2)
66-74: LGTM! Comprehensive test for non-paginated results.The test properly verifies:
- Total count of deployments
- Correct ordering of results
- All expected deployments are returned
75-91: LGTM! Well-structured first page test.The test thoroughly validates:
- Correct page size
- Total count
- Navigation flags
- Expected deployments
api/internal/infrastructure/memory/deployment_test.go (1)
189-209: LGTM! Comprehensive cursor-based pagination test.The test properly validates:
- Cursor pagination parameters
- Start and end cursors
- Navigation flags
- Expected deployments
api/internal/usecase/interactor/deployment.go (1)
54-56: LGTM! Clean signature update for pagination enhancement.The method signature change from
*usecasex.Paginationto*interfaces.PaginationParamcorrectly supports both page-based and cursor-based pagination.api/gql/_shared.graphql (3)
35-40: LGTM! Well-structured pagination input.The
PageBasedPaginationinput type follows standard pagination patterns with required page and size fields, while keeping ordering optional.
42-45: LGTM! Clear enum definition.The
OrderDirectionenum is well-defined with standard ascending and descending options.
47-57: LGTM! Good combination of pagination approaches.The
Paginationinput type effectively combines both cursor-based and page-based pagination, allowing for flexible implementation choices.api/gql/deployment.graphql (1)
82-82: LGTM! Clear and consistent query definition.The new
deploymentsPagequery is well-defined with required pagination, ensuring clients provide necessary pagination parameters.api/internal/adapter/gql/gqlmodel/models_gen.go (4)
233-238: LGTM! Well-structured pagination struct.The
PageBasedPaginationstruct correctly implements the GraphQL input type with appropriate Go types.
245-247: LGTM! Consistent field additions.The new
PageInfofields match the GraphQL schema definition.
251-258: LGTM! Complete pagination options.The
Paginationstruct effectively combines both pagination approaches with appropriate field types.
663-702: LGTM! Complete enum implementation.The
OrderDirectiontype is well-implemented with all necessary methods for GraphQL enum handling:
- Constants for all possible values
- Validation method
- String conversion
- GraphQL marshaling/unmarshaling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
api/internal/infrastructure/mongo/deployment.go (5)
31-33: Consider simplifying the adapter implementation.The
DeploymentAdapterstruct only wraps the implementation without adding significant value. Consider either:
- Adding more functionality in the adapter layer, or
- Removing the adapter pattern if it's not needed for future extensibility.
224-227: Define constants for sort direction.Replace magic numbers with named constants for better readability and maintainability.
+const ( + sortAscending = 1 + sortDescending = -1 +) -direction := 1 // default ascending +direction := sortAscending // default ascending if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 + direction = sortDescending }
216-262: Consider extracting page-based pagination logic.The page-based pagination logic is quite complex and could benefit from being extracted into a separate method for better maintainability.
+func (r *Deployment) paginateByPage(ctx context.Context, filter bson.M, page *interfaces.PageBasedPagination) ([]*deployment.Deployment, *usecasex.PageInfo, error) { + c := mongodoc.NewDeploymentConsumer(r.f.Readable) + skip := (page.Page - 1) * page.PageSize + limit := page.PageSize + + sort := r.buildSort(page) + + total, err := r.client.Count(ctx, filter) + if err != nil { + return nil, nil, rerror.ErrInternalByWithContext(ctx, err) + } + + opts := options.Find(). + SetSort(sort). + SetSkip(int64(skip)). + SetLimit(int64(limit)) + + if err := r.client.Find(ctx, filter, c, opts); err != nil { + return nil, nil, rerror.ErrInternalByWithContext(ctx, err) + } + + pageInfo := buildPageInfo(total, page.Page, page.PageSize) + return c.Result, pageInfo, nil +}
148-167: Add input validation in Update method.The Update method should validate that at least one field is being updated to avoid unnecessary database operations.
func (r *Deployment) Update(ctx context.Context, param interfaces.UpdateDeploymentParam, operator *usecase.Operator) (*deployment.Deployment, error) { + if param.Description == nil && param.Workflow == nil { + return nil, rerror.NewE(rerror.ErrInvalidInput, "at least one field must be updated") + } + d, err := r.FindByID(ctx, param.ID) if err != nil { return nil, err }
187-190: Enhance error messages in Save method.Add more context to the error message to help with debugging.
doc, err := mongodoc.NewDeployment(deployment) if err != nil { - return rerror.ErrInternalByWithContext(ctx, err) + return rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to create deployment document: %w", err)) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/internal/infrastructure/mongo/container.go(1 hunks)api/internal/infrastructure/mongo/deployment.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/infrastructure/mongo/container.go
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
api/internal/infrastructure/memory/deployment.go (1)
52-55: Refactor sorting logic to reduce duplication and maintain consistency.The default sorting logic is duplicated in the custom sorting section. Additionally, the default sorting direction (desc) is inconsistent with the custom sorting default (asc).
Consider extracting the sorting logic into a helper function:
+func sortDeployments(deployments []*deployment.Deployment, orderBy *string, orderDir *string) { + direction := 1 // default ascending + if orderDir != nil && *orderDir == "DESC" { + direction = -1 + } + + sort.Slice(deployments, func(i, j int) bool { + switch { + case orderBy != nil && *orderBy == "version": + if direction == 1 { + return deployments[i].Version() < deployments[j].Version() + } + return deployments[i].Version() > deployments[j].Version() + default: + if direction == 1 { + return deployments[i].UpdatedAt().Before(deployments[j].UpdatedAt()) + } + return deployments[i].UpdatedAt().After(deployments[j].UpdatedAt()) + } + }) +} - // Sort by updatedAt desc by default - sort.Slice(result, func(i, j int) bool { - return result[i].UpdatedAt().After(result[j].UpdatedAt()) - }) + // Apply default sorting + defaultOrderDir := "DESC" + sortDeployments(result, nil, &defaultOrderDir)🧰 Tools
🪛 GitHub Actions: ci
[error] Package test failure with coverage: 15.9% of statements
api/internal/infrastructure/mongo/deployment.go (4)
29-31: Add documentation for the DeploymentAdapter type.The newly introduced
DeploymentAdaptertype lacks documentation explaining its purpose and relationship with theDeploymenttype.Add a documentation comment:
+// DeploymentAdapter wraps the Deployment type to implement the repo.Deployment interface +// and provide workspace filtering capabilities. type DeploymentAdapter struct { *Deployment }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
45-50: Optimize memory allocation in Filtered method.The current implementation creates new instances of both
DeploymentAdapterandDeploymenton every call. Consider reusing the existingclientinstance.func (a *DeploymentAdapter) Filtered(f repo.WorkspaceFilter) repo.Deployment { + // Reuse existing client instance + d := a.Deployment + d.f = d.f.Merge(f) return &DeploymentAdapter{ - Deployment: &Deployment{ - client: a.client, - f: a.f.Merge(f), - }, + Deployment: d, } }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
219-222: Simplify sort direction handling using a map.The current if-else logic for determining sort direction could be simplified.
+ // Define sort direction map at package level + var sortDirectionMap = map[string]int{ + "ASC": 1, + "DESC": -1, + } + - direction := 1 // default ascending - if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 - } + direction := sortDirectionMap["ASC"] // default ascending + if pagination.Page.OrderDir != nil { + if dir, ok := sortDirectionMap[*pagination.Page.OrderDir]; ok { + direction = dir + } + }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
179-185: Improve error messages and handling consistency.The error handling in the Save method could be more descriptive and consistent with other methods.
if !r.f.CanWrite(deployment.Workspace()) { - return interfaces.ErrOperationDenied + return rerror.NewE(interfaces.ErrOperationDenied, "no write permission for workspace") } doc, err := mongodoc.NewDeployment(deployment) if err != nil { - return rerror.ErrInternalByWithContext(ctx, err) + return rerror.NewE(rerror.ErrInternalBy, "failed to create deployment document").WithCause(err) }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/internal/infrastructure/memory/deployment.go(3 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/container.go(1 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/infrastructure/mongo/container.go
🧰 Additional context used
🪛 GitHub Actions: ci
api/internal/infrastructure/memory/deployment.go
[error] Package test failure with coverage: 15.9% of statements
api/internal/infrastructure/memory/deployment_test.go
[error] 253-254: Test failure in TestDeployment_FindByWorkspace: Multiple test cases failed including pagination tests. Mismatch in expected and actual deployment data and page info.
api/internal/infrastructure/mongo/deployment.go
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
🔇 Additional comments (3)
api/internal/infrastructure/mongo/deployment.go (3)
213-214: Add page size validation.The page size is used without validation, which could lead to performance issues with large page sizes.
+ const maxPageSize = 100 // Define at package level + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSize + if pagination.Page.PageSize > maxPageSize { + pagination.Page.PageSize = maxPageSize + } limit := pagination.Page.PageSize🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
Line range hint
1-277: Address skipped tests in CI pipeline.Multiple tests are being skipped due to missing MongoDB URI configuration in the CI environment. This reduces test coverage and could hide potential issues.
Consider these options:
- Use a MongoDB container in CI
- Mock MongoDB for tests
- Add proper error messages when MongoDB URI is missing
🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
132-145: Add input validation for Create method.The Create method should validate the input parameters before creating a new deployment.
Consider adding these validations:
func (r *Deployment) Create(ctx context.Context, param interfaces.CreateDeploymentParam) (*deployment.Deployment, error) { + if err := r.validateCreateParams(param); err != nil { + return nil, err + } d := deployment.New(). NewID(). Workspace(param.Workspace). Project(param.Project). Description(param.Description). WorkflowURL(param.Workflow.Path). MustBuild()🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple skipped tests due to missing MongoDB URI configuration. Tests affected: TestFindByID, TestAsset_TotalSizeByWorkspace, TestDeployment_FindByIDs, and others
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
api/internal/infrastructure/memory/deployment.go (2)
58-62:⚠️ Potential issueAdd validation for order direction.
The order direction should be validated to ensure it's either "ASC" or "DESC".
-if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 -} +if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC", "DESC": + if *pagination.Page.OrderDir == "DESC" { + direction = -1 + } + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "orderDir must be either ASC or DESC") + } +}
94-123:⚠️ Potential issueAdd input validation for pagination parameters.
The page and pageSize parameters should be validated to prevent negative values and potential integer overflow in skip calculation.
if pagination.Page != nil { + if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page and pageSize must be positive") + } + + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSize
🧹 Nitpick comments (4)
api/internal/infrastructure/memory/deployment.go (1)
64-87: Consider extracting sort comparators into separate functions.The sorting logic could be more maintainable by extracting the comparison logic into separate functions.
+func compareVersions(d1, d2 *deployment.Deployment, direction int) bool { + if v1, v2 := d1.Version(), d2.Version(); v1 != v2 { + return direction == 1 && v1 < v2 || direction == -1 && v1 > v2 + } + return d1.ID().String() < d2.ID().String() +} + +func compareUpdatedAt(d1, d2 *deployment.Deployment, direction int) bool { + if t1, t2 := d1.UpdatedAt(), d2.UpdatedAt(); !t1.Equal(t2) { + return direction == 1 && t1.Before(t2) || direction == -1 && t1.After(t2) + } + return d1.ID().String() < d2.ID().String() +} + sort.Slice(result, func(i, j int) bool { if pagination != nil && pagination.Page != nil && pagination.Page.OrderBy != nil && *pagination.Page.OrderBy == "version" { - // Compare versions first - vi, vj := result[i].Version(), result[j].Version() - if vi != vj { - if direction == 1 { - return vi < vj - } - return vi > vj - } - // If versions are equal, use ID as secondary sort key for stability - return result[i].ID().String() < result[j].ID().String() + return compareVersions(result[i], result[j], direction) } - // Compare updatedAt first - ti, tj := result[i].UpdatedAt(), result[j].UpdatedAt() - if !ti.Equal(tj) { - if direction == 1 { - return ti.Before(tj) - } - return ti.After(tj) - } - // If timestamps are equal, use ID as secondary sort key for stability - return result[i].ID().String() < result[j].ID().String() + return compareUpdatedAt(result[i], result[j], direction) })api/internal/adapter/gql/loader_project.go (1)
42-50: Consider adding pagination parameter validation.While the pagination parameter conversion is implemented, consider adding validation for pagination parameters to handle edge cases (e.g., negative page numbers or sizes).
func (c *ProjectLoader) FindByWorkspace(ctx context.Context, wsID gqlmodel.ID, pagination *gqlmodel.Pagination) (*gqlmodel.ProjectConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } + + if err := validatePagination(pagination); err != nil { + return nil, err + } res, pi, err := c.usecase.FindByWorkspace(ctx, tid, gqlmodel.ToPagination(pagination), getOperator(ctx)) if err != nil { return nil, err }api/gql/workspace.graphql (1)
2-2: LGTM: GraphQL schema updated consistently.The schema changes for both assets and projects fields are consistent with the new pagination approach. The multi-line formatting for the projects field improves readability.
Consider updating documentation.
Consider adding GraphQL descriptions for the pagination parameter to help API consumers understand the pagination options.
type Workspace implements Node { - assets(pagination: Pagination): AssetConnection! + """ + Retrieve assets with pagination support. + The pagination parameter supports both cursor-based and page-based pagination. + """ + assets(pagination: Pagination): AssetConnection! id: ID! members: [WorkspaceMember!]! name: String! personal: Boolean! projects( includeArchived: Boolean + """ + Pagination options supporting both cursor-based and page-based pagination. + For page-based pagination, use page and perPage fields. + For cursor-based pagination, use first/last and after/before fields. + """ pagination: Pagination ): ProjectConnection! }Also applies to: 7-10
api/e2e/gql_pagination_test.go (1)
14-192: Add comprehensive test coverage for pagination and sorting.While the basic happy path is well tested, consider adding the following test cases:
- Pagination:
- Navigation using
aftercursor- Invalid pagination parameters
- Sorting:
- Descending order
- Invalid
orderByandorderDirvaluesHere's a suggested test case for cursor-based navigation:
t.Run("test_pagination", func(t *testing.T) { + // Test navigation using after cursor + t.Run("with_after_cursor", func(t *testing.T) { + query := fmt.Sprintf(`{ + projects( + workspaceId: "%s" + pagination: { + first: 2 + after: "%s" + } + ) { + edges { + node { + id + name + } + } + pageInfo { + hasNextPage + endCursor + totalCount + } + } + }`, wId1.String(), result.Data.Projects.PageInfo.EndCursor) + + // ... rest of the test implementation + }) + + // Test invalid pagination + t.Run("with_invalid_first", func(t *testing.T) { + query := fmt.Sprintf(`{ + projects( + workspaceId: "%s" + pagination: { + first: -1 + } + ) { + edges { + node { + id + } + } + } + }`, wId1.String()) + + // ... verify error response + })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/e2e/gql_pagination_test.go(1 hunks)api/gql/project.graphql(1 hunks)api/gql/workspace.graphql(1 hunks)api/internal/adapter/gql/generated.go(36 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/resolver_query.go(2 hunks)api/internal/adapter/gql/resolver_workspace.go(1 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/adapter/gql/resolver_query.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: ci-api / ci-api-lint
- GitHub Check: ci-api / ci-api-test
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
🔇 Additional comments (15)
api/internal/infrastructure/memory/deployment.go (4)
8-8: LGTM!The new import is correctly added to support the updated pagination parameter type.
37-37: LGTM!The method signature is correctly updated to support both page-based and cursor-based pagination.
45-46: LGTM!Good practice to pre-allocate the slice with estimated capacity for better performance.
125-169: LGTM!The cursor-based pagination implementation is well-structured and handles all edge cases appropriately:
- Proper cursor navigation
- Correct calculation of next/previous page flags
- Proper handling of start/end cursors
api/internal/adapter/gql/resolver_workspace.go (2)
19-20: LGTM: Assets method signature updated for consolidated pagination.The method has been correctly updated to use the new consolidated pagination parameter, improving code maintainability.
23-24: Verify error handling for pagination parameter.While the method signature update looks good, we should ensure proper error handling for the pagination parameter.
✅ Verification successful
Pagination error handling is properly implemented ✅
The pagination parameter is properly handled with error checking at multiple levels, including ID conversion, usecase execution, and nil value handling. The implementation is consistent with other resolvers in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns in pagination-related code rg -A 5 'ToPagination|pagination.*error' api/internal/adapter/gql/Length of output: 13072
api/gql/project.graphql (1)
82-82: Verify Pagination type definition.The schema correctly uses the new Pagination type. However, we should verify the definition of the Pagination type to ensure it supports both cursor-based and page-based pagination.
✅ Verification successful
Pagination type definition is properly implemented ✅
The Pagination input type in
api/gql/_shared.graphqlcorrectly supports both cursor-based and page-based pagination with all necessary fields, including sorting capabilities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Pagination type definition rg -l 'type Pagination|input Pagination' api/gql/Length of output: 75
Script:
#!/bin/bash # Get the Pagination type definition with context rg -A 10 'type Pagination|input Pagination' api/gql/_shared.graphqlLength of output: 273
api/e2e/gql_pagination_test.go (2)
1-12: LGTM! Imports are well-organized.All necessary imports are present and properly used throughout the file.
1-440: Add tests for deployment pagination.The PR objectives mention implementing page-based pagination for deployments, but there are no tests covering this functionality. Consider adding a
TestDeploymentsPaginationfunction that follows the same pattern as the existing pagination tests.Let's verify if deployment-related code exists:
api/internal/adapter/gql/generated.go (6)
212-218: LGTM! PageInfo structure enhanced with pagination metadata.The PageInfo type has been correctly extended with essential pagination fields (currentPage, totalPages, totalCount) while maintaining backward compatibility.
2146-2156: LGTM! Well-structured pagination input types.The PageBasedPagination input type and OrderDirection enum are well-defined with clear, required fields.
8593-8717: LGTM! Comprehensive PageInfo field resolvers.The implementation includes all necessary field resolvers for the new PageInfo fields with proper error handling and null checks.
10439-10502: LGTM! Well-structured Query resolver for page-based deployments.The deploymentsPage query implementation includes proper error handling, null checks, and field context management.
2141-2168: Verify the pagination implementation supports both strategies.The schema now supports both cursor-based and page-based pagination, but mixing both strategies in a single Pagination input type could lead to confusion.
Consider separating the pagination strategies into distinct types to prevent misuse:
input Pagination { # Cursor-based pagination first: Int last: Int after: Cursor before: Cursor - # Page-based pagination - page: Int - pageSize: Int - orderBy: String - orderDir: OrderDirection }✅ Verification successful
Separate pagination strategies to maintain consistency and prevent confusion
The codebase already has a dedicated
PageBasedPaginationtype, and mixing both strategies in a singlePaginationtype could lead to confusion and potential runtime errors. The suggested change to separate the strategies is correct.
- The
Jobsquery should be updated to use either the existingPageBasedPaginationtype or a dedicated cursor-based pagination type.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for usages of both pagination types and their implementations echo "=== Searching for PageBasedPagination usage ===" rg "PageBasedPagination" -A 5 echo -e "\n=== Searching for Pagination input type usage ===" rg "input.*Pagination.*{" -A 10 echo -e "\n=== Looking for resolver implementations ===" ast-grep --pattern 'func $_($_ *generated.QueryResolver) $_($_ *generated.Pagination) $_' echo -e "\n=== Checking for pagination documentation ===" fd "README" --exec cat {}Length of output: 27570
272-278: Verify the impact of the new query on existing implementations.The new
deploymentsPagequery has been added alongside the existingdeploymentsquery. Ensure that:
- Both queries maintain consistent sorting behavior
- The response format remains consistent between both endpoints
Also applies to: 2337-2337
✅ Verification successful
Both queries maintain consistency in implementation ✅
The new
deploymentsPagequery:
- Returns the same
DeploymentConnectiontype ensuring response format consistency- Uses the same loader interface maintaining consistent sorting behavior
- Simply adds page-based pagination support alongside the existing cursor-based pagination
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistency in deployment query implementations ast-grep --pattern 'func (r *queryResolver) Deployments($$$) { $$$ }' ast-grep --pattern 'func (r *queryResolver) DeploymentsPage($$$) { $$$ }'Length of output: 151
Script:
#!/bin/bash # Find implementation files for both queries rg -l "func.*Deployments.*workspaceID" --type go # Find GraphQL schema definitions rg "type.*Deployment" --type graphql -A 5 rg "type.*Query" --type graphql -A 10Length of output: 241
Script:
#!/bin/bash # Check the resolver implementation cat api/internal/adapter/gql/resolver_query.go # Look for schema files with different extensions fd "schema" --extension gql --extension graphqls fd "schema.*\.go$" # Backup: search for type definitions in .go files rg "type.*Deployment.*struct" --type go -A 5Length of output: 15056
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/e2e/gql_project_test.go (1)
Line range hint
237-320: Improve overall test coverage for pagination.While the current changes provide a foundation for testing pagination, consider splitting
TestListProjectsinto multiple test cases to thoroughly verify both cursor-based and page-based pagination:
TestListProjectsCursorPagination
- Test forward/backward navigation using cursors
- Verify cursor validity and consistency
TestListProjectsPagePagination
- Test different page numbers and sizes
- Verify total count accuracy
- Test edge cases (empty pages, last page)
This separation will make the tests more maintainable and ensure comprehensive coverage of both pagination methods.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/e2e/gql_project_test.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-api / ci-api-lint
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (3)
api/e2e/gql_project_test.go (3)
285-293: LGTM! Comprehensive field coverage in the response structs.The response structs are well-defined with all necessary fields for both project data and pagination information.
Also applies to: 298-302
237-239: 🛠️ Refactor suggestionAdd test cases for page-based pagination parameters.
The test only covers cursor-based pagination with
first. Since this PR implements page-based pagination for deployments, consider adding test cases that verify the page-based pagination parameters (page,perPage) work correctly.pagination: { - first: 10 + page: 1, + perPage: 10 }Also, consider adding additional test cases to verify:
- Navigation between pages
- Different page sizes
- Edge cases (empty pages, invalid page numbers)
✅ Verification successful
Add test cases for page-based pagination parameters.
The test only covers cursor-based pagination with
first. Since this PR implements page-based pagination for deployments, consider adding test cases that verify the page-based pagination parameters (page,perPage) work correctly.The codebase already has examples of page-based pagination tests in
api/internal/infrastructure/mongo/deployment_test.gothat you can reference.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for GraphQL schema definitions related to pagination ast-grep --pattern 'type $_ { $$$ pagination $$$ }' # Search for pagination-related code rg -g '*.go' -g '*.graphql' -g '*.gql' 'pagination' -B2 -A2 # Look for existing pagination tests rg -g '*_test.go' 'pagination.*page|perPage' -B2 -A2Length of output: 65748
262-262: 🛠️ Refactor suggestionVerify the accuracy of totalCount.
The test should verify that
totalCountreflects the actual total number of projects in the workspace, independent of pagination.Add an assertion like:
assert.NotNil(t, projects.PageInfo) +// Create a known number of projects first +assert.Equal(t, expectedTotalCount, projects.PageInfo.TotalCount, "Total count should match the number of projects in workspace")✅ Verification successful
The review comment is correct and the suggested test is essential.
The codebase shows that
totalCountis implemented to return the total number of projects in the workspace, independent of pagination. The test should verify this by:
- Creating a known number of projects
- Verifying that
totalCountmatches this number, even when using pagination🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for GraphQL schema definitions echo "=== Searching for GraphQL schema files ===" fd -e graphql -e gql echo -e "\n=== Checking schema for totalCount field ===" rg "totalCount" --type graphql -C 2 echo -e "\n=== Looking for project resolver implementation ===" rg "type ProjectConnection" -A 5 rg "func.*Projects.*Resolver" -g "*.go" -C 5 echo -e "\n=== Checking for existing totalCount tests ===" rg "totalCount.*assert" -g "*_test.go" -C 3Length of output: 3948
Script:
#!/bin/bash # Search for project resolver implementation echo "=== Searching for project resolver implementation ===" rg -t go "func.*[Pp]roject.*resolver" -C 10 rg -t go "type.*[Pp]roject.*[Rr]esolver" -C 10 echo -e "\n=== Looking for totalCount calculation ===" rg -t go "TotalCount.*=.*" -C 5Length of output: 8514
Script:
#!/bin/bash # Search for projects query and connection implementations echo "=== Searching for projects query implementation ===" rg -t go "func.*[Qq]uery.*[Pp]rojects" -C 10 echo -e "\n=== Looking for connection building code ===" rg -t go "func.*[Cc]onnection.*" -C 10 rg -t go "func.*[Pp]aginate.*" -C 10Length of output: 89735
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
api/e2e/gql_pagination_test.go (3)
15-193: Add test cases for better coverage.The test function has good basic coverage but could be enhanced with additional test cases:
- Test sorting in descending order
- Test the last page scenario
- Test edge cases with empty results
- Test with invalid pagination parameters
Here's a suggested test case for descending order:
+ // Test sorting in descending order + t.Run("test_sorting_descending", func(t *testing.T) { + query := fmt.Sprintf(`{ + projects( + workspaceId: "%s" + pagination: { + first: 5 + orderBy: "name" + orderDir: DESC + } + ) { + edges { + node { + id + name + } + } + } + }`, wId1.String()) + + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + Projects struct { + Edges []struct { + Node struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"node"` + } `json:"edges"` + } `json:"projects"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + + // Verify sorting + for i := 1; i < len(result.Data.Projects.Edges); i++ { + prev := result.Data.Projects.Edges[i-1].Node.Name + curr := result.Data.Projects.Edges[i].Node.Name + assert.True(t, prev >= curr, "Projects should be sorted by name in descending order") + } + })
195-655: Optimize test execution time and reduce code duplication.The test has good coverage but could be improved in two areas:
- Long sleep durations (1-2 seconds) could slow down the test suite
- Duplicate retry logic across different test cases
Consider extracting the retry logic into a helper function:
+func waitForCondition(t *testing.T, maxRetries int, interval time.Duration, condition func() bool) bool { + for i := 0; i < maxRetries; i++ { + if condition() { + return true + } + time.Sleep(interval) + } + return false +}Then use it to simplify the retry loops:
- for i := 0; i < maxRetries; i++ { - if result.Data.Jobs.PageInfo.TotalCount == 5 { - break - } - time.Sleep(1 * time.Second) - } + success := waitForCondition(t, maxRetries, 200*time.Millisecond, func() bool { + return result.Data.Jobs.PageInfo.TotalCount == 5 + }) + assert.True(t, success, "Failed to get expected number of jobs")Also, consider reducing sleep durations and increasing retry frequency:
- time.Sleep(1 * time.Second) + time.Sleep(200 * time.Millisecond)
781-829: Extract common GraphQL request logic.The GraphQL request setup is duplicated across multiple functions. Consider extracting it into a helper function.
+func executeGraphQLRequest[T any](t *testing.T, e *httpexpect.Expect, query string, variables map[string]any, userId string) T { + request := GraphQLRequest{ + Query: query, + Variables: variables, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result T + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + return result +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/e2e/gql_pagination_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-api / ci-api-lint
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (3)
api/e2e/gql_pagination_test.go (3)
1-14: LGTM! Imports look good.The imports are appropriate for an e2e test file, including necessary testing packages and HTTP expectations library.
831-879: Refactor to use the common GraphQL request helper.This function has the same GraphQL request boilerplate code as createTestProjectForJobs.
Use the suggested
executeGraphQLRequesthelper function from the previous comment to reduce code duplication.
657-779: 🛠️ Refactor suggestionAdd missing test cases and verifications for triggers pagination.
The test function is missing several important test cases and verifications:
- No test data creation
- No verification of pagination results (totalCount, hasNextPage)
- Missing next page and last page tests
- Missing ascending order sort test
Add assertions for pagination results:
err = json.Unmarshal([]byte(resp.Body().Raw()), &result) assert.NoError(t, err) + + // Verify pagination results + assert.NotZero(t, result.Data.Triggers.PageInfo.TotalCount, "Total count should not be zero") + assert.NotEmpty(t, result.Data.Triggers.Edges, "Should return some triggers") + if result.Data.Triggers.PageInfo.TotalCount > 2 { + assert.True(t, result.Data.Triggers.PageInfo.HasNextPage, "Should have next page") + assert.NotEmpty(t, result.Data.Triggers.PageInfo.EndCursor, "End cursor should not be empty") + }Consider adding test data creation similar to the jobs test:
+ // Create test triggers + for i := 0; i < 5; i++ { + // Add trigger creation logic here + time.Sleep(100 * time.Millisecond) + }Likely invalid or redundant comment.
kasugamirai
force-pushed
the
feat/pagination
branch
from
101bf10 to
e792057
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/internal/infrastructure/mongo/deployment.go (1)
149-168:⚠️ Potential issueAddress potential race condition in Update method.
The current implementation is susceptible to lost updates in concurrent scenarios.
Consider implementing optimistic locking using MongoDB's atomic operations:
func (r *Deployment) Update(ctx context.Context, param interfaces.UpdateDeploymentParam) (*deployment.Deployment, error) { - d, err := r.FindByID(ctx, param.ID) - if err != nil { - return nil, err - } + filter := bson.M{"id": param.ID.String()} + update := bson.M{"$set": bson.M{}} if param.Description != nil { - d.SetDescription(*param.Description) + update["$set"].(bson.M)["description"] = *param.Description } if param.Workflow != nil { - d.SetWorkflowURL(param.Workflow.Path) + update["$set"].(bson.M)["workflow.path"] = param.Workflow.Path } - if err := r.Save(ctx, d); err != nil { + opts := options.FindOneAndUpdate().SetReturnDocument(options.After) + var doc mongodoc.Deployment + if err := r.client.FindOneAndUpdate(ctx, filter, update, &doc, opts); err != nil { return nil, err } - return d, nil + return doc.Model(), nil }
🧹 Nitpick comments (10)
api/internal/adapter/gql/loader_project.go (4)
53-62: Consider extracting edge/node construction logic to a helper function.This logic is duplicated in
FindByWorkspacePage. Consider extracting it to a helper function to improve maintainability and reduce code duplication.+func (c *ProjectLoader) createEdgesAndNodes(res []domain.Project) ([]*gqlmodel.ProjectEdge, []*gqlmodel.Project) { + edges := make([]*gqlmodel.ProjectEdge, 0, len(res)) + nodes := make([]*gqlmodel.Project, 0, len(res)) + for _, p := range res { + prj := gqlmodel.ToProject(p) + edges = append(edges, &gqlmodel.ProjectEdge{ + Node: prj, + Cursor: usecasex.Cursor(prj.ID), + }) + nodes = append(nodes, prj) + } + return edges, nodes +}
72-72: Consider renaming the method to better reflect its pagination strategy.The method name could be more descriptive about its pagination strategy. Consider renaming it to
FindByWorkspaceWithPageBasedPaginationto make it clearer that this method uses page-based pagination.
78-80: Add specific error handling for pagination parameters.The method should provide more specific error handling for invalid pagination parameters (e.g., negative page numbers, invalid page sizes).
paginationParam := gqlmodel.ToPageBasedPagination(pagination) + +if err := validatePageBasedPagination(paginationParam); err != nil { + return nil, fmt.Errorf("invalid pagination parameters: %w", err) +} res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam.Cursor, getOperator(ctx))Add this helper function:
func validatePageBasedPagination(p *usecasex.PageBasedPagination) error { if p.Page < 1 { return fmt.Errorf("page number must be positive") } if p.PerPage < 1 { return fmt.Errorf("items per page must be positive") } return nil }
Line range hint
42-98: Add documentation about pagination strategies.Consider adding documentation that explains:
- The differences between cursor-based and page-based pagination
- When to use each pagination strategy
- Examples of typical usage scenarios
Example documentation:
// FindByWorkspace retrieves projects using cursor-based pagination, which is ideal for: // - Handling large datasets efficiently // - Implementing infinite scroll UIs // - Ensuring consistency when items are added/removed // // Example usage: // pagination := &gqlmodel.Pagination{First: ptr.Ref(10)} // result, err := loader.FindByWorkspace(ctx, wsID, pagination) // FindByWorkspacePage retrieves projects using page-based pagination, which is ideal for: // - Displaying traditional numbered page navigation // - Cases where total count and page numbers are important // - Simpler client-side implementation // // Example usage: // pagination := gqlmodel.PageBasedPagination{Page: 1, PerPage: 10} // result, err := loader.FindByWorkspacePage(ctx, wsID, pagination)api/internal/adapter/gql/loader_trigger.go (1)
73-104: Consider refactoring to reduce code duplication.The
FindByWorkspacePagemethod shares significant code withFindByWorkspace. Consider extracting the common logic into a helper function to improve maintainability.+func (c *TriggerLoader) buildTriggerConnection(res []domain.Trigger, pi *usecasex.PageInfo) *gqlmodel.TriggerConnection { + edges := make([]*gqlmodel.TriggerEdge, 0, len(res)) + nodes := make([]*gqlmodel.Trigger, 0, len(res)) + + for _, t := range res { + trig := gqlmodel.ToTrigger(t) + edges = append(edges, &gqlmodel.TriggerEdge{ + Node: trig, + Cursor: usecasex.Cursor(trig.ID), + }) + nodes = append(nodes, trig) + } + + return &gqlmodel.TriggerConnection{ + Edges: edges, + Nodes: nodes, + PageInfo: gqlmodel.ToPageInfo(pi), + TotalCount: int(pi.TotalCount), + } +} func (c *TriggerLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.TriggerConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } paginationParam := gqlmodel.ToPageBasedPagination(pagination) res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam.Cursor, getOperator(ctx)) if err != nil { return nil, err } - edges := make([]*gqlmodel.TriggerEdge, 0, len(res)) - nodes := make([]*gqlmodel.Trigger, 0, len(res)) - - for _, t := range res { - trig := gqlmodel.ToTrigger(t) - edges = append(edges, &gqlmodel.TriggerEdge{ - Node: trig, - Cursor: usecasex.Cursor(trig.ID), - }) - nodes = append(nodes, trig) - } - - return &gqlmodel.TriggerConnection{ - Edges: edges, - Nodes: nodes, - PageInfo: gqlmodel.ToPageInfo(pi), - TotalCount: int(pi.TotalCount), - }, nil + return c.buildTriggerConnection(res, pi), nil }api/internal/adapter/gql/loader_job.go (1)
89-119: Consider refactoring to reduce code duplication.The
FindByWorkspacePagemethod shares significant code withFindByWorkspace. Consider extracting the common logic into a helper function to improve maintainability.+func (c *JobLoader) buildJobConnection(res []domain.Job, pi *usecasex.PageInfo) *gqlmodel.JobConnection { + edges := make([]*gqlmodel.JobEdge, 0, len(res)) + nodes := make([]*gqlmodel.Job, 0, len(res)) + for _, j := range res { + job := gqlmodel.ToJob(j) + edges = append(edges, &gqlmodel.JobEdge{ + Node: job, + Cursor: usecasex.Cursor(job.ID), + }) + nodes = append(nodes, job) + } + + return &gqlmodel.JobConnection{ + Edges: edges, + Nodes: nodes, + PageInfo: gqlmodel.ToPageInfo(pi), + TotalCount: int(pi.TotalCount), + } +} func (c *JobLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.JobConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } paginationParam := gqlmodel.ToPageBasedPagination(pagination) res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam.Cursor, getOperator(ctx)) if err != nil { return nil, err } - edges := make([]*gqlmodel.JobEdge, 0, len(res)) - nodes := make([]*gqlmodel.Job, 0, len(res)) - for _, j := range res { - job := gqlmodel.ToJob(j) - edges = append(edges, &gqlmodel.JobEdge{ - Node: job, - Cursor: usecasex.Cursor(job.ID), - }) - nodes = append(nodes, job) - } - - return &gqlmodel.JobConnection{ - Edges: edges, - Nodes: nodes, - PageInfo: gqlmodel.ToPageInfo(pi), - TotalCount: int(pi.TotalCount), - }, nil + return c.buildJobConnection(res, pi), nil }api/internal/adapter/gql/gqlmodel/convert.go (1)
63-69: Consider adding validation for OrderDirection values.While the conversion is correct, consider validating the input values to ensure only valid order directions are processed.
func OrderDirectionToString(dir *OrderDirection) *string { if dir == nil { return nil } + // Validate direction + switch *dir { + case OrderDirectionASC, OrderDirectionDESC: + s := string(*dir) + return &s + default: + return nil + } - s := string(*dir) - return &s }api/internal/infrastructure/mongo/deployment.go (2)
209-283: Well-structured pagination implementation with room for optimization.The pagination implementation handles both page-based and cursor-based scenarios well, but there's room for optimization.
Consider these improvements:
- Cache the total count for a short duration to reduce database load
- Use MongoDB's aggregation pipeline for better performance
- Add index hints for the sorting fields
func (r *Deployment) paginate(ctx context.Context, filter bson.M, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *usecasex.PageInfo, error) { c := mongodoc.NewDeploymentConsumer(r.f.Readable) if pagination != nil && pagination.Page != nil { + // Add index hint for better query performance + opts := options.Find(). + SetSort(sort). + SetSkip(int64(skip)). + SetLimit(int64(limit)). + SetHint(bson.D{{Key: "workspace_1_updatedat_-1", Value: 1}}) // Get total count for page info + // TODO: Consider implementing caching for total count total, err := r.client.Count(ctx, filter)
170-172: Consider implementing soft delete.The current implementation performs a hard delete. Consider implementing soft delete to maintain historical data and prevent accidental data loss.
func (r *Deployment) Delete(ctx context.Context, id id.DeploymentID) error { - return r.Remove(ctx, id) + update := bson.M{ + "$set": bson.M{ + "deletedAt": time.Now().UTC(), + }, + } + return r.client.UpdateOne(ctx, bson.M{"id": id.String()}, update) }api/internal/adapter/gql/generated.go (1)
2183-2186: Consider making currentPage and totalPages non-nullable in PageInfo.The
totalCountfield is non-nullable butcurrentPageandtotalPagesare nullable. For page-based pagination, these fields should also be non-nullable as they are essential pagination metadata.totalCount: Int! - currentPage: Int - totalPages: Int + currentPage: Int! + totalPages: Int!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/gql/job.graphql(1 hunks)api/gql/project.graphql(1 hunks)api/gql/trigger.graphql(1 hunks)api/internal/adapter/gql/generated.go(74 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/adapter/gql/loader_job.go(1 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/adapter/gql/resolver_query.go(4 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/gql/project.graphql
- api/internal/adapter/gql/resolver_query.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - reearth-flow
- GitHub Check: Header rules - reearth-flow
- GitHub Check: Pages changed - reearth-flow
- GitHub Check: ci-api / ci-api-lint
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (11)
api/internal/adapter/gql/loader_project.go (1)
42-42: Add nil check for pagination parameter.The method should handle the case when the pagination parameter is nil by providing default values.
api/gql/job.graphql (1)
43-43: LGTM!The new
jobsPagequery follows GraphQL best practices with consistent naming and proper type definitions.api/gql/trigger.graphql (1)
73-73: LGTM!The new
triggersPagequery follows GraphQL best practices with consistent naming and proper type definitions.api/internal/adapter/gql/gqlmodel/convert.go (3)
52-61: LGTM! Clean implementation of pagination parameter conversion.The function properly converts GraphQL pagination model to domain pagination parameters.
78-89: LGTM! Symmetrical implementation with ToPageInfo.The function correctly implements the reverse conversion of PageInfo, maintaining symmetry with ToPageInfo.
34-34: Consider potential data loss in TotalCount conversion.Converting
int64tointcould lead to data loss if the total count exceeds the maximum value ofinton 32-bit systems.api/internal/infrastructure/mongo/deployment.go (1)
30-39: Good use of adapter pattern for repository implementation.The adapter pattern implementation provides a clean separation of concerns and maintains backward compatibility.
api/internal/adapter/gql/generated.go (4)
2188-2193: Add input validation for PageBasedPagination.The input type needs validation to ensure:
- Positive page numbers
- Reasonable page sizes to prevent performance issues
426-437: LGTM! The page-based query methods are well-structured.The new query methods follow a consistent pattern and properly handle pagination parameters.
Line range hint
10569-11299: LGTM! The generated resolvers and field contexts are complete.The generated code correctly implements all the required methods for the new page-based pagination functionality.
15965-16011:⚠️ Potential issueAdd validation in unmarshalInputPageBasedPagination.
The function should validate the page and pageSize values to ensure they are within reasonable bounds.
Apply this diff to add validation:
func (ec *executionContext) unmarshalInputPageBasedPagination(ctx context.Context, obj interface{}) (gqlmodel.PageBasedPagination, error) { var it gqlmodel.PageBasedPagination asMap := map[string]interface{}{} for k, v := range obj.(map[string]interface{}) { asMap[k] = v } fieldsInOrder := [...]string{"page", "pageSize", "orderBy", "orderDir"} for _, k := range fieldsInOrder { v, ok := asMap[k] if !ok { continue } switch k { case "page": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("page")) data, err := ec.unmarshalNInt2int(ctx, v) if err != nil { return it, err } + if data <= 0 { + return it, fmt.Errorf("page must be positive") + } it.Page = data case "pageSize": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("pageSize")) data, err := ec.unmarshalNInt2int(ctx, v) if err != nil { return it, err } + if data <= 0 { + return it, fmt.Errorf("pageSize must be positive") + } + if data > 100 { + return it, fmt.Errorf("pageSize must not exceed 100") + } it.PageSize = dataLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (10)
api/internal/infrastructure/memory/trigger.go (4)
49-49: Consider performance implications for large data sets.
Calculatingtotalby len(result) is reasonable for small, in-memory data. However, for larger data sets, repeatedly collecting data intoresultbefore counting can be inefficient.
56-66: Handle absent or invalid 'after' cursor more explicitly.
Currently, if the provided cursor ID is not found, the loop doesn’t setstartand remains zero. Consider returning an error or clarifying this behavior in documentation to avoid surprising pagination results.
67-74: Check sorting or ordering strategy.
For stable pagination, it’s typical to sort the dataset before slicing. Without a defined ordering, the index-based slice might create inconsistent pagination across calls.
91-98: ReturnHasPreviousPagecarefully.
HasPreviousPageis derived fromstart > 0. Ensure all edge cases at the boundary (like start = total - 1) consistently reflect whether more pages truly exist.api/internal/infrastructure/memory/project.go (3)
50-52: Consider moving the empty check before result allocation.The empty check could be performed earlier, right after filtering workspace projects, to avoid unnecessary memory allocation for the result slice.
func (r *Project) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, p *usecasex.Pagination) ([]*project.Project, *usecasex.PageInfo, error) { r.lock.Lock() defer r.lock.Unlock() if !r.f.CanRead(id) { return nil, nil, nil } + total := 0 + for _, d := range r.data { + if d.Workspace() == id { + total++ + } + } + + if total == 0 { + return nil, &usecasex.PageInfo{TotalCount: 0}, nil + } + result := make([]*project.Project, 0, total) for _, d := range r.data { if d.Workspace() == id { result = append(result, d) } } - - total := int64(len(result)) - if total == 0 { - return nil, &usecasex.PageInfo{TotalCount: 0}, nil - }
101-103: Add documentation for pagination behavior.Consider adding documentation to clarify the behavior when pagination is not requested. This helps API consumers understand that they'll receive all results in this case.
+ // When pagination is not requested, return all results with total count return result, &usecasex.PageInfo{ TotalCount: total, }, nil
50-103: Consider implementing a reusable pagination package.The pagination logic could be extracted into a reusable package to maintain consistency across different repository implementations and reduce code duplication. This would be especially beneficial when implementing both cursor-based and page-based pagination.
Key components to consider:
- Common interfaces for both pagination types
- Utility functions for cursor encoding/decoding
- Sorting helpers for consistent pagination
- Error types for pagination-related errors
Would you like me to help create a design proposal for this reusable pagination package?
api/internal/infrastructure/memory/job.go (3)
50-52: Include all PageInfo fields for empty results.For consistency with the cursor-based pagination case, include
HasNextPageandHasPreviousPagefields in the empty result PageInfo.if total == 0 { - return nil, &usecasex.PageInfo{TotalCount: 0}, nil + return nil, &usecasex.PageInfo{ + TotalCount: 0, + HasNextPage: false, + HasPreviousPage: false, + }, nil }
59-65: Consider using binary search for cursor lookup.The current implementation uses a linear search to find the cursor position. Consider using binary search if the jobs are sorted by ID to improve performance.
101-103: Enhance default case PageInfo.When pagination is not requested, still provide complete PageInfo including cursors and page indicators. This helps clients transition to paginated queries.
return result, &usecasex.PageInfo{ TotalCount: total, + HasNextPage: false, + HasPreviousPage: false, + StartCursor: sc, + EndCursor: ec, }, nilWhere
scandecare calculated as:var sc, ec *usecasex.Cursor if total > 0 { first := usecasex.Cursor(result[0].ID().String()) last := usecasex.Cursor(result[total-1].ID().String()) sc = &first ec = &last }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
api/internal/infrastructure/memory/job.go(1 hunks)api/internal/infrastructure/memory/project.go(1 hunks)api/internal/infrastructure/memory/trigger.go(1 hunks)
🔇 Additional comments (6)
api/internal/infrastructure/memory/trigger.go (4)
50-51: Ensure consistent order for an empty dataset scenario.
Returning aPageInfowithTotalCount: 0is correct. Just verify that client code gracefully handles the empty dataset response.
54-55: Validate presence of page-based pagination logic.
Even though the PR mentions page-based pagination, the code only contains cursor-based logic here. Ensure the final solution covers page-based parameters so both modes function consistently.
75-82: Review boundary condition when 'start' >= 'total'.
Returningnilfor the slices is correct, but confirm that in client usage this doesn't cause confusion if the user tries to paginate past the last item.
83-90: Set cursors only when items exist.
This logic properly checks ifstart < end, which helps avoid out-of-range issues. Good job preventing potential panics.api/internal/infrastructure/memory/project.go (1)
55-99: Implement page-based pagination as per PR objectives.The PR objective is to add page-based pagination, but this implements cursor-based pagination. Consider implementing both pagination types or clarify if cursor-based pagination is the intended approach.
api/internal/infrastructure/memory/job.go (1)
55-56: Add input validation for pagination parameters.The code should validate pagination parameters to ensure they are valid (e.g., First/Last > 0) and that After/Before are not used together.
if p != nil && p.Cursor != nil { + if p.Cursor.After != nil && p.Cursor.Before != nil { + return nil, nil, rerror.NewE(rerror.ErrInvalidInput, "cannot use both after and before cursors") + } + if p.Cursor.First != nil && *p.Cursor.First < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidInput, "first must be positive") + } + if p.Cursor.Last != nil && *p.Cursor.Last < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidInput, "last must be positive") + }✅ Verification successful
The suggested pagination validation is necessary and should be implemented.
The codebase currently lacks validation for pagination parameters, and the suggested checks would prevent:
- Invalid page sizes (First/Last < 0)
- Ambiguous cursor navigation (simultaneous Before/After)
These validations align with the codebase's error handling patterns using
rerror.ErrInvalidInputand follow GraphQL pagination best practices.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for Cursor type definition and usage ast-grep --pattern 'type Cursor struct { $$$ }' # Search for pagination-related validation rg -A 5 'Cursor.*struct|pagination.*valid|cursor.*valid' --type go # Look for similar error handling patterns rg -A 2 'rerror\.ErrInvalidInput' --type goLength of output: 166
Script:
#!/bin/bash # Find relevant files fd -e go -x echo "=== {} ===" \; cat {} \; echo # Search for cursor-related code rg -i "cursor" --type go -C 5 # Search for pagination-related code rg -i "pagination|paginate" --type go -C 5Length of output: 102844
Script:
#!/bin/bash # Search for error handling patterns rg "rerror\..*Error|rerror\.New" --type go -C 3 # Search for validation functions rg "func.*valid|validate" --type go -C 3 # Search for pagination-related error handling rg "pagination.*error|cursor.*error" --type go -C 3Length of output: 28020
Script:
#!/bin/bash # Search for rerror package usage and error types rg "rerror\.(New|Err)" --type go -C 3 # Search for error handling in pagination-related code rg "return .*, .*, (rerror|err)" --type go -C 3 # Look for error validation patterns rg "ErrInvalidInput|invalid.*input" --type go -C 3Length of output: 86249
Script:
#!/bin/bash # Search for pagination-related types and validation rg "type.*(Pagination|Cursor)" --type go -C 5 # Look for pagination-related validation rg "func.*(Pagination|Cursor).*valid" --type go -C 5 # Search for pagination package imports and usage rg "usecasex.*Pagination" --type go -C 5Length of output: 33906
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
api/internal/infrastructure/mongo/deployment.go (1)
148-167:⚠️ Potential issueAddress potential race condition in Update method.
The Update method remains susceptible to lost updates in concurrent scenarios. Consider implementing optimistic locking using a separate revision field or MongoDB's atomic operations.
🧹 Nitpick comments (5)
api/internal/infrastructure/mongo/deployment.go (1)
211-249: Optimize pagination performance.Consider the following optimizations for page-based pagination:
- Add index hint for sorting to improve query performance
- Consider caching total count or implementing estimated count for large collections
opts := options.Find(). SetSort(sort). SetSkip(int64(skip)). - SetLimit(int64(limit)) + SetLimit(int64(limit)). + SetHint(bson.D{{Key: "updatedAt", Value: -1}}) // Add index hint for default sortingAlso, ensure there's an index on the
updatedAtfield:-var deploymentIndexes = []string{"workspaceid"} +var deploymentIndexes = []string{"workspaceid", "updatedAt"}api/internal/infrastructure/mongo/asset.go (2)
138-164: Document the pagination implementation.Add documentation to explain:
- The dual pagination strategy (cursor-based vs page-based)
- The page numbering scheme (1-based)
- The relationship between offset/limit and page numbers
Add this documentation above the if block:
// paginate supports two pagination strategies: // 1. Page-based pagination: When pagination.Offset is provided, uses skip/limit // with pages numbered from 1. offset = (page - 1) * limit // 2. Cursor-based pagination: When pagination.Offset is nil, uses the default // cursor-based implementation
151-153: Consider adding sort options for page-based pagination.The page-based implementation doesn't use the
usortparameter that was prepared earlier in the function.Add sorting support to maintain consistent ordering across pages:
opts := options.Find(). SetSkip(skip). - SetLimit(limit) + SetLimit(limit) +if usort != nil { + opts.SetSort(bson.D{{Key: usort.Key, Value: 1}}) +}api/internal/infrastructure/mongo/job.go (2)
132-137: Validate Performance on Large Collections
Counting documents for every page request can be expensive in high-volume scenarios. You might consider using pre-aggregated counts, partial filters, or an alternative approach if performance becomes a concern.
138-145: Potential Missing Sort for Stable Pagination
Skip/limit without an explicit sort can lead to inconsistent result ordering if the data changes or if the default order is not guaranteed. Consider specifying sorting for a more stable and predictable paging sequence.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/infrastructure/mongo/asset.go(3 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)api/internal/infrastructure/mongo/job.go(3 hunks)api/internal/infrastructure/mongo/project.go(2 hunks)api/internal/infrastructure/mongo/trigger.go(3 hunks)api/internal/usecase/interfaces/pageinfo.go(1 hunks)
🔇 Additional comments (13)
api/internal/infrastructure/mongo/deployment.go (4)
29-38: Well-structured adapter pattern implementation!The adapter pattern implementation provides a clean separation of concerns and maintains backward compatibility while enabling interface-based programming.
61-63: Good defensive programming!Early return for empty ID list is a good practice that prevents unnecessary database queries.
178-187: Well-implemented error handling!The error handling is consistent and properly contextual:
- Workspace access control check before save
- Proper error wrapping with context
- Clear error types for different scenarios
132-145: Implement the Execute method.The Execute method implementation is still pending. This functionality should be implemented in the job repository or properly documented if intentionally deferred.
api/internal/infrastructure/mongo/asset.go (1)
19-19: LGTM! Import added for MongoDB options.The addition of the MongoDB options import is necessary for implementing the page-based pagination features.
api/internal/usecase/interfaces/pageinfo.go (2)
5-10: LGTM! Well-structured page info type.The
PageBasedInfostruct is well-designed, embedding the basePageInfoand extending it with page-specific fields.
28-33: LGTM! Proper nil handling in conversion method.The
ToPageInfomethod correctly handles nil receiver and returns the embeddedPageInfo.api/internal/infrastructure/mongo/project.go (1)
156-182: Apply the same improvements as in trigger.go.This implementation has the same concerns as the
paginatemethod intrigger.go:
- Need for input validation
- Protection against division by zero
- Code duplication with
trigger.goPlease refer to the review comments in
trigger.gofor the suggested improvements.api/internal/adapter/gql/gqlmodel/convert.go (2)
75-81: LGTM! Clean and safe string conversion.The
OrderDirectionToStringfunction properly handles nil input and performs a clean conversion.
90-101: LGTM! Proper conversion of page info.The
FromPageInfofunction correctly handles nil input and converts all necessary fields.api/internal/infrastructure/mongo/job.go (3)
7-7: Import for Page-Based Pagination Integration
Importinginterfacesis appropriate here as it provides theNewPageBasedInfoconstructor and other essentials for page-based pagination.
16-16: Import for MongoDB Find Options
Theoptionsimport is needed forSetSkipandSetLimitusage. Looks good.
154-154: Fallback to Cursor-Based Pagination
The seamless fallback to cursor-based pagination when no offset is specified is a good approach to maintain backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
api/internal/infrastructure/memory/project.go (1)
51-76: 🛠️ Refactor suggestionAdd sorting for consistent pagination results.
The current implementation doesn't enforce sorting, which could lead to inconsistent pagination results across pages.
Additionally, consider caching the sorted results to avoid re-sorting for subsequent pagination requests.
api/internal/infrastructure/memory/trigger.go (1)
120-122: 🛠️ Refactor suggestionAdd page-based pagination in the return for non-cursor flows.
When
p == nil || p.Cursor == nil, the code returns the entire list. Consider implementing a fallback for page-based pagination or clarifying that the behavior is intentional.Consider applying this improvement:
- return result, &usecasex.PageInfo{ - TotalCount: total, - }, nil + // Default to first page with a reasonable page size when no pagination is specified + defaultLimit := int64(10) + pageInfo := interfaces.NewPageBasedInfo(total, 1, defaultLimit) + end := defaultLimit + if end > total { + end = total + } + return result[:end], pageInfo.ToPageInfo(), nilapi/internal/infrastructure/memory/deployment.go (2)
58-62:⚠️ Potential issueAdd validation for order direction.
The order direction should be validated to ensure it's either "ASC" or "DESC".
Apply this diff:
-if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 +if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC", "DESC": + if *pagination.Page.OrderDir == "DESC" { + direction = -1 + } + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "orderDir must be either ASC or DESC") + } }
94-113:⚠️ Potential issueAdd input validation for pagination parameters.
The page and pageSize parameters should be validated to prevent negative values and potential integer overflow in skip calculation.
Apply this diff:
if pagination.Page != nil { + if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page and pageSize must be positive") + } + + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } + + // Use int64 to prevent overflow + skip := int64(pagination.Page.Page-1) * int64(pagination.Page.PageSize) + if skip > int64(len(result)) { + return nil, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize).ToPageInfo(), nil + } + - skip := (pagination.Page.Page - 1) * pagination.Page.PageSize - if skip >= len(result) { - return nil, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize).ToPageInfo(), nil - }
🧹 Nitpick comments (4)
api/internal/infrastructure/memory/asset.go (1)
75-77: Handle empty result consistencyWhen
total == 0, you return an empty slice and aPageInfoobject withTotalCount: 0. Consider also setting explicitHasNextPageandHasPreviousPagefields tofalsefor clarity and consistency across all pagination cases.api/internal/infrastructure/memory/project.go (1)
124-126: Consider adding a size limit for unpaginated requests.When no pagination is specified, all results are returned, which could cause memory issues with large datasets. Consider adding a default size limit.
Apply this diff:
+const defaultLimit = 100 + func (r *Project) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, p *usecasex.Pagination) ([]*project.Project, *usecasex.PageInfo, error) { // ... existing code ... return result, &usecasex.PageInfo{ TotalCount: total, - }, nil + }, nil + + if len(result) > defaultLimit { + result = result[:defaultLimit] + }api/internal/infrastructure/memory/deployment.go (2)
45-46: Consider usinginterfaces.NewPageBasedInfofor empty results.For consistency with other return statements, consider using
interfaces.NewPageBasedInfofor empty results when pagination is provided:if total == 0 { - return nil, &usecasex.PageInfo{TotalCount: 0}, nil + if pagination != nil && pagination.Page != nil { + return nil, interfaces.NewPageBasedInfo(0, pagination.Page.Page, pagination.Page.PageSize).ToPageInfo(), nil + } + return nil, &usecasex.PageInfo{TotalCount: 0}, nil }Also applies to: 53-56
115-159: Add validation for cursor pagination parameters.Consider adding validation for the
firstparameter in cursor-based pagination to prevent negative values and limit the maximum page size.if pagination.Cursor != nil { + if pagination.Cursor.Cursor.First != nil { + if *pagination.Cursor.Cursor.First < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "first must be non-negative") + } + if *pagination.Cursor.Cursor.First > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "first cannot exceed 100") + } + } + // Cursor-based pagination
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/internal/infrastructure/memory/asset.go(2 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/job.go(2 hunks)api/internal/infrastructure/memory/project.go(2 hunks)api/internal/infrastructure/memory/trigger.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/infrastructure/memory/job.go
🔇 Additional comments (8)
api/internal/infrastructure/memory/asset.go (3)
8-8: Import for new page-based paginationThis import is required to use the
interfaces.NewPageBasedInfofunction. The addition looks correct and aligns with the updated page-based pagination logic.
80-125: Validate cursor usage edge casesThe cursor-based pagination logic appears correct overall. However, if
afterIDis not found, the iteration exits without updatingstart, defaulting it to0. This can result in an unintended reset of the page rather than an error or empty result. Consider responding with an empty result or an error when the cursor is invalid.
144-146: Fallback for no pagination scenarioReturning the entire result set with basic
PageInfois consistent when no cursor-based or offset-based pagination parameters are provided. This keeps the method flexible and backward compatible.api/internal/infrastructure/memory/project.go (2)
7-7: LGTM!The new import is correctly added to support the page-based pagination implementation.
81-89: Add error handling for invalid cursors and optimize cursor lookup.The cursor lookup implementation needs improvement:
- No error handling for invalid cursors
- Using linear search (O(n)) instead of more efficient methods
api/internal/infrastructure/memory/trigger.go (2)
7-7: LGTM!The added import is necessary for the new page-based pagination functionality.
50-52: LGTM!The early return for empty results is implemented correctly and efficiently.
api/internal/infrastructure/memory/deployment.go (1)
8-8: LGTM! Clean interface change to support both pagination types.The method signature change from
*usecasex.Paginationto*interfaces.PaginationParamappropriately supports both page-based and cursor-based pagination.Also applies to: 37-37
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
api/e2e/gql_pagination_test.go (4)
22-72: Refactor test data setup into a helper function.The project creation logic is repeated in other test functions. Consider extracting it into a reusable helper function to improve maintainability.
+func createTestProjects(t *testing.T, e *httpexpect.Expect, workspaceId string, userId string, count int) []string { + projectIDs := make([]string, count) + for i := 0; i < count; i++ { + query := `mutation($input: CreateProjectInput!) { + createProject(input: $input) { + project { + id + } + } + }` + + variables := fmt.Sprintf(`{ + "input": { + "workspaceId": "%s", + "name": "Test Project %d", + "description": "Test project description %d" + } + }`, workspaceId, i, i) + + var variablesMap map[string]any + err := json.Unmarshal([]byte(variables), &variablesMap) + assert.NoError(t, err) + + request := GraphQLRequest{ + Query: query, + Variables: variablesMap, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + CreateProject struct { + Project struct { + ID string `json:"id"` + } `json:"project"` + } `json:"createProject"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + projectIDs[i] = result.Data.CreateProject.Project.ID + } + return projectIDs +}
800-800: Translate Chinese comments to English.For consistency and maintainability, translate the Chinese comments to English.
- // 2. 创建测试部署 + // 2. Create test deployment - // 3. 发送请求创建部署 + // 3. Send request to create deploymentAlso applies to: 828-828
800-896: Extract test data setup into helper functions.The deployment and trigger creation logic should be extracted into helper functions for better maintainability and reusability.
+func createTestDeployment(t *testing.T, e *httpexpect.Expect, workspaceId string, userId string) string { + deploymentQuery := `mutation($input: CreateDeploymentInput!) { + createDeployment(input: $input) { + deployment { + id + } + } + }` + + deploymentVariables := fmt.Sprintf(`{ + "input": { + "workspaceId": "%s", + "name": "Test Deployment", + "description": "Test deployment description" + } + }`, workspaceId) + + var deploymentVariablesMap map[string]any + err := json.Unmarshal([]byte(deploymentVariables), &deploymentVariablesMap) + assert.NoError(t, err) + + request := GraphQLRequest{ + Query: deploymentQuery, + Variables: deploymentVariablesMap, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + CreateDeployment struct { + Deployment struct { + ID string `json:"id"` + } `json:"deployment"` + } `json:"createDeployment"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + return result.Data.CreateDeployment.Deployment.ID +} + +func createTestTriggers(t *testing.T, e *httpexpect.Expect, workspaceId string, deploymentId string, userId string, count int) []string { + triggerIDs := make([]string, count) + for i := 0; i < count; i++ { + triggerQuery := `mutation($input: CreateTimeDrivenTriggerInput!) { + createTimeDrivenTrigger(input: $input) { + trigger { + id + } + } + }` + + triggerVariables := fmt.Sprintf(`{ + "input": { + "workspaceId": "%s", + "deploymentId": "%s", + "description": "Test Trigger %d", + "schedule": "*/5 * * * *" + } + }`, workspaceId, deploymentId, i) + + var triggerVariablesMap map[string]any + err := json.Unmarshal([]byte(triggerVariables), &triggerVariablesMap) + assert.NoError(t, err) + + request := GraphQLRequest{ + Query: triggerQuery, + Variables: triggerVariablesMap, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + CreateTimeDrivenTrigger struct { + Trigger struct { + ID string `json:"id"` + } `json:"trigger"` + } `json:"createTimeDrivenTrigger"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + triggerIDs[i] = result.Data.CreateTimeDrivenTrigger.Trigger.ID + } + return triggerIDs +}
1-1237: Extract common patterns into helper functions and shared types.The test functions contain repeated patterns that could be extracted to improve maintainability:
- GraphQL query execution
- Response struct definitions
- Common values
+// Common response types +type PageInfo struct { + HasNextPage bool `json:"hasNextPage"` + HasPreviousPage bool `json:"hasPreviousPage"` + EndCursor string `json:"endCursor"` + TotalCount int `json:"totalCount"` + CurrentPage int `json:"currentPage"` + TotalPages int `json:"totalPages"` +} + +// Helper function for executing GraphQL queries +func executeGraphQLQuery(t *testing.T, e *httpexpect.Expect, query string, variables map[string]any, userId string) *httpexpect.Response { + request := GraphQLRequest{ + Query: query, + Variables: variables, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + return e.POST("/api/graphql"). + WithHeader("authorization", "Bearer test"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) +} + +// Constants +const ( + defaultPageSize = 2 + testDataCount = 5 +)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/e2e/gql_pagination_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (1)
api/e2e/gql_pagination_test.go (1)
1070-1072: 🛠️ Refactor suggestionAdd missing assertions in cursor pagination test.
The cursor pagination test is missing assertions for verifying the pagination results.
err = json.Unmarshal([]byte(resp.Body().Raw()), &result) assert.NoError(t, err) + + // Verify pagination results + assert.Len(t, result.Data.Triggers.Edges, 2, "Should return exactly 2 triggers") + assert.NotZero(t, result.Data.Triggers.PageInfo.TotalCount, "Total count should be greater than zero") + if result.Data.Triggers.PageInfo.TotalCount > 2 { + assert.True(t, result.Data.Triggers.PageInfo.HasNextPage, "Should have next page") + assert.NotEmpty(t, result.Data.Triggers.PageInfo.EndCursor, "End cursor should not be empty") + }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
api/internal/adapter/gql/loader_project.go (1)
Line range hint
72-97: Add validation and use common helper function.The function should validate pagination parameters and use the common helper function for creating the connection.
Apply this diff to improve the implementation:
func (c *ProjectLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.ProjectConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } + // Validate pagination parameters + if pagination.Page <= 0 { + pagination.Page = 1 + } + if pagination.PageSize <= 0 { + pagination.PageSize = 10 // default page size + } + paginationParam := gqlmodel.ToPageBasedPagination(pagination) res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam, getOperator(ctx)) if err != nil { return nil, err } - edges := make([]*gqlmodel.ProjectEdge, 0, len(res)) - nodes := make([]*gqlmodel.Project, 0, len(res)) - for _, p := range res { - prj := gqlmodel.ToProject(p) - edges = append(edges, &gqlmodel.ProjectEdge{ - Node: prj, - Cursor: usecasex.Cursor(prj.ID), - }) - nodes = append(nodes, prj) - } - - return &gqlmodel.ProjectConnection{ - Edges: edges, - Nodes: nodes, - PageInfo: gqlmodel.ToPageInfo(pi), - TotalCount: int(pi.TotalCount), - }, nil + return c.createProjectConnection(res, pi), nil }
♻️ Duplicate comments (4)
api/e2e/gql_pagination_test.go (2)
427-434: 🛠️ Refactor suggestionAdd explicit test data setup for jobs.
The test relies on seeded data which could make the tests fragile. Consider adding explicit test data creation similar to the projects test.
1-1231: 🛠️ Refactor suggestionRefactor test functions to reduce duplication.
The test functions share similar structure and boilerplate code. Consider extracting common test patterns into helper functions:
- Server setup
- GraphQL query execution
- Pagination and sorting test templates
Additionally, consider parameterizing the tests to run the same test cases for different entity types (projects, jobs, triggers) to further reduce code duplication.
🧰 Tools
🪛 GitHub Actions: ci
[error] 327-328: Test failure in TestProjectsPagination/test_page_pagination: Expected values 1 and 3, got 0 and 0
[error] 374-375: Test failure in TestProjectsPagination/test_page_pagination: Expected values 2 and 3, got 0 and 0
[error] 422-423: Test failure in TestProjectsPagination/test_page_pagination: Expected values 3 and 3, got 0 and 0
[error] 950-952: Test failure in TestTriggersPagination/test_pagination: Array length mismatch and total count mismatch (expected 5, got 0)
[error] 1130-1135: Test failure in TestTriggersPagination/test_page_pagination: Multiple assertion failures including array length and count mismatches
api/internal/adapter/gql/gqlmodel/convert.go (2)
29-37:⚠️ Potential issueUse safe type assertion to prevent panics.
The type assertion could panic if
pis not nil but the type assertion fails.Apply this diff to make the type assertion safe:
- if pbi, ok := any(p).(*interfaces.PageBasedInfo); ok { + pbi, ok := any(p).(*interfaces.PageBasedInfo) + if p != nil && ok { cp := pbi.CurrentPage tp := pbi.TotalPages currentPage = &cp totalPages = &tp }
74-81:⚠️ Potential issueAdd input validation for page-based pagination parameters.
The function should validate the input parameters before creating the pagination param.
Apply this diff to add validation:
func ToPageBasedPagination(pagination PageBasedPagination) *usecasex.Pagination { + // Ensure positive page and page size + page := pagination.Page + pageSize := pagination.PageSize + if page <= 0 { + page = 1 + } + if pageSize <= 0 { + pageSize = 10 // default page size + } + return &usecasex.Pagination{ Offset: &usecasex.OffsetPagination{ - Offset: int64((pagination.Page - 1) * pagination.PageSize), - Limit: int64(pagination.PageSize), + Offset: int64((page - 1) * pageSize), + Limit: int64(pageSize), }, } }
🧹 Nitpick comments (5)
api/internal/infrastructure/mongo/project.go (1)
168-169: Consider making sort configuration configurable.The default sort by
updatedAtis hardcoded. Consider making it configurable through the pagination parameters to support different use cases.- // Default sort by updatedAt desc - sort := bson.D{{Key: "updatedat", Value: -1}} + // Use sort from pagination or default to updatedAt desc + sort := bson.D{{Key: "updatedat", Value: -1}} + if pagination.Sort != nil { + sort = pagination.Sort + }api/internal/adapter/gql/loader_trigger.go (3)
79-84: Improve error handling and consider optimizing for page-based queries.
- Add error handling for pagination parameter conversion:
-paginationParam := gqlmodel.ToPageBasedPagination(pagination) +paginationParam, err := gqlmodel.ToPageBasedPagination(pagination) +if err != nil { + return nil, fmt.Errorf("invalid pagination parameters: %w", err) +}
- Consider adding a dedicated method in the usecase interface for page-based queries to optimize performance:
FindByWorkspacePage(ctx context.Context, workspaceID accountdomain.WorkspaceID, pagination *usecasex.PageBasedPagination, operator *usecase.Operator) ([]*domain.Trigger, *usecasex.PageInfo, error)
86-104: Reduce code duplication and clarify page-based implementation.
- Extract the common connection building logic to a helper method to avoid duplication:
func (c *TriggerLoader) buildConnection(res []*domain.Trigger, pi *usecasex.PageInfo) *gqlmodel.TriggerConnection { edges := make([]*gqlmodel.TriggerEdge, 0, len(res)) nodes := make([]*gqlmodel.Trigger, 0, len(res)) for _, t := range res { trig := gqlmodel.ToTrigger(t) edges = append(edges, &gqlmodel.TriggerEdge{ Node: trig, Cursor: usecasex.Cursor(trig.ID), }) nodes = append(nodes, trig) } return &gqlmodel.TriggerConnection{ Edges: edges, Nodes: nodes, PageInfo: gqlmodel.ToPageInfo(pi), TotalCount: int(pi.TotalCount), } }
- Consider whether cursor fields are necessary for page-based pagination, as they might confuse clients.
73-104: Add documentation for the new method.Add a documentation comment explaining the purpose and behavior of the new method:
// FindByWorkspacePage retrieves triggers for a workspace using page-based pagination. // It accepts a workspace ID and pagination parameters including page number, page size, // and sorting options. Returns a connection object containing the paginated results // and total count.api/internal/adapter/gql/gqlmodel/convert.go (1)
83-89: Add validation for OrderDirection enum values.The function should validate that the OrderDirection value is one of the expected enum values before conversion.
Apply this diff to add validation:
func OrderDirectionToString(dir *OrderDirection) *string { if dir == nil { return nil } + // Validate enum value + switch *dir { + case OrderDirectionAsc, OrderDirectionDesc: + s := string(*dir) + return &s + default: + return nil // or return a default value + } - s := string(*dir) - return &s }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/e2e/gql_pagination_test.go(1 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/adapter/gql/loader_job.go(1 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/infrastructure/mongo/project.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/internal/adapter/gql/loader_job.go
🧰 Additional context used
🪛 GitHub Actions: ci
api/e2e/gql_pagination_test.go
[error] 327-328: Test failure in TestProjectsPagination/test_page_pagination: Expected values 1 and 3, got 0 and 0
[error] 374-375: Test failure in TestProjectsPagination/test_page_pagination: Expected values 2 and 3, got 0 and 0
[error] 422-423: Test failure in TestProjectsPagination/test_page_pagination: Expected values 3 and 3, got 0 and 0
[error] 950-952: Test failure in TestTriggersPagination/test_pagination: Array length mismatch and total count mismatch (expected 5, got 0)
[error] 1130-1135: Test failure in TestTriggersPagination/test_page_pagination: Multiple assertion failures including array length and count mismatches
🔇 Additional comments (9)
api/e2e/gql_pagination_test.go (2)
22-81: LGTM: Project creation setup is thorough and well-structured.The setup code properly creates and verifies test projects with appropriate delays between creations to ensure deterministic ordering.
144-198: Add test for descending order sorting.The sorting test only covers ascending order. Add a test case for descending order to ensure bi-directional sorting works correctly.
api/internal/infrastructure/mongo/project.go (5)
7-7: LGTM! Required imports added for pagination enhancement.The new imports support the MongoDB Find options and page-based pagination interface.
Also applies to: 10-10
177-179: LGTM! Proper error handling with context.The error handling appropriately wraps MongoDB errors with context information.
Line range hint
188-193: LGTM! Preserved cursor-based pagination.Good practice maintaining backward compatibility by preserving the cursor-based pagination functionality.
Line range hint
156-193: Verify consistent pagination implementation across the codebase.Let's ensure the pagination implementation is consistent across all repositories.
181-184: Prevent division by zero in page calculation.The current page calculation could panic if limit is zero (though this should be prevented by the suggested input validation).
- currentPage := int(skip/limit) + 1 + currentPage := 1 + if limit > 0 { + currentPage = int(skip/limit) + 1 + }api/internal/adapter/gql/loader_trigger.go (1)
73-104: Verify the scope of pagination changes.While the PR objective mentions implementing page-based pagination for deployments, this file implements it for triggers. Please clarify if this is intentional and part of a broader pagination implementation across different entities.
Run the following script to check for similar pagination implementations:
api/internal/adapter/gql/gqlmodel/convert.go (1)
54-63:⚠️ Potential issueAdd input validation for pagination parameters.
The page-based pagination block should validate input parameters to prevent negative offsets.
Apply this diff to add validation:
// Page-based pagination if pagination.Page != nil && pagination.PageSize != nil { + // Ensure positive page and page size + page := *pagination.Page + pageSize := *pagination.PageSize + if page <= 0 { + page = 1 + } + if pageSize <= 0 { + pageSize = 10 // default page size + } + return &usecasex.Pagination{ Offset: &usecasex.OffsetPagination{ - Offset: int64((*pagination.Page - 1) * *pagination.PageSize), - Limit: int64(*pagination.PageSize), + Offset: int64((page - 1) * pageSize), + Limit: int64(pageSize), }, } }Likely invalid or redundant comment.
pyshx
changed the title
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
api/internal/usecase/repo/deployment.go (1)
Line range hint
25-47: Add validation and improve robustness of the WalkDeployments function.While the implementation is functional, consider these improvements for better robustness:
- Add parameter validation
- Handle context cancellation
- Consider encapsulating cursor updates
Here's the suggested implementation:
func WalkDeployments(ctx context.Context, repo Deployment, tid accountdomain.WorkspaceID, pagination *interfaces.Pagination, callback func([]*deployment.Deployment) error) error { + if pagination == nil { + return fmt.Errorf("pagination cannot be nil") + } + if callback == nil { + return fmt.Errorf("callback cannot be nil") + } + for { + if ctx.Err() != nil { + return ctx.Err() + } + deployments, info, err := repo.FindByWorkspace(ctx, tid, pagination) if err != nil { return err } if len(deployments) == 0 { break } if err := callback(deployments); err != nil { return err } if !info.HasNextPage { break } lastID := deployments[len(deployments)-1].ID().String() pagination.Cursor.After = &lastID } return nil }
♻️ Duplicate comments (9)
api/internal/adapter/gql/gqlmodel/convert.go (1)
54-61:⚠️ Potential issueAdd input validation and overflow protection.
Similar to the
ToPaginationfunction, this function needs validation and overflow protection.Apply similar fixes as suggested for the
ToPaginationfunction.api/internal/infrastructure/memory/deployment_test.go (2)
111-115:⚠️ Potential issueSet UpdatedAt field in test data to ensure correct sorting.
The test data doesn't set the UpdatedAt field, which is used for default sorting. This could cause inconsistent test results.
Apply this diff to fix the test data initialization:
-d1 := deployment.New().NewID().Workspace(wsID).Version("v1").MustBuild() -d2 := deployment.New().NewID().Workspace(wsID).Version("v2").MustBuild() -d3 := deployment.New().NewID().Workspace(wsID).Version("v3").MustBuild() +now := time.Now() +d1 := deployment.New().NewID().Workspace(wsID).Version("v1").UpdatedAt(now.Add(-2 * time.Hour)).MustBuild() +d2 := deployment.New().NewID().Workspace(wsID).Version("v2").UpdatedAt(now.Add(-1 * time.Hour)).MustBuild() +d3 := deployment.New().NewID().Workspace(wsID).Version("v3").UpdatedAt(now).MustBuild()
161-166:⚠️ Potential issueFix failing test for second page pagination.
The test is failing due to deployment object comparison. This could be due to incorrect ordering of deployments in the test data.
Verify the deployment objects in the test data:
- want: []*deployment.Deployment{d3}, + want: []*deployment.Deployment{d2},api/internal/infrastructure/mongo/deployment.go (2)
225-244:⚠️ Potential issueThe race condition concern is valid and requires attention.
The current implementation is susceptible to lost updates in concurrent scenarios.
Consider these alternative approaches:
- Add a separate
revisionfield (integer) for optimistic locking- Use MongoDB's atomic findAndModify operation
- Implement document-level locking using MongoDB transactions
228-231:⚠️ Potential issueAddress use of unkeyed composites in the page-based sorting to fix the pipeline failure.
The sort bson.D composites use unkeyed fields which causes linting errors.
Apply this diff to fix the unkeyed composites:
-sort = bson.D{{*pagination.Page.OrderBy, direction}} +sort = bson.D{{Key: *pagination.Page.OrderBy, Value: direction}} -sort = bson.D{{"updatedAt", -1}} +sort = bson.D{{Key: "updatedAt", Value: -1}}api/internal/infrastructure/memory/trigger.go (2)
86-103:⚠️ Potential issueAdd input validation for page-based pagination.
The page-based pagination implementation needs validation to prevent potential issues:
- Missing validation for
limit > 0- Missing validation for
offset >= 0- Potential division by zero in
skip/limit+1
51-107: 🛠️ Refactor suggestionImprove cursor-based pagination implementation.
The cursor-based pagination implementation has several potential issues:
- No sorting is applied before pagination
- Linear search is used for cursor lookup
- Missing validation for the "first" parameter
- Missing error handling when cursor is not found
api/internal/infrastructure/memory/project.go (2)
52-72:⚠️ Potential issueAdd input validation for page-based pagination.
The page-based pagination implementation needs validation to prevent potential issues:
- Missing validation for
limit > 0- Missing validation for
offset >= 0- Potential division by zero in
skip/limit+1
74-107: 🛠️ Refactor suggestionImprove cursor-based pagination implementation.
The cursor-based pagination implementation has several potential issues:
- No sorting is applied before pagination
- Linear search is used for cursor lookup
- Missing validation for the "first" parameter
- Missing error handling when cursor is not found
🧹 Nitpick comments (12)
api/internal/usecase/repo/trigger.go (1)
21-44: Add context cancellation check in WalkTriggers.The function should check for context cancellation between iterations to allow graceful termination of long-running operations.
func WalkTriggers(ctx context.Context, repo Trigger, tid accountdomain.WorkspaceID, pagination *interfaces.Pagination, callback func([]*trigger.Trigger) error) error { for { + if err := ctx.Err(); err != nil { + return err + } + triggers, info, err := repo.FindByWorkspace(ctx, tid, pagination) if err != nil { return errapi/internal/usecase/repo/project.go (1)
Line range hint
23-44: Consider extracting common pagination walk logic.The
WalkProjectsfunction is nearly identical toWalkTriggers. Consider creating a generic helper function to reduce code duplication.Example implementation:
// Generic walk function for paginated results func Walk[T any]( ctx context.Context, fetch func(context.Context, *interfaces.Pagination) ([]T, *interfaces.PageInfo, error), pagination *interfaces.Pagination, callback func([]T) error, ) error { for { if err := ctx.Err(); err != nil { return err } items, info, err := fetch(ctx, pagination) if err != nil { return err } if len(items) == 0 { break } if err := callback(items); err != nil { return err } if !info.HasNextPage { break } lastItem := items[len(items)-1] lastID := reflect.ValueOf(lastItem).MethodByName("ID").Call(nil)[0].Interface().(fmt.Stringer).String() pagination.Cursor.After = &lastID } return nil }api/pkg/job/job.go (1)
112-114: Consider enhancing SetErrorMessage behavior.When setting an error message, consider automatically updating the job status to Failed and the completedAt timestamp.
func (j *Job) SetErrorMessage(errorMsg *string) { j.errorMsg = errorMsg + if errorMsg != nil { + j.SetStatus(StatusFailed) + } }api/internal/usecase/interactor/asset.go (1)
Line range hint
33-43: Consider adding pagination parameter validation.The function should validate the pagination parameter before passing it to the repository layer.
func (i *Asset) FindByWorkspace(ctx context.Context, tid accountdomain.WorkspaceID, keyword *string, sort *asset.SortType, p *interfaces.Pagination, operator *usecase.Operator) ([]*asset.Asset, *interfaces.PageInfo, error) { + if p != nil && p.First != nil && *p.First < 0 { + return nil, nil, fmt.Errorf("invalid pagination: First must be non-negative") + } + if p != nil && p.Last != nil && *p.Last < 0 { + return nil, nil, fmt.Errorf("invalid pagination: Last must be non-negative") + } return Run2( ctx, operator, i.repos, Usecase().WithReadableWorkspaces(tid),api/gql/_shared.graphql (1)
44-48: Consider documenting the pagination strategy.While the implementation supports both page-based and cursor-based pagination, it would be helpful to add documentation comments explaining when to use each strategy and any limitations.
input Pagination { + """ + Page-based pagination parameters. + Note: When both page-based and cursor-based parameters are provided, + page-based pagination takes precedence. + """ page: Int pageSize: Int orderBy: String orderDir: OrderDirection }api/internal/infrastructure/memory/asset.go (1)
Line range hint
54-57: Ensure stable sorting across pagination requests.The current sorting implementation may lead to inconsistent pagination results as:
- No default sort order is specified
- No secondary sort key for stability
Apply this diff:
if filter.Sort != nil { s := *filter.Sort - sort.SliceStable(result, func(i, j int) bool { + sort.Slice(result, func(i, j int) bool { if s == asset.SortTypeID { - return result[i].ID().Compare(result[j].ID()) < 0 + c := result[i].ID().Compare(result[j].ID()) + if c != 0 { + return c < 0 + } } if s == asset.SortTypeSize { - return result[i].Size() < result[j].Size() + if result[i].Size() != result[j].Size() { + return result[i].Size() < result[j].Size() + } } if s == asset.SortTypeName { - return strings.Compare(result[i].Name(), result[j].Name()) < 0 + c := strings.Compare(result[i].Name(), result[j].Name()) + if c != 0 { + return c < 0 + } } - return false + // Use ID as secondary sort key for stability + return result[i].ID().Compare(result[j].ID()) < 0 }) }api/internal/infrastructure/memory/deployment.go (1)
54-57: Consider parameterizing the sort order.The sort order is hardcoded to sort by UpdatedAt in descending order. Consider making this configurable through the pagination parameters.
Apply this diff:
+ // Default sort by UpdatedAt desc sort.SliceStable(result, func(i, j int) bool { - return result[i].UpdatedAt().After(result[j].UpdatedAt()) + // Primary sort by UpdatedAt + if !result[i].UpdatedAt().Equal(result[j].UpdatedAt()) { + return result[i].UpdatedAt().After(result[j].UpdatedAt()) + } + // Secondary sort by ID for stability + return result[i].ID().String() < result[j].ID().String() })api/internal/infrastructure/mongo/deployment.go (1)
28-37: Consider adding documentation for the adapter pattern.The adapter pattern implementation is good, but it lacks documentation explaining its purpose and usage.
Add documentation comments:
+// DeploymentAdapter wraps the Deployment repository to implement the repo.Deployment interface type DeploymentAdapter struct { *Deployment } +// NewDeployment creates a new instance of the DeploymentAdapter implementing repo.Deployment func NewDeployment(client *mongox.Client) repo.Deployment { return &DeploymentAdapter{ Deployment: &Deployment{ client: client.WithCollection("deployment"), }, } }api/internal/adapter/gql/gqlmodel/models_gen.go (1)
229-234: Add documentation for the PageBasedPagination type.Even though this is generated code, adding documentation comments helps with code understanding.
Add documentation comments:
+// PageBasedPagination represents the input parameters for page-based pagination +// Page: The page number (1-based) +// PageSize: Number of items per page +// OrderBy: Field to sort by +// OrderDir: Sort direction (ASC or DESC) type PageBasedPagination struct { Page int `json:"page"` PageSize int `json:"pageSize"` OrderBy *string `json:"orderBy,omitempty"` OrderDir *OrderDirection `json:"orderDir,omitempty"` }api/internal/infrastructure/mongo/job.go (1)
134-141: Improve error handling for MongoDB operations.The error handling for MongoDB operations could be improved by providing more context about the failure.
Apply this diff to enhance error handling:
if err := r.client.Find(ctx, filter, c, opt); err != nil { - return nil, nil, err + return nil, nil, rerror.ErrInternalBy(err) } total, err := r.client.Count(ctx, filter) if err != nil { - return nil, nil, err + return nil, nil, rerror.ErrInternalBy(err) }api/internal/infrastructure/mongo/deployment_test.go (2)
66-130: Add test cases for error scenarios and input validation.The test coverage could be improved by adding the following test cases:
- Invalid pagination parameters (negative offset, zero limit)
- Invalid cursor
- Error scenarios from MongoDB operations
Add these test cases to improve coverage:
// Test invalid pagination parameters pagination := &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: 1, PageSize: 0, // Invalid limit }, } got, pageInfo, err := r.FindByWorkspace(ctx, wid, pagination) assert.Error(t, err) assert.Nil(t, pageInfo) assert.Nil(t, got) // Test invalid cursor pagination = &interfaces.PaginationParam{ Cursor: &interfaces.CursorPaginationParam{ After: proto.String("invalid"), First: proto.Int64(10), }, } got, pageInfo, err = r.FindByWorkspace(ctx, wid, pagination) assert.Error(t, err) assert.Nil(t, pageInfo) assert.Nil(t, got)
222-315: Add test cases for cursor-based pagination.The test suite lacks coverage for cursor-based pagination. Add test cases to verify:
- Forward pagination with After + First
- Backward pagination with Before + Last
- Edge cases like invalid cursors
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
api/e2e/gql_pagination_test.go(1 hunks)api/gql/_shared.graphql(1 hunks)api/gql/asset.graphql(0 hunks)api/gql/deployment.graphql(0 hunks)api/gql/job.graphql(0 hunks)api/gql/project.graphql(1 hunks)api/gql/trigger.graphql(0 hunks)api/internal/adapter/gql/generated.go(42 hunks)api/internal/adapter/gql/gqlmodel/convert.go(2 hunks)api/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)api/internal/adapter/gql/loader_asset.go(1 hunks)api/internal/adapter/gql/loader_deployment.go(1 hunks)api/internal/adapter/gql/loader_job.go(2 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/adapter/gql/resolver_query.go(2 hunks)api/internal/infrastructure/memory/asset.go(3 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/memory/job.go(3 hunks)api/internal/infrastructure/memory/project.go(3 hunks)api/internal/infrastructure/memory/trigger.go(3 hunks)api/internal/infrastructure/mongo/asset.go(3 hunks)api/internal/infrastructure/mongo/deployment.go(3 hunks)api/internal/infrastructure/mongo/deployment_test.go(3 hunks)api/internal/infrastructure/mongo/job.go(3 hunks)api/internal/infrastructure/mongo/project.go(2 hunks)api/internal/usecase/interactor/asset.go(1 hunks)api/internal/usecase/interactor/deployment.go(1 hunks)api/internal/usecase/interactor/job.go(1 hunks)api/internal/usecase/interfaces/asset.go(1 hunks)api/internal/usecase/interfaces/deployment.go(1 hunks)api/internal/usecase/interfaces/job.go(1 hunks)api/internal/usecase/interfaces/pageinfo.go(1 hunks)api/internal/usecase/repo/asset.go(1 hunks)api/internal/usecase/repo/deployment.go(3 hunks)api/internal/usecase/repo/job.go(2 hunks)api/internal/usecase/repo/project.go(2 hunks)api/internal/usecase/repo/trigger.go(1 hunks)api/pkg/job/job.go(3 hunks)
💤 Files with no reviewable changes (4)
- api/gql/job.graphql
- api/gql/deployment.graphql
- api/gql/asset.graphql
- api/gql/trigger.graphql
🚧 Files skipped from review as they are similar to previous changes (7)
- api/internal/adapter/gql/loader_trigger.go
- api/internal/adapter/gql/loader_job.go
- api/gql/project.graphql
- api/internal/usecase/interactor/deployment.go
- api/internal/adapter/gql/loader_project.go
- api/internal/adapter/gql/resolver_query.go
- api/internal/infrastructure/mongo/asset.go
🔇 Additional comments (33)
api/internal/usecase/repo/deployment.go (2)
6-6: LGTM! Import changes align with pagination refactoring.The addition of the interfaces package import supports the transition to the new pagination structure.
16-16: LGTM! Pagination interface change supports new requirements.The change to use
*interfaces.Paginationaligns with the PR objective of implementing page-based pagination while maintaining cursor-based support.api/internal/usecase/repo/asset.go (1)
20-20: LGTM: Return type change maintains consistency.The change in return type from
*usecasex.PageInfoto*interfaces.PageInfomaintains consistency with the pagination refactoring.api/internal/usecase/interfaces/asset.go (1)
31-31: LGTM: Interface method signature updated consistently.The
FindByWorkspacemethod signature has been updated to use the new pagination types while maintaining the same functionality.api/internal/usecase/repo/job.go (2)
15-16: LGTM: Method ordering and pagination type updates are correct.The reordering of methods and updates to pagination types are consistent with the changes across the codebase.
Line range hint
21-42: Consider adding documentation for the WalkJobs function.The implementation of
WalkJobslooks correct, but it would benefit from documentation explaining its purpose and usage.Add documentation above the function:
+// WalkJobs iterates through all jobs in a workspace using pagination. +// It calls the callback function for each batch of jobs until there are no more jobs +// or an error occurs. The pagination cursor is automatically updated between iterations. func WalkJobs(ctx context.Context, repo Job, tid accountdomain.WorkspaceID, pagination *interfaces.Pagination, callback func([]*job.Job) error) error {Also, consider adding error handling for nil pagination:
Let's check if other similar functions handle nil pagination:
✅ Verification successful
Add documentation to WalkJobs function as suggested
The documentation suggestion is valid and consistent with good coding practices. The proposed documentation clearly explains the function's purpose and behavior:
+// WalkJobs iterates through all jobs in a workspace using pagination. +// It calls the callback function for each batch of jobs until there are no more jobs +// or an error occurs. The pagination cursor is automatically updated between iterations. func WalkJobs(ctx context.Context, repo Job, tid accountdomain.WorkspaceID, pagination *interfaces.Pagination, callback func([]*job.Job) error) error {However, adding nil pagination handling is not necessary as the current implementation follows a consistent pattern across all similar Walk* functions in the usecase layer, where pagination handling is delegated to the infrastructure layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for nil pagination checks in similar functions rg "if pagination == nil" --type goLength of output: 245
Script:
#!/bin/bash # Get context around nil pagination checks echo "=== Nil pagination handling ===" rg "if pagination == nil" -B 2 -A 2 --type go echo -e "\n=== Similar Walk functions ===" rg "^func Walk.*Pagination" --type goLength of output: 2260
Script:
#!/bin/bash # Get context around Walk* functions for file in project.go trigger.go deployment.go; do echo -e "\n=== Content of usecase/repo/$file ===" rg "^func Walk.*Pagination" api/internal/usecase/repo/$file -B 3 -A 5 doneLength of output: 1691
api/internal/usecase/repo/trigger.go (1)
15-16: LGTM! Interface changes look good.The reordering of methods and update to use interfaces.Pagination is consistent with the PR objectives.
api/internal/usecase/repo/project.go (1)
15-16: LGTM! Interface changes look good.The reordering of methods and update to use interfaces.Pagination is consistent with the PR objectives.
api/pkg/job/job.go (1)
25-25: LGTM! Error handling improvements look good.The addition of error message field and getter method enhances error reporting capabilities.
Also applies to: 72-74
api/internal/adapter/gql/loader_asset.go (1)
52-54: LGTM! The changes align with the pagination refactor.The modifications correctly adapt the code to the new pagination system by removing cursor-based pagination elements while maintaining the proper functionality.
api/internal/usecase/interactor/job.go (1)
62-66: LGTM! The method signature has been properly updated.The changes correctly adapt the method to use the new pagination types while maintaining the existing permission checks and repository calls.
api/gql/_shared.graphql (2)
27-29: LGTM! The PageInfo type has been enhanced with standard pagination fields.The addition of
totalCount,currentPage, andtotalPagesfields provides essential information for implementing UI pagination controls.
32-37: LGTM! The new pagination types follow GraphQL best practices.The
PageBasedPaginationinput type andOrderDirectionenum provide a clean and type-safe way to handle pagination and sorting parameters.Also applies to: 39-42
api/internal/usecase/interfaces/pageinfo.go (2)
39-51: Add input validation for page-based parameters.The constructor should validate its input parameters to ensure they are within valid ranges.
53-64: LGTM! Clean implementation of the ToPageInfo method.The method properly handles nil checks and maintains a clean mapping between PageBasedInfo and PageInfo types.
api/internal/adapter/gql/gqlmodel/convert.go (3)
23-35: LGTM! Clean conversion function.The changes improve the function by:
- Properly handling nil input
- Adding support for new pagination fields
63-69: LGTM! Clean conversion function.The function correctly handles nil input and performs a straightforward type conversion.
71-90: LGTM! Clean conversion function.The function correctly:
- Handles nil input
- Converts optional pagination fields
- Maintains type safety
api/internal/usecase/interfaces/deployment.go (2)
33-38: 🛠️ Refactor suggestionAdd validation for pagination parameters.
The
PageBasedPaginationParamshould validate its fields to ensure they are positive values.Add a validation method:
type PageBasedPaginationParam struct { Page int PageSize int OrderBy *string OrderDir *string } + +func (p *PageBasedPaginationParam) Validate() error { + if p.Page <= 0 || p.PageSize <= 0 { + return ErrInvalidPagination + } + return nil +}Likely invalid or redundant comment.
40-44: 🛠️ Refactor suggestionEnforce mutual exclusivity of pagination types.
The comment indicates that only one field should be set, but there's no runtime validation.
Add a validation method:
type PaginationParam struct { // Only one of these should be set Cursor *usecasex.Pagination Page *PageBasedPaginationParam } + +func (p *PaginationParam) Validate() error { + if p.Cursor != nil && p.Page != nil { + return ErrInvalidPagination + } + if p.Page != nil { + return p.Page.Validate() + } + return nil +}Likely invalid or redundant comment.
api/internal/infrastructure/memory/asset.go (1)
115-130:⚠️ Potential issueAdd input validation for pagination parameters.
The page-based pagination implementation needs validation to prevent potential issues:
- Missing validation for limit and offset
- Potential divide by zero when calculating current page
- No upper bound for limit
Apply this diff:
} else if filter.Pagination.Offset != nil { // Page-based pagination + if filter.Pagination.Offset.Limit <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "limit must be positive") + } + if filter.Pagination.Offset.Offset < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "offset must be non-negative") + } + if filter.Pagination.Offset.Limit > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "limit cannot exceed 100") + } skip := int(filter.Pagination.Offset.Offset) limit := int(filter.Pagination.Offset.Limit)Likely invalid or redundant comment.
api/internal/infrastructure/mongo/project.go (1)
157-186: LGTM! MongoDB pagination implementation looks solid.The implementation correctly:
- Uses MongoDB's skip and limit for efficient pagination
- Includes proper error handling
- Sets appropriate sort options
api/internal/infrastructure/memory/deployment.go (1)
41-42: Good performance optimization with pre-allocation.The pre-allocation of the result slice with an estimated capacity is a good optimization.
api/internal/adapter/gql/loader_deployment.go (1)
43-74: LGTM! Well-structured GraphQL loader implementation.The implementation correctly:
- Handles ID conversion with proper error checking
- Converts pagination parameters
- Constructs GraphQL-specific response types
api/internal/infrastructure/memory/deployment_test.go (1)
117-204: Add test cases for error scenarios and sorting behavior.The test suite is missing cases for invalid pagination parameters and sorting behavior.
Add the following test cases:
+ { + name: "invalid page number", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: -1, + PageSize: 10, + }, + }, + wantErr: true, + }, + { + name: "invalid order direction", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: 1, + PageSize: 10, + OrderDir: lo.ToPtr("INVALID"), + }, + }, + wantErr: true, + },api/e2e/gql_pagination_test.go (2)
1-575: Refactor test functions to reduce duplication.The test functions share similar structure and boilerplate code.
Extract common test patterns into helper functions:
- Server setup
- GraphQL query execution
- Pagination and sorting test templates
+// Helper function for executing GraphQL queries +func executeGraphQLQuery(e *httpexpect.Expect, query string, userId string) *httpexpect.Response { + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + if err != nil { + panic(err) + } + + return e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect().Status(http.StatusOK) +}
154-208: Add test for descending order sorting.The sorting test only covers ascending order.
Add a test case for descending order to ensure bi-directional sorting works correctly.
api/internal/adapter/gql/generated.go (5)
83-83: LGTM: Edge type simplification aligns with page-based pagination.The removal of cursor fields from Edge types is consistent with the migration from cursor-based to page-based pagination.
Also applies to: 133-133, 159-159, 252-252, 320-320
419-419: LGTM: Consistent pagination parameter usage across interfaces.The Query and Workspace interfaces have been updated consistently to use the new pagination parameter, maintaining a uniform approach across all paginated endpoints.
Also applies to: 432-434
20790-20804: LGTM: OrderDirection implementation follows best practices.The OrderDirection enum implementation properly handles ASC and DESC directions with correct unmarshal and marshal implementations.
15133-15179:⚠️ Potential issueAdd validation for pagination parameters.
The PageBasedPagination unmarshal implementation should validate:
- Positive page numbers
- Reasonable page sizes to prevent performance issues
Apply this diff to add validation:
func (ec *executionContext) unmarshalInputPageBasedPagination(ctx context.Context, obj interface{}) (gqlmodel.PageBasedPagination, error) { var it gqlmodel.PageBasedPagination asMap := map[string]interface{}{} for k, v := range obj.(map[string]interface{}) { asMap[k] = v } fieldsInOrder := [...]string{"page", "pageSize", "orderBy", "orderDir"} for _, k := range fieldsInOrder { v, ok := asMap[k] if !ok { continue } switch k { case "page": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("page")) data, err := ec.unmarshalNInt2int(ctx, v) if err != nil { return it, err } + if data <= 0 { + return it, fmt.Errorf("page must be positive") + } it.Page = data case "pageSize": ctx := graphql.WithPathContext(ctx, graphql.NewPathWithField("pageSize")) data, err := ec.unmarshalNInt2int(ctx, v) if err != nil { return it, err } + if data <= 0 { + return it, fmt.Errorf("pageSize must be positive") + } + if data > 100 { + return it, fmt.Errorf("pageSize must not exceed 100") + } it.PageSize = dataLikely invalid or redundant comment.
208-212: Verify initialization of new PageInfo fields.The new PageInfo fields (CurrentPage, TotalCount, TotalPages) need proper initialization in the resolver implementations to ensure accurate pagination metadata.
Run this script to check resolver implementations:
✅ Verification successful
PageInfo fields are properly initialized ✅
The new PageInfo fields are correctly initialized across the codebase:
- Memory and MongoDB repositories properly calculate pagination metadata
- Conversion functions handle all fields including CurrentPage, TotalCount, and TotalPages
- PageInfo initialization is consistent across different repository implementations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if all PageInfo fields are properly initialized in resolvers # Search for PageInfo initialization patterns rg -A 10 'PageInfo\s*{' --type go # Search for resolver implementations returning these fields rg -A 5 'func.*PageInfo' --type goLength of output: 18972
Script:
#!/bin/bash # Find PageInfo struct definition and initialization rg -A 10 'type PageInfo struct' --type go # Check conversion implementations rg -A 10 'func.*ToPageInfo' --type go rg -A 10 'func.*FromPageInfo' --type go # Check EmptyPageInfo implementation rg -A 5 'func.*EmptyPageInfo' --type goLength of output: 4088
Script:
#!/bin/bash # Check pagination implementation in repositories rg -A 20 'func.*paginate.*PageInfo' --type go # Check any NewPageInfo or similar initialization functions rg -A 10 'New.*PageInfo|Calculate.*PageInfo' --type goLength of output: 8664
api/internal/infrastructure/mongo/job.go (1)
125-129:⚠️ Potential issueAdd input validation for pagination parameters.
The pagination implementation needs validation to prevent potential issues:
- Missing validation for
limit > 0- Missing validation for
offset >= 0- Potential division by zero in
skip/limit+1Apply this diff to add input validation:
skip := pagination.Offset.Offset limit := pagination.Offset.Limit + // Validate input + if limit <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "limit must be positive") + } + if skip < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "offset must be non-negative") + } + opt := options.Find().Likely invalid or redundant comment.
kasugamirai
force-pushed
the
feat/pagination
branch
from
d51e195 to
5d71bc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/gql/trigger.graphql (1)
55-55: Remove unnecessary empty line.Consider removing this empty line for better code organization.
api/gql/deployment.graphql (1)
75-75: LGTM! Core pagination changes implemented correctly.The changes successfully implement page-based pagination for deployments, which was the main objective of this PR.
Consider documenting the following aspects of the pagination implementation:
- Default page size limits
- Maximum page size restrictions
- Sorting defaults when not specified
- Performance implications for large result sets
This will help API consumers understand the pagination behavior and make optimal use of the new interface.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/gql/_shared.graphql(1 hunks)api/gql/asset.graphql(1 hunks)api/gql/deployment.graphql(1 hunks)api/gql/job.graphql(1 hunks)api/gql/project.graphql(1 hunks)api/gql/trigger.graphql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/gql/_shared.graphql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (5)
api/gql/job.graphql (1)
36-36: LGTM! Consistent pagination implementation.The changes align with the PR objective of implementing page-based pagination, making the API more straightforward to use.
api/gql/asset.graphql (1)
50-54: LGTM! Pagination changes maintain existing functionality.The changes correctly preserve the optional filtering and sorting capabilities while standardizing the pagination interface.
api/gql/trigger.graphql (1)
67-67: LGTM! Consistent pagination implementation.The changes align with the standardized page-based pagination approach.
api/gql/project.graphql (1)
73-77: LGTM! Pagination changes preserve filtering capability.The changes correctly maintain the
includeArchivedfilter while standardizing the pagination interface.api/gql/deployment.graphql (1)
Line range hint
36-36: Verify GraphQL client compatibility with the new pagination pattern.The switch from cursor-based to page-based pagination is a breaking change that affects multiple entities (jobs, assets, triggers, projects, deployments). While the new implementation is more straightforward, it requires careful coordination with API clients.
Consider the following:
- Ensure all GraphQL clients are updated to handle the new pagination pattern
- Update API documentation to reflect the new pagination interface
- Consider providing a migration guide for API consumers
- Add deprecation notices if maintaining backward compatibility with cursor-based pagination is needed
Run this script to identify potential GraphQL clients that need updates:
Also applies to: 50-54, 67-67, 73-77, 75-75
✅ Verification successful
Pagination changes are being handled appropriately with proper migration strategy
The codebase shows evidence of a controlled transition from cursor-based to page-based pagination:
- Both pagination patterns coexist in the schema, indicating a migration period
- Client implementations are centralized in feature-specific hooks (
ui/src/lib/gql/*/useQueries.ts)- Backend infrastructure already supports the new pagination pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find GraphQL queries using the old pagination pattern # Test: Search for usage of the old query names and cursor-based pagination # Expected results: List of files that need updates echo "Searching for old query usage..." rg -l 'query.*\b(jobs|assets|triggers|projects|deployments)\b.*Pagination\!' echo "Searching for cursor usage..." rg -l 'cursor.*Connection.*Edge'Length of output: 458
Script:
#!/bin/bash # Search for pagination patterns in schema and client code echo "Searching for cursor-based pagination in schema..." rg -l "type.*Connection|type.*Edge|cursor" echo "Searching for page-based pagination..." rg -l "type.*PageBasedPagination|totalPages|pageSize|currentPage" echo "Searching for pagination-related queries..." rg -l "query.*(jobs|assets|triggers|projects|deployments)" echo "Searching for client-side implementations..." fd -e ts -e js -e jsx -e tsx | xargs rg -l "useQuery|useLazyQuery|gql"Length of output: 6712
…nd simplified setup
This commit removes Edge types (AssetEdge, DeploymentEdge, JobEdge, ProjectEdge, TriggerEdge) from the GraphQL schema and generated code. The changes simplify the pagination model by removing cursor-related fields and focusing on page-based pagination. Modifications include: - Removing Edge types from models - Removing cursor-related fields from PageInfo - Updating loader methods to remove edge generation - Adjusting resolver methods to work with the simplified pagination model
This commit removes cursor-based pagination logic from memory repositories for Deployment, Project, and Trigger. The changes include: - Removing cursor-based pagination handling - Simplifying pagination methods to use page-based pagination - Updating sorting and pagination logic to work with the new approach - Removing unnecessary cursor-related code and imports
…r repositories This commit updates the pagination logic for Job, Project, and Trigger repositories to: - Replace cursor-based pagination with page-based pagination - Modify method signatures to use new PaginationParam interface - Simplify pagination logic in memory and MongoDB implementations - Convert cursor-based pagination requests to page-based pagination
…tion This commit updates the deployment test to use page-based pagination, removing cursor-related imports and logic. The changes align with the ongoing pagination refactoring effort, simplifying the test case to use the new page-based pagination approach.
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (1)
api/internal/adapter/gql/loader_job.go (1)
Line range hint
58-81: Add error handling and input validation.The implementation needs:
- Error handling for invalid workspace ID
- Input validation for pagination parameters
- Error handling for nil results from usecase.FindByWorkspace
Apply this diff to fix the issues:
func (c *JobLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.JobConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { - return nil, err + return nil, rerror.ErrInvalidParameter.WithError(err) + } + + // Validate pagination parameters + if pagination.Page <= 0 { + return nil, rerror.ErrInvalidParameter.WithMessage("page must be positive") + } + if pagination.PageSize <= 0 { + return nil, rerror.ErrInvalidParameter.WithMessage("page size must be positive") } paginationParam := gqlmodel.ToPageBasedPagination(pagination) res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam, getOperator(ctx)) if err != nil { return nil, err } + if res == nil || pi == nil { + return nil, rerror.ErrNotFound + } nodes := make([]*gqlmodel.Job, 0, len(res)) for _, j := range res { nodes = append(nodes, gqlmodel.ToJob(j)) }
♻️ Duplicate comments (11)
api/internal/adapter/gql/gqlmodel/convert.go (3)
29-37:⚠️ Potential issueUse safe type assertion to prevent panics.
The type assertion could panic if
pis not nil but the type assertion fails.Apply this diff to fix the issue:
- if pbi, ok := any(p).(*interfaces.PageBasedInfo); ok { + pbi, ok := any(p).(*interfaces.PageBasedInfo) + if p != nil && ok { cp := pbi.CurrentPage tp := pbi.TotalPages currentPage = &cp totalPages = &tp }
64-71:⚠️ Potential issueAdd input validation for pagination parameters.
The function should validate the input parameters before creating the pagination param.
Apply this diff to fix the issue:
func ToPageBasedPagination(pagination PageBasedPagination) *interfaces.PaginationParam { + // Ensure positive page and page size + if pagination.Page <= 0 { + pagination.Page = 1 + } + if pagination.PageSize <= 0 { + pagination.PageSize = 10 // default page size + } + return &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: pagination.Page,
88-95:⚠️ Potential issueHandle page-based fields in FromPageInfo conversion.
The function should convert page-based fields (currentPage, totalPages) when converting back to usecasex.PageInfo.
Apply this diff to fix the issue:
func FromPageInfo(p *PageInfo) *usecasex.PageInfo { if p == nil { return &usecasex.PageInfo{} } - return &usecasex.PageInfo{ + info := &usecasex.PageInfo{ TotalCount: int64(p.TotalCount), } + + // Convert to PageBasedInfo if page-based fields are present + if p.CurrentPage != nil && p.TotalPages != nil { + return &interfaces.PageBasedInfo{ + PageInfo: *info, + CurrentPage: *p.CurrentPage, + TotalPages: *p.TotalPages, + } + } + + return info }api/internal/infrastructure/memory/job.go (1)
60-79:⚠️ Potential issueAdd input validation and sorting for page-based pagination.
The implementation needs:
- Input validation for page and page size
- Overflow protection for skip calculation
- Sorting for consistent pagination results
Apply this diff to fix the issues:
if pagination.Page != nil { + // Validate input + if pagination.Page.Page <= 0 { + pagination.Page.Page = 1 + } + if pagination.Page.PageSize <= 0 { + pagination.Page.PageSize = 10 // default page size + } + + // Sort results for consistent pagination + sort.Slice(result, func(i, j int) bool { + return result[i].ID().String() < result[j].ID().String() + }) + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSize + + // Check for potential overflow + if pagination.Page.Page > 1 && pagination.Page.PageSize > math.MaxInt/pagination.Page.Page { + return nil, nil, rerror.ErrInvalidParameter + } + if skip >= len(result) { return nil, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize).ToPageInfo(), nil }api/internal/infrastructure/mongo/trigger.go (1)
71-94:⚠️ Potential issueAdd input validation and extract common pagination logic.
The implementation needs:
- Input validation for pagination parameters
- Overflow protection for skip calculation
- Common pagination logic should be extracted to avoid duplication
Apply this diff to fix the issues:
+ // Validate pagination parameters + if pagination != nil && pagination.Page != nil { + if pagination.Page.Page <= 0 { + return nil, nil, rerror.ErrInvalidParameter.WithMessage("page must be positive") + } + if pagination.Page.PageSize <= 0 { + return nil, nil, rerror.ErrInvalidParameter.WithMessage("page size must be positive") + } + + // Check for potential overflow + if pagination.Page.Page > 1 && pagination.Page.PageSize > math.MaxInt64/(pagination.Page.Page-1) { + return nil, nil, rerror.ErrInvalidParameter.WithMessage("pagination parameters too large") + } + }Additionally, consider extracting the common pagination logic into a shared utility function:
// mongox/pagination.go func HandlePageBasedPagination[T any]( ctx context.Context, client *mongox.ClientCollection, filter bson.M, pagination *interfaces.PaginationParam, consumer mongox.Consumer[T], ) ([]*T, *usecasex.PageInfo, error) { if pagination == nil || pagination.Page == nil { if err := client.Find(ctx, filter, consumer); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } total := int64(len(consumer.Result)) return consumer.Result, &usecasex.PageInfo{TotalCount: total}, nil } // Input validation if pagination.Page.Page <= 0 || pagination.Page.PageSize <= 0 { return nil, nil, rerror.ErrInvalidParameter } // Check for potential overflow if pagination.Page.Page > 1 && pagination.Page.PageSize > math.MaxInt64/(pagination.Page.Page-1) { return nil, nil, rerror.ErrInvalidParameter } skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize) total, err := client.Count(ctx, filter) if err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } opts := options.Find(). SetSkip(skip). SetLimit(limit) if err := client.Find(ctx, filter, consumer, opts); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } pageInfo := interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize) return consumer.Result, pageInfo.ToPageInfo(), nil }api/internal/infrastructure/mongo/job.go (1)
179-183:⚠️ Potential issueAdd protection against division by zero.
The page calculation
currentPage := int(skip/limit) + 1will panic if limit is zero.Add validation and safe calculation:
+ if limit <= 0 { + return nil, nil, rerror.ErrInternal("limit must be positive") + } currentPage := int(skip/limit) + 1 pageInfo := interfaces.NewPageBasedInfo(total, currentPage, int(limit))api/internal/infrastructure/mongo/project.go (1)
78-105:⚠️ Potential issueAdd input validation for pagination parameters.
The page-based pagination parameters should be validated to prevent potential issues.
Apply this diff to add validation:
if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be positive") + } + if pagination.Page.PageSize <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page size must be positive") + } // Page-based pagination skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize)api/internal/infrastructure/mongo/deployment.go (1)
148-167:⚠️ Potential issueAddress potential race condition in Update method.
The Update method is susceptible to lost updates in concurrent scenarios.
Consider implementing optimistic locking:
func (r *Deployment) Update(ctx context.Context, param interfaces.UpdateDeploymentParam) (*deployment.Deployment, error) { d, err := r.FindByID(ctx, param.ID) if err != nil { return nil, err } + + // Add optimistic locking using MongoDB's findAndModify + filter := bson.M{ + "id": d.ID().String(), + "version": d.Version(), + } + update := bson.M{ + "$set": bson.M{ + "description": param.Description, + "workflowUrl": param.Workflow.Path, + "version": d.Version() + 1, + }, + } + opts := options.FindOneAndUpdate().SetReturnDocument(options.After) + + var result mongodoc.Deployment + if err := r.client.Collection().FindOneAndUpdate(ctx, filter, update, opts).Decode(&result); err != nil { + if err == mongo.ErrNoDocuments { + return nil, rerror.NewE(rerror.ErrNotFound, "concurrent update detected") + } + return nil, rerror.ErrInternalByWithContext(ctx, err) + } - if param.Description != nil { - d.SetDescription(*param.Description) - } - if param.Workflow != nil { - d.SetWorkflowURL(param.Workflow.Path) - } - if err := r.Save(ctx, d); err != nil { - return nil, err - } return d, nil }api/internal/infrastructure/memory/deployment.go (1)
58-62: 🛠️ Refactor suggestionAdd validation for order direction.
The order direction should be validated to ensure it's either "ASC" or "DESC".
// Apply sorting direction := 1 // default ascending -if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 +if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC": + direction = 1 + case "DESC": + direction = -1 + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "orderDir must be either ASC or DESC") + } }api/internal/infrastructure/memory/deployment_test.go (2)
111-115:⚠️ Potential issueSet UpdatedAt field in test data to ensure correct sorting.
The test data doesn't set the UpdatedAt field, which is used for default sorting. This could cause inconsistent test results.
-d1 := deployment.New().NewID().Workspace(wsID).Version("v1").MustBuild() -d2 := deployment.New().NewID().Workspace(wsID).Version("v2").MustBuild() -d3 := deployment.New().NewID().Workspace(wsID).Version("v3").MustBuild() +now := time.Now() +d1 := deployment.New().NewID().Workspace(wsID).Version("v1").UpdatedAt(now.Add(-2 * time.Hour)).MustBuild() +d2 := deployment.New().NewID().Workspace(wsID).Version("v2").UpdatedAt(now.Add(-1 * time.Hour)).MustBuild() +d3 := deployment.New().NewID().Workspace(wsID).Version("v3").UpdatedAt(now).MustBuild()
117-235: 🛠️ Refactor suggestionAdd test cases for error scenarios and sorting behavior.
The test suite is missing cases for:
- Invalid pagination parameters
- Invalid order direction
- Custom sorting behavior
Add the following test cases to the
testsslice:{ name: "invalid page number", init: map[id.DeploymentID]*deployment.Deployment{ d1.ID(): d1, }, wsID: wsID, pagination: &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: -1, PageSize: 10, }, }, wantErr: true, }, { name: "invalid order direction", init: map[id.DeploymentID]*deployment.Deployment{ d1.ID(): d1, }, wsID: wsID, pagination: &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: 1, PageSize: 10, OrderDir: lo.ToPtr("INVALID"), }, }, wantErr: true, }, { name: "custom sorting by version ASC", init: map[id.DeploymentID]*deployment.Deployment{ d1.ID(): d1, d2.ID(): d2, d3.ID(): d3, }, wsID: wsID, pagination: &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: 1, PageSize: 10, OrderBy: lo.ToPtr("version"), OrderDir: lo.ToPtr("ASC"), }, }, want: []*deployment.Deployment{d1, d2, d3}, wantInfo: &usecasex.PageInfo{ TotalCount: 3, }, },
🧹 Nitpick comments (11)
api/internal/usecase/interactor/trigger.go (1)
50-56: Consider implementing pagination metadata.To improve the API's usability, consider adding pagination metadata to the response, such as:
- Total number of items
- Total number of pages
- Has next page
- Has previous page
Example implementation:
type PaginationMeta struct { TotalItems int `json:"totalItems"` TotalPages int `json:"totalPages"` HasNextPage bool `json:"hasNextPage"` HasPrevPage bool `json:"hasPrevPage"` CurrentPage int `json:"currentPage"` ItemsPerPage int `json:"itemsPerPage"` }api/internal/usecase/repo/deployment.go (1)
27-34: Consider adding input validation for batch size.The pagination parameter construction looks good, but consider validating the batch size to prevent issues with zero or negative values.
func IterateDeploymentsByWorkspace(repo Deployment, ctx context.Context, tid accountdomain.WorkspaceID, batch int64, callback func([]*deployment.Deployment) error) error { + if batch <= 0 { + return fmt.Errorf("batch size must be positive") + } page := 1api/internal/usecase/repo/project.go (2)
17-17: Consider consistent pagination types between input and output.The method signature mixes old and new pagination types:
- Input:
*interfaces.PaginationParam(new)- Output:
*usecasex.PageInfo(old)This inconsistency could lead to maintenance issues. Consider updating the return type to use a corresponding page info type from the new interfaces package.
25-32: Consider adding input validation and safety checks.The pagination setup could benefit from additional safety measures:
- Validate batch size to prevent zero or negative values
- Add a maximum page limit to prevent potential infinite loops
func IterateProjectsByWorkspace(repo Project, ctx context.Context, tid accountdomain.WorkspaceID, batch int64, callback func([]*project.Project) error) error { + if batch <= 0 { + return fmt.Errorf("batch size must be positive") + } + const maxPages = 1000 // adjust as needed page := 1 for { + if page > maxPages { + return fmt.Errorf("exceeded maximum page limit") + } pagination := &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{api/internal/infrastructure/memory/trigger.go (3)
44-45: Consider the trade-offs of slicing with a pre-allocated slice
While pre-allocation can be beneficial when dealing with large data sets, the improvement might be negligible in small or moderate scenarios. Ensure this is balanced with clearer code readability.
57-79: Handle unexpected or unsupported sorting fields
Currently, if an unsupported field is provided inOrderBy, the code falls back to sorting by ID. You may want to log or handle this fallback more explicitly to ensure future maintainers are aware of the behavior.
81-85: Returning all results when pagination is nil
Be mindful that returning the entire dataset without pagination can be expensive in production. Consider using a default page limit to prevent large data retrievals.api/internal/usecase/interfaces/pagination.go (1)
11-14: Clarify naming of PaginationParam
Consider renamingPaginationParam.Pageto something likePageDataorPageOptsif you plan to expand this struct in the future. It can improve clarity for readers.api/internal/infrastructure/mongo/deployment.go (1)
29-31: Document the adapter pattern implementation.The
DeploymentAdapterstruct's purpose and benefits should be documented.Add documentation to explain the adapter pattern:
+// DeploymentAdapter wraps the Deployment struct to implement the repo.Deployment interface, +// providing a clean separation between the MongoDB implementation and the repository interface. type DeploymentAdapter struct { *Deployment }api/e2e/gql_pagination_test.go (1)
83-140: Add edge case tests for pagination.The pagination tests should cover additional edge cases.
Add tests for:
- Invalid page numbers (0 or negative)
- Invalid page sizes (0 or negative)
- Page number beyond total pages
- Empty result set
Example test case:
// Test invalid pagination parameters t.Run("test_invalid_pagination", func(t *testing.T) { query := fmt.Sprintf(`{ projectsPage( workspaceId: "%s" pagination: { page: 0 pageSize: 2 } ) { nodes { id } } }`, wId1.String()) resp := executeGraphQLQuery(e, query, uId1.String()) assertErrorResponse(t, resp, "page must be positive") })api/internal/infrastructure/memory/deployment.go (1)
89-112: Add documentation for pagination logic.The pagination implementation would benefit from additional comments explaining the logic and edge cases.
+// Handle pagination +// If no pagination is requested, return all results if pagination == nil { return result, &usecasex.PageInfo{TotalCount: total}, nil } if pagination.Page != nil { - // Page-based pagination + // Page-based pagination: + // - Calculate skip based on page number and size + // - Handle out-of-range pages + // - Return subset of results with pagination info skip := (pagination.Page.Page - 1) * pagination.Page.PageSize if skip >= len(result) { return nil, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize).ToPageInfo(), nil }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
api/e2e/gql_pagination_test.go(1 hunks)api/gqlgen.yml(0 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)api/internal/adapter/gql/loader_asset.go(1 hunks)api/internal/adapter/gql/loader_deployment.go(1 hunks)api/internal/adapter/gql/loader_job.go(1 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/adapter/gql/resolver_query.go(4 hunks)api/internal/adapter/gql/resolver_workspace.go(1 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/memory/job.go(3 hunks)api/internal/infrastructure/memory/project.go(2 hunks)api/internal/infrastructure/memory/trigger.go(2 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)api/internal/infrastructure/mongo/job.go(4 hunks)api/internal/infrastructure/mongo/project.go(3 hunks)api/internal/infrastructure/mongo/trigger.go(3 hunks)api/internal/usecase/interactor/job.go(1 hunks)api/internal/usecase/interactor/project.go(1 hunks)api/internal/usecase/interactor/trigger.go(1 hunks)api/internal/usecase/interfaces/deployment.go(1 hunks)api/internal/usecase/interfaces/pagination.go(1 hunks)api/internal/usecase/repo/deployment.go(3 hunks)api/internal/usecase/repo/job.go(2 hunks)api/internal/usecase/repo/project.go(2 hunks)api/internal/usecase/repo/trigger.go(2 hunks)
💤 Files with no reviewable changes (1)
- api/gqlgen.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- api/internal/usecase/interfaces/deployment.go
- api/internal/adapter/gql/loader_trigger.go
- api/internal/adapter/gql/loader_project.go
- api/internal/adapter/gql/resolver_query.go
🔇 Additional comments (20)
api/internal/usecase/interactor/trigger.go (1)
50-56: Verify pagination implementation across the codebase.Let's verify the pagination implementation in related files to ensure consistency.
✅ Verification successful
Pagination implementation is consistent across the codebase
The pagination implementation in trigger.go follows the established pattern used throughout the codebase, with consistent parameter structures and handling in both memory and MongoDB repositories.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check pagination implementation across the codebase # Check for pagination parameter definitions echo "Checking pagination parameter definitions..." rg -A 5 "type.*Pagination.*struct" --type go # Check for FindByWorkspace implementations echo "Checking FindByWorkspace implementations..." rg -A 10 "func.*FindByWorkspace.*Pagination" --type go # Check for page-based pagination usage echo "Checking page-based pagination usage..." rg -A 5 "PageBasedPagination" --type go # Check GraphQL schema for pagination echo "Checking GraphQL schema for pagination..." rg -A 5 "type.*Pagination" --type graphqlLength of output: 54966
api/internal/usecase/repo/deployment.go (2)
6-6: LGTM!The new import is correctly added to support the pagination interface changes.
17-17: LGTM with verification needed.The method signature change aligns with the PR objective to implement page-based pagination.
Let's verify if other pagination-related methods in the implementations need similar updates:
✅ Verification successful
Interface change is correctly implemented across repositories
The change from
*usecasex.Paginationto*interfaces.PaginationParamis consistently implemented in all repositories that follow this interface. The Asset repository uses a different signature by design, as it requires additional filtering capabilities through itsAssetFilterstructure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other pagination-related methods in implementations rg --type go -A 5 'func.*Pagination|func.*FindByWorkspace' api/internal/infrastructure/Length of output: 9353
Script:
#!/bin/bash # Check Asset repository interface and related types rg --type go -A 5 'type Asset.*interface|type AssetFilter' api/internal/usecase/repo/Length of output: 1001
api/internal/usecase/repo/project.go (3)
6-6: LGTM! Import added for new pagination interface.
17-17: Verify consistent pagination implementation across the codebase.Since this is part of a broader transition from cursor-based to page-based pagination, let's verify the consistency of the implementation across related components.
Also applies to: 27-32
✅ Verification successful
Pagination implementation is consistent across the codebase
The verification confirms that the transition to page-based pagination is complete and consistent. The remaining cursor-based pagination references are only present in test files and common interface definitions, which is expected and correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for remaining cursor-based pagination usage echo "Checking for remaining cursor-based pagination..." rg -l "CursorPagination|HasNextPage|AfterID|BeforeID" --type go # Search for new page-based pagination implementation echo "Checking page-based pagination implementation..." rg -l "PageBasedPaginationParam|PaginationParam" --type go # Check GraphQL schema for pagination types echo "Checking GraphQL schema..." fd -e graphql | xargs rg "type.*Pagination|input.*Pagination"Length of output: 1791
Script:
#!/bin/bash # Check context of cursor-based pagination usage echo "Checking cursor-based pagination usage context..." rg "CursorPagination|HasNextPage|AfterID|BeforeID" --type go -C 3 # Check if cursor pagination is used in production routes/handlers echo "Checking production usage..." rg "CursorPagination|HasNextPage|AfterID|BeforeID" api/internal/adapter/gql/resolver/ api/internal/adapter/http/ --type goLength of output: 12444
46-46: Guard against integer overflow in pagination calculation.The break condition
info.TotalCount <= int64(page*int(batch))could overflow for large page numbers or batch sizes.Consider adding overflow checks:
- if info.TotalCount <= int64(page*int(batch)) { + // Check for potential overflow + if page > math.MaxInt/int(batch) { + return fmt.Errorf("pagination calculation would overflow") + } + if info.TotalCount <= int64(page*int(batch)) {api/internal/infrastructure/memory/trigger.go (3)
5-8: Use of additional imports is appropriate
Thesortpackage is used for sorting results, and thesyncpackage is already present for concurrency control. This setup is consistent with typical in-memory data handling.
36-36: Method signature aligns with new pagination approach
Changing the function signature to accept*interfaces.PaginationParamis consistent with the new page-based pagination paradigm across the codebase.
52-54: Gracefully returning on empty result
Short-circuiting for an empty result set makes sense and provides quick feedback. This is well-handled.api/internal/usecase/repo/trigger.go (2)
6-6: Import for interfaces package is consistent
Including the newinterfacesimport aligns with the code changes for page-based pagination.
17-17: Verify all usage points of the updated signature
Ensure every caller ofFindByWorkspacenow passes the correct*interfaces.PaginationParam.✅ Verification successful
All usages of
FindByWorkspaceare consistent with the updated signature
All repository implementations correctly use*interfaces.PaginationParam, and callers properly convert their pagination types. The codebase shows consistent implementation across all components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find references to `FindByWorkspace` in the codebase, verifying they include the new signature. rg -A 3 'FindByWorkspace\('Length of output: 20922
api/internal/usecase/repo/job.go (1)
17-17: Breaking change: Job interface method signature updated.The
FindByWorkspacemethod signature has been updated to use*interfaces.PaginationParam. This is a breaking change that requires updates to all implementations of theJobinterface.Let's verify all implementations have been updated:
✅ Verification successful
All implementations are up-to-date with the interface change ✅
All implementations of the
FindByWorkspacemethod in both MongoDB and in-memory repositories have been properly updated to use*interfaces.PaginationParamparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all Job interface implementations ast-grep --pattern 'type $_ struct { $$$ } func ($_ *$_) FindByWorkspace(ctx context.Context, $_, $_ $$$) ($$$) { $$$ }'Length of output: 127
Script:
#!/bin/bash # Try a simpler pattern to find FindByWorkspace implementations echo "=== Using ast-grep with simpler pattern ===" ast-grep --pattern 'func ($_ *$_) FindByWorkspace' echo -e "\n=== Using ripgrep to find FindByWorkspace implementations ===" rg "func.*FindByWorkspace" -A 1Length of output: 6937
api/internal/adapter/gql/loader_asset.go (3)
37-37: Method signature updated to use page-based pagination.The change from
*gqlmodel.Paginationto*gqlmodel.PageBasedPaginationaligns with the new pagination strategy.
48-50: LGTM: Simplified node construction.The direct node construction without intermediate edges improves code readability and performance.
43-43: Verify error handling for pagination conversion.The
ToPageBasedPaginationconversion might need error handling for invalid pagination parameters.✅ Verification successful
The pagination conversion is appropriately implemented
The current implementation is safe as GraphQL schema ensures type safety for the pagination parameters, and the conversion calculation is mathematically sound for all possible integer inputs. Additional error handling isn't necessary as invalid values will be handled gracefully by the database layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check implementation of ToPageBasedPagination ast-grep --pattern 'func ToPageBasedPagination($_) $_ { $$$ }'Length of output: 712
Script:
#!/bin/bash # Check PageBasedPagination struct and schema definition ast-grep --pattern 'type PageBasedPagination struct { $$$ }' # Also check the GraphQL schema fd -e graphql -x cat {}Length of output: 16132
api/internal/usecase/interactor/job.go (1)
62-66: LGTM: Proper workspace access check.The workspace access check before processing pagination is correctly implemented.
api/internal/adapter/gql/gqlmodel/models_gen.go (1)
214-219: LGTM! The pagination models are well-structured.The generated pagination models include all necessary fields for both page-based and cursor-based pagination.
Also applies to: 222-224, 228-231
api/e2e/gql_pagination_test.go (1)
142-193: Add descending order test case.The sorting test only covers ascending order.
Add a test case for descending order to ensure bi-directional sorting works correctly.
// Test descending order query = fmt.Sprintf(`{ projectsPage( workspaceId: "%s" pagination: { page: 1 pageSize: 5 orderBy: "name" orderDir: DESC } ) { nodes { id name } } }`, wId1.String()) // ... execute query and verify results are in descending orderapi/internal/infrastructure/memory/deployment.go (2)
64-87: Refactor sorting logic for better maintainability.The current sorting implementation is complex and uses magic strings. Consider extracting constants and simplifying the logic.
+const ( + OrderFieldVersion = "version" + OrderFieldUpdatedAt = "updatedAt" +) + +func (r *Deployment) compareDeployments(d1, d2 *deployment.Deployment, orderField *string, direction int) bool { + if orderField != nil && *orderField == OrderFieldVersion { + v1, v2 := d1.Version(), d2.Version() + if v1 != vj { + return (v1 < v2) == (direction == 1) + } + } + t1, t2 := d1.UpdatedAt(), d2.UpdatedAt() + if !t1.Equal(t2) { + return (t1.Before(t2)) == (direction == 1) + } + return d1.ID().String() < d2.ID().String() +} -sort.Slice(result, func(i, j int) bool { - if pagination != nil && pagination.Page != nil && pagination.Page.OrderBy != nil && *pagination.Page.OrderBy == "version" { - // Compare versions first - vi, vj := result[i].Version(), result[j].Version() - if vi != vj { - if direction == 1 { - return vi < vj - } - return vi > vj - } - // If versions are equal, use ID as secondary sort key for stability - return result[i].ID().String() < result[j].ID().String() - } - // Compare updatedAt first - ti, tj := result[i].UpdatedAt(), result[j].UpdatedAt() - if !ti.Equal(tj) { - if direction == 1 { - return ti.Before(tj) - } - return ti.After(tj) - } - // If timestamps are equal, use ID as secondary sort key for stability - return result[i].ID().String() < result[j].ID().String() -}) +sort.Slice(result, func(i, j int) bool { + return r.compareDeployments(result[i], result[j], pagination.Page.OrderBy, direction) +})
37-46: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The pagination parameters should be validated to prevent potential issues with negative values or excessive page sizes.
Add validation before processing the parameters:
func (r *Deployment) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *usecasex.PageInfo, error) { r.lock.Lock() defer r.lock.Unlock() if !r.f.CanRead(id) { return nil, nil, nil } + if pagination != nil && pagination.Page != nil { + if pagination.Page.Page < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be greater than 0") + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize must be greater than 0") + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } + } + // Pre-allocate slice with estimated capacity result := make([]*deployment.Deployment, 0, len(r.data))Likely invalid or redundant comment.
This commit updates the Project repository interface to use the new PaginationParam interface for page-based pagination, aligning with the ongoing pagination refactoring effort.
kasugamirai
force-pushed
the
feat/pagination
branch
from
bc5fe33 to
7ac4fec
Compare
This commit refines the FindByWorkspace method in the memory deployment repository by: - Removing pre-allocation of result slice - Simplifying pagination handling - Directly constructing PageInfo with more precise next/previous page flags - Removing unnecessary comments and intermediate variables
This commit removes a cursor-based pagination test case from the memory deployment repository test, continuing the ongoing pagination refactoring effort to simplify pagination logic.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (13)
api/internal/adapter/gql/loader_trigger.go (1)
41-48: 🛠️ Refactor suggestionConsider accepting pagination as a pointer and add parameter validation.
- Change the pagination parameter to a pointer to distinguish between "not provided" and "provided with defaults"
- Add validation for pagination parameters before conversion
Apply this diff:
-func (c *TriggerLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.TriggerConnection, error) { +func (c *TriggerLoader) FindByWorkspacePage(ctx context.Context, wsID gqlmodel.ID, pagination *gqlmodel.PageBasedPagination) (*gqlmodel.TriggerConnection, error) { tid, err := gqlmodel.ToID[accountdomain.Workspace](wsID) if err != nil { return nil, err } + if pagination != nil { + if pagination.Page < 1 { + return nil, fmt.Errorf("page must be greater than 0") + } + if pagination.PageSize < 1 { + return nil, fmt.Errorf("page size must be greater than 0") + } + } + paginationParam := gqlmodel.ToPageBasedPagination(pagination)api/internal/adapter/gql/resolver_workspace.go (3)
19-28:⚠️ Potential issueReturn all assets when pagination is not provided.
The
Assetsmethod currently returnsnil, nilwhen pagination is not provided. This is inconsistent with typical GraphQL behavior where all items should be returned when pagination is not specified.Apply this diff to fix the issue:
func (r *workspaceResolver) Assets(ctx context.Context, obj *gqlmodel.Workspace, pagination *gqlmodel.Pagination) (*gqlmodel.AssetConnection, error) { + if pagination == nil { + return loaders(ctx).Asset.FindByWorkspace(ctx, obj.ID, nil, nil, &gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) + } + if pagination != nil && pagination.Page != nil && pagination.PageSize != nil { return loaders(ctx).Asset.FindByWorkspace(ctx, obj.ID, nil, nil, &gqlmodel.PageBasedPagination{ Page: *pagination.Page, PageSize: *pagination.PageSize, OrderBy: pagination.OrderBy, OrderDir: pagination.OrderDir, }) } - return nil, nil + return loaders(ctx).Asset.FindByWorkspace(ctx, obj.ID, nil, nil, &gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) }The
AssetsPagemethod looks good.Also applies to: 31-33
35-45:⚠️ Potential issueReturn all projects when pagination is not provided.
The
Projectsmethod currently returnsnil, nilwhen pagination is not provided. This is inconsistent with typical GraphQL behavior where all items should be returned when pagination is not specified.Apply this diff to fix the issue:
func (r *workspaceResolver) Projects(ctx context.Context, obj *gqlmodel.Workspace, includeArchived *bool, pagination *gqlmodel.Pagination) (*gqlmodel.ProjectConnection, error) { + if pagination == nil { + return loaders(ctx).Project.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) + } + if pagination != nil && pagination.Page != nil && pagination.PageSize != nil { return loaders(ctx).Project.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ Page: *pagination.Page, PageSize: *pagination.PageSize, OrderBy: pagination.OrderBy, OrderDir: pagination.OrderDir, }) } - return nil, nil + return loaders(ctx).Project.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) }The
ProjectsPagemethod looks good.Also applies to: 47-48
52-61:⚠️ Potential issueReturn all deployments when pagination is not provided.
The
Deploymentsmethod currently returnsnil, nilwhen pagination is not provided. This is inconsistent with typical GraphQL behavior where all items should be returned when pagination is not specified.Apply this diff to fix the issue:
func (r *workspaceResolver) Deployments(ctx context.Context, obj *gqlmodel.Workspace, includeArchived *bool, pagination *gqlmodel.Pagination) (*gqlmodel.DeploymentConnection, error) { + if pagination == nil { + return loaders(ctx).Deployment.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) + } + if pagination != nil && pagination.Page != nil && pagination.PageSize != nil { return loaders(ctx).Deployment.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ Page: *pagination.Page, PageSize: *pagination.PageSize, OrderBy: pagination.OrderBy, OrderDir: pagination.OrderDir, }) } - return nil, nil + return loaders(ctx).Deployment.FindByWorkspacePage(ctx, obj.ID, gqlmodel.PageBasedPagination{ + Page: 1, + PageSize: 100, // default page size + }) }The
DeploymentsPagemethod looks good.Also applies to: 63-64
api/internal/infrastructure/mongo/trigger.go (1)
71-94:⚠️ Potential issueAdd input validation and extract common pagination logic.
The page-based pagination implementation needs improvements:
- Missing validation for page and page size
- Potential integer overflow in skip calculation
- Common pagination logic could be extracted
Apply this diff to add input validation:
if pagination != nil && pagination.Page != nil { // Page-based pagination + // Validate pagination parameters + if pagination.Page.Page <= 0 { + return nil, nil, interfaces.ErrInvalidPagination + } + if pagination.Page.PageSize <= 0 { + return nil, nil, interfaces.ErrInvalidPagination + } + + // Use safe multiplication to prevent integer overflow + if pagination.Page.Page > 1 && pagination.Page.PageSize > math.MaxInt64/(pagination.Page.Page-1) { + return nil, nil, interfaces.ErrInvalidPagination + } + skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize)Extract common pagination logic to a shared utility function:
// mongox/pagination.go func HandlePageBasedPagination[T any]( ctx context.Context, client *mongox.ClientCollection, filter bson.M, pagination *interfaces.PaginationParam, consumer mongox.Consumer[T], ) ([]*T, *usecasex.PageInfo, error) { if pagination == nil || pagination.Page == nil { if err := client.Find(ctx, filter, consumer); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } total := int64(len(consumer.Result)) return consumer.Result, &usecasex.PageInfo{TotalCount: total}, nil } // Validate pagination parameters if pagination.Page.Page <= 0 { return nil, nil, interfaces.ErrInvalidPagination } if pagination.Page.PageSize <= 0 { return nil, nil, interfaces.ErrInvalidPagination } // Use safe multiplication to prevent integer overflow if pagination.Page.Page > 1 && pagination.Page.PageSize > math.MaxInt64/(pagination.Page.Page-1) { return nil, nil, interfaces.ErrInvalidPagination } skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize) // Get total count for page info total, err := client.Count(ctx, filter) if err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } // Execute find with skip and limit opts := options.Find(). SetSkip(skip). SetLimit(limit) if err := client.Find(ctx, filter, consumer, opts); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } // Create page-based info pageInfo := interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize) return consumer.Result, pageInfo.ToPageInfo(), nil }api/internal/infrastructure/memory/asset.go (1)
126-132:⚠️ Potential issueAdd input validation for page-based pagination.
The page-based pagination implementation needs validation for the input parameters.
if pagination.Page != nil { + if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page and pageSize must be positive") + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSizeapi/internal/infrastructure/memory/project.go (2)
58-62:⚠️ Potential issueAdd validation for order direction.
The order direction should be validated to ensure it's either "ASC" or "DESC".
-if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 -} +if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC", "DESC": + if *pagination.Page.OrderDir == "DESC" { + direction = -1 + } + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "orderDir must be either ASC or DESC") + } +}
95-99:⚠️ Potential issueAdd input validation for pagination parameters.
The page-based pagination implementation needs validation for the input parameters.
if pagination.Page != nil { + if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page and pageSize must be positive") + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSizeapi/internal/adapter/gql/loader_deployment.go (1)
69-70:⚠️ Potential issueFix incorrect total count in DeploymentConnection.
The
TotalCountis incorrectly set tolen(res)which only represents the current page size, not the total number of items.return &gqlmodel.DeploymentConnection{ Nodes: nodes, PageInfo: gqlmodel.ToPageInfo(pageInfo), - TotalCount: len(res), + TotalCount: int(pageInfo.TotalCount), }, nilapi/internal/infrastructure/mongo/asset.go (2)
139-142:⚠️ Potential issueAdd input validation for pagination parameters.
The offset-based pagination implementation should validate the input parameters.
if pagination != nil && pagination.Offset != nil { + if pagination.Offset.Limit <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "limit must be positive") + } + if pagination.Offset.Offset < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "offset must be non-negative") + } skip := pagination.Offset.Offset limit := pagination.Offset.Limit
160-161:⚠️ Potential issueHandle edge cases in page calculation.
The current page calculation needs protection against integer overflow and division by zero.
- currentPage := int(skip/limit) + 1 - pageInfo := interfaces.NewPageBasedInfo(total, currentPage, int(limit)) + currentPage := 1 + if limit > 0 { + // Prevent integer overflow + currentPage = int(skip/limit) + if currentPage > interfaces.MaxPageNumber { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page number exceeds maximum allowed") + } + currentPage++ // Add 1 since pages are 1-based + } + pageInfo := interfaces.NewPageBasedInfo(total, currentPage, int(limit))api/internal/infrastructure/mongo/deployment.go (1)
208-249:⚠️ Potential issueFix the unkeyed struct literals and add input validation.
The pagination implementation has the same issues as in project.go:
- The sort operations use unkeyed struct literals which could cause issues.
- Missing input validation for pagination parameters.
Apply the same fixes as suggested for project.go.
api/e2e/gql_pagination_test.go (1)
73-75: 🛠️ Refactor suggestionReplace time.Sleep with deterministic approach.
Using
time.Sleepfor test synchronization can make tests flaky.Replace the sleep with a deterministic approach that verifies project creation before proceeding.
🧹 Nitpick comments (6)
api/internal/adapter/gql/gqlmodel/models_gen.go (1)
222-224: Consider adding validation for pagination metadata.While the PageInfo structure looks good, consider adding validation to ensure:
- TotalCount is non-negative
- CurrentPage is within bounds (1 to TotalPages)
- TotalPages is calculated correctly based on TotalCount and PageSize
api/internal/infrastructure/memory/trigger.go (2)
36-45: Check concurrency correctness and capacity usage.
Lock usage ensures thread safety when accessing the shared map. Pre-allocating the slice capacity tolen(r.data)can improve efficiency, but be mindful that if the size of the result is consistently smaller thanr.data, most of that capacity may remain unused.
57-79: Robust sorting mechanism.
Sorting based oncreatedAtor ID is correct. Consider documenting or extending this feature to handle additional fields if needed.api/internal/adapter/gql/resolver_query.go (1)
130-131: Remove or utilize the unused parameter.The
includeArchivedparameter is not being used in the function call.Either remove the unused parameter or pass it to the loader:
-func (r *queryResolver) ProjectsPage(ctx context.Context, workspaceID gqlmodel.ID, includeArchived *bool, pagination gqlmodel.PageBasedPagination) (*gqlmodel.ProjectConnection, error) { - return loaders(ctx).Project.FindByWorkspacePage(ctx, workspaceID, pagination) +func (r *queryResolver) ProjectsPage(ctx context.Context, workspaceID gqlmodel.ID, pagination gqlmodel.PageBasedPagination) (*gqlmodel.ProjectConnection, error) { + return loaders(ctx).Project.FindByWorkspacePage(ctx, workspaceID, pagination)Or utilize it:
func (r *queryResolver) ProjectsPage(ctx context.Context, workspaceID gqlmodel.ID, includeArchived *bool, pagination gqlmodel.PageBasedPagination) (*gqlmodel.ProjectConnection, error) { - return loaders(ctx).Project.FindByWorkspacePage(ctx, workspaceID, pagination) + return loaders(ctx).Project.FindByWorkspacePage(ctx, workspaceID, pagination, includeArchived) }api/internal/infrastructure/mongo/deployment_test.go (1)
75-91: Add test cases for error scenarios.The test suite should include cases for invalid pagination parameters and error handling.
Add these test cases:
// Test invalid page number pagination := &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: -1, PageSize: 10, }, } got, pageInfo, err := r.FindByWorkspace(ctx, wid, pagination) assert.Error(t, err) assert.Nil(t, got) assert.Nil(t, pageInfo) // Test invalid page size pagination = &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: 1, PageSize: 0, }, } got, pageInfo, err = r.FindByWorkspace(ctx, wid, pagination) assert.Error(t, err) assert.Nil(t, got) assert.Nil(t, pageInfo)api/internal/infrastructure/memory/deployment_test.go (1)
188-207: Fix misleading test case name.The test case is labeled as "cursor based pagination" but it's actually using page-based pagination parameters. This could be confusing for developers maintaining the tests.
Apply this diff to fix the test case name:
- name: "cursor based pagination", + name: "page based pagination: first page (duplicate)",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
api/e2e/gql_pagination_test.go(1 hunks)api/e2e/gql_project_test.go(3 hunks)api/gql/_shared.graphql(1 hunks)api/gql/asset.graphql(1 hunks)api/gql/deployment.graphql(1 hunks)api/gql/job.graphql(1 hunks)api/gql/project.graphql(1 hunks)api/gql/trigger.graphql(1 hunks)api/gql/workspace.graphql(1 hunks)api/gqlgen.yml(0 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/adapter/gql/gqlmodel/models_gen.go(7 hunks)api/internal/adapter/gql/loader_asset.go(1 hunks)api/internal/adapter/gql/loader_deployment.go(1 hunks)api/internal/adapter/gql/loader_job.go(1 hunks)api/internal/adapter/gql/loader_project.go(1 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/adapter/gql/resolver_query.go(4 hunks)api/internal/adapter/gql/resolver_workspace.go(1 hunks)api/internal/infrastructure/memory/asset.go(2 hunks)api/internal/infrastructure/memory/deployment.go(3 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/memory/job.go(3 hunks)api/internal/infrastructure/memory/project.go(2 hunks)api/internal/infrastructure/memory/trigger.go(2 hunks)api/internal/infrastructure/mongo/asset.go(3 hunks)api/internal/infrastructure/mongo/container.go(1 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)api/internal/infrastructure/mongo/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/job.go(4 hunks)api/internal/infrastructure/mongo/mongodoc/deployment.go(3 hunks)api/internal/infrastructure/mongo/project.go(3 hunks)api/internal/infrastructure/mongo/trigger.go(3 hunks)api/internal/usecase/interactor/deployment.go(1 hunks)api/internal/usecase/interactor/job.go(1 hunks)api/internal/usecase/interactor/project.go(1 hunks)api/internal/usecase/interactor/trigger.go(1 hunks)api/internal/usecase/interfaces/deployment.go(1 hunks)api/internal/usecase/interfaces/pageinfo.go(1 hunks)api/internal/usecase/interfaces/pagination.go(1 hunks)api/internal/usecase/repo/deployment.go(3 hunks)api/internal/usecase/repo/job.go(2 hunks)api/internal/usecase/repo/project.go(3 hunks)api/internal/usecase/repo/trigger.go(2 hunks)
💤 Files with no reviewable changes (1)
- api/gqlgen.yml
🚧 Files skipped from review as they are similar to previous changes (15)
- api/internal/usecase/interfaces/deployment.go
- api/gql/job.graphql
- api/internal/usecase/interfaces/pagination.go
- api/internal/usecase/repo/trigger.go
- api/gql/workspace.graphql
- api/gql/trigger.graphql
- api/internal/usecase/interactor/job.go
- api/internal/usecase/interactor/deployment.go
- api/internal/infrastructure/mongo/container.go
- api/internal/usecase/interactor/project.go
- api/gql/project.graphql
- api/internal/usecase/repo/project.go
- api/gql/asset.graphql
- api/gql/deployment.graphql
- api/gql/_shared.graphql
🧰 Additional context used
🪛 GitHub Actions: ci
api/e2e/gql_pagination_test.go
[error] 136-139: TestProjectsPagination/test_page_based_pagination failed: Expected array to have 2 items but has 0, and total count mismatch (expected: 5, got: 0)
[error] 296-309: TestJobsPagination failed: HTTP status 422 Unprocessable Entity instead of expected 200 OK
[error] 755-776: TestTriggersPagination failed: HTTP status 422 Unprocessable Entity instead of expected 200 OK
api/internal/adapter/gql/gqlmodel/convert.go
[error] 81-81: Function 'intToInt64' is unused
api/e2e/gql_project_test.go
[error] 278-309: TestListProjects failed: HTTP status 422 Unprocessable Entity instead of expected 200 OK
🔇 Additional comments (36)
api/internal/adapter/gql/gqlmodel/models_gen.go (4)
49-51: LGTM! Connection types are properly updated for page-based pagination.The connection types now include the necessary fields for page-based pagination: nodes, pageInfo, and totalCount.
Also applies to: 152-154, 191-193, 267-269, 353-355
214-219: LGTM! PageBasedPagination type is well-structured.The type includes all necessary fields for implementing page-based pagination with sorting capabilities:
- page and pageSize for pagination
- orderBy and orderDir for sorting
624-663: LGTM! OrderDirection enum is well-implemented.The OrderDirection enum implementation is complete with:
- Clear constants (ASC/DESC)
- Proper validation
- GraphQL marshaling/unmarshaling
- String conversion
228-231: Verify the optional nature of pagination fields.The Pagination type makes all fields optional. While this provides flexibility, ensure that your resolvers handle cases where these fields are nil appropriately to prevent runtime panics.
✅ Verification successful
Optional pagination fields are properly handled
The codebase demonstrates proper handling of nil pagination fields through:
- Explicit nil checks in resolvers before accessing fields
- Safe conversion functions that handle nil cases
- Consistent patterns across different resource types (assets, projects, deployments, etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for resolver implementations handling these pagination fields ast-grep --pattern 'func $_(pagination *gqlmodel.Pagination) { $$$ }'Length of output: 73
Script:
#!/bin/bash # Search for files containing Pagination type references rg -l "Pagination" --type go # Search for functions accepting Pagination parameter with context rg -A 5 "func.*\*?Pagination\b.*\{" --type go # Search for nil checks on pagination fields rg "pagination\.(Page|PageSize|OrderBy|OrderDir)\s*==\s*nil" --type goLength of output: 19657
api/internal/infrastructure/memory/trigger.go (6)
5-5: Importingsortlooks appropriate.
This import is needed for the custom sorting logic below and appears to be properly utilized.
8-8: New import for pagination parameters.
Includinginterfacesis essential for the new page-based pagination.
52-54: Graceful handling of empty results.
Whentotal == 0, returning an empty slice and totalCount=0 is consistent with standard pagination behavior.
81-85: Fallback for no pagination.
Whenpaginationisnil, returning the full set of results along withtotalCountis acceptable. No concerns here.
106-107: Return remaining results for undefined pagination flow.
This fallback ensures a well-defined behavior even if no pagination parameters are provided.
86-105: 🛠️ Refactor suggestionValidate page values to prevent edge cases.
Ifpagination.Page.Pageorpagination.Page.PageSizeare zero or negative, slicing could behave unexpectedly. Adding validation or defaults helps avoid out-of-range errors.if pagination.Page.Page <= 0 { + pagination.Page.Page = 1 } if pagination.Page.PageSize <= 0 { + pagination.Page.PageSize = 10 }Likely invalid or redundant comment.
api/internal/usecase/interfaces/pageinfo.go (3)
5-10: IntroducingPageBasedInfo.
EmbeddingPageInfowhile addingCurrentPageandTotalPagesis a clear approach to handle page-based metadata.
28-33: Safe conversion toPageInfo.
Returningnilif the struct isnilprevents possible nil-pointer dereferences. This method is straightforward and well implemented.
12-26: 🛠️ Refactor suggestionConsider validating constructor parameters.
IfpageSizeis zero or negative, or ifcurrentPageis less than 1, calculations might behave unexpectedly. Add sanity checks or defaults for safer usage.func NewPageBasedInfo(totalCount int64, currentPage, pageSize int) *PageBasedInfo { + if pageSize <= 0 { + pageSize = 10 + } + if currentPage < 1 { + currentPage = 1 + } totalPages := (int(totalCount) + pageSize - 1) / pageSize ... }Likely invalid or redundant comment.
api/internal/usecase/repo/job.go (5)
6-6: Addedinterfacesimport for new pagination logic.
Including this import is necessary to handle the new page-based pagination structure.
17-17: Method signature updated for page-based pagination.
Replacing*usecasex.Paginationwith*interfaces.PaginationParamaligns the repository interface with the new pagination approach.
23-31: Setting up page-based pagination parameters.
Be aware of potential invalid inputs (e.g., negativebatch). Consider validation or defaulting to a safe page size whenbatch <= 0.
44-44: Potential off-by-one error in pagination break condition.
Ifinfo.TotalCount == int64(page*int(batch)), the loop breaks, possibly missing the final page if items match the boundary.-if info.TotalCount <= int64(page*int(batch)) { +if info.TotalCount < int64(page*int(batch)) { break }
48-48: Incrementing page index.
Automatically moving to the next page is correct, but ensurepagedoesn’t overflow ifbatchor the total count is extremely large. This is unlikely in practice, but worth noting in high-volume scenarios.api/internal/usecase/repo/deployment.go (1)
48-52:⚠️ Potential issueGuard against integer overflow in pagination calculation.
The multiplication of
pageandbatchcould potentially overflow for large values.Apply this diff to add overflow protection:
- if info.TotalCount <= int64(page*int(batch)) { + nextOffset := int64(page) * batch + if nextOffset < 0 || info.TotalCount <= nextOffset { break } page++Likely invalid or redundant comment.
api/internal/adapter/gql/gqlmodel/convert.go (3)
64-71:⚠️ Potential issueAdd input validation and overflow protection.
Similar to ToPagination, this function needs input validation and overflow protection.
Apply this diff:
func ToPageBasedPagination(pagination PageBasedPagination) *usecasex.Pagination { + // Validate input + if pagination.Page <= 0 || pagination.PageSize <= 0 { + return nil + } + + // Check for potential overflow + offset := int64(pagination.Page-1) * int64(pagination.PageSize) + if offset < 0 { + return nil + } return &usecasex.Pagination{ Offset: &usecasex.OffsetPagination{ - Offset: int64((pagination.Page - 1) * pagination.PageSize), + Offset: offset, Limit: int64(pagination.PageSize), }, } }Likely invalid or redundant comment.
51-62:⚠️ Potential issueAdd input validation for pagination parameters.
The function should validate the input parameters before creating the pagination param.
Apply this diff:
if pagination.Page != nil && pagination.PageSize != nil { + // Validate input + if *pagination.Page <= 0 || *pagination.PageSize <= 0 { + return nil + } + + // Check for potential overflow + offset := int64((*pagination.Page - 1)) * int64(*pagination.PageSize) + if offset < 0 { + return nil + } return &usecasex.Pagination{ Offset: &usecasex.OffsetPagination{ - Offset: int64((*pagination.Page - 1) * *pagination.PageSize), + Offset: offset, Limit: int64(*pagination.PageSize), }, } }Likely invalid or redundant comment.
32-37:⚠️ Potential issueUse safe type assertion to prevent panics.
The type assertion could panic if
pis not nil but the type assertion fails.Apply this diff:
- if pbi, ok := any(p).(*interfaces.PageBasedInfo); ok { + pbi, ok := any(p).(*interfaces.PageBasedInfo) + if p != nil && ok { cp := pbi.CurrentPage tp := pbi.TotalPages currentPage = &cp totalPages = &tp }Likely invalid or redundant comment.
api/internal/infrastructure/mongo/mongodoc/deployment.go (1)
33-41: LGTM! Improved error handling.The changes enhance error handling by adding proper nil checks and using consistent error types.
api/internal/adapter/gql/loader_asset.go (1)
Line range hint
37-57: LGTM! Clean pagination implementation.The changes correctly implement page-based pagination while simplifying the node construction logic.
api/internal/adapter/gql/loader_project.go (1)
Line range hint
41-63: LGTM! Clean implementation of page-based pagination.The function correctly implements page-based pagination by:
- Converting workspace ID using
gqlmodel.ToID- Converting pagination parameters using
gqlmodel.ToPageBasedPagination- Returning a well-structured connection object
api/internal/adapter/gql/loader_job.go (1)
Line range hint
58-81: LGTM! Clean implementation of page-based pagination.The function correctly implements page-based pagination by:
- Converting workspace ID using
gqlmodel.ToID- Converting pagination parameters using
gqlmodel.ToPageBasedPagination- Proper error handling
- Well-structured response construction
api/internal/infrastructure/memory/asset.go (1)
83-91: 🛠️ Refactor suggestionOptimize cursor lookup with a map for O(1) complexity.
The current implementation uses linear search to find the cursor position. Use a map for better performance.
if filter.Pagination.Cursor.After != nil { afterID := string(*filter.Pagination.Cursor.After) + // Create a map for O(1) lookup + posMap := make(map[string]int64) + for i, d := range result { + posMap[d.ID().String()] = int64(i) + } + if pos, ok := posMap[afterID]; ok { + start = pos + 1 + } else { + return nil, nil, fmt.Errorf("invalid cursor: %s", afterID) + } - for i, d := range result { - if d.ID().String() == afterID { - start = int64(i + 1) - break - } - } }Likely invalid or redundant comment.
api/internal/infrastructure/mongo/job.go (2)
179-181:⚠️ Potential issuePrevent division by zero in page calculation.
The currentPage calculation will panic if limit is zero. Add a safeguard.
- currentPage := int(skip/limit) + 1 + // Ensure limit is not zero before division + if limit == 0 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("limit cannot be zero")) + } + currentPage := int(skip/limit) + 1 pageInfo := interfaces.NewPageBasedInfo(total, currentPage, int(limit))Likely invalid or redundant comment.
159-163: 🛠️ Refactor suggestionAdd input validation for offset-based pagination.
The offset and limit parameters should be validated to prevent potential issues.
if pagination != nil && pagination.Offset != nil { + // Validate pagination parameters + if pagination.Offset.Offset < 0 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("offset cannot be negative")) + } + if pagination.Offset.Limit < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("limit must be positive")) + } + if pagination.Offset.Limit > 100 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("limit cannot exceed 100")) + } + // Page-based pagination skip := pagination.Offset.Offset limit := pagination.Offset.LimitLikely invalid or redundant comment.
api/internal/infrastructure/memory/deployment.go (1)
57-61: 🛠️ Refactor suggestionAdd validation for order direction.
The order direction should be validated to ensure it's either "ASC" or "DESC".
-if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - direction = -1 -} +if pagination != nil && pagination.Page != nil && pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC", "DESC": + if *pagination.Page.OrderDir == "DESC" { + direction = -1 + } + default: + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("orderDir must be either ASC or DESC")) + } +}Likely invalid or redundant comment.
api/internal/infrastructure/mongo/project.go (3)
70-76: LGTM!The method signature has been updated to use the new pagination parameter type and correctly delegates to the
paginatemethod.
Line range hint
177-187: LGTM!The error type has been updated to use
interfaces.ErrOperationDenied, which is consistent with the codebase's error handling patterns.
Line range hint
208-260: Fix the unkeyed struct literals and add input validation.
- The sort operations use unkeyed struct literals which could cause issues.
- Missing input validation for pagination parameters.
Apply this diff to fix the issues:
func (r *Deployment) paginate(ctx context.Context, filter bson.M, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *usecasex.PageInfo, error) { c := mongodoc.NewDeploymentConsumer(r.f.Readable) if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be positive") + } + if pagination.Page.PageSize <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page size must be positive") + } + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSize limit := pagination.Page.PageSize // Add sorting var sort bson.D if pagination.Page.OrderBy != nil { direction := 1 // default ascending if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { direction = -1 } - sort = bson.D{{*pagination.Page.OrderBy, direction}} + sort = bson.D{{Key: *pagination.Page.OrderBy, Value: direction}} } else { // Default sort by updatedAt desc - sort = bson.D{{"updatedAt", -1}} + sort = bson.D{{Key: "updatedAt", Value: -1}} }🧰 Tools
🪛 golangci-lint (1.62.2)
190-190: func
(*Project).paginateis unused(unused)
🪛 GitHub Check: ci-api / ci-api-lint
[failure] 190-190:
func(*Project).paginateis unused (unused)api/internal/infrastructure/memory/deployment_test.go (2)
111-115: Set UpdatedAt field in test data to ensure correct sorting.The test data doesn't set the UpdatedAt field, which is used for default sorting. This could cause inconsistent test results.
Apply this diff to fix the test data initialization:
-d1 := deployment.New().NewID().Workspace(wsID).Version("v1").MustBuild() -d2 := deployment.New().NewID().Workspace(wsID).Version("v2").MustBuild() -d3 := deployment.New().NewID().Workspace(wsID).Version("v3").MustBuild() +now := time.Now() +d1 := deployment.New().NewID().Workspace(wsID).Version("v1").UpdatedAt(now.Add(-2 * time.Hour)).MustBuild() +d2 := deployment.New().NewID().Workspace(wsID).Version("v2").UpdatedAt(now.Add(-1 * time.Hour)).MustBuild() +d3 := deployment.New().NewID().Workspace(wsID).Version("v3").UpdatedAt(now).MustBuild()
235-235: Add test cases for error scenarios and sorting behavior.The test suite is missing cases for:
- Invalid pagination parameters (negative page, page size)
- Invalid order direction
- Custom sorting behavior
Apply this diff to add the missing test cases:
+ { + name: "invalid page number", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: -1, + PageSize: 10, + }, + }, + wantErr: true, + }, + { + name: "invalid order direction", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: 1, + PageSize: 10, + OrderDir: lo.ToPtr("INVALID"), + }, + }, + wantErr: true, + }, + { + name: "custom sorting by version ASC", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + d2.ID(): d2, + d3.ID(): d3, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: 1, + PageSize: 10, + OrderBy: lo.ToPtr("version"), + OrderDir: lo.ToPtr("ASC"), + }, + }, + want: []*deployment.Deployment{d1, d2, d3}, + wantInfo: &usecasex.PageInfo{ + TotalCount: 3, + }, + },api/e2e/gql_project_test.go (1)
311-320: Enhance pagination-specific assertions.While the basic assertions are good, consider adding more comprehensive checks for pagination behavior.
Apply this diff to enhance the assertions:
// Verify the response - projects := result.Data.Projects + projects := result.Data.ProjectsPage assert.NotNil(t, projects.Edges) + // Verify page size + assert.LessOrEqual(t, len(projects.Edges), 10, "Number of edges should not exceed page size") + for _, edge := range projects.Edges { assert.NotEmpty(t, edge.Node.ID) assert.NotEmpty(t, edge.Node.Name) assert.Equal(t, wId1.String(), edge.Node.WorkspaceID) assert.NotEmpty(t, edge.Cursor) } assert.NotNil(t, projects.PageInfo) + // Verify total count + assert.GreaterOrEqual(t, projects.PageInfo.TotalCount, len(projects.Edges)) + + // Verify hasNextPage when we know there are more items + if projects.PageInfo.TotalCount > 10 { + assert.True(t, projects.PageInfo.HasNextPage) + }
This commit removes cursor-based pagination test cases for Jobs and Triggers in the GraphQL end-to-end tests, continuing the ongoing pagination refactoring effort to simplify pagination logic.
…fo calculation This commit improves project pagination by: - Adding support for custom sorting in MongoDB repository - Implementing dynamic page info calculation in GraphQL loader - Creating a new conversion method for pagination parameters - Ensuring accurate current page and total pages computation
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
api/internal/usecase/interfaces/job.go (1)
16-16:⚠️ Potential issueImplement missing Job interface methods.
The interface change looks good, but the implementation of required methods is still pending.
Required implementations:
FindByDeployment(context.Context, id.DeploymentID, *usecase.Operator) (*job.Job, error)Create(context.Context, CreateJobParam, *usecase.Operator) (*job.Job, error)Update(context.Context, UpdateJobParam, *usecase.Operator) (*job.Job, error)Delete(context.Context, id.JobID, *usecase.Operator) errorapi/e2e/gql_pagination_test.go (3)
73-75: 🛠️ Refactor suggestionReplace time.Sleep with deterministic approach.
Using
time.Sleepfor test synchronization can make tests flaky.Replace the sleep with a deterministic approach that verifies project creation:
- // Add a small delay between project creations - time.Sleep(100 * time.Millisecond) + // Verify project creation before proceeding + verifyProject := fmt.Sprintf(`{ + project(id: "%s") { + id + name + } + }`, projectIDs[i]) + + request := GraphQLRequest{ + Query: verifyProject, + } + resp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var verifyResult struct { + Data struct { + Project struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"project"` + } `json:"data"` + } + assert.NoError(t, json.Unmarshal([]byte(resp.Body().Raw()), &verifyResult)) + assert.Equal(t, projectIDs[i], verifyResult.Data.Project.ID)🧰 Tools
🪛 GitHub Actions: ci
[warning] Low test coverage: 24.8% of statements covered
255-551: 🛠️ Refactor suggestion
⚠️ Potential issueFix failing job pagination tests and reduce code duplication.
The test is failing with HTTP 422 (Unprocessable Entity) errors, and there's significant code duplication.
Fix the failing tests by verifying:
- Request payload format
- GraphQL query syntax
- Input validation rules
Reduce code duplication by extracting common test patterns into helper functions:
// Helper function for executing GraphQL queries func executeGraphQLQuery(e *httpexpect.Expect, query string, userId string) *httpexpect.Response { request := GraphQLRequest{ Query: query, } jsonData, err := json.Marshal(request) if err != nil { panic(err) } return e.POST("/api/graphql"). WithHeader("authorization", "Bearer test"). WithHeader("Content-Type", "application/json"). WithHeader("X-Reearth-Debug-User", userId). WithBytes(jsonData). Expect().Status(http.StatusOK) } // Generic pagination test template func testPagination[T any](t *testing.T, e *httpexpect.Expect, queryTemplate string, workspaceId string, userId string) { query := fmt.Sprintf(queryTemplate, workspaceId) resp := executeGraphQLQuery(e, query, userId) var result T err := json.Unmarshal([]byte(resp.Body().Raw()), &result) assert.NoError(t, err) // Add common pagination assertions }
- Add explicit test data setup:
// Create test jobs jobIDs := createTestJobs(t, e, wId1.String(), uId1.String(), 5)🧰 Tools
🪛 GitHub Actions: ci
[error] 296-321: TestJobsPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[warning] Low test coverage: 24.8% of statements covered
553-937: 🛠️ Refactor suggestion
⚠️ Potential issueFix failing trigger pagination tests and add missing test cases.
The test is failing with HTTP 422 errors, has code duplication issues, and missing test cases.
Fix the failing tests by verifying:
- Request payload format
- GraphQL query syntax
- Input validation rules
Add test for ascending order sorting:
+ // Test sorting in ascending order + t.Run("test_sorting_ascending", func(t *testing.T) { + query := fmt.Sprintf(`{ + triggers( + workspaceId: "%s" + pagination: { + first: 5 + orderBy: "createdAt" + orderDir: ASC + } + ) { + edges { + node { + id + createdAt + } + } + } + }`, wId1.String()) + + // ... (rest of the test setup) + + // Verify sorting + for i := 1; i < len(result.Data.Triggers.Edges); i++ { + prev := result.Data.Triggers.Edges[i-1].Node.CreatedAt + curr := result.Data.Triggers.Edges[i].Node.CreatedAt + assert.True(t, prev.Before(curr), "Triggers should be sorted by createdAt in ascending order") + } + })
- Apply the same helper functions suggested for jobs pagination to reduce code duplication.
🧰 Tools
🪛 GitHub Actions: ci
[error] 592-888: TestTriggersPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[warning] Low test coverage: 24.8% of statements covered
api/internal/infrastructure/mongo/job.go (1)
83-106:⚠️ Potential issueAdd input validation for pagination parameters.
The pagination parameters should be validated to prevent potential issues with negative values or excessive page sizes.
Apply this diff to add validation:
if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page number must be positive")) + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size must be positive")) + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size cannot exceed 100")) + } + // Page-based pagination skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize)🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
🧹 Nitpick comments (7)
api/internal/usecase/interfaces/asset.go (1)
32-32: Document the new parameters in FindByWorkspace.The method signature has been expanded to include filtering and sorting capabilities. Please add documentation to clarify:
- Purpose of the
*stringparameter (likely a search term/filter)- Usage of
*asset.SortTypeConsider adding a comment in this format:
+// FindByWorkspace retrieves assets for a workspace with optional filtering and sorting +// Parameters: +// - ctx: context for the operation +// - workspaceID: ID of the workspace to search in +// - filter: optional string filter (e.g., search term) +// - sortType: optional sorting criteria +// - pagination: page-based pagination parameters +// - operator: operation contextapi/internal/adapter/gql/loader_project.go (1)
48-50: Remove debug print statements before merging.The code contains multiple debug print statements that should be removed or replaced with proper logging.
- fmt.Printf("DEBUG: Received pagination params: page=%d, pageSize=%d, orderBy=%v, orderDir=%v\n", - pagination.Page, pagination.PageSize, pagination.OrderBy, pagination.OrderDir) - - // Convert pagination parameters using ToPageBasedPagination paginationParam := gqlmodel.ToPageBasedPagination(pagination) - - fmt.Printf("DEBUG: Converted pagination params: page=%d, pageSize=%d\n", - paginationParam.Page.Page, paginationParam.Page.PageSize) // Use the pagination param for the usecase call res, pi, err := c.usecase.FindByWorkspace(ctx, tid, paginationParam, getOperator(ctx)) - fmt.Printf("DEBUG: Returning %d nodes\n", len(nodes)) - for _, n := range nodes { - fmt.Printf("DEBUG: Node name=%s\n", n.Name) - }Also applies to: 54-56, 78-81
api/internal/infrastructure/memory/deployment_test.go (2)
111-115: Set UpdatedAt field in test data for consistent sorting.The test data should include UpdatedAt fields to ensure consistent sorting behavior across tests.
+ now := time.Now() - d1 := deployment.New().NewID().Workspace(wsID).Version("v1").MustBuild() - d2 := deployment.New().NewID().Workspace(wsID).Version("v2").MustBuild() - d3 := deployment.New().NewID().Workspace(wsID).Version("v3").MustBuild() + d1 := deployment.New().NewID().Workspace(wsID).Version("v1").UpdatedAt(now.Add(-2 * time.Hour)).MustBuild() + d2 := deployment.New().NewID().Workspace(wsID).Version("v2").UpdatedAt(now.Add(-1 * time.Hour)).MustBuild() + d3 := deployment.New().NewID().Workspace(wsID).Version("v3").UpdatedAt(now).MustBuild()
117-214: Add test cases for error scenarios.The test suite should include cases for invalid pagination parameters and error handling.
tests := []struct { name string init map[id.DeploymentID]*deployment.Deployment wsID accountdomain.WorkspaceID pagination *interfaces.PaginationParam want []*deployment.Deployment wantInfo *usecasex.PageInfo wantErr bool }{ + { + name: "invalid page number", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: -1, + PageSize: 10, + }, + }, + wantErr: true, + }, + { + name: "invalid page size", + init: map[id.DeploymentID]*deployment.Deployment{ + d1.ID(): d1, + }, + wsID: wsID, + pagination: &interfaces.PaginationParam{ + Page: &interfaces.PageBasedPaginationParam{ + Page: 1, + PageSize: 0, + }, + }, + wantErr: true, + },api/internal/infrastructure/mongo/project.go (2)
123-126: Remove debug print statements.Remove debug logging statements or replace them with proper logging.
- // Print debug info for results - fmt.Printf("DEBUG: Found %d results\n", len(c.Result)) - for _, p := range c.Result { - fmt.Printf("DEBUG: Project name=%s\n", p.Name()) - }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
90-110: Improve field name mapping for sorting.The current field name mapping is inconsistent and could be improved.
- // Default sort by updatedAt desc - sort := bson.D{{Key: "updatedat", Value: -1}} + // Map of GraphQL field names to MongoDB field names + fieldMap := map[string]string{ + "name": "name", + "createdAt": "createdat", + "updatedAt": "updatedat", + } // Handle custom sorting if pagination.Page.OrderBy != nil { sortDir := -1 // default DESC if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "ASC" { sortDir = 1 } - // Convert field name to MongoDB field name (lowercase) - field := *pagination.Page.OrderBy - if field == "name" { - field = "name" - } else if field == "createdAt" { - field = "createdat" - } else if field == "updatedAt" { - field = "updatedat" - } + field := fieldMap[*pagination.Page.OrderBy] + if field == "" { + field = "updatedat" // default to updatedAt if field not found + } sort = bson.D{{Key: field, Value: sortDir}} - fmt.Printf("DEBUG: Sorting with field=%s, direction=%d\n", field, sortDir) }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
api/internal/infrastructure/mongo/job.go (1)
75-113: Consider extracting pagination logic to avoid duplication.The pagination logic is duplicated in both
FindByWorkspaceandpaginatemethods. Consider reusing thepaginatemethod to maintain DRY principles.func (r *Job) FindByWorkspace(ctx context.Context, workspace accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*job.Job, *usecasex.PageInfo, error) { if !r.f.CanRead(workspace) { return nil, usecasex.EmptyPageInfo(), nil } - c := mongodoc.NewJobConsumer(r.f.Readable) - filter := bson.M{"workspaceid": workspace.String()} - - if pagination != nil && pagination.Page != nil { - // Page-based pagination - skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) - limit := int64(pagination.Page.PageSize) - - // Get total count for page info - total, err := r.client.Count(ctx, filter) - if err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - - // Execute find with skip and limit - opts := options.Find(). - SetSkip(skip). - SetLimit(limit) - - if err := r.client.Find(ctx, filter, c, opts); err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - - // Create page-based info - pageInfo := interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize) - return c.Result, pageInfo.ToPageInfo(), nil - } - - // No pagination - if err := r.client.Find(ctx, filter, c); err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - total := int64(len(c.Result)) - return c.Result, &usecasex.PageInfo{TotalCount: total}, nil + filter := bson.M{"workspaceid": workspace.String()} + return r.paginate(ctx, filter, pagination) }🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
api/e2e/gql_pagination_test.go(1 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/adapter/gql/loader_project.go(2 hunks)api/internal/adapter/gql/loader_trigger.go(1 hunks)api/internal/infrastructure/memory/asset.go(2 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/asset.go(3 hunks)api/internal/infrastructure/mongo/job.go(4 hunks)api/internal/infrastructure/mongo/project.go(3 hunks)api/internal/usecase/interactor/asset.go(1 hunks)api/internal/usecase/interactor/job.go(1 hunks)api/internal/usecase/interactor/project.go(1 hunks)api/internal/usecase/interactor/trigger.go(1 hunks)api/internal/usecase/interfaces/asset.go(1 hunks)api/internal/usecase/interfaces/job.go(1 hunks)api/internal/usecase/interfaces/project.go(1 hunks)api/internal/usecase/interfaces/trigger.go(1 hunks)api/internal/usecase/repo/asset.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/internal/usecase/interactor/job.go
- api/internal/usecase/interactor/project.go
- api/internal/adapter/gql/loader_trigger.go
🧰 Additional context used
🪛 GitHub Actions: ci
api/e2e/gql_pagination_test.go
[error] 296-321: TestJobsPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[error] 592-888: TestTriggersPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[warning] Low test coverage: 24.8% of statements covered
api/internal/infrastructure/mongo/asset.go
[warning] Multiple tests skipped due to missing MongoDB URI configuration
api/internal/infrastructure/mongo/job.go
[warning] Multiple tests skipped due to missing MongoDB URI configuration
api/internal/adapter/gql/gqlmodel/convert.go
[error] 94-94: Function 'intToInt64' is unused
api/internal/infrastructure/mongo/project.go
[warning] Multiple tests skipped due to missing MongoDB URI configuration
🪛 golangci-lint (1.62.2)
api/internal/infrastructure/mongo/job.go
156-156: func (*Job).paginate is unused
(unused)
🪛 GitHub Check: ci-api / ci-api-lint
api/internal/infrastructure/mongo/job.go
[failure] 156-156:
func (*Job).paginate is unused (unused)
🔇 Additional comments (16)
api/internal/usecase/interactor/trigger.go (2)
49-52:⚠️ Potential issueLGTM on permission check, but pagination validation is missing.
The addition of the workspace permission check is a good security practice. However, the pagination parameter validation is still missing.
Add parameter validation before the workspace permission check:
func (i *Trigger) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, p *interfaces.PaginationParam, operator *usecase.Operator) ([]*trigger.Trigger, *usecasex.PageInfo, error) { + if p == nil || p.Page == nil { + return nil, nil, fmt.Errorf("pagination parameter is required") + } + if p.Page.Page < 1 { + return nil, nil, fmt.Errorf("page must be greater than 0") + } + if p.Page.PageSize < 1 { + return nil, nil, fmt.Errorf("page size must be greater than 0") + } + if err := i.CanReadWorkspace(id, operator); err != nil { return nil, nil, err } return i.triggerRepo.FindByWorkspace(ctx, id, p) }Likely invalid or redundant comment.
49-52: Verify pagination implementation across the codebase.Let's ensure consistent pagination implementation across related components.
✅ Verification successful
Pagination implementation is consistent across the codebase
The pagination implementation follows a uniform pattern across all components (Trigger, Job, Project, Deployment, Asset) with consistent:
- Interface definitions
- Repository implementations
- Parameter handling
- Return types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify pagination implementation consistency # Check repository interface definition echo "Checking repository interface for FindByWorkspace..." rg -A 5 "type Trigger interface" "api/internal/usecase/repo/" # Check test coverage echo "Checking test coverage for pagination..." rg -A 10 "TestTrigger.*FindByWorkspace" "api/internal/usecase/interactor/" # Check other pagination implementations echo "Checking other pagination implementations..." rg -A 5 "PaginationParam.*Page" "api/internal/"Length of output: 17023
api/internal/usecase/repo/asset.go (1)
6-6: LGTM: Consistent pagination type update.The changes to import and use
interfaces.PaginationParamalign with the page-based pagination implementation.Also applies to: 16-16
api/internal/usecase/interfaces/project.go (1)
44-44: Consider consistent pagination return types.The parameter change to
*PaginationParamis good, but like injob.go, the return type still uses*usecasex.PageInfo. Consider updating all pagination-related return types consistently across interfaces.Let's verify the consistency:
api/internal/usecase/interfaces/trigger.go (1)
51-51: Verify implementations of the Trigger interface.The parameter type change from
*usecasex.Paginationto*PaginationParamrequires updates to all implementations of theTriggerinterface.✅ Verification successful
All implementations are already updated to use PaginationParam
The interface implementations in the following files are already using the new
*interfaces.PaginationParamtype:
api/internal/usecase/interactor/trigger.goapi/internal/infrastructure/mongo/trigger.goapi/internal/infrastructure/memory/trigger.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all implementations of the Trigger interface ast-grep --pattern 'type $_ struct { $$$ } func ($_ *$_) FindByWorkspace(context.Context, accountdomain.WorkspaceID, *$_, *usecase.Operator) ([]*trigger.Trigger, *$_, error) { $$$ }'Length of output: 189
Script:
#!/bin/bash # First, find relevant Go files that might contain implementations fd -e go trigger # Then search for FindByWorkspace method implementations rg -U "func.*FindByWorkspace.*context\.Context.*WorkspaceID.*usecase\.Operator.*\[.*trigger\.Trigger" -A 5Length of output: 1455
Script:
#!/bin/bash # Check repository implementation rg -U "func.*FindByWorkspace.*context\.Context.*WorkspaceID.*Pagination.*\[.*trigger\.Trigger" api/internal/infrastructure/mongo/trigger.go api/internal/infrastructure/memory/trigger.goLength of output: 635
api/internal/usecase/interactor/asset.go (1)
Line range hint
34-43: LGTM! Function signature updated correctly.The function signature has been properly updated to use the new pagination parameter type, and the implementation maintains proper error handling and operator checks.
api/internal/adapter/gql/gqlmodel/convert.go (4)
32-37: Use safe type assertion to prevent panics.The type assertion could panic if
pis not nil but the type assertion fails.
51-59: Add input validation and overflow protection.The offset calculation
(*pagination.Page - 1) * *pagination.PageSizecould overflow for large page numbers.
64-73: Add input validation for pagination parameters.The function should validate the input parameters before creating the pagination param.
Line range hint
94-99: Remove unused functionintToInt64.The pipeline failure indicates that this function is unused.
api/internal/infrastructure/memory/asset.go (1)
8-8: LGTM! Required import added.The import is necessary for the new pagination implementation.
api/internal/infrastructure/mongo/project.go (1)
Line range hint
156-255: Remove unused paginate method.The
paginatemethod is unused according to static analysis and should be removed.Let's verify if the method is truly unused:
🧰 Tools
🪛 golangci-lint (1.62.2)
216-216: func
(*Project).paginateis unused(unused)
🪛 GitHub Check: ci-api / ci-api-lint
[failure] 216-216:
func(*Project).paginateis unused (unused)🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
api/e2e/gql_pagination_test.go (1)
1-937: Improve test coverage.The current test coverage is only 24.8%, which is significantly low. Consider adding test cases for:
- Error scenarios (invalid input, unauthorized access)
- Edge cases (empty results, maximum page size)
- Boundary conditions (page numbers, sorting with null values)
Run this script to identify untested code paths:
🧰 Tools
🪛 GitHub Actions: ci
[error] 296-321: TestJobsPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[error] 592-888: TestTriggersPagination failed with multiple assertion errors including unexpected HTTP status 422 instead of 200
[warning] Low test coverage: 24.8% of statements covered
api/internal/infrastructure/mongo/job.go (3)
7-7: LGTM: Import changes are appropriate.The new imports support the pagination implementation with the required types and MongoDB options.
Also applies to: 16-16
🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
75-75: LGTM: Method signature update is appropriate.The change from
*usecasex.Paginationto*interfaces.PaginationParamaligns with the new pagination system.Also applies to: 79-79
🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
Line range hint
75-189: Verify test coverage with MongoDB configuration.Multiple tests are being skipped due to missing MongoDB URI configuration in CI. Please ensure:
- The pagination implementation is thoroughly tested
- The CI environment is properly configured with MongoDB URI
To help set up the test environment, I can assist with:
- Generating test cases for pagination
- Providing MongoDB configuration examples for CI
Would you like me to help with either of these?🧰 Tools
🪛 GitHub Actions: ci
[warning] Multiple tests skipped due to missing MongoDB URI configuration
…oach This commit updates the Jobs pagination test in the GraphQL end-to-end tests to: - Replace cursor-based pagination with page-based pagination - Update query structure to use `jobsPage` instead of `jobs` - Modify result struct to match new pagination response format - Add tests for current page and total pages information
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
api/e2e/gql_pagination_test.go (2)
73-75: 🛠️ Refactor suggestionReplace time.Sleep with deterministic synchronization.
Using
time.Sleepfor test synchronization is an anti-pattern that can make tests flaky and slow.Replace the sleep with a deterministic approach that verifies project creation:
- // Add a small delay between project creations - time.Sleep(100 * time.Millisecond) + // Verify project creation + verifyQuery := fmt.Sprintf(`{ + project(id: "%s") { + id + name + } + }`, projectIDs[i]) + + verifyRequest := GraphQLRequest{ + Query: verifyQuery, + } + verifyJsonData, err := json.Marshal(verifyRequest) + assert.NoError(t, err) + + verifyResp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(verifyJsonData). + Expect().Status(http.StatusOK) + + var verifyResult struct { + Data struct { + Project struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"project"` + } `json:"data"` + } + assert.NoError(t, json.Unmarshal([]byte(verifyResp.Body().Raw()), &verifyResult)) + assert.Equal(t, projectIDs[i], verifyResult.Data.Project.ID)
255-539: 🛠️ Refactor suggestionImprove test structure and coverage for jobs pagination.
The jobs pagination tests need improvements:
- Add explicit test data setup
- Add error case tests
- Reduce code duplication using helper functions
Extract common test patterns into helper functions and add error cases:
+// Helper function for executing GraphQL queries +func executeGraphQLQuery(e *httpexpect.Expect, query string, userId string) *httpexpect.Response { + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + if err != nil { + panic(err) + } + + return e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", userId). + WithBytes(jsonData). + Expect() +} + +// Helper function for creating test jobs +func createTestJobs(t *testing.T, e *httpexpect.Expect, workspaceId string, userId string, count int) []string { + jobIDs := make([]string, count) + for i := 0; i < count; i++ { + // Add job creation logic here + } + return jobIDs +} + func TestJobsPagination(t *testing.T) { e, _ := StartGQLServer(t, &config.Config{ Origins: []string{"https://example.com"}, AuthSrv: config.AuthSrvConfig{ Disabled: true, }, }, true, baseSeederUser) + + // Create test jobs + jobIDs := createTestJobs(t, e, wId1.String(), uId1.String(), 5) + + // Test error cases + t.Run("test_invalid_input", func(t *testing.T) { + // Test negative page number + resp := executeGraphQLQuery(e, fmt.Sprintf(`{ + jobsPage( + workspaceId: "%s" + pagination: { + page: -1 + pageSize: 2 + } + ) { + nodes { + id + } + } + }`, wId1.String()), uId1.String()) + + resp.Status(http.StatusOK) + // Add assertions for error response + })
🧹 Nitpick comments (2)
api/e2e/gql_pagination_test.go (2)
596-643: Validate trigger schedule format and add error cases.The trigger creation needs improvements:
- Add validation for the schedule format
- Add error cases for invalid schedules
- Add test cases for invalid pagination input
Add schedule validation and error cases:
+ // Test invalid schedule format + invalidScheduleQuery := `mutation($input: CreateTimeDrivenTriggerInput!) { + createTimeDrivenTrigger(input: $input) { + trigger { + id + } + } + }` + + invalidScheduleVariables := fmt.Sprintf(`{ + "input": { + "workspaceId": "%s", + "deploymentId": "%s", + "description": "Invalid Schedule", + "schedule": "invalid cron" + } + }`, wId1.String(), deploymentId) + + var invalidScheduleVariablesMap map[string]any + err = json.Unmarshal([]byte(invalidScheduleVariables), &invalidScheduleVariablesMap) + assert.NoError(t, err) + + request = GraphQLRequest{ + Query: invalidScheduleQuery, + Variables: invalidScheduleVariablesMap, + } + jsonData, err = json.Marshal(request) + assert.NoError(t, err) + + resp = e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + // Assert error response for invalid schedule
1-925: Consider restructuring tests using table-driven approach.The test file would benefit from a more structured approach:
- Use table-driven tests for different pagination scenarios
- Create a common test framework for pagination testing
- Implement proper test data setup and cleanup
Consider restructuring the tests using this pattern:
type paginationTestCase struct { name string page int pageSize int orderBy string orderDir string expectedCount int expectedError string } func runPaginationTests(t *testing.T, e *httpexpect.Expect, testCases []paginationTestCase, queryTemplate string) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { // Test implementation }) } }This approach would:
- Reduce code duplication
- Make it easier to add new test cases
- Provide better test coverage visualization
- Make the test suite more maintainable
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/e2e/gql_pagination_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (1)
api/e2e/gql_pagination_test.go (1)
83-193: Enhance pagination test coverage.The pagination tests need improvements:
- Add test cases for edge cases (invalid page numbers, zero page size)
- Add test for descending order sorting
- Make test data setup more explicit
Add test cases for edge cases and descending order:
+ // Test invalid page number + t.Run("test_invalid_page", func(t *testing.T) { + query := fmt.Sprintf(`{ + projectsPage( + workspaceId: "%s" + pagination: { + page: 999 + pageSize: 2 + } + ) { + nodes { + id + } + pageInfo { + totalCount + totalPages + currentPage + } + } + }`, wId1.String()) + + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + ProjectsPage struct { + Nodes []struct{} `json:"nodes"` + PageInfo struct { + TotalCount int `json:"totalCount"` + TotalPages int `json:"totalPages"` + CurrentPage int `json:"currentPage"` + } `json:"pageInfo"` + } `json:"projectsPage"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + assert.Empty(t, result.Data.ProjectsPage.Nodes) + }) + + // Test descending order sorting + t.Run("test_sorting_desc", func(t *testing.T) { + query := fmt.Sprintf(`{ + projectsPage( + workspaceId: "%s" + pagination: { + page: 1 + pageSize: 5 + orderBy: "name" + orderDir: DESC + } + ) { + nodes { + id + name + } + } + }`, wId1.String()) + + request := GraphQLRequest{ + Query: query, + } + jsonData, err := json.Marshal(request) + assert.NoError(t, err) + + resp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var result struct { + Data struct { + ProjectsPage struct { + Nodes []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"nodes"` + } `json:"projectsPage"` + } `json:"data"` + } + + err = json.Unmarshal([]byte(resp.Body().Raw()), &result) + assert.NoError(t, err) + + // Verify sorting + for i := 1; i < len(result.Data.ProjectsPage.Nodes); i++ { + prev := result.Data.ProjectsPage.Nodes[i-1].Name + curr := result.Data.ProjectsPage.Nodes[i].Name + assert.True(t, prev >= curr, "Projects should be sorted by name in descending order") + } + })
This commit introduces metadata generation and file storage functionality for job artifacts by: - Adding a new metadata directory constant in file system infrastructure - Implementing metadata file generation with job ID and asset information - Creating methods to upload, read, and remove metadata files - Updating file repository to support metadata-related operations - Enhancing job repository pagination with default sorting
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
api/internal/infrastructure/mongo/job.go (2)
83-86:⚠️ Potential issueAdd input validation for pagination parameters.
The pagination parameters should be validated to prevent potential issues with negative values or excessive page sizes.
if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page number must be positive")) + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size must be positive")) + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size cannot exceed 100")) + } + // Page-based pagination skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize)
177-210:⚠️ Potential issueRemove unused paginate method.
The
paginatemethod is unused and contains logic that's already implemented inFindByWorkspace. Remove it to maintain a cleaner codebase.-func (r *Job) paginate(ctx context.Context, filter bson.M, pagination *interfaces.PaginationParam) ([]*job.Job, *usecasex.PageInfo, error) { - c := mongodoc.NewJobConsumer(r.f.Readable) - - if pagination != nil && pagination.Page != nil { - // Page-based pagination - skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) - limit := int64(pagination.Page.PageSize) - - // Get total count for page info - total, err := r.client.Count(ctx, filter) - if err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - - // Execute find with skip and limit - opts := options.Find(). - SetSkip(skip). - SetLimit(limit) - - if err := r.client.Find(ctx, filter, c, opts); err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - - // Create page-based info - pageInfo := interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize) - return c.Result, pageInfo.ToPageInfo(), nil - } - - // No pagination - if err := r.client.Find(ctx, filter, c); err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) - } - total := int64(len(c.Result)) - return c.Result, &usecasex.PageInfo{TotalCount: total}, nil -}🧰 Tools
🪛 GitHub Check: ci-api / ci-api-lint
[failure] 177-177:
func(*Job).paginateis unused (unused)🪛 golangci-lint (1.62.2)
177-177: func
(*Job).paginateis unused(unused)
api/e2e/gql_pagination_test.go (2)
75-77: 🛠️ Refactor suggestionReplace time.Sleep with deterministic approach.
Using
time.Sleepfor test synchronization can make tests flaky. Consider using a more deterministic approach by verifying each project creation before proceeding.- // Add a small delay between project creations - time.Sleep(100 * time.Millisecond) + // Verify project creation before proceeding + verifyQuery := fmt.Sprintf(`{ + project(id: "%s") { + id + name + } + }`, projectIDs[i]) + + verifyRequest := GraphQLRequest{ + Query: verifyQuery, + } + verifyResp := e.POST("/api/graphql"). + WithHeader("Content-Type", "application/json"). + WithHeader("X-Reearth-Debug-User", uId1.String()). + WithBytes(jsonData). + Expect().Status(http.StatusOK) + + var verifyResult struct { + Data struct { + Project struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"project"` + } `json:"data"` + } + assert.NoError(t, json.Unmarshal([]byte(verifyResp.Body().Raw()), &verifyResult)) + assert.Equal(t, projectIDs[i], verifyResult.Data.Project.ID)
138-141:⚠️ Potential issueFix failing pagination tests.
The pipeline shows failures in the pagination tests. These failures suggest issues with:
- The page size calculation in the resolver
- The total count calculation
- The page info fields population
Run this script to verify the implementation:
#!/bin/bash # Description: Check pagination implementation in resolver and schema # Check resolver implementation echo "=== Checking resolver implementation ===" ast-grep --pattern 'func (*_) Projects($_, $_) (*_, error) { $$$ pageInfo := $_ $$$ }' echo -e "\n=== Checking schema definition ===" rg -A 10 'type PageInfo' echo -e "\n=== Checking GraphQL query validation ===" rg -A 5 'func validate.*Query'
🧹 Nitpick comments (5)
api/internal/infrastructure/fs/file.go (3)
47-48: Consider upfront empty name checks for metadata.
While you rely onf.readto handle file not found scenarios, it may be more transparent to validatenamefor emptiness before constructing the path.func (f *fileRepo) ReadMetadata(ctx context.Context, name string) (io.ReadCloser, error) { + if name == "" { + return nil, rerror.ErrNotFound + } return f.read(ctx, filepath.Join(metadataDir, sanitize.Path(name))) }
62-75: Ensure jobID is validated and consistently sanitized.
Long or malformed job IDs might lead to unusual filenames (e.g., “metadata-???...json”). If jobID can contain invalid path characters, consider adding checks or normalizing the string.func (f *fileRepo) UploadMetadata(ctx context.Context, jobID string, assets []string) (*url.URL, error) { + if jobID == "" { + return nil, gateway.ErrInvalidFile + } metadataFile, err := f.generateMetadata(jobID, assets) if err != nil { return nil, err } ... }
77-104: Externalize hard-coded URLs.
artifactBaseUrlandassetBaseUrlare currentlyfile://artifactsandfile://assets. If these should be configurable or environment-driven, consider injecting them via constructor arguments for better flexibility and less potential for deployment mismatches.api/internal/infrastructure/mongo/job.go (1)
96-112: Refactor field name mapping for better maintainability.The current field name mapping implementation could be improved by:
- Using a map for field name translations
- Making the default sort configuration more configurable
+ // Define field name mappings + fieldMappings := map[string]string{ + "startedAt": "startedat", + "completedAt": "completedat", + } + + // Define default sort configuration + defaultSortField := "startedat" + defaultSortDir := -1 + if pagination.Page.OrderBy != nil { dir := 1 if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { dir = -1 } - // Convert field name to MongoDB field name (lowercase) fieldName := *pagination.Page.OrderBy - if fieldName == "startedAt" { - fieldName = "startedat" - } else if fieldName == "completedAt" { - fieldName = "completedat" + if mapped, ok := fieldMappings[fieldName]; ok { + fieldName = mapped } sort = bson.D{{Key: fieldName, Value: dir}} } else { - // Default sort by startedAt desc - sort = bson.D{{Key: "startedat", Value: -1}} + sort = bson.D{{Key: defaultSortField, Value: defaultSortDir}} }api/e2e/gql_pagination_test.go (1)
273-311: Extract multipart form data setup into a helper function.The multipart form data setup is complex and could be reused in other tests. Consider extracting it into a helper function to improve readability and maintainability.
+func createMultipartFormData(t *testing.T, query string, workspaceId string, workflowContent string) (*bytes.Buffer, string) { + var b bytes.Buffer + w := multipart.NewWriter(&b) + + // Add operations field + operations := map[string]any{ + "query": query, + "variables": map[string]any{ + "input": map[string]any{ + "workspaceId": workspaceId, + "description": "Test deployment description", + "file": nil, + }, + }, + } + operationsJSON, err := json.Marshal(operations) + assert.NoError(t, err) + + err = w.WriteField("operations", string(operationsJSON)) + assert.NoError(t, err) + + // Add map field + err = w.WriteField("map", `{"0": ["variables.input.file"]}`) + assert.NoError(t, err) + + // Add file + part, err := w.CreateFormFile("0", "workflow.json") + assert.NoError(t, err) + _, err = part.Write([]byte(workflowContent)) + assert.NoError(t, err) + + err = w.Close() + assert.NoError(t, err) + + return &b, w.FormDataContentType() +}Usage:
- // Create multipart form data - var b bytes.Buffer - w := multipart.NewWriter(&b) - // ... existing multipart setup code ... + b, contentType := createMultipartFormData(t, deploymentQuery, wId1.String(), workflowContent)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
api/e2e/gql_pagination_test.go(1 hunks)api/internal/infrastructure/fs/common.go(1 hunks)api/internal/infrastructure/fs/file.go(2 hunks)api/internal/infrastructure/mongo/job.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/internal/infrastructure/fs/common.go
🧰 Additional context used
🪛 GitHub Actions: ci
api/e2e/gql_pagination_test.go
[error] 599-599: Test failure: TestJobsPagination/test_page_pagination - Expected value 1, got 0
[error] 601-601: Test failure: TestJobsPagination/test_page_pagination - Expected value 3, got 0
🪛 GitHub Check: ci-api / ci-api-lint
api/internal/infrastructure/mongo/job.go
[failure] 177-177:
func (*Job).paginate is unused (unused)
🪛 golangci-lint (1.62.2)
api/internal/infrastructure/mongo/job.go
177-177: func (*Job).paginate is unused
(unused)
🔇 Additional comments (3)
api/internal/infrastructure/fs/file.go (2)
51-59: Validate presence of subdirectories if intended.
By default,validateURLchecks whetherpath.Dir(u.Path) == base.Path. This means subdirectories of the base path are not allowed. If that’s the desired behavior, this is fine. Otherwise, consider adjustingvalidateURL.
4-20: Imports and package references look good.
All newly introduced imports appear necessary. Just verify thatmetadataDiris declared somewhere, as it’s used extensively but not shown in this diff.✅ Verification successful
metadataDiris properly declared and implemented
Found inapi/internal/infrastructure/fs/common.goas a package-level constant with value "metadata", used consistently throughout the package.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that metadataDir is defined. # We expect exactly one declaration of metadataDir as a package-level variable or constant. rg -A 3 'metadataDir'Length of output: 1098
api/internal/infrastructure/mongo/job.go (1)
7-7: LGTM!The new imports are necessary for implementing page-based pagination and are used appropriately throughout the code.
Also applies to: 16-16
…s repositories This commit introduces several improvements to pagination and job metadata handling: - Remove dependencies on usecasex pagination - Update repository and interactor interfaces to use PageBasedInfo - Enhance job metadata handling in file and MongoDB repositories - Improve job document mapping with additional metadata fields - Simplify pagination logic across different repository implementations
…ation This commit updates the deployment repository tests in both memory and MongoDB implementations to: - Replace usecasex pagination with interfaces.PageBasedInfo - Modify test cases to use new page-based pagination methods - Update assertions to check current page and total pages - Remove HasNextPage and HasPreviousPage flags - Ensure consistent pagination behavior across repository tests
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
api/internal/infrastructure/mongo/deployment.go (1)
Line range hint
1-285: Improve test coverage.The package has critically low test coverage (0.1%). Consider adding tests for:
- Pagination functionality
- CRUD operations
- Error handling
- Edge cases
Would you like me to help generate comprehensive test cases for this package?
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
♻️ Duplicate comments (7)
api/internal/usecase/repo/project.go (1)
45-47:⚠️ Potential issueGuard against integer overflow in break condition.
Similar to the issue in job.go, the multiplication could overflow.
-if info.TotalCount <= int64(page*int(batch)) { +nextPage := int64(page) * batch +if nextPage < 0 || info.TotalCount <= nextPage { break }api/internal/usecase/repo/deployment.go (1)
47-51:⚠️ Potential issueGuard against integer overflow in break condition.
The multiplication of page and batch could overflow.
-if info.TotalCount <= int64(page*int(batch)) { +nextPage := int64(page) * batch +if nextPage < 0 || info.TotalCount <= nextPage { break }api/internal/adapter/gql/gqlmodel/convert.go (3)
55-64: 🛠️ Refactor suggestionAdd input validation in ToPageBasedPagination.
The function should validate pagination parameters before conversion.
Add validation:
func ToPageBasedPagination(pagination PageBasedPagination) *interfaces.PaginationParam { + // Ensure positive page and page size + if pagination.Page <= 0 { + pagination.Page = 1 + } + if pagination.PageSize <= 0 { + pagination.PageSize = 10 // default page size + } + return &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: pagination.Page,
66-75: 🛠️ Refactor suggestionRemove duplicate function.
ToPageBasedPaginationParamis identical toToPageBasedPagination.Remove this duplicate function and use
ToPageBasedPaginationinstead.
85-85:⚠️ Potential issueRemove unused function.
The pipeline failure indicates that this function is unused.
Remove the unused function to fix the pipeline failure.
🧰 Tools
🪛 golangci-lint (1.62.2)
85-85: func
intToInt64is unused(unused)
🪛 GitHub Check: ci-api / ci-api-lint
[failure] 85-85:
funcintToInt64is unused (unused)🪛 GitHub Actions: ci
[error] 85-85: Function 'intToInt64' is unused
api/internal/infrastructure/mongo/project.go (1)
218-247: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The page-based pagination implementation needs validation to prevent potential issues.
Apply this diff to add input validation:
if pagination != nil && pagination.Offset != nil { + // Validate pagination parameters + if pagination.Offset.Limit <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "limit must be positive") + } + if pagination.Offset.Offset < 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "offset must be non-negative") + } + // Page-based pagination skip := pagination.Offset.Offset limit := pagination.Offset.Limit🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/memory/asset.go (1)
79-92: 🛠️ Refactor suggestionAdd validation for pagination parameters.
The pagination implementation needs validation.
Apply this diff to add validation:
if filter.Pagination != nil && filter.Pagination.Page != nil { + // Validate pagination parameters + if filter.Pagination.Page.Page <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be positive") + } + if filter.Pagination.Page.PageSize <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize must be positive") + } + // Page-based pagination skip := (filter.Pagination.Page.Page - 1) * filter.Pagination.Page.PageSize🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
🧹 Nitpick comments (11)
api/internal/infrastructure/memory/trigger.go (1)
35-40: Improve empty result handling with pagination parameters.When access is denied, the PageBasedInfo should reflect the requested pagination parameters rather than using hardcoded values.
- return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil + pageSize := 10 // default page size + page := 1 // default page + if pagination != nil && pagination.Page != nil { + pageSize = pagination.Page.PageSize + page = pagination.Page.Page + } + return nil, interfaces.NewPageBasedInfo(0, page, pageSize), nil🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/fs/file.go (4)
44-58: Enhance artifact listing implementation.Consider these improvements:
- Add path sanitization for jobID
- Filter out non-regular files from the listing
- Consider adding sorting for consistent results
Apply this diff to enhance the implementation:
func (f *fileRepo) ListJobArtifacts(ctx context.Context, jobID string) ([]string, error) { - artifactsPath := filepath.Join(metadataDir, fmt.Sprintf("job-%s-artifacts", jobID)) + artifactsPath := filepath.Join(metadataDir, fmt.Sprintf("job-%s-artifacts", sanitize.Path(jobID))) files, err := afero.ReadDir(f.fs, artifactsPath) if err != nil { if os.IsNotExist(err) { return []string{}, nil } return nil, rerror.ErrInternalByWithContext(ctx, err) } var artifacts []string for _, file := range files { + if !file.Mode().IsRegular() { + continue + } artifacts = append(artifacts, file.Name()) } + sort.Strings(artifacts) return artifacts, nil }
80-93: Consider adding rollback capability.If the upload fails, the partially written file might need to be cleaned up. Consider implementing a transaction-like behavior.
Apply this diff to add cleanup on failure:
func (f *fileRepo) UploadMetadata(ctx context.Context, jobID string, assets []string) (*url.URL, error) { metadataFile, err := f.generateMetadata(jobID, assets) if err != nil { return nil, err } filename := filepath.Join(metadataDir, sanitize.Path(metadataFile.Path)) _, err = f.upload(ctx, filename, metadataFile.Content) if err != nil { + // Clean up the partially written file if upload fails + _ = f.delete(ctx, filename) return nil, err } return getFileURL(f.workflowUrlBase, metadataFile.Path), nil }
95-122: Improve metadata generation flexibility and validation.Consider these improvements:
- Make base URLs configurable instead of hardcoding
- Add validation for jobID and assets
- Consider adding more metadata fields for future extensibility
Apply this diff to enhance the implementation:
func (f *fileRepo) generateMetadata(jobID string, assets []string) (*file.File, error) { + if jobID == "" { + return nil, errors.New("jobID cannot be empty") + } + + // Validate assets + for _, asset := range assets { + if asset == "" { + return nil, errors.New("asset path cannot be empty") + } + } + - artifactBaseUrl := "file://artifacts" - assetBaseUrl := "file://assets" + artifactBaseUrl := f.workflowUrlBase.String() + assetBaseUrl := f.assetUrlBase.String() created := time.Now() metadata := &workflow.Metadata{ ArtifactBaseUrl: artifactBaseUrl, Assets: workflow.Asset{ BaseUrl: assetBaseUrl, Files: assets, }, JobID: jobID, Timestamps: workflow.Timestamp{ Created: created, + Version: "1.0", // Add version for future compatibility }, } metadataJSON, err := json.Marshal(metadata) if err != nil { return nil, err } return &file.File{ Content: io.NopCloser(bytes.NewReader(metadataJSON)), Path: fmt.Sprintf("metadata-%s.json", sanitize.Path(jobID)), Size: int64(len(metadataJSON)), ContentType: "application/json", }, nil }
Line range hint
1-122: Consider architectural improvements for better maintainability.The current implementation mixes file system operations with business logic. Consider these architectural improvements:
- Extract metadata generation to a separate service
- Add an interface for URL generation strategy
- Consider using a configuration object for base paths and URLs
- Add metrics/logging for better observability
api/internal/usecase/repo/project.go (1)
26-31: Extract pagination logic to a shared utility.The pagination initialization code is duplicated across multiple files (job.go, project.go, deployment.go).
Consider creating a shared utility:
// utils/pagination/pagination.go package pagination func NewPageParam(page int, batch int64) *interfaces.PaginationParam { return &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: page, PageSize: validatePageSize(int(batch)), }, } } func validatePageSize(size int) int { if size <= 0 { return 10 // default page size } return size }api/internal/infrastructure/mongo/job.go (1)
70-78: Enhance error handling for count operation.The error handling for the count operation could be more specific and informative.
Apply this diff to improve error handling:
total, err := r.client.Count(ctx, filter) if err != nil { - return nil, nil, rerror.ErrInternalByWithContext(ctx, err) + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("failed to count documents: %w", err)) }🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/memory/deployment.go (1)
36-41: Improve error handling for workspace permission check.Consider returning an error instead of an empty result when workspace access is denied.
Apply this diff to improve error handling:
func (r *Deployment) FindByWorkspace(_ context.Context, wid accountdomain.WorkspaceID, p *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { r.lock.Lock() defer r.lock.Unlock() if !r.f.CanRead(wid) { - return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil + return nil, nil, repo.ErrOperationDenied }🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/mongo/deployment.go (1)
29-38: Add documentation for the adapter pattern implementation.Consider adding documentation to explain the purpose of the adapter pattern and its benefits.
+// DeploymentAdapter implements repo.Deployment interface by wrapping the base Deployment struct. +// This pattern allows for interface segregation and better extensibility. type DeploymentAdapter struct { *Deployment } +// NewDeployment creates a new deployment repository instance implementing repo.Deployment interface. +// It uses the adapter pattern to wrap the base Deployment implementation. func NewDeployment(client *mongox.Client) repo.Deployment { return &DeploymentAdapter{ Deployment: &Deployment{ client: client.WithCollection("deployment"), }, } }🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/memory/job.go (1)
66-93: Improve null handling in sorting logic.The sorting implementation for
completedAtcould be simplified and made more robust.Apply this diff to improve null handling:
case "completedAt": ti := result[i].CompletedAt() tj := result[j].CompletedAt() - if ti == nil && tj == nil { - return result[i].ID().String() < result[j].ID().String() - } - if ti == nil { - return direction == 1 - } - if tj == nil { - return direction != 1 - } - if !ti.Equal(*tj) { - if direction == 1 { - return ti.Before(*tj) - } - return ti.After(*tj) + // Handle null cases + switch { + case ti == nil && tj == nil: + return result[i].ID().String() < result[j].ID().String() + case ti == nil: + return direction == 1 // null first in ascending + case tj == nil: + return direction != 1 // null last in ascending + default: + if !ti.Equal(*tj) { + return (direction == 1) == ti.Before(*tj) + } }🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/memory/asset.go (1)
49-51: Add documentation for the FindByWorkspace method.The method lacks documentation explaining its parameters and return values.
Add documentation:
+// FindByWorkspace retrieves assets for a given workspace with optional pagination. +// Parameters: +// - ctx: context for the operation +// - wid: workspace ID to filter assets +// - filter: filter parameters including pagination and sorting options +// Returns: +// - []*asset.Asset: slice of assets matching the criteria +// - *interfaces.PageBasedInfo: pagination information +// - error: any error that occurred during the operation func (r *Asset) FindByWorkspace(_ context.Context, wid accountdomain.WorkspaceID, filter repo.AssetFilter) ([]*asset.Asset, *interfaces.PageBasedInfo, error) {🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
api/e2e/gql_pagination_test.go(1 hunks)api/internal/adapter/gql/gqlmodel/convert.go(3 hunks)api/internal/infrastructure/fs/file.go(2 hunks)api/internal/infrastructure/memory/asset.go(3 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/job.go(3 hunks)api/internal/infrastructure/memory/project.go(2 hunks)api/internal/infrastructure/memory/trigger.go(2 hunks)api/internal/infrastructure/mongo/asset.go(3 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)api/internal/infrastructure/mongo/job.go(3 hunks)api/internal/infrastructure/mongo/mongodoc/job.go(2 hunks)api/internal/infrastructure/mongo/project.go(3 hunks)api/internal/infrastructure/mongo/trigger.go(2 hunks)api/internal/usecase/interactor/asset.go(1 hunks)api/internal/usecase/interactor/deployment.go(3 hunks)api/internal/usecase/interactor/job.go(1 hunks)api/internal/usecase/interactor/project.go(1 hunks)api/internal/usecase/interactor/trigger.go(1 hunks)api/internal/usecase/interfaces/asset.go(1 hunks)api/internal/usecase/interfaces/deployment.go(1 hunks)api/internal/usecase/interfaces/job.go(1 hunks)api/internal/usecase/interfaces/project.go(1 hunks)api/internal/usecase/interfaces/trigger.go(1 hunks)api/internal/usecase/repo/asset.go(1 hunks)api/internal/usecase/repo/deployment.go(3 hunks)api/internal/usecase/repo/job.go(2 hunks)api/internal/usecase/repo/project.go(3 hunks)api/internal/usecase/repo/trigger.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- api/internal/usecase/interactor/asset.go
- api/internal/usecase/interactor/job.go
- api/internal/usecase/interfaces/trigger.go
- api/internal/usecase/repo/trigger.go
- api/internal/usecase/interfaces/asset.go
- api/internal/usecase/interfaces/job.go
- api/internal/usecase/interfaces/project.go
- api/internal/usecase/interactor/deployment.go
- api/e2e/gql_pagination_test.go
- api/internal/usecase/repo/asset.go
- api/internal/infrastructure/mongo/trigger.go
🧰 Additional context used
🪛 GitHub Actions: ci
api/internal/infrastructure/memory/deployment.go
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/memory/job.go
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/memory/project.go
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/memory/asset.go
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/memory/trigger.go
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/mongo/job.go
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/mongo/asset.go
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/mongo/project.go
[warning] Package test coverage is only 0.1% of statements
api/internal/adapter/gql/gqlmodel/convert.go
[error] 85-85: Function 'intToInt64' is unused
api/internal/infrastructure/mongo/mongodoc/job.go
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/mongo/deployment.go
[warning] Package test coverage is only 0.1% of statements
🪛 golangci-lint (1.62.2)
api/internal/infrastructure/mongo/deployment.go
233-233: func (*Deployment).paginate is unused
(unused)
🪛 GitHub Check: ci-api / ci-api-lint
api/internal/infrastructure/mongo/deployment.go
[failure] 233-233:
func (*Deployment).paginate is unused (unused)
🔇 Additional comments (24)
api/internal/infrastructure/memory/trigger.go (4)
5-5: LGTM: Import changes are appropriate.The added imports support the new sorting and pagination functionality.
Also applies to: 8-8
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
51-53: Use consistent pagination parameters for empty results.Similar to the access denied case, empty results should use the requested pagination parameters.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
85-104:⚠️ Potential issueAdd comprehensive pagination parameter validation.
The pagination implementation needs validation to prevent potential issues:
- Negative or zero page numbers
- Negative or zero page sizes
- Integer overflow in skip calculation
if pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be positive") + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize must be positive") + } + // Check for potential integer overflow + if pagination.Page.Page > 1 && pagination.Page.PageSize > 0 { + maxSkip := len(result) + if (maxSkip/pagination.Page.PageSize)+1 < pagination.Page.Page { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page number too large") + } + } + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSizeLikely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
35-106: Improve test coverage for pagination and sorting logic.The current test coverage of 14.3% is concerning given the complexity of the pagination and sorting implementation. Please add comprehensive tests covering:
- Pagination parameter validation
- Sorting with different fields and directions
- Edge cases (empty results, last page, etc.)
- Error cases (invalid parameters)
✅ Verification successful
Test coverage improvement needed for memory.Trigger implementation
The verification confirms that the memory implementation of
Trigger.FindByWorkspacelacks test coverage. No test file exists for this implementation, while similar functionality in other packages (e.g., deployment) has test coverage.Critical test scenarios needed:
- Pagination with different page sizes and page numbers
- Sorting by createdAt (both ASC and DESC)
- Edge cases: empty results, last page scenarios
- Access control validation via
CanRead- Default sorting fallback to ID
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze test coverage for the trigger package # Check existing test files echo "Existing test files for trigger package:" fd -e go -e _test.go trigger # Check test coverage details go test -coverprofile=coverage.out ./... go tool cover -func=coverage.out | grep -E "trigger.*FindByWorkspace"Length of output: 1136
Script:
#!/bin/bash # Check for test file existence and content in memory package echo "=== Checking for test file in memory package ===" fd -e go -e _test.go trigger api/internal/infrastructure/memory/ echo -e "\n=== Looking for test cases for FindByWorkspace ===" rg -A 5 "TestFindByWorkspace|func.*FindByWorkspace.*test" api/internal/infrastructure/memory/Length of output: 940
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/fs/file.go (3)
4-20: LGTM! Imports are appropriate for the added functionality.
61-66: LGTM! Good use of code reuse and path sanitization.
69-77: LGTM! Secure implementation with proper validation.api/internal/infrastructure/mongo/mongodoc/job.go (2)
13-20: LGTM! BSON tags are properly defined.The BSON tags follow MongoDB naming conventions and are consistently applied across all fields.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
66-67: Add unit tests to improve coverage.The pipeline shows critically low test coverage (0.1%). Please add unit tests for the
Modelmethod, especially covering the newGCPJobIDfield handling.Would you like me to help generate comprehensive unit tests for this struct?
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/usecase/repo/job.go (1)
24-29: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The page size is used without validation. Consider adding checks for negative or zero values.
pagination := &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: page, - PageSize: int(batch), + PageSize: validatePageSize(int(batch)), }, } +func validatePageSize(size int) int { + if size <= 0 { + return 10 // default page size + } + return size +}Likely invalid or redundant comment.
api/internal/usecase/repo/deployment.go (2)
28-33: Extract pagination logic to a shared utility.The pagination initialization code is duplicated.
Use the shared utility suggested in project.go review.
Line range hint
12-22: LGTM! Interface changes are consistent.The interface changes for pagination are consistently applied across all repository interfaces.
api/internal/usecase/interfaces/deployment.go (2)
35-35: LGTM: Error variable for invalid pagination.The error variable is appropriately defined for handling invalid pagination parameters.
42-42: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The interface method should document the validation requirements for the pagination parameters.
Add documentation to clarify validation requirements:
+// FindByWorkspace finds deployments by workspace ID. +// Returns ErrInvalidPagination if: +// - Page.Page is less than 1 +// - Page.PageSize is less than 1 +// - Both Cursor and Page are set FindByWorkspace(context.Context, accountdomain.WorkspaceID, *PaginationParam, *usecase.Operator) ([]*deployment.Deployment, *PageBasedInfo, error)Likely invalid or redundant comment.
api/internal/infrastructure/mongo/job.go (1)
82-105: 🛠️ Refactor suggestionImprove pagination and sorting implementation.
Several improvements needed:
- Missing pagination parameter validation
- Magic strings used for field names
- No validation of order direction
Apply this diff to address the issues:
+const ( + fieldStartedAt = "startedat" + fieldCompletedAt = "completedat" + defaultSortField = fieldStartedAt +) + if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page number must be positive")) + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size must be positive")) + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size cannot exceed 100")) + } + // Page-based pagination skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize) // Set up sort options var sort bson.D if pagination.Page.OrderBy != nil { dir := 1 - if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { - dir = -1 + if pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC": + dir = 1 + case "DESC": + dir = -1 + default: + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("invalid order direction: must be ASC or DESC")) + } } // Convert field name to MongoDB field name (lowercase) fieldName := *pagination.Page.OrderBy if fieldName == "startedAt" { - fieldName = "startedat" + fieldName = fieldStartedAt } else if fieldName == "completedAt" { - fieldName = "completedat" + fieldName = fieldCompletedAt + } else { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("invalid order field: %s", fieldName)) } sort = bson.D{{Key: fieldName, Value: dir}} } else { // Default sort by startedAt desc - sort = bson.D{{Key: "startedat", Value: -1}} + sort = bson.D{{Key: defaultSortField, Value: -1}} }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/memory/deployment.go (1)
56-70:⚠️ Potential issueAdd input validation and overflow protection.
The pagination implementation needs several improvements:
- Missing pagination parameter validation
- Potential integer overflow in skip calculation
- No handling of negative values
Apply this diff to address the issues:
if p != nil && p.Page != nil { + // Validate pagination parameters + if p.Page.Page < 1 { + return nil, nil, fmt.Errorf("page number must be positive") + } + if p.Page.PageSize < 1 { + return nil, nil, fmt.Errorf("page size must be positive") + } + if p.Page.PageSize > 100 { + return nil, nil, fmt.Errorf("page size cannot exceed 100") + } + + // Check for potential integer overflow + if p.Page.Page > (1<<31-1)/p.Page.PageSize { + return nil, nil, fmt.Errorf("page number too large: would cause integer overflow") + } + skip := (p.Page.Page - 1) * p.Page.PageSize if skip >= len(result) { return nil, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
api/internal/infrastructure/mongo/asset.go (1)
128-135:⚠️ Potential issueAdd input validation for pagination parameters.
The pagination implementation needs validation to prevent invalid inputs.
Apply this diff to add validation:
func (r *Asset) paginate(ctx context.Context, filter any, sort *asset.SortType, pagination *interfaces.PaginationParam) ([]*asset.Asset, *interfaces.PageBasedInfo, error) { c := mongodoc.NewAssetConsumer(r.f.Readable) if pagination != nil && pagination.Page != nil { + // Validate pagination parameters + if pagination.Page.Page < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page number must be positive")) + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size must be positive")) + } + if pagination.Page.PageSize > 100 { + return nil, nil, rerror.ErrInternalByWithContext(ctx, fmt.Errorf("page size cannot exceed 100")) + } + // Page-based pagination skip := (pagination.Page.Page - 1) * pagination.Page.PageSize limit := pagination.Page.PageSizeLikely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/usecase/interactor/trigger.go (1)
49-52: 🛠️ Refactor suggestionAdd pagination parameter validation.
While the workspace permission check is good, the method should also validate pagination parameters.
Apply this diff to add validation:
func (i *Trigger) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, p *interfaces.PaginationParam, operator *usecase.Operator) ([]*trigger.Trigger, *interfaces.PageBasedInfo, error) { if err := i.CanReadWorkspace(id, operator); err != nil { return nil, nil, err } + + if p != nil && p.Page != nil { + if p.Page.Page < 1 { + return nil, nil, fmt.Errorf("page number must be positive") + } + if p.Page.PageSize < 1 { + return nil, nil, fmt.Errorf("page size must be positive") + } + if p.Page.PageSize > 100 { + return nil, nil, fmt.Errorf("page size cannot exceed 100") + } + } + return i.triggerRepo.FindByWorkspace(ctx, id, p) }Likely invalid or redundant comment.
api/internal/infrastructure/mongo/deployment.go (3)
79-111:⚠️ Potential issueRemove debug statements and add input validation.
The implementation needs cleanup and validation:
- Remove debug print statements
- Add input validation for pagination parameters
- Add error handling for negative page numbers
func (r *DeploymentAdapter) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { if !r.f.CanRead(id) { return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil } + // Validate pagination parameters + if pagination != nil && pagination.Page != nil { + if pagination.Page.Page < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page number must be positive") + } + if pagination.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page size must be positive") + } + } c := mongodoc.NewDeploymentConsumer(r.f.Readable) filter := bson.M{"workspace": id.String()} if pagination != nil && pagination.Page != nil { // Page-based pagination skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) limit := int64(pagination.Page.PageSize) // Get total count for page info total, err := r.client.Count(ctx, filter) if err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } // Execute find with skip and limit opts := options.Find().SetSkip(skip).SetLimit(limit) if err := r.client.Find(ctx, filter, c, opts); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } - // Print debug info for results - fmt.Printf("DEBUG: Found %d results\n", len(c.Result)) - for _, p := range c.Result { - fmt.Printf("DEBUG: Project name=%s\n", p.Name()) - } return c.Result, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize), nil } if err := r.client.Find(ctx, filter, c); err != nil { return nil, nil, rerror.ErrInternalByWithContext(ctx, err) } total := int64(len(c.Result)) return c.Result, interfaces.NewPageBasedInfo(total, 1, len(c.Result)), nil }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
173-192:⚠️ Potential issueAdd optimistic locking to prevent race conditions.
The Update method is susceptible to race conditions in concurrent scenarios.
func (r *Deployment) Update(ctx context.Context, param interfaces.UpdateDeploymentParam) (*deployment.Deployment, error) { d, err := r.FindByID(ctx, param.ID) if err != nil { return nil, err } + // Get current version for optimistic locking + currentVersion := d.Version() + if param.Description != nil { d.SetDescription(*param.Description) } if param.Workflow != nil { d.SetWorkflowURL(param.Workflow.Path) } - if err := r.Save(ctx, d); err != nil { + // Update version and save with optimistic locking + d.SetVersion(currentVersion + 1) + if err := r.SaveWithVersion(ctx, d, currentVersion); err != nil { return nil, err } return d, nil }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
233-285: 🛠️ Refactor suggestionRemove or utilize the unused paginate method.
The
paginatemethod is well-implemented but unused according to static analysis.Let's verify if this method is truly unused:
If the method is confirmed unused, consider either:
- Removing it if it's no longer needed
- Using it in the FindByWorkspace implementation to reduce code duplication
✅ Verification successful
Confirmed: The paginate method is unused and duplicates existing logic
The paginate method in Deployment struct is indeed unused, while similar pagination logic is implemented directly in FindByWorkspace. Consider:
- Using the paginate method in FindByWorkspace to reduce code duplication, or
- Removing the unused paginate method if there are no plans to reuse it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of the paginate method echo "=== Searching for paginate method usage ===" rg -l "paginate\(" --type go echo -e "\n=== Checking if method is referenced in tests ===" rg -l "paginate" --type go --glob "*_test.go"Length of output: 424
Script:
#!/bin/bash # Check the context of paginate occurrences echo "=== Checking paginate usage in project.go ===" rg "paginate" api/internal/infrastructure/mongo/project.go -B 2 -A 2 echo -e "\n=== Checking paginate usage in asset.go ===" rg "paginate" api/internal/infrastructure/mongo/asset.go -B 2 -A 2 echo -e "\n=== Checking paginate usage in deployment.go ===" rg "paginate" api/internal/infrastructure/mongo/deployment.go -B 2 -A 2 # Look for potential method calls that might use paginate echo -e "\n=== Checking for potential calls to methods that might use paginate ===" rg "FindByWorkspace|List" api/internal/infrastructure/mongo/deployment.go -B 2 -A 2Length of output: 1920
Script:
#!/bin/bash # Get the full implementation of FindByWorkspace echo "=== Checking FindByWorkspace implementation ===" ast-grep --pattern 'FindByWorkspace($$$) { $$$ }' api/internal/infrastructure/mongo/deployment.go # Also check DeploymentAdapter implementation to understand the relationship echo -e "\n=== Checking DeploymentAdapter structure ===" ast-grep --pattern 'type DeploymentAdapter struct { $$$ }'Length of output: 574
Script:
#!/bin/bash # Get FindByWorkspace implementation with more context echo "=== Getting FindByWorkspace implementation ===" rg -U "func \(r \*DeploymentAdapter\) FindByWorkspace" api/internal/infrastructure/mongo/deployment.go -B 2 -A 15 # Also check if there are any other pagination-related implementations echo -e "\n=== Checking for other pagination implementations ===" rg "Pagination|PageInfo|PageBased" api/internal/infrastructure/mongo/deployment.go -B 2 -A 2Length of output: 2121
🧰 Tools
🪛 golangci-lint (1.62.2)
233-233: func
(*Deployment).paginateis unused(unused)
🪛 GitHub Check: ci-api / ci-api-lint
[failure] 233-233:
func(*Deployment).paginateis unused (unused)🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
api/internal/infrastructure/memory/project.go (3)
36-54: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The implementation should validate pagination parameters before processing to prevent potential issues.
Add validation at the beginning of the method:
func (r *Project) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*project.Project, *interfaces.PageBasedInfo, error) { + // Validate pagination parameters + if pagination != nil && pagination.Page != nil { + if pagination.Page.Page <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be positive") + } + if pagination.Page.PageSize <= 0 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize must be positive") + } + // Prevent potential integer overflow + if pagination.Page.Page > 1 && pagination.Page.PageSize > len(r.data)/(pagination.Page.Page-1) { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pagination parameters would cause integer overflow") + } + }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
56-84: 🛠️ Refactor suggestionImprove sorting implementation.
The sorting implementation needs validation and could be more robust:
- Missing validation for order direction values
- Missing validation for orderBy field values
- No error handling for invalid field comparisons
Apply this diff to improve the sorting implementation:
+ // Validate sort parameters + if pagination != nil && pagination.Page != nil { + if pagination.Page.OrderDir != nil { + switch *pagination.Page.OrderDir { + case "ASC", "DESC": + if *pagination.Page.OrderDir == "DESC" { + direction = -1 + } + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "orderDir must be either ASC or DESC") + } + } + if pagination.Page.OrderBy != nil { + switch *pagination.Page.OrderBy { + case "name", "createdAt": + // valid fields + default: + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "invalid orderBy field") + } + } + } sort.Slice(result, func(i, j int) bool { if pagination != nil && pagination.Page != nil && pagination.Page.OrderBy != nil { switch *pagination.Page.OrderBy { case "name": ni, nj := result[i].Name(), result[j].Name() if ni != nj { if direction == 1 { return ni < nj } return ni > nj } case "createdAt": ti, tj := result[i].CreatedAt(), result[j].CreatedAt() if !ti.Equal(tj) { if direction == 1 { return ti.Before(tj) } return ti.After(tj) } } } - return result[i].ID().String() < result[j].ID().String() + // Always include ID in sort for stable ordering + if direction == 1 { + return result[i].ID().String() < result[j].ID().String() + } + return result[i].ID().String() > result[j].ID().String() })Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
90-101: Add test coverage for pagination logic.The pagination implementation lacks sufficient test coverage (14.3%). Add test cases to verify:
- Basic pagination functionality
- Edge cases (empty results, last page)
- Error cases (invalid parameters)
Run this script to analyze current test coverage:
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 14.3% of statements
| func (f *fileRepo) GetJobLogURL(jobID string) string { | ||
| return fmt.Sprintf("file://%s/job-%s.log", metadataDir, jobID) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URL generation for job logs.
Two issues to address:
- Add path sanitization for jobID
- Use the workflowUrlBase instead of hardcoding the file:// scheme
Apply this diff to improve the implementation:
func (f *fileRepo) GetJobLogURL(jobID string) string {
- return fmt.Sprintf("file://%s/job-%s.log", metadataDir, jobID)
+ return getFileURL(f.workflowUrlBase, fmt.Sprintf("job-%s.log", sanitize.Path(jobID))).String()
}Committable suggestion skipped: line range outside the PR's diff.
| fmt.Printf("DEBUG: Found %d results\n", len(c.Result)) | ||
| for _, p := range c.Result { | ||
| fmt.Printf("DEBUG: Project name=%s\n", p.Name()) | ||
| } |
There was a problem hiding this comment.
Remove debug print statements.
Debug print statements should not be in production code.
Remove the debug logging:
- // Print debug info for results
- fmt.Printf("DEBUG: Found %d results\n", len(c.Result))
- for _, p := range c.Result {
- fmt.Printf("DEBUG: Project name=%s\n", p.Name())
- }🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
| // Default sort by updatedAt desc | ||
| sort := bson.D{{Key: "updatedat", Value: -1}} | ||
|
|
||
| // Handle custom sorting | ||
| if pagination.Page.OrderBy != nil { | ||
| sortDir := -1 // default DESC | ||
| if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "ASC" { | ||
| sortDir = 1 | ||
| } | ||
| // Convert field name to MongoDB field name (lowercase) | ||
| field := *pagination.Page.OrderBy | ||
| if field == "name" { | ||
| field = "name" | ||
| } else if field == "createdAt" { | ||
| field = "createdat" | ||
| } else if field == "updatedAt" { | ||
| field = "updatedat" | ||
| } | ||
| sort = bson.D{{Key: field, Value: sortDir}} | ||
| fmt.Printf("DEBUG: Sorting with field=%s, direction=%d\n", field, sortDir) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent field names and improve field mapping.
The field mapping for MongoDB has several issues:
- Inconsistent field names (e.g., "updatedat" vs "updatedAt")
- Redundant field mapping logic
- Debug print in production code
Apply this diff to improve the field mapping:
- // Default sort by updatedAt desc
- sort := bson.D{{Key: "updatedat", Value: -1}}
+ // Define MongoDB field mapping
+ fieldMap := map[string]string{
+ "name": "name",
+ "createdAt": "createdat",
+ "updatedAt": "updatedat",
+ }
+
+ // Default sort by updatedAt desc
+ sort := bson.D{{Key: fieldMap["updatedAt"], Value: -1}}
// Handle custom sorting
if pagination.Page.OrderBy != nil {
sortDir := -1 // default DESC
if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "ASC" {
sortDir = 1
}
- // Convert field name to MongoDB field name (lowercase)
- field := *pagination.Page.OrderBy
- if field == "name" {
- field = "name"
- } else if field == "createdAt" {
- field = "createdat"
- } else if field == "updatedAt" {
- field = "updatedat"
+
+ // Get MongoDB field name
+ field, ok := fieldMap[*pagination.Page.OrderBy]
+ if !ok {
+ return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "invalid orderBy field")
}
sort = bson.D{{Key: field, Value: sortDir}}
- fmt.Printf("DEBUG: Sorting with field=%s, direction=%d\n", field, sortDir)
}📝 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.
| // Default sort by updatedAt desc | |
| sort := bson.D{{Key: "updatedat", Value: -1}} | |
| // Handle custom sorting | |
| if pagination.Page.OrderBy != nil { | |
| sortDir := -1 // default DESC | |
| if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "ASC" { | |
| sortDir = 1 | |
| } | |
| // Convert field name to MongoDB field name (lowercase) | |
| field := *pagination.Page.OrderBy | |
| if field == "name" { | |
| field = "name" | |
| } else if field == "createdAt" { | |
| field = "createdat" | |
| } else if field == "updatedAt" { | |
| field = "updatedat" | |
| } | |
| sort = bson.D{{Key: field, Value: sortDir}} | |
| fmt.Printf("DEBUG: Sorting with field=%s, direction=%d\n", field, sortDir) | |
| } | |
| // Define MongoDB field mapping | |
| fieldMap := map[string]string{ | |
| "name": "name", | |
| "createdAt": "createdat", | |
| "updatedAt": "updatedat", | |
| } | |
| // Default sort by updatedAt desc | |
| sort := bson.D{{Key: fieldMap["updatedAt"], Value: -1}} | |
| // Handle custom sorting | |
| if pagination.Page.OrderBy != nil { | |
| sortDir := -1 // default DESC | |
| if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "ASC" { | |
| sortDir = 1 | |
| } | |
| // Get MongoDB field name | |
| field, ok := fieldMap[*pagination.Page.OrderBy] | |
| if !ok { | |
| return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "invalid orderBy field") | |
| } | |
| sort = bson.D{{Key: field, Value: sortDir}} | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[warning] Package test coverage is only 0.1% of statements
This commit addresses the sorting and pagination order in the memory deployment repository by: - Adding explicit sorting of deployments by version in ascending order - Updating test cases to reflect the correct deployment order - Ensuring consistent pagination behavior in FindByWorkspace method
…methods This commit removes unused imports and commented-out pagination methods across multiple files, cleaning up code and reducing unnecessary dependencies.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
api/internal/infrastructure/memory/deployment_test.go (2)
110-114:⚠️ Potential issueSet UpdatedAt field in test data.
The test data should include UpdatedAt timestamps to ensure consistent sorting behavior.
116-199:⚠️ Potential issueAdd test cases for error scenarios and sorting behavior.
The test suite is missing cases for invalid pagination parameters and custom sorting.
api/internal/infrastructure/mongo/project.go (2)
89-109:⚠️ Potential issueFix inconsistent field names and improve field mapping.
The field mapping for MongoDB has several issues:
- Inconsistent field names (e.g., "updatedat" vs "updatedAt")
- Redundant field mapping logic
- Debug print in production code
122-125:⚠️ Potential issueRemove debug print statements.
Debug print statements should not be in production code.
api/internal/infrastructure/mongo/deployment.go (1)
172-191:⚠️ Potential issueAddress potential race condition in Update method.
The Update method is susceptible to lost updates in concurrent scenarios. Consider implementing optimistic locking or using MongoDB transactions to prevent this issue.
api/internal/adapter/gql/gqlmodel/convert.go (1)
54-74:⚠️ Potential issueRemove duplicate function and add input validation.
ToPageBasedPaginationParamis identical toToPageBasedPagination.- Both functions lack input validation for pagination parameters.
Apply this diff to fix both issues:
-func ToPageBasedPaginationParam(pagination PageBasedPagination) *interfaces.PaginationParam { - return &interfaces.PaginationParam{ - Page: &interfaces.PageBasedPaginationParam{ - Page: pagination.Page, - PageSize: pagination.PageSize, - OrderBy: pagination.OrderBy, - OrderDir: OrderDirectionToString(pagination.OrderDir), - }, - } -} func ToPageBasedPagination(pagination PageBasedPagination) *interfaces.PaginationParam { + // Ensure positive page and page size + if pagination.Page <= 0 { + pagination.Page = 1 + } + if pagination.PageSize <= 0 { + pagination.PageSize = 10 // default page size + } + return &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: pagination.Page, PageSize: pagination.PageSize, OrderBy: pagination.OrderBy, OrderDir: OrderDirectionToString(pagination.OrderDir), }, } }
🧹 Nitpick comments (8)
api/internal/infrastructure/memory/deployment.go (1)
56-59: Add support for custom sorting fields and directions.The current implementation only sorts by version in ascending order. Consider adding support for custom sorting fields and directions.
Add support for custom sorting:
- // Sort by version in ascending order - sort.Slice(result, func(i, j int) bool { - return result[i].Version() < result[j].Version() - }) + // Default sort by updatedAt desc + sort.Slice(result, func(i, j int) bool { + if p != nil && p.Page != nil && p.Page.OrderBy != nil { + switch *p.Page.OrderBy { + case "version": + if result[i].Version() != result[j].Version() { + return (result[i].Version() < result[j].Version()) == (p.Page.OrderDir != nil && *p.Page.OrderDir == "ASC") + } + case "updatedAt": + if !result[i].UpdatedAt().Equal(result[j].UpdatedAt()) { + return (result[i].UpdatedAt().Before(result[j].UpdatedAt())) == (p.Page.OrderDir != nil && *p.Page.OrderDir == "ASC") + } + } + } + // Use ID as tie-breaker for stable sorting + return result[i].ID().String() < result[j].ID().String() + })api/e2e/gql_project_test.go (2)
286-288: Consider using test cleanup.The test creates multiple projects but doesn't clean them up after the test completes.
Add cleanup logic:
+ t.Cleanup(func() { + for _, id := range projectIDs { + testDeleteProject(t, e, id) + } + })
347-358: Enhance pagination-specific assertions.While the basic assertions are good, consider adding more comprehensive checks for pagination behavior.
Add additional assertions:
// Verify the response projects := result.Data.ProjectsPage assert.NotNil(t, projects.Nodes) assert.Len(t, projects.Nodes, 2) // Should get 2 projects per page + // Verify sorting + assert.Equal(t, "Test Project 0", projects.Nodes[0].Name) + assert.Equal(t, "Test Project 1", projects.Nodes[1].Name) + + // Verify pagination info assert.Equal(t, 3, projects.PageInfo.TotalCount) assert.Equal(t, 2, projects.PageInfo.TotalPages) assert.Equal(t, 1, projects.PageInfo.CurrentPage) + + // Verify second page + // TODO: Add test for second page to ensure all projects are accessibleapi/internal/infrastructure/mongo/deployment.go (1)
24-24: Consider making the client field private.The
clientfield in theDeploymentstruct is exported but should be private since it's an implementation detail. This maintains better encapsulation and prevents external code from directly accessing the MongoDB client.type Deployment struct { - client *mongox.Collection + client_ *mongox.Collection f repo.WorkspaceFilter }Also applies to: 28-30
api/internal/adapter/gql/gqlmodel/convert.go (1)
84-89: Remove commented out code.The commented out
intToInt64function should be removed if it's not needed. Keeping commented out code makes the codebase harder to maintain.api/internal/infrastructure/mongo/deployment_test.go (3)
58-61: Consider using timestamp constants for better test readability.While the test data setup is good, using timestamp constants would make the ordering more explicit and the test intentions clearer.
+const ( + baseTime = time.Now() + twoHoursAgo = baseTime.Add(-2 * time.Hour) + oneHourAgo = baseTime.Add(-1 * time.Hour) +) _, _ = c.Collection("deployment").InsertMany(ctx, []any{ - bson.M{"id": "d1", "workspaceid": wid.String(), "version": "v1", "updatedat": time.Now().Add(-2 * time.Hour)}, - bson.M{"id": "d2", "workspaceid": wid.String(), "version": "v2", "updatedat": time.Now().Add(-1 * time.Hour)}, - bson.M{"id": "d3", "workspaceid": wid.String(), "version": "v3", "updatedat": time.Now()}, - bson.M{"id": "d4", "workspaceid": wid2.String(), "version": "v1", "updatedat": time.Now()}, + bson.M{"id": "d1", "workspaceid": wid.String(), "version": "v1", "updatedat": twoHoursAgo}, + bson.M{"id": "d2", "workspaceid": wid.String(), "version": "v2", "updatedat": oneHourAgo}, + bson.M{"id": "d3", "workspaceid": wid.String(), "version": "v3", "updatedat": baseTime}, + bson.M{"id": "d4", "workspaceid": wid2.String(), "version": "v1", "updatedat": baseTime},
66-124: Add test cases for pagination edge cases.While the current test coverage is good for basic scenarios, consider adding test cases for:
- Invalid page sizes (0, negative)
- Invalid page numbers (0, negative)
- Very large page sizes (exceeding total count)
Example test case:
// Test invalid pagination parameters pagination := &interfaces.PaginationParam{ Page: &interfaces.PageBasedPaginationParam{ Page: -1, // or 0 PageSize: -1, // or 0 }, } got, pageInfo, err := r.FindByWorkspace(ctx, wid, pagination) assert.Error(t, err) // or assert specific behavior defined in requirements
126-136: Add test case for mixed workspace access.Consider adding a test case where the readable workspace list includes partial access to verify correct filtering behavior.
// Test with mixed workspace access r2 := r.Filtered(repo.WorkspaceFilter{ Readable: accountdomain.WorkspaceIDList{wid, wid2}, // Access to both workspaces }) got, pageInfo, err = r2.FindByWorkspace(ctx, wid, pagination) assert.NoError(t, err) assert.NotNil(t, pageInfo) assert.Equal(t, 3, len(got)) // Should return all items from wid
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/e2e/gql_project_test.go(3 hunks)api/internal/adapter/gql/context.go(1 hunks)api/internal/adapter/gql/gqlmodel/convert.go(2 hunks)api/internal/infrastructure/memory/deployment.go(2 hunks)api/internal/infrastructure/memory/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/deployment.go(4 hunks)api/internal/infrastructure/mongo/deployment_test.go(2 hunks)api/internal/infrastructure/mongo/project.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/internal/adapter/gql/context.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci-api / ci-api-test
🔇 Additional comments (8)
api/internal/infrastructure/memory/deployment.go (1)
36-41: 🛠️ Refactor suggestionAdd input validation for pagination parameters.
The method should validate pagination parameters to prevent potential issues with negative page numbers or page sizes.
Add validation before processing:
func (r *Deployment) FindByWorkspace(_ context.Context, wid accountdomain.WorkspaceID, p *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { + if p != nil && p.Page != nil { + if p.Page.Page < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "page must be greater than 0") + } + if p.Page.PageSize < 1 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize must be greater than 0") + } + if p.Page.PageSize > 100 { + return nil, nil, rerror.NewE(rerror.ErrInvalidParameter, "pageSize cannot exceed 100") + } + } r.lock.Lock() defer r.lock.Unlock()Likely invalid or redundant comment.
api/internal/infrastructure/mongo/deployment.go (2)
Line range hint
1-300: Fix unkeyed composite literals in MongoDB queries.Previous pipeline failures indicate issues with unkeyed composite literals in MongoDB queries. Ensure all
bson.Dliterals use keyed fields.
Line range hint
1-300: Add unit tests for pagination functionality.The new pagination implementation requires comprehensive test coverage to ensure correct behavior, especially around edge cases and input validation.
api/internal/adapter/gql/gqlmodel/convert.go (4)
24-33: Use safe pointer conversions for CurrentPage and TotalPages.Direct pointer assignments could be unsafe if the fields are zero values.
Apply this diff to safely handle the pointer conversions:
func ToPageInfo(p *interfaces.PageBasedInfo) *PageInfo { if p == nil { return nil } return &PageInfo{ TotalCount: int(p.TotalCount), - CurrentPage: &p.CurrentPage, - TotalPages: &p.TotalPages, + CurrentPage: lo.ToPtr(p.CurrentPage), + TotalPages: lo.ToPtr(p.TotalPages), } }
39-51: Add input validation and overflow protection.The offset calculation could overflow for large page numbers, and there's no validation of input parameters.
Apply this diff to add validation and overflow protection:
func ToPagination(pagination *Pagination) *usecasex.Pagination { if pagination == nil { return nil } if pagination.Page != nil && pagination.PageSize != nil { + // Validate input + if *pagination.Page <= 0 || *pagination.PageSize <= 0 { + return nil + } + + // Check for potential overflow + if *pagination.Page > 1 && *pagination.PageSize > math.MaxInt64/(*pagination.Page-1) { + return nil + } + return &usecasex.Pagination{ Offset: &usecasex.OffsetPagination{ Offset: int64((*pagination.Page - 1) * *pagination.PageSize), Limit: int64(*pagination.PageSize), }, } } return nil }
76-82: Add validation for order direction.The function should validate that the order direction is either "ASC" or "DESC".
Apply this diff:
func OrderDirectionToString(dir *OrderDirection) *string { if dir == nil { return nil } + switch *dir { + case OrderDirectionAsc, OrderDirectionDesc: + s := string(*dir) + return &s + default: + return lo.ToPtr("ASC") // default to ascending + } - s := string(*dir) - return &s }
91-97: Verify the intentional removal of page-based fields.The function no longer handles page-based fields (CurrentPage, TotalPages). Please confirm if this is intentional as part of the pagination changes.
Run this script to check if page-based fields are needed in the conversion:
✅ Verification successful
Page-based fields removal is intentional
The removal of
CurrentPageandTotalPagesfields from the conversion function aligns with the codebase's transition from page-based to cursor-based pagination in the GraphQL layer, where these fields are replaced byendCursorandhasNextPage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if page-based fields are used in usecasex.PageInfo # Search for usages of PageInfo with page-based fields rg -A 5 "usecasex\.PageInfo.*CurrentPage|usecasex\.PageInfo.*TotalPages"Length of output: 74
Script:
#!/bin/bash # Check PageInfo struct definitions and pagination patterns # Find PageInfo struct definitions echo "=== PageInfo struct definitions ===" ast-grep --pattern 'type PageInfo struct { $$$ }' # Look for pagination-related patterns echo -e "\n=== Pagination patterns ===" rg -g '!*.sum' -g '!*.mod' "pagination|page.*size|limit.*offset" -A 3Length of output: 66066
api/internal/infrastructure/mongo/deployment_test.go (1)
8-8: LGTM: Import added for pagination types.The new import is correctly added to support the pagination functionality.
| if p != nil && p.Page != nil { | ||
| skip := (p.Page.Page - 1) * p.Page.PageSize | ||
| if skip >= len(result) { | ||
| return nil, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | ||
| } | ||
|
|
||
| end := skip + p.Page.PageSize | ||
| if end > len(result) { | ||
| end = len(result) | ||
| } | ||
|
|
||
| return result[skip:end], interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle edge cases in pagination logic.
The pagination logic needs to handle edge cases better, such as when skip is equal to the result length.
Improve edge case handling:
if p != nil && p.Page != nil {
skip := (p.Page.Page - 1) * p.Page.PageSize
- if skip >= len(result) {
+ if skip > len(result) {
return nil, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil
}
+ if skip == len(result) {
+ return []*deployment.Deployment{}, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil
+ }📝 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.
| if p != nil && p.Page != nil { | |
| skip := (p.Page.Page - 1) * p.Page.PageSize | |
| if skip >= len(result) { | |
| return nil, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | |
| } | |
| end := skip + p.Page.PageSize | |
| if end > len(result) { | |
| end = len(result) | |
| } | |
| return result[skip:end], interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | |
| } | |
| if p != nil && p.Page != nil { | |
| skip := (p.Page.Page - 1) * p.Page.PageSize | |
| if skip > len(result) { | |
| return nil, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | |
| } | |
| if skip == len(result) { | |
| return []*deployment.Deployment{}, interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | |
| } | |
| end := skip + p.Page.PageSize | |
| if end > len(result) { | |
| end = len(result) | |
| } | |
| return result[skip:end], interfaces.NewPageBasedInfo(total, p.Page.Page, p.Page.PageSize), nil | |
| } |
| // func (r *Deployment) paginate(ctx context.Context, filter bson.M, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *usecasex.PageInfo, error) { | ||
| // c := mongodoc.NewDeploymentConsumer(r.f.Readable) | ||
|
|
||
| // if pagination != nil && pagination.Page != nil { | ||
| // // Page-based pagination | ||
| // skip := (pagination.Page.Page - 1) * pagination.Page.PageSize | ||
| // limit := pagination.Page.PageSize | ||
|
|
||
| // // Add sorting | ||
| // var sort bson.D | ||
| // if pagination.Page.OrderBy != nil { | ||
| // direction := 1 // default ascending | ||
| // if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { | ||
| // direction = -1 | ||
| // } | ||
| // sort = bson.D{{Key: *pagination.Page.OrderBy, Value: direction}} | ||
| // } else { | ||
| // // Default sort by updatedAt desc | ||
| // sort = bson.D{{Key: "updatedAt", Value: -1}} | ||
| // } | ||
|
|
||
| // // Get total count for page info | ||
| // total, err := r.client.Count(ctx, filter) | ||
| // if err != nil { | ||
| // return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| // } | ||
|
|
||
| // // Execute find with skip and limit | ||
| // opts := options.Find(). | ||
| // SetSort(sort). | ||
| // SetSkip(int64(skip)). | ||
| // SetLimit(int64(limit)) | ||
|
|
||
| // if err := r.client.Find(ctx, filter, c, opts); err != nil { | ||
| // return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| // } | ||
|
|
||
| // // Create page-based info | ||
| // pageInfo := interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize) | ||
|
|
||
| // return c.Result, pageInfo.ToPageInfo(), nil | ||
| // } | ||
|
|
||
| // // No pagination, return all results | ||
| // if err := r.client.Find(ctx, filter, c); err != nil { | ||
| // return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| // } | ||
|
|
||
| // return c.Result, &usecasex.PageInfo{ | ||
| // HasNextPage: false, | ||
| // HasPreviousPage: false, | ||
| // TotalCount: int64(len(c.Result)), | ||
| // }, nil | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out pagination code.
The commented-out pagination implementation should be removed since it's no longer used and can lead to confusion. If this code is kept for reference, consider moving it to documentation or a separate design document.
| func (r *DeploymentAdapter) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { | ||
| if !r.f.CanRead(id) { | ||
| return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil | ||
| } | ||
| return r.paginate(ctx, bson.M{ | ||
| "workspaceid": workspace.String(), | ||
| }, pagination) | ||
|
|
||
| c := mongodoc.NewDeploymentConsumer(r.f.Readable) | ||
| filter := bson.M{"workspace": id.String()} | ||
|
|
||
| if pagination != nil && pagination.Page != nil { | ||
| // Page-based pagination | ||
| skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) | ||
| limit := int64(pagination.Page.PageSize) | ||
|
|
||
| // Get total count for page info | ||
| total, err := r.client.Count(ctx, filter) | ||
| if err != nil { | ||
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| } | ||
|
|
||
| // Execute find with skip and limit | ||
| opts := options.Find().SetSkip(skip).SetLimit(limit) | ||
| if err := r.client.Find(ctx, filter, c, opts); err != nil { | ||
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| } | ||
|
|
||
| return c.Result, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize), nil | ||
| } | ||
|
|
||
| if err := r.client.Find(ctx, filter, c); err != nil { | ||
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | ||
| } | ||
| total := int64(len(c.Result)) | ||
| return c.Result, interfaces.NewPageBasedInfo(total, 1, len(c.Result)), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and sorting to pagination.
The current pagination implementation has several areas for improvement:
- Missing input validation for page and pageSize
- No sorting implementation
- Potential integer overflow in skip calculation
Consider this improved implementation:
func (r *DeploymentAdapter) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) {
if !r.f.CanRead(id) {
return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil
}
c := mongodoc.NewDeploymentConsumer(r.f.Readable)
filter := bson.M{"workspace": id.String()}
if pagination != nil && pagination.Page != nil {
+ // Validate input
+ if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 {
+ return nil, nil, rerror.ErrInvalidParameter
+ }
+
// Page-based pagination
- skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize)
+ skip := int64(pagination.Page.Page-1) * int64(pagination.Page.PageSize)
limit := int64(pagination.Page.PageSize)
+ // Add sorting
+ var sort bson.D
+ if pagination.Page.OrderBy != nil {
+ direction := 1
+ if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" {
+ direction = -1
+ }
+ sort = bson.D{{Key: *pagination.Page.OrderBy, Value: direction}}
+ } else {
+ sort = bson.D{{Key: "updatedAt", Value: -1}}
+ }
+
// Get total count for page info
total, err := r.client.Count(ctx, filter)
if err != nil {
return nil, nil, rerror.ErrInternalByWithContext(ctx, err)
}
// Execute find with skip and limit
- opts := options.Find().SetSkip(skip).SetLimit(limit)
+ opts := options.Find().
+ SetSort(sort).
+ SetSkip(skip).
+ SetLimit(limit)📝 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.
| func (r *DeploymentAdapter) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { | |
| if !r.f.CanRead(id) { | |
| return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil | |
| } | |
| return r.paginate(ctx, bson.M{ | |
| "workspaceid": workspace.String(), | |
| }, pagination) | |
| c := mongodoc.NewDeploymentConsumer(r.f.Readable) | |
| filter := bson.M{"workspace": id.String()} | |
| if pagination != nil && pagination.Page != nil { | |
| // Page-based pagination | |
| skip := int64((pagination.Page.Page - 1) * pagination.Page.PageSize) | |
| limit := int64(pagination.Page.PageSize) | |
| // Get total count for page info | |
| total, err := r.client.Count(ctx, filter) | |
| if err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| // Execute find with skip and limit | |
| opts := options.Find().SetSkip(skip).SetLimit(limit) | |
| if err := r.client.Find(ctx, filter, c, opts); err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| return c.Result, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize), nil | |
| } | |
| if err := r.client.Find(ctx, filter, c); err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| total := int64(len(c.Result)) | |
| return c.Result, interfaces.NewPageBasedInfo(total, 1, len(c.Result)), nil | |
| func (r *DeploymentAdapter) FindByWorkspace(ctx context.Context, id accountdomain.WorkspaceID, pagination *interfaces.PaginationParam) ([]*deployment.Deployment, *interfaces.PageBasedInfo, error) { | |
| if !r.f.CanRead(id) { | |
| return nil, interfaces.NewPageBasedInfo(0, 1, 1), nil | |
| } | |
| c := mongodoc.NewDeploymentConsumer(r.f.Readable) | |
| filter := bson.M{"workspace": id.String()} | |
| if pagination != nil && pagination.Page != nil { | |
| // Validate input | |
| if pagination.Page.Page < 1 || pagination.Page.PageSize < 1 { | |
| return nil, nil, rerror.ErrInvalidParameter | |
| } | |
| // Page-based pagination | |
| skip := int64(pagination.Page.Page-1) * int64(pagination.Page.PageSize) | |
| limit := int64(pagination.Page.PageSize) | |
| // Add sorting | |
| var sort bson.D | |
| if pagination.Page.OrderBy != nil { | |
| direction := 1 | |
| if pagination.Page.OrderDir != nil && *pagination.Page.OrderDir == "DESC" { | |
| direction = -1 | |
| } | |
| sort = bson.D{{Key: *pagination.Page.OrderBy, Value: direction}} | |
| } else { | |
| sort = bson.D{{Key: "updatedAt", Value: -1}} | |
| } | |
| // Get total count for page info | |
| total, err := r.client.Count(ctx, filter) | |
| if err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| // Execute find with skip and limit | |
| opts := options.Find(). | |
| SetSort(sort). | |
| SetSkip(skip). | |
| SetLimit(limit) | |
| if err := r.client.Find(ctx, filter, c, opts); err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| return c.Result, interfaces.NewPageBasedInfo(total, pagination.Page.Page, pagination.Page.PageSize), nil | |
| } | |
| if err := r.client.Find(ctx, filter, c); err != nil { | |
| return nil, nil, rerror.ErrInternalByWithContext(ctx, err) | |
| } | |
| total := int64(len(c.Result)) | |
| return c.Result, interfaces.NewPageBasedInfo(total, 1, len(c.Result)), nil |
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Based on the comprehensive changes, here are the updated release notes:
Pagination Overhaul
GraphQL Schema Updates
PageBasedPaginationinput.Backend Improvements