Updated swagger schema of get workflow by id endpoint #2762
Conversation
|
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Possibly related PRs
Suggested reviewers
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
CodeRabbit Configuration File (
|
tomer-shvadron
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (2)
services/workflows-service/src/workflow/workflow.controller.external.ts (2)
50-56: Rename theTestclass to a more descriptive name.The newly added
Testclass appears to be a DTO for API responses. While the property declarations and decorators are correct, the class name "Test" is not descriptive and doesn't follow naming conventions. Consider renaming it to something more meaningful, such asWorkflowResponseDtoorRunnableWorkflowDataDto, which better reflects its purpose and content.
Line range hint
194-224: Update return statement to use the new response structure.The method body hasn't been updated to reflect the new response structure defined by the
Testclass. To ensure consistency with the API response type, modify the return statement to construct and return an instance of theTestclass.Apply this change to the return statement:
- return { - workflowDefinition, - workflowRuntimeData, - }; + return new Test({ + workflowDefinition, + workflowRuntimeData, + });Also, ensure that the
Testclass constructor accepts these properties, or use property assignment if it doesn't have a constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- services/workflows-service/prisma/schema.prisma (0 hunks)
- services/workflows-service/src/filter/dtos/temp-zod-schemas.ts (0 hunks)
- services/workflows-service/src/workflow/dtos/workflow-definition-create.ts (0 hunks)
- services/workflows-service/src/workflow/workflow-definition.model.ts (3 hunks)
- services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts (4 hunks)
- services/workflows-service/src/workflow/workflow.controller.external.ts (3 hunks)
- services/workflows-service/src/workflow/workflow.controller.internal.ts (0 hunks)
- services/workflows-service/src/workflow/workflow.service.ts (0 hunks)
💤 Files with no reviewable changes (5)
- services/workflows-service/prisma/schema.prisma
- services/workflows-service/src/filter/dtos/temp-zod-schemas.ts
- services/workflows-service/src/workflow/dtos/workflow-definition-create.ts
- services/workflows-service/src/workflow/workflow.controller.internal.ts
- services/workflows-service/src/workflow/workflow.service.ts
🧰 Additional context used
🔇 Additional comments (10)
services/workflows-service/src/workflow/workflow.controller.external.ts (1)
7-8: LGTM: Import statements updated appropriately.The changes to the import statements are correct and align with the modifications in the code. The addition of
ApiPropertyandWorkflowRuntimeListItemModelimports, as well as the type-only import ofWorkflowRuntimeData, reflect the updates in the API documentation and response types.Also applies to: 48-48
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts (1)
72-72:⚠️ Potential issueMissing
@Expose()decorator foruiDefinitionIdSimilar to
assigneeId, theuiDefinitionIdproperty is missing the@Expose()decorator, which is necessary for serialization.Add the
@Expose()decorator:+@Expose() @ApiProperty({ required: false, type: String }) @IsOptional() @IsString() uiDefinitionId?: string;Likely invalid or redundant comment.
services/workflows-service/src/workflow/workflow-definition.model.ts (8)
18-21: New propertycrossEnvKeyadded.The optional string property
crossEnvKeyhas been introduced with appropriate validation and API documentation. Ensure that this property is correctly utilized throughout the application and any necessary documentation is updated.
23-26: New propertyprojectIdadded.The optional string property
projectIdhas been added with proper validation and API documentation. Confirm thatprojectIdis properly handled in relevant parts of the application.
31-34: New propertydisplayNameadded.An optional string property
displayNamehas been added with appropriate validation and API documentation.
36-39: New propertyreviewMachineIdadded.The optional string property
reviewMachineIdhas been introduced. Ensure it is utilized appropriately within the application.
41-44: New propertyvariantadded.The optional string property
varianthas been added with proper validation and API documentation.
134-137: New propertycreatedByadded.An optional string property
createdByhas been added with proper validation and API documentation. Ensure that this field is correctly set when creating workflow definitions and handled appropriately in related services.
112-112: Property renamed frombackendtoextensions.The property
backendhas been renamed toextensions. Ensure all references toworkflowDefinition.backendare updated toworkflowDefinition.extensionsthroughout the codebase.Run this command to search for outdated references:
#!/bin/bash # Description: Find references to workflowDefinition.backend rg --type typescript 'workflowDefinition\.backend\b'
88-88: Property renamed fromcontexttocontextSchema.The property
contexthas been renamed tocontextSchema. Verify that all references toworkflowDefinition.contextare updated toworkflowDefinition.contextSchemathroughout the codebase to prevent runtime errors.Run the following script to identify any remaining references to the old property name:
✅ Verification successful
Verification Successful: All references to
workflowDefinition.contexthave been updated toworkflowDefinition.contextSchemathroughout the codebase, preventing potential runtime errors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find references to workflowDefinition.context rg --type typescript 'workflowDefinition\.context\b'Length of output: 89
Script:
#!/bin/bash # Description: Find references to workflowDefinition.context in .ts and .tsx files rg --glob '*.ts' --glob '*.tsx' 'workflowDefinition\.context\b'Length of output: 65
services/workflows-service/src/workflow/workflow.controller.external.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts
Outdated
Show resolved
Hide resolved
services/workflows-service/src/workflow/workflow-definition.model.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts (1)
Line range hint
88-91: Use@IsEnum()for enum validation onstatuspropertyThe
statusproperty is of typeWorkflowRuntimeDataStatus, which is an enum. Using@IsString()validates it as a string, but@IsEnum()provides stronger validation for enum types and ensures that only valid enum values are assigned.Apply this diff to improve the validation:
@Expose() @ApiProperty() -@IsString() +@IsEnum(WorkflowRuntimeDataStatus) status!: WorkflowRuntimeDataStatus;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- services/workflows-service/src/workflow/workflow-definition.model.ts (3 hunks)
- services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (1)
services/workflows-service/src/workflow/workflow-runtime-list-item.model.ts (1)
108-111:⚠️ Potential issueSpecify a concrete type for
contextpropertyThe
contextproperty is typed asJSON, which is not a valid TypeScript type and may lead to issues during type checking. Consider using a more accurate type likeRecord<string, any>or defining a specific interface to improve type safety.Apply this diff to update the property type:
@Expose() @ApiProperty() @IsJSON() -context!: JSON; +context!: Record<string, any>;Likely invalid or redundant comment.
services/workflows-service/src/workflow/workflow-definition.model.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/workflow/workflow-definition.model.ts (1)
115-115: Note the inconsistency in validation decorators forextensions.The
extensionsproperty (formerlybackend) continues to use@IsNotEmptyObject(), while similar properties (contextSchema,documentsSchema, andconfig) now use@IsObject().Consider standardizing the validation approach across all these properties for consistency, unless there's a specific reason for this difference.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/workflow/workflow-definition.model.ts (3 hunks)
🧰 Additional context used
🔇 Additional comments (7)
services/workflows-service/src/workflow/workflow-definition.model.ts (7)
5-5: LGTM: Import statement added for new property.The addition of
IsBooleanimport is consistent with the newisPublicproperty in the class.
19-22: LGTM: New propertycrossEnvKeyadded with correct decorators.The
crossEnvKeyproperty is correctly defined as an optional string with appropriate decorators. The@ApiPropertydecorator includesrequired: falseandtype: String, which is consistent with the suggested best practices.
24-27: LGTM: New propertyprojectIdadded with correct decorators.The
projectIdproperty is correctly defined as an optional string with appropriate decorators, following the same pattern ascrossEnvKey.
29-32: LGTM: New propertyisPublicadded with correct decorators.The
isPublicproperty is correctly defined as an optional boolean with appropriate decorators. The@ApiPropertydecorator includesrequired: falseandtype: Boolean, which is consistent with the suggested best practices.
34-47: LGTM: New properties added with correct decorators.The
displayName,reviewMachineId, andvariantproperties are correctly defined as optional strings with appropriate decorators, following the same pattern ascrossEnvKeyandprojectId.
137-140: LGTM: New propertycreatedByadded with correct decorators.The
createdByproperty is correctly defined as an optional string with appropriate decorators, following the same pattern as other new string properties in this class.
91-91: Clarify the use of@IsObject()vs@IsNotEmptyObject().The properties
contextSchema,documentsSchema, andconfignow use@IsObject()instead of@IsNotEmptyObject(). This change allows for empty objects, which may or may not be intentional. However, theextensionsproperty still uses@IsNotEmptyObject().Could you please clarify if this inconsistency is intentional? If not, consider standardizing the validation approach across all these properties.
To check the usage of these properties in the codebase, you can run:
This will help determine if empty objects are being used or if they should be disallowed.
Also applies to: 99-99, 105-107
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
services/workflows-service/src/workflow/workflow.controller.external.unit.test.ts (1)
Line range hint
1-152: Consider reviewing and expanding test coverageThe changes in this file have reduced the overall test coverage of the workflow controller. One test case has been removed entirely, and another has been simplified. This reduction in test coverage could potentially lead to undetected bugs or regressions in the future.
Consider reviewing the current test suite and potentially expanding it to ensure comprehensive coverage of the workflow controller's functionality. This might include:
- Reinstating the removed test case for GET /workflows if the functionality still exists.
- Adding more assertions to the existing test cases to cover all aspects of the response.
- Adding new test cases for any new or modified functionality in the workflow controller.
To help identify areas that might need additional testing, you can run the following script:
#!/bin/bash # Description: Analyze workflow controller for potential untested areas # Test: List all public methods in the workflow controller echo "Public methods in workflow controller:" ast-grep --lang typescript --pattern $'class WorkflowControllerExternal { $$$ @$_($$$) $_($_) { $$$ } $$$ }' # Test: List all test cases in the current file echo "Current test cases:" rg --type ts "test\(.*\)" services/workflows-service/src/workflow/workflow.controller.external.unit.test.ts # Test: Check for TODO comments related to testing echo "TODO comments related to testing:" rg --type ts "TODO.*test" services/workflows-service/src/workflowThis script will help identify public methods in the controller that might not have corresponding test cases, as well as any TODO comments related to testing that might need to be addressed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- services/workflows-service/src/workflow/workflow.controller.external.unit.test.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
services/workflows-service/src/workflow/workflow.controller.external.unit.test.ts (2)
Line range hint
86-109: Clarify the removal of the GET /workflows test caseA test case for the GET /workflows endpoint has been commented out. This removes coverage for the list operation of workflows, which could lead to potential regressions going undetected.
Can you please clarify why this test case was removed? If the functionality is still supported, consider keeping the test or replacing it with an updated version.
To verify the current state of workflow listing functionality, you can run the following script:
#!/bin/bash # Description: Check for workflow listing endpoint and its tests # Test: Search for workflow listing endpoint in controller echo "Searching for workflow listing endpoint in controller:" rg --type ts "GET /workflows" -g "*controller*.ts" # Test: Search for tests related to workflow listing echo "Searching for tests related to workflow listing:" rg --type ts "test.*GET /workflows" -g "*test.ts"This script will help identify if the workflow listing functionality is still present in the controller and if there are any remaining tests for it.
146-146: Verify the simplified test assertion for GET /workflows/:idThe test assertion for the GET /workflows/:id endpoint has been simplified to only check for the state object in the response. While this aligns with the current mock implementation, it may not fully test the endpoint's response.
Please confirm if this simplification is intentional. Consider whether testing only the state object is sufficient, or if additional assertions should be added to ensure the complete response structure is correct.
To verify the current implementation of the getWorkflowRuntimeDataById method, you can run the following script:
#!/bin/bash # Description: Check the implementation of getWorkflowRuntimeDataById # Test: Search for getWorkflowRuntimeDataById method implementation echo "Searching for getWorkflowRuntimeDataById implementation:" ast-grep --lang typescript --pattern $'class $_ { $$$ async getWorkflowRuntimeDataById($_) { $$$ } $$$ }'This script will help identify the current implementation of the getWorkflowRuntimeDataById method, which can be used to determine if the test assertion adequately covers the method's return value.
services/workflows-service/src/workflow/workflow.controller.external.unit.test.ts
Show resolved
Hide resolved
tomer-shvadron
deleted the
chore/tomer/update-get-workflow-by-id-swagger-schema
branch
Updated swagger schema of get workflow by id endpoint (ballerine-io#2762)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
WorkflowDefinitionmodel and related schemas to streamline workflow definitions.backendandsupportedPlatforms.Documentation