Add typings result #458
Add typings result #458
Conversation
WalkthroughThe changes update internal backend logic for type conversion and response structure. In the aggregation controller, the conversion of the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as runRequest/runBulkRequest
participant ES as Data Source
Client->>API: Submit request (query/scroll or bulk body)
API->>ES: Process request/query
alt Success
ES-->>API: Return valid response data
API-->>Client: Respond with structured data (RequestResult/BulkRequestResult)
else Error
ES-->>API: Return error details
API-->>Client: Respond with structured error { took, hits:[], status, statusText, error }
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/runRequest.ts (1)
96-96: Consider typing the body parameterWhile updating the return type to
Promise<BulkRequestResult>is good, thebodyparameter is still typed asany. Consider creating a specific interface for this parameter as well.-export const runBulkRequest = async (body: any): Promise<BulkRequestResult> => { +export const runBulkRequest = async (body: BulkRequestBody): Promise<BulkRequestResult> => {Where
BulkRequestBodycould be defined in your models directory.backend/src/models/result.ts (1)
161-162: Consider more specific typing if possibleWhile an index signature (
[key: string]: any) provides flexibility for additional properties, it reduces type safety. If you know the specific properties that might be present, consider defining them explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/controllers/aggregation.controller.ts(1 hunks)backend/src/models/result.ts(3 hunks)backend/src/runRequest.ts(4 hunks)
🔇 Additional comments (11)
backend/src/controllers/aggregation.controller.ts (1)
216-216: Type safety improvement in delay conversionThe change from using a type assertion (
delay as string) to using theString()function is a good improvement. TheString()function ensures proper runtime conversion of the value to a string, while type assertions only affect TypeScript's type checking without changing the runtime behavior.backend/src/runRequest.ts (5)
4-4: Good addition of typed interfacesUsing specific interfaces
RequestResultandBulkRequestResultinstead of relying onanytypes improves type safety and code readability.
15-15: Improved return type specificityUpdating the return type from
Promise<any>toPromise<RequestResult>enhances type safety and makes the function's contract clearer.
57-70: Better error response structureThe updated error response structure provides a more consistent format that matches the success response, which makes error handling easier for consumers of the API.
77-89: Consistent error handling patternThis error response follows the same pattern, reinforcing consistency throughout the codebase.
108-120: Consistent error response in bulk requestsThe error response for bulk requests now follows the same structure as single requests, which is good for consistency.
backend/src/models/result.ts (5)
140-148: Good addition of specific response interfacesThe new
RequestResultandBulkRequestResultinterfaces properly define the structure of responses fromrunRequestandrunBulkRequestfunctions, improving type safety and documentation.
164-168: Improved aggregation structure typingThe more specific structure for the
myBucketsproperty improves type safety and makes the code more self-documenting.
169-171: Good addition of error propertiesThese optional properties align well with the updated error response structure in
runRequest.ts, ensuring consistency between interfaces and implementation.
174-177: Well-defined BucketItem interfaceCreating a specific interface for bucket items rather than using a generic type improves code clarity and type safety.
209-209: Fixed interface closureThis appears to be fixing the proper closure of the
ResultRawHitinterface, ensuring syntactic correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/src/models/result.ts (4)
140-148: Add JSDoc comments to the new interfacesThe new
RequestResultandBulkRequestResultinterfaces will be useful for type safety, but they lack documentation. Consider adding JSDoc comments to explain their purpose and provide examples, similar to what's done for other interfaces in this file.+/** + * Interface for single request response + * @tsoaModel + * @example + * { + * "data": { + * "took": 10, + * "hits": { ... } + * } + * } + */ export interface RequestResult { data: ResultRawES; } +/** + * Interface for bulk request response containing multiple result sets + * @tsoaModel + * @example + * { + * "data": { + * "responses": [ + * { + * "took": 10, + * "hits": { ... } + * } + * ] + * } + * } + */ export interface BulkRequestResult { data: { responses: ResultRawES[]; } }
161-161: Consider using more specific typing for index signatureWhile
[key: string]: anyprovides flexibility, it can hide potential type errors. If possible, it would be better to use a more specific type thananyor to enumerate the known properties instead.
164-167: Reconsider the naming of "myBuckets"The property name "myBuckets" isn't a conventional naming pattern and may be confusing. Consider using a more descriptive and conventional name like "bucketResults" or simply "results".
- myBuckets: { + bucketResults: { buckets: BucketItem[]; after_key: number; }
169-171: Consider extending error handlingAdding status, statusText, and error properties to ResultRawES is a good start for error handling. However, you might want to consider creating a separate interface for error responses to maintain separation of concerns.
export interface ResultRawES { '_scroll_id'?: string; took: number; hits: { total: { value: number; } 'max_score': number; hits: ResultRawHit[] }; aggregations?: { [key: string]: any; 'doc_count_error_upper_bound': number; 'sum_other_doc_count': number; myBuckets: { buckets: BucketItem[]; after_key: number; } } status?: number; statusText?: string; error?: boolean; } +/** + * Interface for error responses from Elasticsearch + */ +export interface ESErrorResponse { + status: number; + statusText: string; + error: boolean; + message?: string; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/controllers/aggregation.controller.ts(4 hunks)backend/src/models/result.ts(3 hunks)backend/src/runRequest.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/runRequest.ts
- backend/src/controllers/aggregation.controller.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🐳 Build docker image
🔇 Additional comments (1)
backend/src/models/result.ts (1)
174-177: Good addition of the BucketItem interfaceThe
BucketIteminterface provides good type safety for the aggregation results. Consider adding a JSDoc comment to explain its purpose and possibly expanding it with additional fields if needed.+/** + * Interface for aggregation bucket items returned by Elasticsearch + */ interface BucketItem { key: string; doc_count: number; }
cristianpb
force-pushed
the
feat/typings-result
branch
from
ab41908 to
63bec12
Compare
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/performance/scenarios/test-backend-v1.yml (1)
57-63: Align POST flow ordering for readabilityTo mirror the GET flow and improve consistency, consider moving the
expectblock abovecapturein the POST step:- capture: - - json: '$' - as: resultPost - expect: - - statusCode: 200 - - contentType: json + expect: + - statusCode: 200 + - contentType: json + capture: + - json: '$' + as: resultPost
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Makefile(1 hunks)backend/src/buildRequest.ts(1 hunks)backend/src/controllers/aggregation.controller.ts(6 hunks)backend/src/models/result.ts(3 hunks)backend/src/runRequest.ts(4 hunks)backend/tests/performance/scenarios/test-backend-v1.yml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/src/buildRequest.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- backend/src/runRequest.ts
- backend/src/controllers/aggregation.controller.ts
- backend/src/models/result.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: 🐳 Build docker image
🔇 Additional comments (3)
backend/tests/performance/scenarios/test-backend-v1.yml (3)
2-9: Verify plugin configuration & threshold semanticsEnsure that the
expectandensureplugins are installed and properly loaded by Artillery. Double-check that:
maxErrorRate: 1(1%) andmax: 500requests align with your SLA.p95: 200is interpreted in milliseconds by theensureplugin.Run a quick local run to confirm no plugin load errors and that thresholds behave as expected.
29-34: Confirm CSV header alignment withfieldsmappingYou’ve swapped
lastNameandfirstNamein the payload. Verify thatnames.csv’s header row actually lists the columns in this order. If the CSV hasn’t changed, the mapping will be misaligned, causing incorrect substitutions at runtime.
44-49: Validate GET request assertionsThe new
expectchecks for status 200 and JSON content type, plus capturing the full response asresultGet, are well-placed to catch failures early. Nice addition.
| skipHeader: true | ||
| #delimiter: ";" | ||
| delimiter: ";" |
There was a problem hiding this comment.
Ensure correct CSV parsing settings
The comment indicates skipHeader isn’t working. According to Artillery’s payload docs, the key should be skipFirstRow rather than skipHeader. Without this change, the header row may still be injected as data. For example:
payload:
- path: names.csv
fields: [...]
- skipHeader: true
+ skipFirstRow: true
delimiter: ";"🤖 Prompt for AI Agents
In backend/tests/performance/scenarios/test-backend-v1.yml around lines 37 to
38, the CSV parsing setting uses the incorrect key `skipHeader`. Replace
`skipHeader` with `skipFirstRow` to correctly skip the header row during CSV
parsing as per Artillery's payload documentation, ensuring the header is not
injected as data.
afabbe2 to
d8d5d32
Compare
cristianpb
force-pushed
the
feat/typings-result
branch
from
5231229 to
93a9bb7
Compare
cristianpb
force-pushed
the
feat/typings-result
branch
from
93a9bb7 to
c8235c7
Compare
Summary by CodeRabbit
New Features
Refactor