fix(deployment): require state filter for offset-based pagination#3038
Open
ygrishajev wants to merge 1 commit intomainfrom
Open
fix(deployment): require state filter for offset-based pagination#3038ygrishajev wants to merge 1 commit intomainfrom
ygrishajev wants to merge 1 commit intomainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors deployment listing to use a new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3038 +/- ##
==========================================
- Coverage 59.63% 59.63% -0.01%
==========================================
Files 1034 1034
Lines 24248 24252 +4
Branches 6009 6012 +3
==========================================
+ Hits 14461 14462 +1
- Misses 8536 8539 +3
Partials 1251 1251
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts (1)
141-142:⚠️ Potential issue | 🟡 MinorAvoid using
anytype cast.Line 141 casts to
anywhich violates the coding guidelines.🛠️ Proposed fix
return new AxiosError("HTTP Error", undefined, undefined, undefined, { status, data: {}, statusText: "Error", headers: {}, - config: {} as any + config: { headers: {} } as AxiosRequestConfig });You'll need to import
AxiosRequestConfigfrom axios, or use a more complete mock object.As per coding guidelines:
**/*.{ts,tsx,js}: Never use typeanyor cast to typeany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts` around lines 141 - 142, Replace the unsafe cast "{} as any" used for the config in the deployment-reader test by importing and using the proper AxiosRequestConfig type (or constructing a minimal/mock AxiosRequestConfig object) so the test no longer uses type any; update the spec to import AxiosRequestConfig from "axios" and change the config declaration passed to the DeploymentReader test setup (the object currently cast to any) to a correctly typed AxiosRequestConfig or a typed mock matching the properties your tests need.apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts (1)
334-344:⚠️ Potential issue | 🔴 CriticalType mismatch:
fallbackDeploymentReaderService.findAll()expects incompatible parameter structure.At line 339, you're passing
FindAllParams(with nestedpagination: { offset, limit, key }) tofallbackDeploymentReaderService.findAll(), which expectsDatabaseDeploymentListParams(with flatskip,limit,keyat the top level). This type incompatibility causes the fallback service to ignore pagination parameters and always use defaults (skip=0, limit=100), breaking pagination when the blockchain service fails.🐛 Proposed fix - transform params for fallback service
private async getDeploymentsList(params: FindAllParams): Promise<DeploymentListResponse> { try { return await this.deploymentHttpService.findAll(params); } catch (error) { if (this.shouldFallbackToDatabase(error)) { - return await this.fallbackDeploymentReaderService.findAll(params); + return await this.fallbackDeploymentReaderService.findAll({ + owner: params.owner, + state: params.state, + skip: params.pagination?.offset, + limit: params.pagination?.limit, + key: params.pagination?.key, + countTotal: params.pagination?.countTotal, + reverse: params.pagination?.reverse + }); } throw error; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts` around lines 334 - 344, getDeploymentsList currently passes a FindAllParams (with nested pagination) into fallbackDeploymentReaderService.findAll which expects a flat DatabaseDeploymentListParams, causing pagination to be ignored; fix by mapping/transforming the incoming params before calling fallbackDeploymentReaderService.findAll (extract pagination.offset → skip, pagination.limit → limit, pagination.key → key, and copy any other top-level filters) so you call fallbackDeploymentReaderService.findAll(mappedParams) instead of passing the original FindAllParams.
🧹 Nitpick comments (1)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts (1)
160-184: Inconsistent use ofjest.fn()in a Vitest test file.The file imports from
vitest-mock-extendedbut thesetup()function usesjest.fn()instead ofvi.fn(). While this may work if jest globals are shimmed by vitest, it's inconsistent with the project's testing conventions.♻️ Proposed fix to use vi.fn()
Add the import at the top of the file:
+import { vi } from "vitest"; import { mock } from "vitest-mock-extended";Then replace
jest.fn()withvi.fn()in the setup function:const mocks = { providerService: mock<ProviderService>({ - getLeaseStatus: jest.fn().mockResolvedValue(null), - toProviderAuth: jest.fn().mockResolvedValue({ type: "jwt", token: "test" }) + getLeaseStatus: vi.fn().mockResolvedValue(null), + toProviderAuth: vi.fn().mockResolvedValue({ type: "jwt", token: "test" }) }), deploymentHttpService: mock<DeploymentHttpService>({ - findByOwnerAndDseq: jest.fn().mockResolvedValue(defaultDeploymentInfo), - findAll: jest.fn().mockResolvedValue(defaultDeploymentList) + findByOwnerAndDseq: vi.fn().mockResolvedValue(defaultDeploymentInfo), + findAll: vi.fn().mockResolvedValue(defaultDeploymentList) }), // ... apply same pattern to other mocks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts` around lines 160 - 184, The test uses jest.fn() inside the setup() mock objects (e.g., getLeaseStatus, toProviderAuth, deploymentHttpService.findByOwnerAndDseq, findAll, leaseHttpService.list, fallbackLeaseReaderService.list, walletReaderService.getWalletByUserId) but the project uses Vitest; replace all jest.fn() calls with vi.fn() and ensure vi is available by importing it (import { vi } from 'vitest') at the top of the spec file if not already imported so all mocks use Vitest's vi.fn().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts`:
- Around line 141-142: Replace the unsafe cast "{} as any" used for the config
in the deployment-reader test by importing and using the proper
AxiosRequestConfig type (or constructing a minimal/mock AxiosRequestConfig
object) so the test no longer uses type any; update the spec to import
AxiosRequestConfig from "axios" and change the config declaration passed to the
DeploymentReader test setup (the object currently cast to any) to a correctly
typed AxiosRequestConfig or a typed mock matching the properties your tests
need.
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.ts`:
- Around line 334-344: getDeploymentsList currently passes a FindAllParams (with
nested pagination) into fallbackDeploymentReaderService.findAll which expects a
flat DatabaseDeploymentListParams, causing pagination to be ignored; fix by
mapping/transforming the incoming params before calling
fallbackDeploymentReaderService.findAll (extract pagination.offset → skip,
pagination.limit → limit, pagination.key → key, and copy any other top-level
filters) so you call fallbackDeploymentReaderService.findAll(mappedParams)
instead of passing the original FindAllParams.
---
Nitpick comments:
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts`:
- Around line 160-184: The test uses jest.fn() inside the setup() mock objects
(e.g., getLeaseStatus, toProviderAuth, deploymentHttpService.findByOwnerAndDseq,
findAll, leaseHttpService.list, fallbackLeaseReaderService.list,
walletReaderService.getWalletByUserId) but the project uses Vitest; replace all
jest.fn() calls with vi.fn() and ensure vi is available by importing it (import
{ vi } from 'vitest') at the top of the spec file if not already imported so all
mocks use Vitest's vi.fn().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de02f814-3cb7-492d-a426-f1d824a307d4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tspackages/http-sdk/package.jsonpackages/http-sdk/src/deployment/deployment-http.service.spec.tspackages/http-sdk/src/deployment/deployment-http.service.ts
ygrishajev
force-pushed
the
fix/deployment-require-state-for-offset-pagination
branch
from
044b9fb to
8614677
Compare
ygrishajev
force-pushed
the
fix/deployment-require-state-for-offset-pagination
branch
from
8614677 to
392fca2
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts (1)
114-126: Consider strengthening the assertion to verify offset is absent.The test verifies
ownerandstateare passed, but doesn't explicitly assert thatpagination.offsetis undefined/absent whenskipis not provided. This would more directly verify the discriminated union branch.🔧 Suggested enhancement
it("passes status as state without offset when skip is not provided", async () => { const address = "akash1abc"; const { service, deploymentHttpService } = setup(); await service.listWithResources({ address, status: "active" }); expect(deploymentHttpService.findAll).toHaveBeenCalledWith( expect.objectContaining({ owner: address, - state: "active" + state: "active", + pagination: expect.not.objectContaining({ offset: expect.any(Number) }) }) ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts` around lines 114 - 126, The test currently checks owner and state but doesn't confirm pagination.offset is absent; update the spec for listWithResources to assert that deploymentHttpService.findAll was called with an object containing owner: address, state: "active", and a pagination object that does not include offset (e.g. use expect.objectContaining with pagination: expect.not.objectContaining({ offset: expect.anything() }) or assert pagination.offset === undefined) when calling service.listWithResources; reference the service.listWithResources call and the deploymentHttpService.findAll expectation to add this assertion.apps/stats-web/src/app/addresses/[address]/deployments/AddressDeployments.tsx (1)
31-33: Add runtime validation for status filter values.Line 32 uses
as stringwithout validating that the value matches expected statuses. While TanStack Table's UI likely constrains values, the code should guard against unexpected values reaching the API. Narrow the type and validate at the boundary:Suggested refactor
- const [statusFilter, setStatusFilter] = useState("active"); + type DeploymentStatus = "*" | "active" | "closed"; + const [statusFilter, setStatusFilter] = useState<DeploymentStatus>("active"); const onColumnFiltersChange = (columnFilters: ColumnFiltersState) => { - const statusFilter = (columnFilters.find(filter => filter.id === "status")?.value as string) || "active"; - setStatusFilter(statusFilter); + const rawStatus = columnFilters.find(filter => filter.id === "status")?.value; + const nextStatus: DeploymentStatus = ["*", "active", "closed"].includes(rawStatus as string) ? (rawStatus as DeploymentStatus) : "active"; + setStatusFilter(nextStatus); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/stats-web/src/app/addresses/`[address]/deployments/AddressDeployments.tsx around lines 31 - 33, The onColumnFiltersChange handler currently blindly casts filter.value to string for the "status" filter; add runtime validation in onColumnFiltersChange (and before calling setStatusFilter) by extracting the value from the ColumnFiltersState, checking typeof value === "string" and that it exists in an allowedStatusValues array (e.g. const allowedStatusValues = ["active","inactive","all"] or your app's canonical statuses), and if it is invalid fall back to the default "active" (or another canonical default) before calling setStatusFilter; this keeps the boundary around the "status" filter safe and avoids sending unexpected values to the API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.ts`:
- Around line 114-126: The test currently checks owner and state but doesn't
confirm pagination.offset is absent; update the spec for listWithResources to
assert that deploymentHttpService.findAll was called with an object containing
owner: address, state: "active", and a pagination object that does not include
offset (e.g. use expect.objectContaining with pagination:
expect.not.objectContaining({ offset: expect.anything() }) or assert
pagination.offset === undefined) when calling service.listWithResources;
reference the service.listWithResources call and the
deploymentHttpService.findAll expectation to add this assertion.
In
`@apps/stats-web/src/app/addresses/`[address]/deployments/AddressDeployments.tsx:
- Around line 31-33: The onColumnFiltersChange handler currently blindly casts
filter.value to string for the "status" filter; add runtime validation in
onColumnFiltersChange (and before calling setStatusFilter) by extracting the
value from the ColumnFiltersState, checking typeof value === "string" and that
it exists in an allowedStatusValues array (e.g. const allowedStatusValues =
["active","inactive","all"] or your app's canonical statuses), and if it is
invalid fall back to the default "active" (or another canonical default) before
calling setStatusFilter; this keeps the boundary around the "status" filter safe
and avoids sending unexpected values to the API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bb1b5ac-834f-4baf-b9c1-3028f01091e3
⛔ Files ignored due to path filters (2)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
apps/api/src/deployment/http-schemas/deployment.schema.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.spec.tsapps/api/src/deployment/services/deployment-reader/deployment-reader.service.tsapps/api/test/functional/addresses.spec.tsapps/api/test/functional/deployments.spec.tsapps/stats-web/src/app/addresses/[address]/deployments/AddressDeployments.tsxpackages/http-sdk/package.jsonpackages/http-sdk/src/deployment/deployment-http.service.spec.tspackages/http-sdk/src/deployment/deployment-http.service.ts
✅ Files skipped from review due to trivial changes (2)
- packages/http-sdk/package.json
- packages/http-sdk/src/deployment/deployment-http.service.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/test/functional/addresses.spec.ts
- apps/api/test/functional/deployments.spec.ts
- packages/http-sdk/src/deployment/deployment-http.service.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The Akash blockchain endpoint
/akash/deployment/v1beta4/deployments/listrejects requests that usepagination.offsetwithoutfilters.state, returning a 400 error:The
listWithResourcesroute (/v1/addresses/:address/deployments/:skip/:limit) always providesskip(offset) as a path param butstatuswas an optional query param, so requests withoutstatuswere failing.What
BREAKING CHANGE: The
statusquery parameter is now required onGET /v1/addresses/{address}/deployments/{skip}/{limit}. Requests without?status=activeor?status=closedwill return 400.statusrequired in theListWithResourcesQuerySchema(OpenAPI)statusrequired inDeploymentReaderService.listWithResourcessignature, removing the silent"active"defaultFindAllParamsunion type inpackages/http-sdkthat enforcesstateis required whenpagination.offsetis set — catches violations at compile timelistmethod to branch onskipfor type narrowingAddressDeploymentsto default the status filter to"active"instead of"*"so it always sends a valid statusDeploymentHttpService.findAllandDeploymentReaderService.listWithResourcesSummary by CodeRabbit
Tests
Refactor