Rules & rule-engine backend completed in swift #30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several updates across multiple files, primarily focusing on version increments for Changes
Poem
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
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (13)
src/swift-server/routes.swift (3)
107-111: Consider atomic transaction for condition managementFor the
ApiRule_addConditionendpoint, consider wrapping the implementation in a database transaction to ensure atomicity when adding conditions to rules. This prevents partial updates if something fails during the operation.Example pattern to consider:
func ApiRule_addCondition(_ input: Operations.ApiRule_addCondition.Input) async throws -> Operations.ApiRule_addCondition.Output { return try await request.db.transaction { db in // 1. Validate rule exists // 2. Create and save condition // 3. Associate condition with rule // 4. Return success response } }
101-105: Consider validation middleware for rule applicationThe
ApiRule_applyendpoint will likely need careful validation of the input data before applying rules. Consider adding a dedicated middleware for validation to keep the endpoint implementation clean.Example pattern:
struct RuleValidationMiddleware: AsyncMiddleware { func respond(to request: Request, chainingTo next: AsyncResponder) async throws -> Response { // Validate rule exists and is active // Validate input data matches rule requirements return try await next.respond(to: request) } }
89-93: Add logging for rule creationFor the
ApiRule_createendpoint, consider adding logging to track rule creation events. This will be valuable for auditing and debugging purposes.Example enhancement:
func ApiRule_create(_ input: Operations.ApiRule_create.Input) async throws -> Operations.ApiRule_create.Output { // Implementation here request.logger.info("Rule created: \(rule.id) by \(request.auth.get()?.id ?? "unknown")") }src/swift-server/migrations/initial-migration.swift (1)
61-63: Consider adding validation for the operation field.The
operationfield appears to be used for comparison operations, but there's no constraint ensuring valid values. Consider:
- Using an enum type if your database supports it
- Adding a check constraint for valid operations
Example of valid operations might include: "equals", "contains", "greater_than", "less_than", etc. This would prevent invalid operations from being stored.
Additionally, the flexible value storage pattern using
value_strandvalue_floatis a good approach for handling different data types, but consider documenting which operations are valid with which value types to prevent runtime errors.src/@typespec/openapi3/openapi.yaml (4)
560-583: Add request body validation descriptions.The
RuleParamschema is used in the request body but lacks descriptions for its properties. This makes it harder for API consumers to understand the validation requirements.Consider adding descriptions for each property in the
RuleParamschema.
560-583: Add pagination support for GET /rules endpoint.The GET /rules endpoint might return a large number of rules. Consider adding pagination support similar to other list endpoints in the API (e.g.,
/users).Add cursor-based pagination parameters and update the response schema:
get: operationId: ApiRule_list - parameters: [] + parameters: + - name: cursor + in: query + required: false + schema: + type: string + explode: false + - name: limit + in: query + required: false + schema: + type: integer + format: uint16 + default: 100 + explode: false responses: '200': description: The request has succeeded. content: application/json: schema: - type: array - items: - $ref: '#/components/schemas/Rule' + type: object + required: + - results + properties: + results: + type: array + items: + $ref: '#/components/schemas/Rule' + next: + type: string
Line range hint
1088-1101: Add validation constraints for float values.The
ConditionFloatschema should include validation constraints for the float value to prevent issues with special values (NaN, Infinity).value: type: number + format: double + minimum: -1e308 + maximum: 1e308
1454-1460: Add descriptions for operation enums.The
OperationFloatandOperationStringenums lack descriptions for their values, making it unclear how each operation behaves.Add descriptions for each enum value:
OperationFloat: type: string + description: Numeric comparison operations enum: - - greater - - greaterEqual - - lower - - lowerEqual + - greater # Value must be greater than the condition value + - greaterEqual # Value must be greater than or equal to the condition value + - lower # Value must be lower than the condition value + - lowerEqual # Value must be lower than or equal to the condition value OperationString: type: string + description: String matching operations enum: - - contains - - prefix - - regularExpression - - suffix + - contains # Value must contain the condition string + - prefix # Value must start with the condition string + - regularExpression # Value must match the condition regular expression + - suffix # Value must end with the condition stringAlso applies to: 1461-1467
src/typespec/rules.tsp (3)
15-20: Consider renaming 'relations' to 'relation' in 'RuleParam' for clarityThe property
relationsinRuleParamis of typeConditionalRelation, which suggests it represents a single relation rather than multiple. For clarity and to adhere to naming conventions, consider renaming it torelation.
35-39: Rename 'lower' to 'less' in 'OperationFloat' for consistency with mathematical terminologyIn the
OperationFloatenum, renaminglowertolessandlowerEqualtolessEqualwould align the terms with standard mathematical language, improving readability and understanding.Apply this diff to make the changes:
enum OperationFloat { greater, greaterEqual, - lower, - lowerEqual, + less, + lessEqual, }
28-31: Consider adding an 'equals' operation to 'OperationString'The
OperationStringenum lacks anequalsoperation, which is commonly used for string comparisons. Adding this operation would enhance the flexibility of string conditions.Apply this diff to include the
equalsoperation:enum OperationString { + equals, contains, prefix, regularExpression, suffix, }src/swift-server/rules/rules.models.swift (2)
4-7: Consider removing redundant string enum values.SwiftLint suggests that string enum values can be omitted when they are equal to the enumcase name. This can help reduce verbosity and improve readability.
Apply this diff to remove the redundant string enum values:
enum ConditionalRelationType: String, Codable { - case and = "and" - case notOr = "notOr" + case and + case notOr }🧰 Tools
🪛 SwiftLint
[Warning] 5-5: String enum values can be omitted when they are equal to the enumcase name
(redundant_string_enum_value)
[Warning] 6-6: String enum values can be omitted when they are equal to the enumcase name
(redundant_string_enum_value)
90-101: Consider renaming the class toRuleLabelActionPivotfor consistency.The class name
RuleLabelActiondoesn't follow the naming convention used for the other pivot classRuleLabelPivot. For consistency and clarity, consider renaming it toRuleLabelActionPivot.Apply this diff to rename the class:
-final class RuleLabelAction: Model, @unchecked Sendable { +final class RuleLabelActionPivot: Model, @unchecked Sendable {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
package.yaml(1 hunks)src/@typespec/openapi3/openapi.yaml(5 hunks)src/swift-server/bank-transaction/bank-transaction.models.swift(0 hunks)src/swift-server/migrations/initial-migration.swift(1 hunks)src/swift-server/routes.swift(1 hunks)src/swift-server/rules/rules.models.swift(1 hunks)src/typespec/rules.tsp(2 hunks)
💤 Files with no reviewable changes (1)
- src/swift-server/bank-transaction/bank-transaction.models.swift
✅ Files skipped from review due to trivial changes (1)
- package.yaml
🧰 Additional context used
🪛 SwiftLint
src/swift-server/rules/rules.models.swift
[Warning] 5-5: String enum values can be omitted when they are equal to the enumcase name
(redundant_string_enum_value)
[Warning] 6-6: String enum values can be omitted when they are equal to the enumcase name
(redundant_string_enum_value)
🔇 Additional comments (7)
src/typespec/rules.tsp (4)
22-26: Verify that 'Rule' model correctly extends 'RuleParam'
The Rule model uses the spread operator ...RuleParam;. Confirm that this correctly includes all properties from RuleParam and that any required properties are appropriately handled.
54-58: Ensure all condition types are accounted for in 'Condition' union
Currently, the Condition union includes ConditionFloat and ConditionString. Consider whether conditions for other data types (e.g., integers, dates) are required for the rules engine.
Run the following script to check for other condition models in the codebase:
#!/bin/bash
# Description: Search for other 'Condition' models that might need inclusion.
# Test: Find all models starting with 'Condition' that are not 'ConditionString' or 'ConditionFloat'.
# Expect: No additional models or identify any that should be included.
rg -A 2 '^model Condition\w+' --glob '!src/typespec/rules.tsp' | rg -v 'ConditionString|ConditionFloat'79-79:
Missing '@path condId: UUID' parameter in 'deleteCondition' method
The method deleteCondition has a route that includes {condId}, but the parameter @path condId: UUID is missing in the method signature. This will result in an error due to the unresolved path parameter.
Apply this diff to fix the method signature:
@delete @route("{ruleId}/condition/{condId}") deleteCondition(@path ruleId: UUID): Date;
+@delete @route("{ruleId}/condition/{condId}") deleteCondition(@path ruleId: UUID, @path condId: UUID): Date;Likely invalid or redundant comment.
71-72: Verify the usage of spread operator '...RuleParam' in method parameters
In the create and update methods, ...RuleParam is used to spread properties into the method parameters. Ensure that this usage is valid in the TypeSpec language and functions as intended.
Run the following script to verify the correct usage of ...RuleParam:
✅ Verification successful
Spread operator usage in RuleParam is valid and consistent with the codebase
The spread operator usage in create(...RuleParam) and update(@path ruleId: UUID, ...RuleParam) is valid and follows established patterns in the TypeSpec codebase. This is evidenced by:
RuleParamis properly defined as a model with required properties- Similar spread operator patterns are used consistently across the codebase in other TypeSpec files:
login(...UserCredentials)create(...CreateLabel)create(...GraphParam)create(...UserGroup)updateMe(...UpdateMyProfile)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for correct usage of '...RuleParam' in method definitions.
# Test: Search for method definitions using '...RuleParam' and verify syntax.
# Expect: Method definitions should correctly use '...RuleParam' without syntax errors.
ast-grep --pattern $'interface $_ {
$$$
$_ (...) ...RuleParam : $_;
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Let's try different approaches to verify the spread operator usage
# Search for RuleParam definition
rg "RuleParam" -A 5
# Search for spread operator usage in .tsp files
rg "\.\.\." --type-add 'tsp:*.tsp' --type tsp -A 2
# Look for similar parameter spreading patterns in other TypeSpec files
fd -e tsp -x cat {} | grep -A 2 "@\(post\|put\)"
Length of output: 9018
src/swift-server/rules/rules.models.swift (3)
20-49: LGTM!
The Condition class is well-structured and follows best practices:
- Proper use of Fluent's
ModelandContentprotocols - Correct usage of Fluent's property wrappers like
@ID,@Parent, and@Field - Initializers with default values and required properties
51-87: LGTM!
The Rule class is well-designed and adheres to best practices:
- Proper use of Fluent's
ModelandContentprotocols - Correct usage of Fluent's property wrappers like
@ID,@Parent,@Children,@Enum, and@OptionalParent - Initializer with default values and required properties
- Establishes parent-child relationships with
Conditionand self-referential relationships
105-116: LGTM!
The RuleLabelPivot class is well-structured and follows best practices:
- Proper use of Fluent's
Modelprotocol - Correct usage of Fluent's property wrappers like
@IDand@Parent - Establishes relationships with
RuleandLabelTransaction
Dracks
force-pushed
the
3.0.0/rule-engine
branch
from
b2bf84a to
ad623a6
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
auto_title_placeholder: '@coderabbitai'
review_status: true
commit_status: true
poem: true
collapse_walkthrough: false
sequence_diagrams: true
changed_files_summary: true
labeling_instructions: []
path_filters: []
path_instructions: []
abort_on_close: true
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: true
base_branches: []
tools:
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
chat:
auto_reply: true
knowledge_base:
opt_out: false
learnings:
scope: auto
issues:
scope: auto
jira:
project_keys: []
linear:
team_keys: []
pull_requests:
scope: auto
|
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range and nitpick comments (34)
src/swift-server/rules/rules.service.swift (3)
1-3: Consider adopting protocol-based design for better testability.Extract the rules service interface into a protocol to facilitate mock injection in tests and improve modularity.
import Fluent +protocol RulesServiceProtocol { + func applyRules(on db: Database, for transaction: TransactionSummary) async throws +} + -class RulesService { +class RulesService: RulesServiceProtocol {
4-7: Add input validation and consider pagination for large rule sets.The current implementation has several potential issues:
- Missing validation for transaction properties
- No pagination for rule queries which could impact performance with large rule sets
- No specific error handling for database failures
Consider implementing:
- Input validation
- Pagination using Fluent's
paginate()method- Custom error types for better error handling
func applyRules(on db: Database, for transaction: TransactionSummary) async throws { + guard let groupOwnerId = transaction.groupOwnerId else { + throw RuleServiceError.invalidTransaction + } + let rules = try await Rule.query(on: db).filter( - \.$groupOwner.$id == transaction.groupOwnerId + \.$groupOwner.$id == groupOwnerId ).with(\.$conditions).all()
22-23: Remove or implement the commented code.The commented code
rules.extends(rule.children)suggests an unimplemented feature for rule inheritance or extension.Would you like me to help implement the rule extension feature or create an issue to track this?
src/swift-server-tests/factories.swift (1)
66-69: Consider parameterizing the operation in RuleConditionFactory.The current implementation hardcodes
.containsas the operation, which might limit test coverage. Consider adding a parameter to allow testing different operations.Here's a suggested improvement:
class RuleConditionFactory: AbstractFactory<Condition> { + private var defaultOperation: Condition.Operation = .contains + + func withOperation(_ operation: Condition.Operation) -> RuleConditionFactory { + defaultOperation = operation + return self + } + override func create(id: Int) -> Condition { - return .init(ruleId: UUID(), operation: .contains) + return .init(ruleId: UUID(), operation: defaultOperation) } }This allows for more flexible testing:
let containsCondition = RuleConditionFactory().build() let startsWithCondition = RuleConditionFactory().withOperation(.startsWith).build()src/typespec/rules.tsp (5)
10-14: Consider renaming 'notAnd' for clarityThe enum value 'notAnd' is not a standard logical operator name and might be confusing. Consider using 'nand' or 'nor' if that's the intended logic, or add documentation to explain the behavior.
15-20: Add documentation for the RuleParam modelThe model would benefit from documentation explaining:
- The purpose of optional
parentRule(hierarchical rules?)- The relationship between
groupOwnerIdand rules- How
relationsaffects the rule evaluation
28-40: Add documentation for operation enumsPlease add documentation for each operation to specify:
- Expected behavior for string operations (e.g., case sensitivity for contains/prefix/suffix)
- Format requirements for regularExpression
- Precision handling for float comparisons
42-58: Consider adding validation constraintsThe condition models could benefit from additional validation:
- Add pattern validation for regularExpression values in ConditionString
- Consider adding min/max bounds for float values if applicable
73-74: Clarify the purpose of Date return typeThe
applyanddeleteoperations return a Date type. Please add documentation to explain what this timestamp represents (e.g., last execution time, deletion time).src/swift-server/user/user.service.swift (1)
33-36: LGTM! Consider applying similar formatting to other methods.The vertical alignment of parameters improves readability of this long method signature.
For consistency, consider applying similar formatting to other methods with multiple parameters, such as
validateGroupId. Example:- func validateGroupId(on db: Database, groupId: String, forUserId userId: UUID) async throws + func validateGroupId( + on db: Database, + groupId: String, + forUserId userId: UUID + ) async throwssrc/swift-server-tests/importers/importer-n26.tests.swift (1)
49-49: LGTM! Consider additional error scenarios.The queue parameter addition maintains consistency with the successful case. While the basic error handling is tested, consider adding test cases for queue-specific error scenarios (e.g., queue unavailability or processing failures).
Consider adding test cases like:
func testN26ImportWithQueueFailure() async throws { // Mock queue failure scenario let failingQueue = MockQueue(shouldFail: true) // Test error handling }src/swift-server/importer/importer.service.swift (1)
17-18: Add documentation for the queue parameterConsider adding a documentation comment explaining the purpose and usage of the queue parameter.
+/// Creates a new file import operation +/// - Parameters: +/// - db: The database connection +/// - queue: Queue for processing the import asynchronously +/// - groupOwnerId: The ID of the group owner +/// - key: The import key +/// - fileName: Name of the file being imported +/// - filePath: Path to the file being imported +/// - Returns: The created FileImportReport func createFileImport( on db: Database, withQueue queue: Queue, groupOwnerId: UUID, key: String, fileName: String, filePath: String )Package.swift (1)
17-19: Consider pinning the queues dependency version.While updating Vapor to 4.106.0 is good, the queues dependency at 1.16.1 might be critical for the rule-engine. Consider pinning to a specific version range for better stability.
-.package(url: "https://github.com/vapor/queues.git", from: "1.16.1"), +.package(url: "https://github.com/vapor/queues.git", .upToNextMinor(from: "1.16.1")),src/swift-server-tests/importers/importer-helper.swift (1)
127-132: Consider enhancing error handling and documentation.The implementation is correct and follows the established pattern of the
getDb()method. However, there are a few potential improvements:Consider applying these enhancements:
+ /// Returns the queue instance from the application. + /// - Throws: TestError if the application instance is not initialized. + /// - Returns: The queue instance for processing async jobs. func getQueue() throws -> Queue { guard let app = app else { - throw TestError() + throw TestError(message: "Application not initialized when accessing queue") } return app.queues.queue }The changes would:
- Add documentation explaining the method's purpose and error conditions
- Provide a more descriptive error message for debugging
src/swift-server-tests/profile.tests.swift (3)
Line range hint
8-25: Enhance user list validation.While the basic admin access test is good, consider adding assertions to validate the actual user data returned, not just the count.
XCTAssertEqual(data.results.count, 2) +// Verify user properties +XCTAssertTrue(data.results.contains { $0.username == testAdmin.username }) +XCTAssertTrue(data.results.contains { $0.username == testUser.username })
Line range hint
27-43: Add edge cases to profile update tests.The authorization and update tests are well structured, but consider adding tests for:
- Partial updates (updating only some fields)
- Invalid email format
- Username uniqueness constraints
- Password complexity rules
Also applies to: 45-63, 65-106
Line range hint
108-116: Critical: Implement missing test methods.Several crucial test methods are currently empty:
testUpdateWithInvalidDefaultGroupId: Validates group ID constraintstestCreateUser: Tests user creation flowtestCreateUserAsAdmin: Verifies admin-specific creation capabilitiestestDeleteUser: Ensures proper user deletiontestDeleteUserAsAdmin: Validates admin deletion privilegesThese tests are essential for ensuring the reliability of user management operations.
Would you like me to help generate the implementation for these test methods?
src/swift-server-tests/importers/importer-commerz-bank.swift (1)
31-32: Consider adding queue-specific test scenarios.While the basic import functionality is well tested, consider adding test cases that specifically verify the queue behavior:
- Test queue processing order
- Test concurrent imports
- Test queue error handling
Would you like me to help create these additional test scenarios?
src/swift-server/user/user-api-impl.swift (1)
Line range hint
8-144: Consider architectural improvements for better maintainability and security.
The unimplemented endpoints (
ApiUser_create,ApiUser_deleteGroup,ApiUser_addGroup,ApiUser_delete) should be prioritized for implementation to ensure API completeness.The admin permission check is repeated across endpoints. Consider extracting it to a middleware:
struct AdminMiddleware: AsyncMiddleware { func respond(to request: Request, chainingTo next: AsyncResponder) async throws -> Response { guard try await getUser(fromRequest: request).isAdmin else { return .unauthorized(.init(body: .json(.init( message: "Admin access required", code: ApiError.API10014.rawValue)))) } return try await next.respond(to: request) } }
- Consider adding:
- Rate limiting for API endpoints
- Request validation middleware
- API versioning support
- Comprehensive error handling middleware
Would you like me to provide implementation details for any of these suggestions?
src/swift-server/importer/importers/new-import.service.swift (1)
Line range hint
64-156: Consider transaction atomicity with queue-based processingThe introduction of queue-based processing raises some architectural considerations:
- What happens if the transaction is saved but the queued job fails?
- Is there a need for transaction rollback in case of queue failures?
- How are queued jobs monitored and retried?
Consider documenting the failure handling strategy and ensuring proper error recovery mechanisms are in place.
src/swift-server-tests/importers/importer-caixa-enginyers.tests.swift (1)
25-26: Consider documenting queue configuration and resource implications.The introduction of queue management suggests a shift towards asynchronous processing for imports. Consider:
- Documenting the queue configuration and its impact on system resources
- Adding comments about expected queue behavior in the test files
- Including error handling tests for queue-related scenarios
This will help maintain the system's reliability as import volumes grow.
Also applies to: 74-75
src/swift-server-tests/rules.tests.swift (2)
8-24: Add documentation and improve type safety.Consider the following improvements:
- Add documentation explaining the purpose of the struct and its relationship with
ConditionOperation.- Add validation to ensure the operation type matches the value type (e.g., string operations with string values).
+/// Represents a condition in a rule with either string or float value. +/// The value type must match the operation type (string operations with string values, +/// numeric operations with float values). struct BaseCondition { let operation: ConditionOperation let valueStr: String? let valueFloat: Double? init(operation: ConditionOperation, valueStr: String) { + precondition(operation.isStringOperation, "Operation must be a string operation") self.operation = operation self.valueStr = valueStr self.valueFloat = nil } init(operation: ConditionOperation, valueFloat: Double) { + precondition(operation.isNumericOperation, "Operation must be a numeric operation") self.operation = operation self.valueStr = nil self.valueFloat = valueFloat } }
57-88: Refactor test data creation for better maintainability.The
createBasicRulesmethod has repetitive code that could be simplified using a data-driven approach.private func createBasicRules() async throws { let db = app!.db - - // String operations - let _ = try await createBasicRule( - on: db, for: testGroup, with: .init(operation: .prefix, valueStr: "needle"), - toApply: labels[0]) + let testCases = [ + (operation: .prefix, value: "needle" as Any, labelIndex: 0), + (operation: .regularExpression, value: "needle", labelIndex: 1), + // ... other cases + ] + + for testCase in testCases { + let condition = testCase.value is String + ? BaseCondition(operation: testCase.operation, valueStr: testCase.value as! String) + : BaseCondition(operation: testCase.operation, valueFloat: testCase.value as! Double) + + try await createBasicRule( + on: db, + for: testGroup, + with: condition, + toApply: labels[testCase.labelIndex] + ) + }src/swift-server/routes.swift (1)
83-123: Add documentation for rule-engine endpoints.Consider adding documentation comments for each rule-engine endpoint to describe:
- Expected input parameters and their constraints
- Possible error conditions and responses
- Example usage
Example format:
/// Creates a new rule with the specified parameters /// - Parameter input: The rule creation parameters /// - Throws: `ValidationError` if the rule parameters are invalid /// - Returns: The created rule details func ApiRule_create(_ input: Operations.ApiRule_create.Input) async throwssrc/swift-server-tests/app-test.swift (1)
46-49: Consider adding documentation for the factory propertiesThe factory properties are well-structured and appropriately declared as constants. Consider adding documentation comments to describe their testing purposes and usage patterns.
+ /// Factory for creating test Rule instances let ruleFactory = RuleFactory() + /// Factory for creating test RuleCondition instances let conditionFactory = RuleConditionFactory() + /// Factory for creating test BankTransaction instances let transactionFactory = BankTransactionFactory() + /// Factory for creating test Label instances let labelFactory = LabelFactory()src/swift-server/migrations/initial-migration.swift (2)
63-64: Address the TODO comment about enums.The comment suggests using enums for link_reason. This would provide type safety and prevent invalid values.
Would you like me to help implement the enum for link_reason?
67-74: Enhance schema with index and type safety.Two suggestions for improvement:
- Add an index on parent_id for better hierarchical query performance
- Consider using an enum for conditions_relation instead of string
try await database.schema("core_rule") .id() .field("group_owner_id", .uuid, .required, .references("user_groups", "id")) .field("name", .string, .required) .field("conditions_relation", .string, .required) .field("parent_id", .uuid, .references("core_rule", "id")) + .index("parent_id") .create()src/swift-server-tests/importers/importer-service.tests.swift (2)
41-42: Consider adding queue-specific test cases.While the
withQueueparameter has been consistently added across all test methods, consider adding specific test cases to verify:
- Queue failure scenarios
- Queue timeout scenarios
- Concurrent import handling
Also applies to: 68-69, 112-113, 143-144, 148-149, 177-178
177-178: Enhance error handling for queued operations.With the introduction of queue-based processing, ensure that:
- Errors are properly propagated through the queue
- Failed imports don't block the queue
- Error states are consistently reported in the status reports
src/swift-server-tests/bank-transactions.tests.swift (3)
11-33: LGTM! Consider adding documentation for test data setup.The test data creation is well-structured with good variety in dates and group ownership. The factory pattern is properly implemented.
Consider adding a comment block explaining the test data setup strategy, particularly the significance of the different dates (2022/2023) and group ownership patterns.
Line range hint
200-209: LGTM! Consider adding negative test cases.The test setup is thorough with pre-attached labels.
Consider adding test cases for error scenarios, such as:
- Unlinking a non-existent label
- Unlinking from a transaction that doesn't exist
- Unlinking a label that wasn't attached
Line range hint
1-338: Consider implementing a shared test fixture setup.The test class shows consistent use of the factory pattern across all test methods. To reduce code duplication and improve maintainability, consider:
- Moving common transaction creation logic to a shared setup method
- Using test fixtures for common test data scenarios
- Implementing helper methods for frequently used assertions
src/@typespec/openapi3/openapi.yaml (2)
Line range hint
1085-1505: Add schema descriptions for better API documentation.The new schemas lack descriptions that would help API consumers understand their purpose and usage. Consider adding descriptions for:
Condition,ConditionFloat,ConditionString: Explain their role in rule evaluationOperationFloat,OperationString: Document the behavior of each operationRuleParam: Describe required fields and their impactExample addition:
OperationFloat: type: string + description: Defines comparison operations for numeric values enum: - greater - greaterEqual - lower - lowerEqual
560-732: Consider rate limiting and validation for rule operations.The rule engine endpoints could potentially impact system performance if misused. Consider:
- Implementing rate limiting for rule creation and application
- Adding validation for:
- Maximum number of conditions per rule
- Maximum number of rules per group
- Complexity of regular expressions in string conditions
- Adding appropriate caching headers for GET operations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
Package.resolved(9 hunks)Package.swift(3 hunks)package.yaml(1 hunks)src/@typespec/openapi3/openapi.yaml(5 hunks)src/swift-server-tests/app-test.swift(4 hunks)src/swift-server-tests/bank-transactions.tests.swift(6 hunks)src/swift-server-tests/factories.swift(1 hunks)src/swift-server-tests/importers/importer-caixa-enginyers.tests.swift(2 hunks)src/swift-server-tests/importers/importer-commerz-bank.swift(1 hunks)src/swift-server-tests/importers/importer-helper.swift(2 hunks)src/swift-server-tests/importers/importer-n26.tests.swift(2 hunks)src/swift-server-tests/importers/importer-service.tests.swift(6 hunks)src/swift-server-tests/profile.tests.swift(1 hunks)src/swift-server-tests/rules.tests.swift(1 hunks)src/swift-server/bank-transaction/bank-transaction.models.swift(0 hunks)src/swift-server/bank-transaction/bank-transaction.service.swift(2 hunks)src/swift-server/configure.swift(3 hunks)src/swift-server/importer/importer-api-impl.swift(1 hunks)src/swift-server/importer/importer.service.swift(2 hunks)src/swift-server/importer/importers/new-import.service.swift(4 hunks)src/swift-server/migrations/initial-migration.swift(2 hunks)src/swift-server/routes.swift(2 hunks)src/swift-server/rules/new-transaction.job.swift(1 hunks)src/swift-server/rules/rules.models.swift(1 hunks)src/swift-server/rules/rules.service.swift(1 hunks)src/swift-server/user/create-user.command.swift(1 hunks)src/swift-server/user/user-api-impl.swift(1 hunks)src/swift-server/user/user.service.swift(1 hunks)src/typespec/rules.tsp(1 hunks)
💤 Files with no reviewable changes (1)
- src/swift-server/bank-transaction/bank-transaction.models.swift
✅ Files skipped from review due to trivial changes (1)
- src/swift-server/user/create-user.command.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- package.yaml
- src/swift-server/rules/rules.models.swift
🧰 Additional context used
📓 Learnings (1)
src/typespec/rules.tsp (1)
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/typespec/rules.tsp:49-52
Timestamp: 2024-11-07T16:01:46.093Z
Learning: In `src/typespec/rules.tsp`, the `ConditionString` model does not have a `field` property.
🪛 SwiftLint
src/swift-server-tests/app-test.swift
[Warning] 66-66: Prefer at least one space after slashes for comments
(comment_spacing)
🔇 Additional comments (48)
src/swift-server/rules/new-transaction.job.swift (1)
12-25: Verify job registration in application configuration.
Ensure this job is properly registered in the application's queue configuration.
✅ Verification successful
Job is properly registered in application configuration ✅
The NewTransactionJob is correctly registered in the application's queue configuration:
- Registered via
app.queues.add(NewTransactionJob())inconfigure.swift - Queue system is properly configured with Fluent driver in production
- In-process jobs and scheduled jobs are started appropriately
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewTransactionJob registration in configuration files
echo "Searching for job registration..."
rg -l "NewTransactionJob" --type swift
# Search for queue configuration
echo -e "\nSearching for queue configuration..."
rg "queues\.use|configureQueues" --type swift
Length of output: 607
Script:
#!/bin/bash
# Check the registration of NewTransactionJob in configure.swift
echo "Checking job registration in configure.swift..."
rg -A 5 -B 5 "NewTransactionJob" src/swift-server/configure.swift
# Check queue configuration and job registration in app-test.swift
echo -e "\nChecking test configuration..."
rg -A 5 -B 5 "NewTransactionJob|queues\.use" src/swift-server-tests/app-test.swift
Length of output: 1039
src/swift-server-tests/factories.swift (1)
60-64: LGTM! RuleFactory implementation follows established patterns.
The implementation is clean and consistent with other factories in the file.
src/typespec/rules.tsp (3)
1-6: LGTM: Imports and namespace setup are correct
The necessary TypeSpec modules are properly imported and the namespace is correctly configured.
22-26: LGTM: Rule model is well-structured
The model correctly extends RuleParam and includes an array of conditions.
67-80: Verify OpenAPI documentation completeness
Let's ensure all operations are properly documented in the OpenAPI specification.
✅ Verification successful
Let me verify the OpenAPI documentation by checking the content of the OpenAPI spec files.
Let me do one final check to verify the completeness of the OpenAPI spec against the TypeSpec definitions.
OpenAPI documentation is complete and consistent
All TypeSpec operations for the rules API are properly documented in the OpenAPI specification:
- List operation (GET /rules)
- Create operation (POST /rules)
- Update operation (PUT /rules/{ruleId})
- Apply operation (POST /rules/{ruleId}/apply)
- Delete operation (DELETE /rules/{ruleId})
- Add condition (POST /rules/{ruleId}/condition)
- Update condition (PUT /rules/{ruleId}/condition/{condId})
- Delete condition (DELETE /rules/{ruleId}/condition/{condId})
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all API operations are documented in OpenAPI spec
# Expected: Each operation should have a corresponding path in the OpenAPI spec
# Look for the OpenAPI spec file
echo "Searching for OpenAPI specification files..."
fd -e yaml -e json --exec grep -l "openapi:" {}
# Search for API operations in TypeSpec
echo -e "\nAPI operations defined in TypeSpec:"
rg "@(post|put|delete|get)" "src/typespec/rules.tsp"
Length of output: 991
Script:
#!/bin/bash
# Check the content of OpenAPI spec files for the rule endpoints
# First, let's check the main OpenAPI spec
echo "Content of src/swift-server/openapi.yaml for rule endpoints:"
rg -A 5 "/rules" "src/swift-server/openapi.yaml"
# Also check if there are any rule-related paths
echo -e "\nAll rule-related paths in OpenAPI spec:"
rg "^\s+/.*rule" "src/swift-server/openapi.yaml"
# Check for operation IDs related to rules
echo -e "\nOperation IDs related to rules:"
rg "operationId:.*rule" "src/swift-server/openapi.yaml"
Length of output: 1232
Script:
#!/bin/bash
# Compare TypeSpec operations with OpenAPI spec paths
# Get all HTTP methods and paths from TypeSpec
echo "TypeSpec operations:"
rg -o "@(post|put|delete|get).*@route\([^)]*\)" "src/typespec/rules.tsp"
# Get all operations from OpenAPI spec
echo -e "\nOpenAPI spec operations:"
rg "^\s+(get|post|put|delete):" "src/swift-server/openapi.yaml" -A 1
Length of output: 2278
src/swift-server-tests/importers/importer-n26.tests.swift (1)
23-23: LGTM! Consider adding queue-specific test cases.
The addition of the queue parameter is consistent with the broader changes across importers. While the basic functionality is well-tested, consider adding specific test cases to verify queue-related behavior.
Let's verify if other importers have been updated consistently:
✅ Verification successful
Queue parameter has been consistently implemented across all importers
The verification shows that the withQueue: getQueue() parameter is consistently used across all importer test files:
- importer-n26.tests.swift
- importer-commerz-bank.swift
- importer-caixa-enginyers.tests.swift
- importer-service.tests.swift
The implementation is uniform and follows the same pattern in all importers, confirming that the change is part of a broader, consistent update across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all importer tests use the new queue parameter
# Expected: All importers should use withQueue parameter
rg -l "importFromFile.*withQueue:" "src/swift-server-tests/importers/"
Length of output: 70
Script:
#!/bin/bash
# Let's try a broader search to understand the importer test patterns
rg -A 2 "importFromFile" "src/swift-server-tests/importers/"
Length of output: 4118
Script:
#!/bin/bash
# Let's also check for any queue-related patterns in the importers directory
rg -A 2 "getQueue\(\)" "src/swift-server-tests/importers/"
Length of output: 5102
Script:
#!/bin/bash
# List all files in the importers directory to ensure we're searching in the right place
fd . "src/swift-server-tests/importers/"
Length of output: 346
src/swift-server/configure.swift (3)
6-6: LGTM: Import statement is correctly placed.
The addition of QueuesFluentDriver import aligns with the new queue functionality being implemented.
63-63: LGTM: Queue driver configuration.
The Fluent queue driver setup is correctly configured.
43-43: Verify migration order dependencies.
While the addition of JobModelMigration is correct, please ensure there are no dependencies between JobModelMigration and InitialMigration that would require a specific order.
✅ Verification successful
Let me generate another script to check the JobModelMigration content since it wasn't found in the previous search.
Based on the search results, I can now generate the final response:
Migration order appears safe to proceed
The JobModelMigration is added between SessionRecord.migration and InitialMigration. From the extensive search of the codebase:
- No jobs-related tables were found in
InitialMigration - No foreign key dependencies were found between jobs tables and other tables
- The job system appears to be a new, independent feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential migration dependencies
# Look for references between migrations and shared table names
# Search for table names and foreign key references in both migrations
rg -A 10 "create|addForeignKey|references" --glob "**/*migration*.swift"
Length of output: 14735
Script:
#!/bin/bash
# Search for JobModelMigration file and its content
rg -l "JobModelMigration"
fd "JobModelMigration"
Length of output: 81
Script:
#!/bin/bash
# Search for job-related migrations and tables
rg -A 10 "create|addForeignKey|references" --glob "**/migrations/*job*.swift"
# Search for any file containing JobModelMigration class definition
ast-grep --pattern 'struct JobModelMigration {
$$$
}'
Length of output: 286
Script:
#!/bin/bash
# Try to find the job migration file with a broader search
fd -e swift -x cat {} | rg -A 10 -B 2 "JobModelMigration"
# Also search for any jobs table creation
rg -A 10 "schema.*jobs?"
Length of output: 547
src/swift-server/importer/importer.service.swift (2)
2-2: LGTM: Queue import added for async processing support
The addition of the Queues import is appropriate for the new queue-based processing functionality.
17-18: Verify all callers of createFileImport are updated
The implementation correctly integrates queue-based processing, but this is a breaking change that requires updates to all callers.
Also applies to: 21-22
✅ Verification successful
Based on the ripgrep results, I can generate the final response as we have found all the callers of createFileImport:
All callers of createFileImport are properly updated
The only caller of createFileImport in importer-api-impl.swift is already passing the new queue parameter correctly with the expected withQueue label. No further updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to createFileImport to ensure they include the new queue parameter
rg -A 3 "createFileImport\(" --type swift
Length of output: 798
Package.swift (3)
35-35: Major version bump for swift-format requires attention.
The swift-format dependency has been updated from 510.1.0 to 600.0.0, which is a major version bump. This could introduce breaking changes in code formatting.
#!/bin/bash
# Description: Check for swift-format configuration file
# Expected: Find format configuration to ensure compatibility
# Search for swift-format configuration
fd -e swift-format.json -e swift-format.yaml55-57: Verify queue integration with rule-engine.
The addition of QueuesFluentDriver and XCTQueues suggests background job processing for the rule-engine. Ensure proper test coverage for asynchronous rule processing.
Also applies to: 77-77
✅ Verification successful
Queue integration with rule-engine is properly implemented and tested
The codebase shows a well-structured integration between queues and rule-engine:
NewTransactionJobprocesses rules for new transactions asynchronously- Comprehensive test coverage in
rules.tests.swiftverifies rule application and job dispatching - Queue configuration is properly set up in
configure.swiftwith Fluent driver
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for rule-engine related test files
# Expected: Find test files covering rule processing jobs
# Search for rule-engine related test files
rg -l "class.*Test.*Rule" "src/swift-server-tests"
Length of output: 50
Script:
#!/bin/bash
# Let's broaden our search to find any rule engine related files and tests
rg -l "rule.*engine|RuleEngine"
# Also search for queue-related tests
rg -l "class.*Test.*Queue|func.*test.*Queue"
# Check for any background job processing related code
rg -l "background.*job|BackgroundJob|JobQueue"
Length of output: 137
Script:
#!/bin/bash
# Let's check the todo.md file that mentioned rule engine
cat todo.md
# Let's also check for any queue-related implementations
rg -l "Queue|queue"
# And check Package.swift for more context about the queues integration
cat Package.swift
Length of output: 6041
Script:
#!/bin/bash
# Let's check the implementation of rules and queue processing
cat src/swift-server/rules/new-transaction.job.swift
# Check the test file for rules
cat src/swift-server-tests/rules.tests.swift
# Check how queues are configured
cat src/swift-server/configure.swift
Length of output: 6973
13-14: Verify OpenAPI version compatibility with the rule-engine endpoints.
The OpenAPI dependencies have been updated to newer versions (generator: 1.4.0, runtime: 1.6.0). While these updates are good, we should ensure the new rule-engine endpoints are properly defined in the OpenAPI spec.
✅ Verification successful
OpenAPI specification and rule-engine endpoints are properly defined and compatible with the updated dependencies.
The OpenAPI specification (openapi.yaml) includes comprehensive rule-engine endpoints under the /rules path with proper operations for:
- Listing rules (
GET /rules) - Creating rules (
POST /rules) - Updating rules (
PUT /rules/{ruleId}) - Deleting rules (
DELETE /rules/{ruleId}) - Adding conditions (
POST /rules/{ruleId}/condition) - Updating conditions (
PUT /rules/{ruleId}/condition/{condId}) - Deleting conditions (
DELETE /rules/{ruleId}/condition/{condId}) - Applying rules (
POST /rules/{ruleId}/apply)
The endpoints are well-defined with appropriate request/response schemas and error handling. The updated OpenAPI dependencies (generator: 1.4.0, runtime: 1.6.0) are compatible with these endpoints and their implementation in the rules service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if rule-engine endpoints are defined in OpenAPI spec
# Expected: Find rule-engine related endpoint definitions
# Search for rule-engine endpoints in OpenAPI spec
rg -A 5 "paths:.*rule" "src/swift-server/openapi.yaml"
Length of output: 140
Script:
#!/bin/bash
# Let's first check if the OpenAPI spec file exists and its location
fd -e yaml -e yml | grep -i "openapi"
# Also search for any rule-engine related files to understand the context
fd -e swift | grep -i "rule"
Length of output: 354
Script:
#!/bin/bash
# Let's examine the OpenAPI spec file content
cat src/swift-server/openapi.yaml
# Also check the rules service implementation to understand the API requirements
cat src/swift-server/rules/rules.service.swift
Length of output: 42186
src/swift-server-tests/importers/importer-helper.swift (1)
2-2: LGTM!
The addition of the Queues import is appropriate for the new queue functionality being added.
src/swift-server-tests/profile.tests.swift (1)
Line range hint 1-7: LGTM! Appropriate test framework imports.
The imports and class declaration are well-structured, with necessary testing frameworks and database access through Fluent.
src/swift-server/importer/importer-api-impl.swift (1)
99-100: LGTM! Consider enhancing error handling and documentation.
The addition of queue-based processing aligns well with the new rule-engine architecture. However, consider the following suggestions:
- Ensure proper error handling for queue-related failures
- Document the queuing behavior, particularly:
- What happens if the queue is unavailable?
- Are there any timeout considerations?
- How are failed imports retried?
Let's verify the error handling implementation:
src/swift-server-tests/importers/importer-commerz-bank.swift (1)
31-32: LGTM! Verify queue configuration.
The addition of queue management through withQueue parameter is a good improvement for handling asynchronous imports.
Let's verify the queue configuration across other importers:
✅ Verification successful
Queue implementation is consistent across importers
The verification shows that the withQueue: getQueue() pattern is consistently implemented across all importer test files:
- All importFromFile calls include the withQueue parameter
- The getQueue() method is defined in importer-helper.swift
- The pattern is used uniformly in all test files (caixa-enginyers, n26, commerz-bank, and importer-service)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent queue usage across importers
# Expected: All importers should use the same queue pattern
# Search for importFromFile calls in test files
rg -t swift "importFromFile.*withQueue:" "src/swift-server-tests"
Length of output: 65
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# 1. First, find all test files related to importers
echo "=== Importer Test Files ==="
fd "importer.*\.swift$" src/swift-server-tests
# 2. Then look for importFromFile usage pattern
echo -e "\n=== ImportFromFile Usage ==="
rg "importFromFile" src/swift-server-tests
# 3. Check for queue-related methods
echo -e "\n=== Queue Usage Patterns ==="
rg "getQueue|withQueue" src/swift-server-tests
# 4. Look at the base test class
echo -e "\n=== Base Test Class Content ==="
fd "BaseImporterTests" src/swift-server-tests --exec cat {}
Length of output: 4291
src/swift-server/importer/importers/new-import.service.swift (3)
3-3: LGTM: Queue import added appropriately
The Queues import is correctly placed with other framework imports and supports the new queue-based transaction processing.
119-119: Document the queue usage in transaction processing
While the queue parameter is consistently passed to bankTransactionService.addTransaction, it's unclear what type of jobs are being queued and how they relate to the rule engine implementation.
Please add documentation explaining:
- The purpose of the queue in transaction processing
- What jobs are being queued
- How this integrates with the new rule engine
#!/bin/bash
# Check the bankTransactionService implementation to understand queue usage
ast-grep --pattern 'func addTransaction(on: Database, withQueue: Queue, transaction: BankTransaction)'Also applies to: 132-133
64-65: Verify all callers of importFromFile are updated
The signature change is breaking and requires updates to all calling code. The AI summary mentions test files were updated, but let's verify all callers are covered.
✅ Verification successful
Let me try a different approach to find the callers and the method definition. The previous search might have failed due to various reasons like case sensitivity or incomplete pattern. The new search includes more specific parameters and uses both ripgrep and ast-grep to get a comprehensive view.
Let me try a different approach by first locating the file and understanding the complete context, then searching for any files that might be using this method, including test files. This will help us verify the impact of the signature change.
We found several files that reference importFromFile. Let's examine the main service file and a test file to verify if they're using the updated signature with the queue parameter.
All callers have been properly updated with the new queue parameter
The verification shows that all callers of importFromFile are already using the updated signature with the withQueue parameter:
- In
FileImportService.swift, the main service calls it correctly:
try await uploadImportService.importFromFile(on: db, withQueue: queue, groupOwnerId: groupOwnerId, key: key, fileName: fileName, filePath: filePath)- In
importer-service.tests.swift, all test cases use the updated signature:
try await importerService.importFromFile(on: db, withQueue: getQueue(), groupOwnerId: groupOwnerId, key: "test-account", fileName: "test-file.csv", filePath: "whatever")The breaking change has been properly propagated to all calling code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all importFromFile method calls
rg -A 2 "importFromFile\("
Length of output: 104
Script:
#!/bin/bash
# Search for importFromFile method calls with better pattern matching
rg -A 2 "importFromFile\(.*db.*fileName.*filePath"
# Also search for the method definition to understand its full context
ast-grep --pattern 'func importFromFile($$$) {'
Length of output: 102
Script:
#!/bin/bash
# Try to find the file and method definition first
fd "new-import.service.swift" --exec cat {}
# Search for any files containing 'importFromFile' to cast a wider net
rg -l "importFromFile"
# Look for test files that might be using this method
fd -e swift -e test --exec rg -l "importFromFile" {}
Length of output: 5052
Script:
#!/bin/bash
# Let's check the content of importer.service.swift as it's likely the main caller
cat src/swift-server/importer/importer.service.swift
# Also check one of the test files to see the test usage
cat src/swift-server-tests/importers/importer-service.tests.swift
Length of output: 9727
src/swift-server-tests/importers/importer-caixa-enginyers.tests.swift (2)
74-75: LGTM! Consistent implementation across import types.
The queue management implementation is consistently applied across both account and credit card imports, which is a good practice.
25-26: Verify consistent queue usage across import tests.
The addition of queue management looks good. Let's verify that this pattern is consistently applied across all importer tests.
✅ Verification successful
This will help us see all test files and the context around importFromFile calls to verify queue parameter usage.
Queue parameter is consistently used across all importer tests
The verification shows that all importFromFile calls in the importer test files (importer-service.tests.swift, importer-n26.tests.swift, importer-caixa-enginyers.tests.swift, and importer-commerz-bank.swift) consistently include the withQueue: getQueue() parameter. The changes are properly aligned with the codebase's patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of withQueue parameter across import tests
# Expect: All importFromFile calls should include withQueue parameter
# Search for importFromFile calls without withQueue parameter
rg "importFromFile\([^)]*\)" --type swift "src/swift-server-tests" -A 3 | rg -v "withQueue"
Length of output: 89
Script:
#!/bin/bash
# Let's try a different approach to find importFromFile calls
# First, let's find all files that might contain import tests
fd ".*tests\.swift$" "src/swift-server-tests/importers"
# Then let's look for all importFromFile patterns with more context
rg "importFromFile" --type swift "src/swift-server-tests/importers" -B 2 -A 2
Length of output: 6449
src/swift-server-tests/rules.tests.swift (1)
1-7: LGTM! All necessary imports are present.
The imports cover all required frameworks for testing, database operations, and queue management.
src/swift-server/routes.swift (1)
146-146: Verify the server URL change impact.
The server URL configuration has changed from using server1() to Server1.url(). This might be a breaking change if other parts of the codebase still reference the old method.
✅ Verification successful
Server URL change appears to be an isolated update
The change from server1() to Server1.url() appears to be an isolated update, as:
- No other references to the old
Servers.server1()method exist in the codebase - The new
Servers.Server1.url()is only used in this single location - This suggests the change is part of a coordinated API update and not a breaking change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old server URL method
rg "Servers\.server1\(\)" --type swift
Length of output: 40
Script:
#!/bin/bash
# Let's search for any references to both old and new server URL patterns
# to understand the usage patterns better
rg "Servers\.(server1|Server1)" --type swift -A 2
# Also search for the Servers enum/class definition
ast-grep --pattern 'enum Servers {
$$$
}'
# And search for any other server URL related patterns
rg "\.url\(\)" --type swift -A 2
Length of output: 564
src/swift-server-tests/app-test.swift (4)
3-3: LGTM: XCTQueues import added for queue testing support
The import is correctly placed and aligns with the new queue testing functionality being introduced.
64-65: LGTM: Queue configuration added for async testing
The queue configuration is correctly set up for async testing using the test driver.
Line range hint 91-97: LGTM: Label factory usage is well-implemented
The label creation logic is clean and properly handles multiple groups. The use of functional programming patterns with proper error handling is commendable.
66-66: Fix comment spacing and clarify NewTransactionJob status
The commented code suggests incomplete implementation. Either implement the NewTransactionJob or remove the comment if it's no longer needed.
Also, fix the comment spacing to comply with SwiftLint:
-//app.queues.add(NewTransactionJob())
+// app.queues.add(NewTransactionJob())✅ Verification successful
Keep the commented NewTransactionJob line with proper spacing
The NewTransactionJob is properly implemented in src/swift-server/rules/new-transaction.job.swift. The commented line appears in a test setup context where it's intentionally disabled while using the async test queue driver. The comment should be kept with proper spacing as a reference for future testing scenarios.
Fix the spacing as suggested:
-//app.queues.add(NewTransactionJob())
+// app.queues.add(NewTransactionJob())🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NewTransactionJob is implemented elsewhere in the codebase
rg "class NewTransactionJob" -A 5
Length of output: 635
Script:
#!/bin/bash
# Check if NewTransactionJob is used/imported in the test file
rg -l "NewTransactionJob" src/swift-server-tests/app-test.swift
# Check the context around the commented line in the test file
rg -B 5 -A 5 "//app.queues.add\(NewTransactionJob\(\)\)" src/swift-server-tests/app-test.swift
Length of output: 607
🧰 Tools
🪛 SwiftLint
[Warning] 66-66: Prefer at least one space after slashes for comments
(comment_spacing)
src/swift-server/bank-transaction/bank-transaction.service.swift (2)
2-2: LGTM: Import statement correctly added for queue functionality.
The addition of the Queues import aligns with the new asynchronous transaction processing architecture.
120-124: Verify NewTransactionJob implementation.
The code dispatches to NewTransactionJob, but we should verify:
- The job class exists and is properly implemented
- The job can handle the provided transaction data
- Error handling is in place within the job processing
✅ Verification successful
Let me gather more information about the NewTransactionJob implementation.
Let me check the TransactionSummary type and rules service implementation since they are key components.
NewTransactionJob implementation is properly structured and handles the transaction data correctly
The verification confirms:
- NewTransactionJob exists and properly implements AsyncJob protocol with required dequeue and error methods
- TransactionSummary payload matches the dispatched data structure with id, movementName, value, and groupOwnerId
- Error handling is in place with a dedicated error method
- The job processes transactions through RulesService which has proper implementation for applying rules and handling database operations
The implementation is sound and correctly processes the transaction data passed from the bank-transaction service.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for NewTransactionJob implementation
echo "Searching for NewTransactionJob implementation..."
rg -l "class NewTransactionJob"
# Check for proper job protocol conformance
echo "Checking for proper Job protocol implementation..."
ast-grep --pattern 'class NewTransactionJob: Job {
$$$
func dequeue($_) $$$
$$$
}'
Length of output: 393
Script:
#!/bin/bash
# Get the full implementation of NewTransactionJob
echo "Getting NewTransactionJob implementation..."
rg -A 20 "class NewTransactionJob" src/swift-server/rules/new-transaction.job.swift
# Check initialization and property handling
echo "Checking initialization and properties..."
ast-grep --pattern 'struct NewTransactionJob.Payload {
$$$
}'
# Check error handling
echo "Checking error handling in dequeue..."
rg -B 2 -A 5 "catch" src/swift-server/rules/new-transaction.job.swift
Length of output: 981
Script:
#!/bin/bash
# Check TransactionSummary definition
echo "Checking TransactionSummary definition..."
rg -A 10 "struct TransactionSummary"
# Check rulesService implementation
echo "Checking rulesService implementation..."
rg -A 10 "class RulesService"
# Check applyRules method
echo "Checking applyRules method..."
ast-grep --pattern 'func applyRules(on $_: $_,$_) $$$'
Length of output: 2149
src/swift-server/migrations/initial-migration.swift (2)
76-83: Uncomment the index and document operations.
- The commented index should be enabled for better query performance
- The operation field values should be documented for maintainability
try await database.schema("core_condition")
.id()
.field("rule_id", .uuid, .required, .references("core_rule", "id"))
.field("operation", .string, .required)
.field("value_str", .string)
.field("value_float", .float)
- // .index("rule_id")
+ .index("rule_id")
.create()Consider adding a comment documenting the valid operations and when to use value_str vs value_float.
190-195: LGTM! Proper cleanup order.
The deletion order correctly respects foreign key constraints, ensuring clean removal of all schemas.
src/swift-server-tests/importers/importer-service.tests.swift (3)
Line range hint 1-200: Well-structured test suite with comprehensive coverage.
The test suite maintains good organization and covers essential scenarios including:
- Happy path
- Error cases
- Duplicate handling
- Invalid data scenarios
The addition of queue processing maintains the existing test patterns while enhancing the functionality.
106-108: Review concurrent transaction handling.
With the introduction of queue-based processing, there's a potential for race conditions between transaction additions and imports. Consider:
- Adding explicit transaction ordering assertions
- Implementing queue synchronization in test scenarios
- Adding test cases for concurrent import attempts
#!/bin/bash
# Search for transaction ordering and synchronization mechanisms
rg -t swift "class BankTransactionService" -A 20
rg -t swift "func addTransaction.*withQueue" -A 1020-21: Verify queue implementation in BaseImporterTests.
The addition of withQueue parameter suggests a shift to queue-based processing. Ensure that getQueue() is properly implemented in BaseImporterTests and that it provides a test-appropriate queue instance.
✅ Verification successful
Let me gather more specific information about the test file and queue implementation.
Let me search for the getQueue implementation since it's not visible in the base class.
Queue implementation is correctly set up for testing
The getQueue() method is properly implemented in BaseImporterTests through importer-helper.swift. It returns the application's queue instance (app.queues.queue) which is appropriate for testing purposes. The implementation is supported by the necessary imports (XCTQueues) and follows testing best practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for getQueue implementation in BaseImporterTests
rg -t swift "class BaseImporterTests" -A 10
rg -t swift "func getQueue" -A 5
Length of output: 3238
Script:
#!/bin/bash
# Search for the test file content and BaseImporterTests
rg -t swift "class BaseImporterTests" -A 20
rg -t swift "import.*Queue"
cat src/swift-server-tests/importers/importer-service.tests.swift
Length of output: 9736
Script:
#!/bin/bash
# Search for getQueue implementation in test files
rg -t swift "func getQueue" -A 5 src/swift-server-tests/
rg -t swift "import.*XCTQueues" -A 10 src/swift-server-tests/
Length of output: 1883
src/swift-server-tests/bank-transactions.tests.swift (4)
112-115: LGTM! Factory usage is clean and consistent.
The transaction creation with group ownership assignment is well-implemented.
Line range hint 154-158: LGTM! Test setup is comprehensive.
The sequence creation for testing label linking is well-structured and provides good test coverage.
Line range hint 246-252: LGTM! Test setup is appropriate for comment testing.
The transaction sequence creation is well-implemented for testing comment functionality.
Line range hint 291-297: LGTM! Consider adding validation for empty strings.
The test setup with initial comments is well-implemented.
Consider verifying the handling of empty strings vs nil comments:
Package.resolved (5)
2-2: LGTM: Hash update reflects dependency changes
The originHash update is consistent with the dependency changes in this PR.
54-55: Core dependency updates look appropriate
The version updates for core dependencies are consistent and compatible:
- fluent: 4.11.0 → 4.12.0
- fluent-postgres-driver: 2.9.2 → 2.10.0
- fluent-sqlite-driver: 4.7.4 → 4.8.0
- vapor: 4.105.2 → 4.106.3
These are minor version bumps that maintain backward compatibility.
Also applies to: 72-73, 81-82, 441-443
378-379: OpenAPI updates are consistent
The OpenAPI-related dependencies have been updated appropriately:
- swift-openapi-generator: 1.3.1 → 1.4.0
- swift-openapi-runtime: 1.5.0 → 1.6.0
These updates align with the AI summary mentioning OpenAPI specification enhancements.
Also applies to: 388-388
279-280: Major version update requires attention
The Swift toolchain-related updates include breaking changes:
- swift-format: 510.1.0 → 600.0.0
- swift-syntax: 510.0.3 → 600.0.1
These are major version bumps that may require code adjustments.
Let's check for potential impact:
#!/bin/bash
# Check for Swift format configuration
echo "Checking for .swift-format configuration..."
fd -t f ".swift-format"
# Check for custom syntax rules
echo "Checking for custom syntax rules..."
rg -l "SwiftSyntax|SwiftFormat"Also applies to: 414-415
139-147: Verify queue implementation requirements
The addition of vapor-queues-fluent-driver and its dependencies suggests implementation of background job processing. This aligns with the rule-engine implementation, but using a non-versioned branch (main) could lead to stability issues.
Let's check the implementation requirements:
Consider:
- Pinning to a specific version instead of using the
mainbranch - Implementing retry mechanisms for failed jobs
- Setting appropriate job timeouts
Also applies to: 446-451
src/@typespec/openapi3/openapi.yaml (3)
629-634: Standardize delete operation response.
The delete operation returns a Date type while other delete operations in the API (e.g., /users/{id}) return a boolean. This inconsistency could confuse API consumers.
668-687: Add missing request body for condition creation.
The POST endpoint for adding conditions lacks a request body definition. This is required to specify the condition details.
668-732: Add error responses for condition operations.
The condition endpoints are missing error responses for common failure cases (400, 404).
Dracks
force-pushed
the
3.0.0/rule-engine
branch
from
40f68c3 to
d8e4984
Compare
Dracks
force-pushed
the
3.0.0/rule-engine
branch
4 times, most recently
from
1351f08 to
e0d603b
Compare
Dracks
force-pushed
the
3.0.0/rule-engine
branch
3 times, most recently
from
7eb8a66 to
215bdf2
Compare
Dracks
force-pushed
the
3.0.0/rule-engine
branch
4 times, most recently
from
2fd3d71 to
3cf1970
Compare
Dracks
force-pushed
the
3.0.0/rule-engine
branch
from
3cf1970 to
de46f21
Compare
Dracks
changed the title
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (48)
src/swift-server/label/label.service.swift (3)
Line range hint
6-10: Consider adding validation before saving the label.While the method correctly handles saving and relationship loading, consider adding validation for:
- Required fields
- Group ownership permissions
- Duplicate label names within the same group
Example validation approach:
func createLabel(label: Label) async throws -> Label { + try await validateLabel(label) try await label.save(on: db) try await label.$groupOwner.load(on: db) return label } + +private func validateLabel(_ label: Label) async throws { + guard let groupId = label.$groupOwner.id else { + throw Abort(.badRequest, reason: "Group owner is required") + } + // Add additional validation logic +}
Line range hint
19-24: Validate groupIds parameter.The method accepts groupIds without validation. Consider adding checks for:
- Empty array
- Invalid UUIDs
- User's access rights to these groups
Example validation:
func getAll(pageQuery: PageQuery = .init(), groupIds: [UUID]) async throws -> ListWithCursor<Label> { + guard !groupIds.isEmpty else { + throw Abort(.badRequest, reason: "At least one group ID is required") + } + try await validateGroupAccess(groupIds) var query = Label.query(on: db) .filter(\.$groupOwner.$id ~~ groupIds)
Line range hint
4-41: Consider defining a protocol for the label service.To improve testability and maintain a clear contract, consider extracting the service interface into a protocol:
protocol LabelServiceProtocol { func createLabel(label: Label) async throws -> Label func getAll(pageQuery: PageQuery, groupIds: [UUID]) async throws -> ListWithCursor<Label> } class LabelService: ServiceWithDb, LabelServiceProtocol { // Current implementation }This would:
- Make the service contract explicit
- Facilitate mock creation for testing
- Enable easier dependency injection
src/swift-server/bank-transaction/bank-transaction-api-impl.swift (6)
Line range hint
27-30: Avoid force unwrapping in response construction.Using force unwrapping (
!) could lead to runtime crashes if the data is unexpectedly nil.Consider using safe unwrapping:
- bankTransaction: transaction.list.first!, - labelIds: labelIds[transactionId]! + bankTransaction: transaction.list.first ?? BankTransaction(), // Add appropriate default + labelIds: labelIds[transactionId] ?? []
Line range hint
87-90: Similar force unwrapping issue in unlinkLabel response.The same force unwrapping issue exists in this method's response construction.
Apply similar safe unwrapping:
- bankTransaction: transaction.list.first!, - labelIds: labelIds[transactionId]! + bankTransaction: transaction.list.first ?? BankTransaction(), + labelIds: labelIds[transactionId] ?? []
Line range hint
128-129: Unsafe UUID parsing in group ID mapping.Force unwrapping UUIDs could crash if invalid strings are provided.
Consider using safe transformation:
- inputGroups = inputGroupsStr.map { UUID(uuidString: $0)! } + inputGroups = inputGroupsStr.compactMap { UUID(uuidString: $0) }
Line range hint
130-134: Group validation logic could be simplified.The current validation approach using filter is less readable.
Consider using set operations for clearer intent:
- if !groupIds.filter({ return !validGroupsIds.contains($0) }).isEmpty { + let invalidGroups = Set(groupIds).subtracting(Set(validGroupsIds)) + if !invalidGroups.isEmpty {
Line range hint
193-196: Force unwrapping in comment endpoint response.Similar force unwrapping issues in response construction.
Apply safe unwrapping:
- bankTransaction: transaction.list.first!, - labelIds: labelIds[transactionId]! + bankTransaction: transaction.list.first ?? BankTransaction(), + labelIds: labelIds[transactionId] ?? []
Line range hint
1-199: Consider extracting common response patterns.The file has several repeated patterns that could be extracted into helper methods:
- Transaction response construction
- Error response construction
- Group validation logic
This would improve maintainability and reduce the chance of inconsistencies.
Consider creating helper methods like:
private func createTransactionResponse( transaction: BankTransaction, transactionId: UUID, labelIds: [UUID] ) -> Components.Schemas.BankTransaction { // Common response construction logic } private func validateGroups(_ inputGroups: [UUID], against validGroups: [UUID]) throws { // Common group validation logic }src/swift-server-tests/app-test.swift (2)
42-42: Consider adding documentation for the new propertiesThe factory properties are well-structured as constants and follow a consistent naming pattern. However, adding documentation comments would help explain their purpose in the test setup.
Consider adding documentation like this:
+ /// Group used for additional test scenarios var testGroup3: UserGroup! + /// Factory for creating test rules let ruleFactory = RuleFactory() + /// Factory for creating test rule conditions let conditionFactory = RuleConditionFactory() + /// Factory for creating test bank transactions let transactionFactory = BankTransactionFactory() + /// Factory for creating test labels let labelFactory = LabelFactory()Also applies to: 47-50
Line range hint
94-98: Consider using map for more idiomatic Swift codeThe current implementation using forEach could be more concise.
Consider this more idiomatic approach:
-labelFactory.createSequence(8) { - $0.$groupOwner.id = testGroupId2 - return $0 -}.forEach { label in - labels.append(label) -} +labels.append(contentsOf: labelFactory.createSequence(8) { + $0.$groupOwner.id = testGroupId2 + return $0 +})src/swift-server/importer/importers/new-import.service.swift (3)
131-131: Fix inconsistent indentationThe indentation at line 131 is inconsistent with Swift style guidelines.
- transaction: transaction + transaction: transaction
Line range hint
93-136: Consider refactoring complex transaction processing logicThe transaction processing logic contains multiple nested conditions and repeated patterns. Consider extracting the logic into smaller, more focused methods for better maintainability.
Suggested refactor:
private func handleExistingTransaction( transaction: BankTransaction, status: FileImportReport, statusTransaction: FileImportRow, previousState: inout FileImportRow?, previous: inout BankTransaction?, discarting: inout Bool, on db: Database ) async throws { if discarting { try await handleDiscardedTransaction(status: status, statusTransaction: statusTransaction, previousState: previousState, on: db) } else { previous = transaction previousState = statusTransaction discarting = true } } private func handleDiscardedTransaction( status: FileImportReport, statusTransaction: FileImportRow, previousState: FileImportRow?, on db: Database ) async throws { let msg = "Repeated row, not inserted" status.status = .warn statusTransaction.message = msg if let previousStateValidated = previousState { previousStateValidated.message = msg try await previousStateValidated.save(on: db) } try await statusTransaction.save(on: db) }
Line range hint
138-152: Enhance error handling specificityThe catch block could be more specific about the types of errors it handles. Consider:
- Adding specific catch blocks for known error types
- Improving error logging with more context
do { // existing code } catch let error as Exception { status.description = String(describing: error) let jsonData = try JSONSerialization.data( withJSONObject: error.allContext, options: []) status.context = String(data: jsonData, encoding: .utf8) status.status = .error } catch { status.description = "Unexpected error: \(error)" status.status = .error // Consider logging the error stack trace in development #if DEBUG status.context = String(describing: error) #endif }src/swift-server/importer/importer.service.swift (2)
Line range hint
36-75: Consider using a more robust date handling approachThe current implementation uses RFC1123 date format parsing which can be brittle. Consider using a more standardized approach for cursor date handling.
Consider using ISO8601 format instead:
- if let dateString = cursorData["created"], - let date = Date(rfc1123: dateString) + if let dateString = cursorData["created"], + let date = ISO8601DateFormatter().date(from: dateString)
Line range hint
76-87: Consider wrapping delete operations in a transactionThe cascading delete operations should be wrapped in a transaction to ensure data consistency in case of failures.
Consider wrapping the delete operations in a transaction:
func delete(groupIds: [UUID], importId: UUID) async throws { let exists = try await FileImportReport.query(on: db).filter(\.$id == importId) .filter(\.$groupOwnerId ~~ groupIds).count() if exists > 0 { + try await db.transaction { transaction in try await FileImportRow.query(on: db) .filter(\.$report.$id == importId) .delete() try await FileImportReport.query(on: db) .filter(\.$id == importId) .delete() + } } }src/swift-server/importer/importer-api-impl.swift (2)
Line range hint
81-104: Enhance file upload security and validationThe file upload implementation could benefit from additional security measures:
- File size validation is missing
- File extension handling is basic
- Temporary file path could be made more secure
Consider adding these improvements:
+ // Add file size validation + guard upload.file.data.readableBytes < 50_000_000 else { // 50MB limit + throw Abort(.payloadTooLarge) + } + + // Validate file extension + let allowedExtensions = ["csv", "xlsx", "xls"] + guard let ext = upload.file.extension, + allowedExtensions.contains(ext.lowercased()) else { + throw Abort(.badRequest, reason: "Unsupported file type") + } + let tmpDir = NSTemporaryDirectory() - let filePath = "\(tmpDir)/mr-scrooge-\(UUID().uuidString).\(upload.file.extension ?? "unknown")" + let sanitizedFilename = UUID().uuidString + let filePath = (tmpDir as NSString).appendingPathComponent(sanitizedFilename)
Line range hint
109-146: Add validation for required fields in FileImport schemaThe schema initialization should validate required fields before accessing them.
Consider adding validation:
init(file: FileImportReport) { + guard let fileId = file.id else { + throw Abort(.internalServerError, reason: "File import missing ID") + } - id = file.id!.uuidString + id = fileId.uuidStringsrc/typespec/labels.tsp (1)
39-41: Ensure consistent error handling in theupdatemethodThe
updatemethod currently returnsLabel | NotFound | InvalidUUID. Since updates might also involve invalidgroupOwnerId, consider addingInvalidGroupOwnerIdto the return types for consistency with thecreatemethod, if applicable.Consider modifying the return type as follows:
- ): Label | NotFound | InvalidUUID; + ): Label | NotFound | InvalidGroupOwnerId | InvalidUUID;src/swift-server-macrosMacros/app-dependency.swift (1)
25-26: Fix the typo in the error messageThere is a misspelling in the error message "Missing anotation". It should be "Missing annotation".
Apply this diff to correct the typo:
- node: node.root, message: "Missing anotation" as! DiagnosticMessage) + node: node.root, message: "Missing annotation" as! DiagnosticMessage)src/swift-server-macrosMacros/basic-errors.swift (2)
17-18: Provide more descriptive error messages for invalid parametersThe error "First parameter should be an string" could be more descriptive. Consider specifying the expected type and the usage context to aid in debugging.
For example:
throw CustomError.message("The first parameter must be a string literal representing the error message.")
24-25: Clarify the error message for invalid code expressionsThe message "Invalid code" is generic. Providing more context about what constitutes a valid code expression can help users understand and fix the issue.
Consider revising the error message:
throw CustomError.message("Expected a member access expression (e.g., ErrorCode.someValue) as the second parameter.")src/swift-server/core/cache.swift (1)
62-68: Conform toEquatablefor cleaner equality checksRather than overriding
isEqual(_:), consider conformingWrappedKeytoEquatablefor better integration with Swift's equality protocols.Modify the class declaration and implement
==:fileprivate final class WrappedKey: NSObject, Equatable { static func == (lhs: WrappedKey, rhs: WrappedKey) -> Bool { return lhs.key == rhs.key } }Update
isEqual(_:)accordingly:override func isEqual(_ object: Any?) -> Bool { guard let otherKey = object as? WrappedKey else { return false } return self == otherKey }src/swift-server/label/label-api-impl.swift (2)
24-25: Improve error message grammarThe error message "GroupOwner ID is not an UUID" has a grammatical error. It should be "GroupOwner ID is not a valid UUID" for clarity.
Apply this diff to correct the message:
.init( - message: "GroupOwner ID is not an UUID", + message: "GroupOwner ID is not a valid UUID", code: ApiError.API10025.rawValue))))
64-74: Provide meaningful implementations or detailed responses for unimplemented endpointsThe
ApiLabels_updateandApiLabels_deletemethods currently return a 501 status code with an undocumented payload. This may not provide sufficient information to API consumers.Consider implementing these methods or returning a structured error response:
func ApiLabels_update(_ input: Operations.ApiLabels_update.Input) async throws -> Operations.ApiLabels_update.Output { - return .undocumented(statusCode: 501, UndocumentedPayload()) + return .notImplemented( + .init( + body: .json( + .init( + message: "The update label functionality is not yet implemented", + code: ApiError.API501.rawValue)))) } func ApiLabels_delete(_ input: Operations.ApiLabels_delete.Input) async throws -> Operations.ApiLabels_delete.Output { - return .undocumented(statusCode: 501, UndocumentedPayload()) + return .notImplemented( + .init( + body: .json( + .init( + message: "The delete label functionality is not yet implemented", + code: ApiError.API501.rawValue)))) }Ensure that
ApiError.API501is properly defined in your error handling.src/swift-server/core/validations.swift (2)
19-21: Avoid using cryptic enum casesThe enum case
.notUuidmay not clearly convey its purpose. Consider renaming it to.invalidUUIDfor better readability and understanding.Apply this diff to rename the enum case:
enum GroupOwnerIdState { - case notUuid + case invalidUUID case notOwned(validGroups: [UUID]) case ok(id: UUID) }And update its usages accordingly:
guard let groupOwnerId = UUID(uuidString: obj.groupOwnerId) else { - return .notUuid + return .invalidUUID }
48-49: Consolidate enum cases for clarityIn
ParentRuleState, both.notUuidand.notFoundrepresent error states. Consider providing more descriptive error information or consolidating these cases if appropriate.src/typespec/rules.tsp (2)
55-59: Consider adding a discriminator to theConditionunion for better OpenAPI generationWhen using
@oneOfunions, adding a discriminator helps code generators and API consumers to distinguish between types.Define a discriminator field:
@oneOf @discriminator("conditionType") union Condition { double: ConditionDouble, string: ConditionString, }Ensure that each variant includes the
conditionTypeproperty with a unique value.
104-115: Remove duplicateNotFoundresponse inapplymethodThe
applymethod inApiRuleinterface includesNotFoundresponse twice. This is redundant.Apply this diff to remove the duplicate:
@post @route("{ruleId}/apply") apply(@path ruleId: UUID): | NotFound | InvalidGroupOwnerId | InvalidUUID - | NotFound | Date;src/swift-server/rules/rule-engine.swift (2)
25-29: Simplify condition evaluation logicIn
checkConditionals, the logic for.notAndcan be simplified for better readability.Apply this diff to simplify the condition:
case .or: return conditions.contains { $0(transaction) } case .notAnd: - return conditions.map { $0(transaction) }.filter { $0 }.isEmpty + return !conditions.allSatisfy { $0(transaction) }
49-52: Handle missing parent rules gracefullyWhen a parent rule is not found, the engine throws an exception. Consider handling this scenario more gracefully, possibly by logging an error and skipping the rule.
Modify the code to handle missing parents without throwing, if appropriate.
src/swift-server/rules/rules.service.swift (1)
208-210: Provide detailed feedback on group owner ID mismatchIn the
addLabelmethod, when the group owner IDs do not match, returninginvalidOwnerIdmight not give enough context to the client.Consider returning an error with a message explaining the mismatch.
src/swift-server/rules/rule-api-impl.swift (2)
37-78: Consider refactoring repetitive validation logic into helper functionsThe validation switch cases for
groupOwner(lines 37-57) andparentRule(lines 59-78) share similar structures. Refactoring these into helper functions can reduce code duplication and enhance maintainability.
101-108: Correct grammatical error: Change "an UUID" to "a UUID" in error messagesThe error messages currently use "an UUID," which is grammatically incorrect. It should be "a UUID" because "UUID" starts with a consonant sound.
Apply the following changes:
- message: "Rule ID should be an UUID", + message: "Rule ID should be a UUID",(This change applies to all instances where "an UUID" appears in the error messages.)
Also applies to: 230-236, 263-270, 271-278, 306-309, 311-314, 338-340, 341-344, 372-374, 375-378
src/swift-server-tests/rules.tests.swift (1)
503-506: UseXCTFail()instead ofXCTAssertTrue(false)to indicate test failuresIn your test cases, replace
XCTAssertTrue(false)withXCTFail()to clearly indicate a test failure. This provides better clarity and more informative test results.Apply this change:
-case .ConditionDouble: - XCTAssertTrue(false) -case .none: - XCTAssertTrue(false) +case .ConditionDouble: + XCTFail("Unexpected ConditionDouble case") +case .none: + XCTFail("Condition is nil")(This applies to the relevant cases in your switch statements.)
Also applies to: 562-565, 621-624
src/swift-server-macrosMacros/entrypoint.swift (1)
9-9: Renameswift_macrosPlugintoSwiftMacrosPluginto follow Swift naming conventionsSwift naming conventions recommend using UpperCamelCase for type names. Consider renaming the struct to
SwiftMacrosPluginfor consistency.src/swift-server/core/cursor-handler.swift (1)
Line range hint
33-36: Prevent potential crash when data is emptyThe force unwrap
data.last!could crash ifdatais empty andhasMoreis true. Also consider validating thatlimitis greater than 0.Consider this safer implementation:
func getListWithCursor<T>(data: [T], generateCursor: (T) -> String) -> ListWithCursor<T> { + guard limit > 0 else { + return ListWithCursor(list: [], next: nil) + } let hasMore = data.count >= limit - return ListWithCursor(list: data, next: hasMore ? generateCursor(data.last!) : nil) + return ListWithCursor(list: data, next: hasMore ? data.last.map(generateCursor) : nil) }.coderabbit.yaml (2)
28-78: Optimize tool configuration for Swift projectThe configuration enables many linting tools that aren't relevant for a Swift project. Consider disabling unused tools to optimize the review process and reduce noise.
swiftlint: enabled: true - phpstan: - enabled: true - golangci-lint: - enabled: true - rubocop: - enabled: true
82-92: Consider adding environment variable configurationSince the project uses environment variables (e.g., queue worker count in
configure.swift), consider adding environment variable configuration to the knowledge base scope.knowledge_base: opt_out: false learnings: scope: auto + environment: + scope: auto + variables: + - QUEUE_WORKERS issues: scope: autosrc/swift-server/configure.swift (2)
75-75: Remove or implement commented codeThe commented line for scheduled jobs should either be implemented or removed if not needed.
- //try app.queues.startScheduledJobs() + // TODO: Implement scheduled jobs if needed + // try app.queues.startScheduledJobs()
Line range hint
76-80: Enhance error handling with loggingThe catch block could benefit from structured logging to aid debugging in production.
} catch { - print(error) + app.logger.error("Failed to configure application: \(error)") + if app.environment == .development { + print("Development details: \(error)") + } throw error }src/swift-server/core/error.swift (2)
50-57: Enhance error messages for rule-related errorsThe error messages for rule-related errors could be more specific and actionable:
.E10014: ErrorInfo( - message: "Retrieving an String from a condition when it doesn't have it"), + message: "Invalid condition: Expected string value but none found"), .E10015: ErrorInfo( - message: "Retrieving the Double from a condition when it doesn't have it"), + message: "Invalid condition: Expected numeric value but none found"), .E10016: ErrorInfo( message: "Rule parent cannot be found by the ID", - additionalInfo: "this can be because the parent rule is with another groupOwnerId" + additionalInfo: "Verify that the parent rule exists and belongs to the same group (groupOwnerId mismatch)" ),
8-19: Add documentation for API error codesConsider adding documentation comments to describe the scenarios where each API error code is used, especially for the newly added codes (API10020-API10050).
+ /// API Error codes for the rules engine + /// - API10020-API10029: Rule validation errors + /// - API10030-API10039: Rule execution errors + /// - API10040-API10049: Rule configuration errors + /// - API10050: General rule engine error enum ApiError: String, StringEnumType {src/swift-server/core/demo-data.command.swift (1)
58-77: Consider enhancing error handling and using constantsThe implementation could benefit from the following improvements:
- Add error handling for label creation failures
- Consider using constants for label names and condition values
- Consider capturing and logging the created rule IDs for verification
Here's a suggested improvement:
+ private enum DemoLabels { + static let income = "income" + static let expenses = "expenses" + static let aral = "aral" + } + private enum DemoConditions { + static let zeroValue = 0.0 + static let aralKeyword = "aral" + } private func addRulesAndLabels(app: Application, groupOwnerId: UUID) async throws { - let income = Label(groupOwnerId: groupOwnerId, name: "income") + let income = Label(groupOwnerId: groupOwnerId, name: DemoLabels.income) try await income.save(on: app.db) - let expenses = Label(groupOwnerId: groupOwnerId, name: "expenses") + let expenses = Label(groupOwnerId: groupOwnerId, name: DemoLabels.expenses) try await expenses.save(on: app.db) - let aral = Label(groupOwnerId: groupOwnerId, name: "aral") + let aral = Label(groupOwnerId: groupOwnerId, name: DemoLabels.aral) try await aral.save(on: app.db) + // Create rules and store IDs for verification + let rules = try await [ - let _ = try await Rule.createRule( + Rule.createRule( - on: app.db, for: groupOwnerId, with: .init(.less, valueDouble: 0), + on: app.db, for: groupOwnerId, with: .init(.less, valueDouble: DemoConditions.zeroValue), toApply: expenses), - let _ = try await Rule.createRule( + Rule.createRule( - on: app.db, for: groupOwnerId, with: .init(.greater, valueDouble: 0), + on: app.db, for: groupOwnerId, with: .init(.greater, valueDouble: DemoConditions.zeroValue), toApply: income), - let _ = try await Rule.createRule( + Rule.createRule( - on: app.db, for: groupOwnerId, with: .init(.contains, valueStr: "aral"), + on: app.db, for: groupOwnerId, with: .init(.contains, valueStr: DemoConditions.aralKeyword), toApply: aral) + ] + + app.logger.info("Created demo rules with IDs: \(rules.map { $0.id?.uuidString ?? "nil" })") }src/@typespec/openapi3/openapi.yaml (1)
786-791: Standardize error response descriptions.The description "Bad request, usually when providing an invalid string as UUID" should be more generic to cover all validation errors.
description: >- - Bad request, usually when providing an invalid string as UUID + Bad Request - The request was invalid or cannot be served. The exact error is explained in the error message.Also applies to: 824-829, 870-875, 915-920
src/swift-server/bank-transaction/bank-transaction.service.swift (2)
4-4: LGTM! Good architectural improvement.The inheritance from
ServiceWithQueueAndDbenhances the service layer by providing standardized database and queue capabilities.
Line range hint
28-63: Remove commented out code.The commented line
.with(\.$labels)should be removed if it's no longer needed. Keeping commented-out code can lead to confusion and maintenance issues.- // .with(\.$labels)src/swift-server/graph/graph.service.swift (1)
161-161: LGTM! Consistent architectural improvement.The inheritance from
ServiceWithDbaligns with the service layer standardization across the application.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (57)
.coderabbit.yaml(1 hunks)Package.resolved(8 hunks)Package.swift(4 hunks)src/@typespec/openapi3/openapi.yaml(11 hunks)src/cli/cli.module.ts(0 hunks)src/cli/config/cli-config.module.ts(0 hunks)src/cli/config/cli-config.ts(0 hunks)src/cli/demo-data.service.ts(0 hunks)src/cli/demo.command.ts(0 hunks)src/cli/logs.command.ts(0 hunks)src/cli/migrations.command.ts(0 hunks)src/swift-server-macros/entrypoint.swift(1 hunks)src/swift-server-macrosMacros/app-dependency.swift(1 hunks)src/swift-server-macrosMacros/basic-errors.swift(1 hunks)src/swift-server-macrosMacros/entrypoint.swift(1 hunks)src/swift-server-tests/app-test.swift(5 hunks)src/swift-server-tests/importers/importer-caixa-enginyers.tests.swift(2 hunks)src/swift-server-tests/importers/importer-commerz-bank.swift(1 hunks)src/swift-server-tests/importers/importer-helper.swift(3 hunks)src/swift-server-tests/importers/importer-n26.tests.swift(2 hunks)src/swift-server-tests/importers/importer-service.tests.swift(8 hunks)src/swift-server-tests/rules.tests.swift(1 hunks)src/swift-server/bank-transaction/bank-transaction-api-impl.swift(9 hunks)src/swift-server/bank-transaction/bank-transaction.service.swift(8 hunks)src/swift-server/configure.swift(3 hunks)src/swift-server/core/cache.swift(1 hunks)src/swift-server/core/cursor-handler.swift(1 hunks)src/swift-server/core/demo-data.command.swift(1 hunks)src/swift-server/core/error.swift(2 hunks)src/swift-server/core/services.swift(1 hunks)src/swift-server/core/validations.swift(1 hunks)src/swift-server/graph/graph-api-impl.swift(4 hunks)src/swift-server/graph/graph.service.swift(6 hunks)src/swift-server/importer/importer-api-impl.swift(5 hunks)src/swift-server/importer/importer.service.swift(2 hunks)src/swift-server/importer/importers/new-import.service.swift(5 hunks)src/swift-server/label/label-api-impl.swift(3 hunks)src/swift-server/label/label.service.swift(2 hunks)src/swift-server/migrations/initial-migration.swift(2 hunks)src/swift-server/routes.swift(1 hunks)src/swift-server/rules/new-transaction.job.swift(1 hunks)src/swift-server/rules/rule-api-impl.swift(1 hunks)src/swift-server/rules/rule-engine.swift(1 hunks)src/swift-server/rules/rules.models.swift(1 hunks)src/swift-server/rules/rules.service.swift(1 hunks)src/swift-server/session/session-api-impl.swift(1 hunks)src/swift-server/user/user-api-impl.swift(3 hunks)src/swift-server/user/user.service.swift(3 hunks)src/typespec/bank-transactions.tsp(2 hunks)src/typespec/global.tsp(2 hunks)src/typespec/graphs.tsp(1 hunks)src/typespec/imports.tsp(2 hunks)src/typespec/labels.tsp(2 hunks)src/typespec/profile.tsp(2 hunks)src/typespec/rules.tsp(1 hunks)src/typespec/session.tsp(2 hunks)todo.md(3 hunks)
💤 Files with no reviewable changes (7)
- src/cli/config/cli-config.ts
- src/cli/config/cli-config.module.ts
- src/cli/cli.module.ts
- src/cli/demo.command.ts
- src/cli/demo-data.service.ts
- src/cli/migrations.command.ts
- src/cli/logs.command.ts
✅ Files skipped from review due to trivial changes (1)
- src/typespec/imports.tsp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/swift-server/routes.swift
- src/swift-server-tests/importers/importer-helper.swift
- src/swift-server-tests/importers/importer-n26.tests.swift
- src/swift-server-tests/importers/importer-commerz-bank.swift
- src/swift-server-tests/importers/importer-service.tests.swift
- src/swift-server-tests/importers/importer-caixa-enginyers.tests.swift
- src/swift-server/user/user-api-impl.swift
- src/swift-server/rules/new-transaction.job.swift
- src/swift-server/user/user.service.swift
- src/swift-server/rules/rules.models.swift
- src/swift-server/migrations/initial-migration.swift
- Package.resolved
🧰 Additional context used
📓 Learnings (1)
src/typespec/rules.tsp (1)
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/typespec/rules.tsp:49-52
Timestamp: 2024-11-12T09:47:03.059Z
Learning: In `src/typespec/rules.tsp`, the `ConditionString` model does not have a `field` property.
🔇 Additional comments (48)
src/swift-server/label/label.service.swift (2)
4-5: LGTM! Service refactoring aligns with the codebase architecture.
The conversion from struct to class with ServiceWithDb inheritance follows the established pattern for service classes, improving consistency across the codebase.
Line range hint 25-41: LGTM! Well-implemented cursor-based pagination.
The implementation includes:
- Proper cursor parsing and handling
- Efficient sorting and limiting
- Relationship eager loading
- Clean cursor generation for the next page
src/swift-server/bank-transaction/bank-transaction-api-impl.swift (3)
10-10: Service access pattern improvement looks good.
The transition to application-scoped service access is a good architectural improvement for dependency injection and service lifetime management.
Also applies to: 19-19, 24-24
68-68: Service access changes are consistent.
The changes maintain consistency with the new service access pattern across the codebase.
Also applies to: 77-77, 82-82
153-153: Service access changes maintain consistency.
The changes align with the new service access pattern established across the codebase.
Also applies to: 174-174, 188-188
src/swift-server-tests/app-test.swift (3)
3-3: LGTM: Import statement properly added
The XCTQueues import is appropriately placed with other test-related imports and is necessary for the queue testing functionality being introduced.
72-72: LGTM: Test group setup is consistent
The creation and attachment of testGroup3 follows the established pattern and is properly integrated into the test setup.
Also applies to: 81-81
64-68: Verify the status of the commented NewTransactionJob
The queue configuration is correct for testing, but there's a commented line that might need attention:
//app.queues.add(NewTransactionJob())✅ Verification successful
The commented NewTransactionJob line in tests is intentional and correct
The commented line in the test file is appropriate because:
- NewTransactionJob is properly registered in the main application configuration (
configure.swift) - The test environment specifically overrides the queue driver to
.asyncTestfor testing purposes - Not registering the job in tests allows for better isolation and control of the test environment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NewTransactionJob is used elsewhere in the codebase
rg "NewTransactionJob" --type swift
Length of output: 368
Script:
#!/bin/bash
# Check the implementation and usage context of NewTransactionJob
ast-grep --pattern 'class NewTransactionJob $_' -A 20
Length of output: 55
Script:
#!/bin/bash
# Let's try to get the content of the job file and its usage
cat src/swift-server/rules/new-transaction.job.swift
echo "---"
cat src/swift-server/configure.swift
Length of output: 3064
src/swift-server/importer/importers/new-import.service.swift (3)
65-66: LGTM: Queue parameter addition aligns with architectural changes
The method signature change from Database to Queue parameter aligns well with the new queue-based architecture.
38-41: Verify error handling for bankTransactionService initialization
The service is now properly initialized using dependency injection via the Application context. However, ensure that app.bankTransactionService access is properly handled.
✅ Verification successful
Let me analyze the results and generate a final response.
Access to bankTransactionService appears safe and well-handled
The bankTransactionService is properly registered as a service dependency in core/services.swift and is consistently accessed through the Application context across the codebase. The service access pattern follows the established dependency injection pattern, and there's no evidence of unsafe access or missing error handling in the initialization path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how bankTransactionService is registered and if there's error handling
rg -A 5 "bankTransactionService" "src/swift-server"
Length of output: 9981
3-3: Verify consistent service architecture across the codebase
The change to inherit from ServiceWithQueueAndDb indicates an architectural shift towards queue-based processing. This is a good practice for handling potentially long-running import operations.
Also applies to: 34-34
✅ Verification successful
Let me gather more information to verify the architectural consistency.
Architectural change is consistent with the codebase
The addition of the Queues import and inheritance from ServiceWithQueueAndDb is verified to be consistent with the existing architecture. Both FileImportService and NewImportService in the importer module follow the same pattern, using queue-based processing and the standardized service base class. The remaining direct database usage in the codebase is primarily in models and utility functions, which is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other similar services follow the same pattern
ast-grep --pattern 'class $SERVICE_NAME: ServiceWithQueueAndDb {
$$$
}'
# Check for any remaining direct Database usage
rg "on\s+db:" "src/swift-server"
Length of output: 1454
Script:
#!/bin/bash
# Check for other service classes in the importer directory
ast-grep --pattern 'class $SERVICE_NAME {
$$$
}' src/swift-server/importer
# Check for Queue usage patterns
rg "Queue" --type swift src/swift-server/importer -A 2
Length of output: 1788
src/swift-server/importer/importer.service.swift (1)
2-2: LGTM: Improved service structure with proper dependency injection
The refactoring to inherit from ServiceWithQueueAndDb and the structured initialization with proper dependency injection is a good improvement. The parser initialization in the constructor ensures all dependencies are properly set up at creation time.
Also applies to: 5-5, 13-20
Package.swift (2)
27-29: Security Consideration: Using 'main' branch for 'vapor-queues-fluent-driver'
Using the main branch for dependencies can lead to unexpected behavior or security issues. Please consider pinning vapor-queues-fluent-driver to a specific version or commit hash to ensure stability.
36-36: Verify the version specification for swift-format
The version 600.0.0 for swift-format may not correspond to a stable release. Ensure that this version is correct and consider pinning to a specific stable release to maintain compatibility.
Run the following script to list available versions of swift-format:
src/swift-server/core/services.swift (4)
6-11: Proper initialization in ServiceWithDb
The ServiceWithDb class correctly initializes the database connection, providing a solid base for services that require database access.
13-20: Correct inheritance and initialization in ServiceWithQueueAndDb
The ServiceWithQueueAndDb class appropriately extends ServiceWithDb and initializes both the queue and database, ensuring that services inheriting from it have access to both dependencies.
22-49: Effective use of StorageKey protocol for services
By extending each service to conform to StorageKey, you facilitate dependency injection and service retrieval, enhancing modularity and testability.
50-71: Consistent application of @ServiceDependency
The Application extension uses @ServiceDependency consistently for all services, promoting clean and manageable access to services throughout the application.
src/typespec/rules.tsp (2)
49-52: [Duplicate Comment] Ensure consistency in condition models
The ConditionDouble model does not include a field property, similar to the ConditionString model.
23-26: Ensure correct merging of RuleParam into Rule
When extending RuleParam in Rule, confirm that properties are correctly merged and there are no conflicts.
No action needed here if the merging is intentional and tested.
src/swift-server/rules/rules.service.swift (1)
258-263: Check for side effects when detaching all pivots
In removeLabelActionPivot, detaching all pivots without condition may have unintended consequences. Ensure this is the desired behavior.
Consider adding checks or logging to confirm that only intended pivots are detached.
src/swift-server/rules/rule-api-impl.swift (1)
217-221: Verify if ApiRule_apply endpoint should be implemented
The ApiRule_apply function currently returns a 501 Not Implemented status. Confirm whether this endpoint is intended for future implementation or should be removed if not needed.
src/swift-server-tests/rules.tests.swift (2)
25-25: Replace force unwrapping with safe alternatives
Using force unwrapping (!) can lead to runtime crashes if the value is nil. Use try label.requireID() or optional binding to safely unwrap the id.
Also applies to: 133-135
138-140: Add descriptive messages and constants for magic numbers in assertions
Consider adding descriptive messages to your XCTAssertEqual statements to clarify the purpose of each test. Defining magic numbers as constants improves readability and maintainability.
Also applies to: 152-175
src/swift-server-macros/entrypoint.swift (1)
3-3: Verify module naming for external macros
The module name "swift_macrosMacros" appears in the external macro declarations. Ensure that this module name is correct and consistent with your project's naming conventions.
Also applies to: 9-9, 13-13
src/swift-server/core/cursor-handler.swift (1)
38-41: LGTM: Clean generic struct implementation
The ListWithCursor<T> structure is well-designed and follows Swift naming conventions.
src/typespec/session.tsp (2)
56-56: LGTM: Consistent error type usage
The change from InvalidGroupId to InvalidUUID aligns with the global error handling approach.
33-37: Verify password validation logic
Please ensure that the password validation logic in the implementation enforces:
- Old password verification when newPassword is provided
- Both password and newPassword are required together for password changes
✅ Verification successful
Password validation logic is properly implemented
The implementation in session-api-impl.swift correctly enforces the password validation requirements:
- When
newPasswordis provided, it verifies the old password usinguser.verifyPassword(pwd: password) - Both
passwordandnewPasswordare required together, with appropriate error responses:- Returns API10019 if
passwordis missing whennewPasswordis provided - Returns API10020 if the provided
passworddoesn't match the current password
- Returns API10019 if
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for password validation in the implementation
rg -A 5 "func.*updateMe.*UpdateMyProfile" src/swift-server/
Length of output: 61
Script:
#!/bin/bash
# Search for updateMe function implementation with broader pattern
rg -A 10 "updateMe.*\{" src/swift-server/
# Search for password validation patterns
rg -A 5 "password.*validation|validatePassword" src/swift-server/
# Search for files containing updateMe implementation
fd -e swift -x grep -l "updateMe"
Length of output: 195
Script:
#!/bin/bash
# Get the implementation of updateMe from the identified file
cat src/swift-server/session/session-api-impl.swift
Length of output: 3224
src/typespec/graphs.tsp (2)
71-74: LGTM: Improved response type clarity
The block structure for the status code and return type is more readable and follows TypeSpec best practices.
80-84: LGTM: Better union type formatting
The vertical layout of union types improves readability without changing functionality.
src/typespec/bank-transactions.tsp (2)
38-38: LGTM: Parameter formatting improvement
The spacing around the default value assignment improves readability while maintaining the same functionality.
58-61: LGTM: Method signature formatting improvement
The multi-line formatting of the comment method improves readability while maintaining the same functionality.
Let's verify the error handling after LabelGroupIdMismatch model removal:
src/typespec/profile.tsp (2)
16-19: LGTM: Model definitions formatting improvements
The improved indentation in model definitions enhances code readability.
Also applies to: 23-25, 29-30
47-50: LGTM: List method formatting improvement
The multi-line parameter formatting improves readability.
todo.md (2)
63-63: LGTM: Architectural improvement with dependency injection
The completion of moving services to dependency injection is a significant architectural improvement that will enhance testability and maintainability.
40-40: Verify rules engine implementation completeness
The todo list indicates that rule application is complete but CRUD operations are pending. This suggests a potential gap in the rules engine implementation.
Consider implementing the CRUD operations before marking the rules engine as complete to ensure full functionality.
Also applies to: 49-51
src/swift-server/configure.swift (1)
71-75: Consider enhancing queue configuration robustness.
Several improvements could make the queue configuration more production-ready:
- The worker count is hardcoded. Consider making it configurable via environment variables.
- Starting jobs immediately after configuration might need error handling or retry logic.
- The TODO comment suggests missing configuration - this should be addressed before production deployment.
src/swift-server/session/session-api-impl.swift (1)
65-68: LGTM! Service access pattern improved
The change to access userGroupService through request.application follows better dependency injection practices and aligns with the application's architecture.
src/swift-server/graph/graph-api-impl.swift (4)
32-32: LGTM! Consistent service access pattern
Service access moved to application context, improving dependency management.
60-63: LGTM! Pagination parameters properly handled
Service call properly handles pagination with limit and cursor while using the application context.
105-106: LGTM! Clear parameter passing
Update operation clearly passes all required parameters while using application context.
147-148: LGTM! Proper error handling maintained
Delete operation maintains proper error handling while using application context.
src/swift-server/core/demo-data.command.swift (1)
52-53: LGTM! Demo data generation enhanced
Added rules and labels generation to complete the demo data setup.
src/@typespec/openapi3/openapi.yaml (2)
1355-1397: LGTM! Well-structured condition schemas.
The condition schemas are well-designed with:
- Clear separation between string and double operations
- Proper required field definitions
- Good use of oneOf for polymorphic types
780-785: 🛠️ Refactor suggestion
Standardize response type for rule application.
The /rules/{ruleId}/apply endpoint returns a Date type while other similar operations return boolean. Consider standardizing this for consistency.
schema:
- $ref: '#/components/schemas/Date'
+ type: booleanLikely invalid or redundant comment.
src/swift-server/bank-transaction/bank-transaction.service.swift (1)
98-107: Previous review comment about error handling is still applicable.
The concerns about error handling and safe unwrapping of transaction.id remain valid.
src/swift-server/graph/graph.service.swift (2)
186-192: Good implementation of database parameter.
The optional database parameter with fallback to self.db is a clean way to support both transaction and non-transaction contexts.
530-533: LGTM! Well-structured transaction handling.
The implementation correctly:
- Uses transactions for atomic operations
- Maintains referential integrity in deletion order
- Handles errors appropriately
Dracks
force-pushed
the
3.0.0/rule-engine
branch
from
9a1b717 to
021672d
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (15)
src/swift-server-tests/session.tests.swift (1)
Line range hint
103-123: LGTM! Well-structured test case.The test follows good testing practices with clear arrange-act-assert pattern and proper validation of error responses.
Consider these improvements:
- Add a comment explaining what error code "API10017" represents
- Add more test cases with different invalid UUID formats:
func testUpdateMeInvalidUUID() async throws { + // Test cases for various invalid UUID formats + let invalidUUIDs = [ + "1111111", // Too short + "not-a-uuid", // Invalid characters + "123e4567-e89b-12d3-a456-42665544", // Incomplete UUID + ] + let app = try getApp() let headers = try await app.getHeaders( forUser: .init(username: testUser.username, password: "test-password")) + // API10017: Invalid UUID format error + for invalidUUID in invalidUUIDs { let newUserInfo = Components.Schemas.UpdateMyProfile( username: testUser.username, email: "[email protected]", firstName: "First name 2", lastName: "Last Name 2", - defaultGroupId: "1111111" + defaultGroupId: invalidUUID ) let response = try await app.sendRequest( .PUT, "/api/session/me", body: newUserInfo, headers: headers) XCTAssertEqual(response.status, .badRequest) let error = try response.content.decode(Components.Schemas._Error.self) XCTAssertEqual(error.code, "API10017") + } }src/swift-server-tests/graphs.tests.swift (1)
Line range hint
379-379: Remove outdated TODO comment.The comment "Todo add a test to try to update a not existing graph" can be removed since the test case is already implemented in the
testUpdateNonExistingGraphmethod below..github/workflows/test-swift.yml (1)
Line range hint
19-21: Consider adding test coverage reportingWhile parallel testing is good for performance, adding test coverage reporting would provide better visibility into test quality.
Consider adding a step for test coverage:
- name: Run tests with coverage run: | swift test --enable-code-coverage --parallel xcrun llvm-cov export -format="lcov" .build/debug/mr-scroogePackageTests.xctest/Contents/MacOS/mr-scroogePackageTests > coverage.lcov - name: Upload coverage uses: codecov/codecov-action@v3 with: files: ./coverage.lcovsrc/swift-server/graph/graph-api-impl.swift (2)
32-32: LGTM: Improved service access patternAccessing the service through
request.applicationcontext is a better architectural approach as it:
- Makes dependencies explicit and manageable
- Facilitates testing through dependency injection
- Ensures consistent service instance usage across requests
60-63: Consider adding input validation for pagination parametersWhile the service access change is good, consider adding validation for the pagination limit to ensure it doesn't exceed reasonable bounds. This can prevent potential performance issues with very large limit values.
+ let validLimit = min(input.query.limit ?? 100, 1000) // Add reasonable upper bound let data = try await request.application.graphService.getGraphs( pageQuery: .init( - limit: input.query.limit ?? 100, + limit: validLimit, cursor: input.query.cursor), groupsId: validGroupsId, graphsIds: graphIds).coderabbit.yaml (4)
27-28: Fix indentation and trailing spaces.There are formatting issues in the base branches configuration:
- Line 27 has trailing spaces
- Line 28 uses 8 spaces for indentation instead of the expected 6 spaces
Apply this diff to fix the formatting:
- base_branches: - - "mr-scrooge-3.0.0" + base_branches: + - "mr-scrooge-3.0.0"🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
Line range hint
531-538: Improve error response description.The error description for status code 403 is too specific to group ownership. Consider making it more generic as this status code could be used for other authorization scenarios.
Apply this diff:
- description: Forbidden, usually when using a group that the user doesn't have access to + description: Forbidden - The user does not have the necessary permissions🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
Line range hint
770-806: Inconsistent error response schema for 404.The error response for status code 404 uses the same schema reference twice in the
anyOfarray, which is redundant.Apply this diff:
- anyOf: - - $ref: '#/components/schemas/Error' - - $ref: '#/components/schemas/Error' + $ref: '#/components/schemas/Error'🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
Line range hint
1787-1806: Consider adding validation constraints for Rule schema.The Rule schema could benefit from additional constraints:
- Minimum length for
name- Non-empty array validation for
conditionsandlabelIdsApply this diff:
required: - id - groupOwnerId - name - relations - conditions - labelIds properties: id: $ref: '#/components/schemas/UUID' parentRuleId: $ref: '#/components/schemas/UUID' groupOwnerId: $ref: '#/components/schemas/UUID' name: type: string + minLength: 1 relations: $ref: '#/components/schemas/ConditionalRelation' conditions: type: array items: $ref: '#/components/schemas/Condition' + minItems: 1 labelIds: type: array items: $ref: '#/components/schemas/UUID' + minItems: 1🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
src/swift-server/label/label-api-impl.swift (1)
64-68: Add documentation for unimplemented endpointWhile returning 501 is appropriate for unimplemented endpoints, consider:
- Adding a TODO comment explaining the planned implementation
- Documenting when this endpoint will be implemented
Would you like help implementing this endpoint or creating a GitHub issue to track this task?
src/swift-server/core/validations.swift (1)
105-107: Use Logging Framework Instead of Print StatementsThe
Apply this diff to replace
- print( + logger.error( "Rule \(try parentRule.requireID()) is corrupted as it points to a parent that is not found for the user group \(userGroup)." )Ensure that
loggeris properly initialized and configured.src/@typespec/openapi3/openapi.yaml (1)
809-849: Add request body validation for condition creation.The condition creation endpoint should validate the request body structure. Consider adding:
- Required fields validation
- Value type validation based on the condition type
Add these validations to the request body schema:
requestBody: required: true content: application/json: schema: type: object + required: + - condition properties: condition: $ref: '#/components/schemas/ConditionParams' - required: - - conditionsrc/swift-server/rules/rule-api-impl.swift (3)
92-93: Refactor repeated user and group ID retrieval into a helper functionThe retrieval of the
userandvalidGroupsIdsis repeated across multiple API methods. Refactoring these lines into a shared helper function can enhance code maintainability and reduce duplication.Apply this diff to refactor the code:
- let user = try await getUser(fromRequest: request) - let validGroupsIds = try user.groups.map { return try $0.requireID() } + let (user, validGroupsIds) = try await getUserAndGroupIDs()Define the helper function outside of the API methods:
func getUserAndGroupIDs() async throws -> (User, [UUID]) { let user = try await getUser(fromRequest: request) let validGroupsIds = try user.groups.map { try $0.requireID() } return (user, validGroupsIds) }Also applies to: 185-186, 226-227, 260-261, 303-304, 334-335, 368-369
188-191: Create a utility function for UUID parsing and error handlingParsing UUIDs from path parameters and handling invalid UUID errors are performed in multiple methods. Consider creating a utility function to parse UUIDs and handle errors consistently, reducing code duplication.
Apply this diff to refactor the code:
- guard let ruleId = UUID(uuidString: input.path.ruleId) else { - return #BasicBadRequest( - msg: "Rule ID should be an UUID", code: ApiError.API10033) - } + let ruleId = try parseUUID(input.path.ruleId, fieldName: "Rule ID", errorCode: ApiError.API10033)Define the utility function:
func parseUUID(_ uuidString: String, fieldName: String, errorCode: ApiError) throws -> UUID { guard let uuid = UUID(uuidString: uuidString) else { throw #BasicBadRequest(msg: "\(fieldName) should be a UUID", code: errorCode) } return uuid }Also applies to: 230-236, 263-270, 306-309, 337-340, 371-374
217-221: Implement theApiRule_applymethod or remove the placeholderThe
ApiRule_applymethod currently returns a 501 Not Implemented status, indicating that it's a placeholder. If this endpoint is intended to be implemented, consider adding the necessary logic. Otherwise, if it's not required at this time, you might remove it to avoid exposing unimplemented endpoints.Would you like assistance in implementing this method? I can help draft the implementation or open a GitHub issue to track this task.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
.coderabbit.yaml(1 hunks).github/workflows/test-swift.yml(1 hunks)Package.swift(4 hunks)src/@typespec/openapi3/openapi.yaml(14 hunks)src/swift-server-macrosMacros/app-dependency.swift(1 hunks)src/swift-server-tests/graphs.tests.swift(1 hunks)src/swift-server-tests/profile.tests.swift(2 hunks)src/swift-server-tests/session.tests.swift(1 hunks)src/swift-server/core/validations.swift(1 hunks)src/swift-server/graph/graph-api-impl.swift(6 hunks)src/swift-server/label/label-api-impl.swift(3 hunks)src/swift-server/rules/rule-api-impl.swift(1 hunks)src/typespec/global.tsp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/typespec/global.tsp
- src/swift-server-tests/profile.tests.swift
- src/swift-server-macrosMacros/app-dependency.swift
- Package.swift
🧰 Additional context used
🪛 yamllint (1.35.1)
.coderabbit.yaml
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
🔇 Additional comments (13)
src/swift-server-tests/graphs.tests.swift (1)
Line range hint 192-196: LGTM! Appropriate status code change for permission-based rejection.
The change from .unauthorized to .forbidden is correct. HTTP 403 (Forbidden) is the appropriate status code when an authenticated user attempts to perform an action they don't have permission for, such as creating a graph for another owner's group. The error code "API10004" is also properly verified.
src/swift-server/graph/graph-api-impl.swift (2)
Line range hint 20-31: LGTM: Correct HTTP status code for authorization failure
The change from .unauthorized (401) to .forbidden (403) is semantically correct. The 403 status code better represents the scenario where an authenticated user attempts to access a resource they don't have permission for.
105-106: LGTM: Consistent service access with proper authorization
The service method calls properly include the user parameter, ensuring authorization checks at the service level. This implements defense in depth by validating permissions at both the API and service layers.
Also applies to: 147-148
.coderabbit.yaml (2)
29-79: Comprehensive tool configuration looks good.
The configuration includes a wide range of code quality tools that will help maintain high code standards:
- Security: gitleaks, checkov
- Linting: shellcheck, ruff, eslint, swiftlint, etc.
- Code quality: markdownlint, hadolint, phpstan, etc.
Line range hint 1355-1397: Well-structured condition schemas.
The condition schemas are well-designed with:
- Clear separation between string and double operations
- Proper type discrimination using
oneOf - Reusable parameter schemas
🧰 Tools
🪛 yamllint (1.35.1)
[error] 27-27: trailing spaces
(trailing-spaces)
[warning] 28-28: wrong indentation: expected 6 but found 8
(indentation)
src/swift-server/label/label-api-impl.swift (4)
9-40: LGTM! Good improvements to validation and error handling
The changes demonstrate several improvements:
- Proper encapsulation of validation logic using TransformerAndValidator
- Clear error messages with specific error codes
- Correct use of HTTP status codes
- Better service access pattern using dependency injection
51-51: LGTM! Consistent use of dependency injection
The change to use request.application.labelService aligns with the pattern used in other methods, improving consistency and testability.
70-74: Add documentation for unimplemented endpoint
Similar to the update endpoint, consider adding documentation about the implementation timeline and requirements.
77-77: Verify InputWithUserGroup protocol requirements
Let's ensure CreateLabel properly implements all requirements of the InputWithUserGroup protocol.
✅ Verification successful
Based on the search results from previous scripts, I can generate the final response:
Protocol conformance verified successfully
The InputWithUserGroup protocol requires only one property groupOwnerId: String { get }, and the CreateLabel type satisfies this requirement as evidenced by:
- The protocol definition in
src/swift-server/core/validations.swift - Usage in tests showing
CreateLabelbeing instantiated with agroupOwnerIdparameter - Similar conformance pattern used by other types like
RuleParam
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for InputWithUserGroup protocol definition and its requirements
ast-grep --pattern 'protocol InputWithUserGroup {
$$$
}'
# Search for CreateLabel structure to verify it has all required properties
ast-grep --pattern 'struct CreateLabel {
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Let's try a broader search for InputWithUserGroup
rg "InputWithUserGroup" -A 5 -B 5
# Search for CreateLabel in Components.Schemas namespace
rg "Components\.Schemas\.CreateLabel" -A 5 -B 5
Length of output: 5184
Script:
#!/bin/bash
# Let's find the CreateLabel struct definition to verify it has groupOwnerId
rg "struct CreateLabel" -A 10 -B 2
# Also search for any other references to CreateLabel properties
rg "CreateLabel\." -A 2 -B 2
Length of output: 67
src/swift-server/core/validations.swift (1)
84-110: Confirmed Fix for Potential Infinite Loop in Parent Rule Traversal
The updated code includes a visitedRuleIds set within the while true loop to prevent infinite loops due to cyclical parent relationships. This effectively addresses the previously identified issue.
src/@typespec/openapi3/openapi.yaml (3)
551-615: LGTM! Label management endpoints are well-defined.
The new PUT and DELETE operations for labels follow RESTful conventions and include appropriate error handling.
Line range hint 1355-1397: LGTM! Well-structured schema definitions for rules and conditions.
The polymorphic condition types and operations are well-defined using OpenAPI's oneOf discriminator. The separation between Double and String operations is clean and appropriate.
Also applies to: 1765-1775
770-786: 🛠️ Refactor suggestion
Standardize success response for rule application.
The /rules/{ruleId}/apply endpoint returns a Date type while similar action endpoints return boolean. Consider standardizing this for consistency.
- $ref: '#/components/schemas/Date'
+ type: booleanLikely invalid or redundant comment.
| '403': | ||
| description: Forbidden, usually when using a group that the user doesn't have access to |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error response descriptions.
Several endpoints have inconsistent descriptions for 400 error responses. Consider standardizing them across all endpoints:
Apply this change to all 400 responses:
- description: Bad request, usually when providing an invalid string as UUID
+ description: Bad Request - Invalid input parameters or validation errorsThis provides a more generic and consistent description that covers all possible validation errors, not just UUID formatting issues.
Also applies to: 251-252, 531-538, 567-572, 602-607, 661-666, 703-708, 786-791, 824-829, 870-875, 915-920, 950-955, 990-995
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/swift-server/migrations/initial-migration.swift (2)
78-89: Consider adding a cycle prevention constraint for hierarchical rules.The
parent_idrelationship allows for hierarchical rules, but there's no built-in protection against circular references. Consider adding a trigger or application-level validation to prevent cycles in the rule hierarchy.Would you like help implementing a cycle prevention solution?
Line range hint
185-186: Document the datetime handling decision.The comment references an external Discord discussion about datetime handling. Consider documenting the decision and implementation details in the codebase for future reference.
- // when problems look at the answer of Gwynne - // https://discord.com/channels/431917998102675485/684159753189982218/1282405735455522950 + // DateTime handling: Using required datetime field as discussed in + // https://discord.com/channels/431917998102675485/684159753189982218/1282405735455522950 + // Key points: + // 1. [Add the key points from the discussion] + // 2. [Add any implementation details]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/swift-server/migrations/initial-migration.swift(6 hunks)
🔇 Additional comments (6)
src/swift-server/migrations/initial-migration.swift (6)
51-54: LGTM! Index addition will improve query performance.
The addition of an index on group_owner_id for the core_bank_transaction table is a good performance optimization for queries filtering by group ownership.
136-139: LGTM! Consistent indexing strategy.
The addition of an index on group_owner_id for the graph_graph table maintains consistency with the indexing strategy used across other tables.
193-196: LGTM! Performance optimization with appropriate indexes.
The addition of indexes on group_owner_id and report_id will improve query performance for import-related operations.
Also applies to: 214-217
229-234: LGTM! Proper deletion order.
The revert method correctly orders the table deletions to respect foreign key constraints, ensuring clean removal of the schema.
63-73: 🛠️ Refactor suggestion
Add an index on label_id for better query performance.
Since this is a junction table, adding an index on label_id would improve query performance for operations filtering or joining on labels.
62-71: Address the enum implementation for link_reason.
There are two comments indicating that enums should be used for the link_reason field. Consider implementing this using one of these approaches:
- Use a CHECK constraint to enforce valid values
- Create an enum type in the database
- Handle the validation at the application level
Would you like help implementing the enum solution?
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
src/swift-server/core/error.swift (2)
50-58: Error messages are clear and well-documented.The new error messages are descriptive and provide good context, especially E10016's additional security context. Consider adding similar contextual information for E10017 as it also relates to data integrity.
.E10017: ErrorInfo( - message: "Import from file returned an ID that was not found in the DB"), + message: "Import from file returned an ID that was not found in the DB", + additionalInfo: "This might indicate data corruption or synchronization issues" +),
Line range hint
1-58: Consider adding explicit mapping between ErrorCode and ApiError.While the separation between internal
ErrorCodeand externalApiErroris good practice, consider adding explicit mapping between them to ensure consistent error translation from internal to API errors. This would help maintain error handling consistency across the application.Example approach:
extension ErrorCode { var apiError: ApiError { switch self { case .E10014: return .API10021 // ... other mappings } } }src/swift-server/bank-transaction/bank-transaction.service.swift (4)
Line range hint
79-89: Consider extracting duplicate detection criteria into a reusable function.The duplicate detection logic uses multiple criteria that might be reused elsewhere. Consider extracting these conditions into a separate function for better maintainability and reusability.
+private func applyDuplicateDetectionCriteria( + query: QueryBuilder<BankTransaction>, + transaction: BankTransaction +) -> QueryBuilder<BankTransaction> { + query + .filter(\.$groupOwner.$id == transaction.$groupOwner.id) + .filter(\.$_date == transaction.date.toString()) + .filter(\.$details == transaction.details) + .filter(\.$kind == transaction.kind) + .filter(\.$movementName == transaction.movementName) + .filter(\.$value == transaction.value) +} func existsSimilar(transaction: BankTransaction) async throws -> Bool { - let similarCount = try await BankTransaction.query(on: db) - .filter(\.$groupOwner.$id == transaction.$groupOwner.id) - .filter(\.$_date == transaction.date.toString()) - .filter(\.$details == transaction.details) - .filter(\.$kind == transaction.kind) - .filter(\.$movementName == transaction.movementName) - .filter(\.$value == transaction.value) - .count() + let similarCount = try await applyDuplicateDetectionCriteria( + query: BankTransaction.query(on: db), + transaction: transaction + ).count() return similarCount > 0 }
92-96: Consider enhancing error handling and logging for batch operations.The batch insert could benefit from more detailed error handling and logging, especially for partial failures.
func insertBatch(movements: [BankTransaction]) -> EventLoopFuture<Void> { - return movements.create(on: db).map { _ in - self.logger.info("Insert batch", metadata: ["sql": "Bulk insert"]) + return movements.create(on: db).flatMapAlways { result in + switch result { + case .success: + self.logger.info("Insert batch successful", metadata: [ + "count": "\(movements.count)", + "sql": "Bulk insert" + ]) + case .failure(let error): + self.logger.error("Insert batch failed", metadata: [ + "error": "\(error)", + "count": "\(movements.count)" + ]) + } + return result } }
Line range hint
120-191: Enhance guard statement messages for better debugging.Consider adding more descriptive error messages in guard statements to aid in debugging.
- guard let transaction = transaction else { + guard let transaction = transaction else { + logger.debug("Transaction not found or not accessible", metadata: [ + "transactionId": "\(transactionId)", + "groupIds": "\(groupIds)" + ]) return .transactionNotFound }
Line range hint
192-195: Fix enum naming to follow Swift conventions.The enum name
setCommentReturnshould follow Swift's naming convention for types (PascalCase).- enum setCommentReturn { + enum SetCommentReturn { case ok case notFound }src/swift-server/rules/rules.service.swift (1)
12-12: Consider injecting RuleEngine dependency.Instead of creating a RuleEngine instance directly, consider injecting it through the constructor for better testability and flexibility.
- let ruleEngine = RuleEngine() + let ruleEngine: RuleEngine + + init(db: Database, ruleEngine: RuleEngine = RuleEngine()) { + super.init(db: db) + self.ruleEngine = ruleEngine + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/swift-server/bank-transaction/bank-transaction.service.swift(8 hunks)src/swift-server/core/error.swift(2 hunks)src/swift-server/importer/importer.service.swift(2 hunks)src/swift-server/rules/rules.service.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/swift-server/importer/importer.service.swift
🧰 Additional context used
📓 Learnings (2)
src/swift-server/rules/rules.service.swift (1)
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/swift-server/rules/rules.service.swift:0-0
Timestamp: 2024-12-01T21:23:33.848Z
Learning: In `src/swift-server/rules/rules.service.swift`, the `applyRules` method has been completely rewritten, rendering previous comments about error handling and performance improvements no longer applicable.
src/swift-server/bank-transaction/bank-transaction.service.swift (1)
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/swift-server/bank-transaction/bank-transaction.service.swift:0-0
Timestamp: 2024-12-04T17:01:23.715Z
Learning: Prefer using the `requireID()` method instead of force unwrapping optionals in the code.
🔇 Additional comments (8)
src/swift-server/core/error.swift (2)
1-5: LGTM! New error codes follow consistent naming pattern.
The additional error codes (E10014-E10017) maintain the existing sequential naming convention and are well-organized.
8-19: Verify the necessity of adding 30 new API error codes.
While the new error codes follow the existing pattern, adding such a large range of new codes (API10021-API10050) seems excessive. Let's verify their intended usage across the codebase.
✅ Verification successful
The addition of new API error codes (API10021-API10050) is justified
The search results show that all the newly added error codes (API10021-API10050) are actively used in the codebase, primarily in the rules and label API implementations. Each error code serves a specific purpose:
- Multiple UUID validation errors (API10033, API10041-API10047) for Rule IDs, Condition IDs, and Label IDs
- Resource not found errors (API10048 for condition/label, API10050 for rule)
- Various validation and processing errors in rule-api-impl.swift (API10021-API10040)
- Label-specific error in label-api-impl.swift (API10025)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of new API error codes in the codebase
# Search for any usage of the new API error codes
echo "Searching for usage of new API error codes (API10021-API10050)..."
rg "API100(2[1-9]|[34][0-9]|50)" --type swift
# Count total API errors actually used
echo "Counting total API errors used in the codebase..."
rg "API\d{5}" --type swift
Length of output: 10186
src/swift-server/bank-transaction/bank-transaction.service.swift (4)
4-6: LGTM! Good architectural improvement.
The inheritance from ServiceWithQueueAndDb provides a clean way to access both database and queue functionality, following good dependency injection principles.
Line range hint 8-24: LGTM! Clean implementation of active labels retrieval.
The method effectively retrieves and maps active labels to transactions while properly handling relationships and filtering.
98-109: Consider adding error handling for queue operations.
While the use of requireID() is good, consider implementing error handling for queue operations and transaction rollback if queue dispatch fails.
Line range hint 28-75: Clean up commented code if no longer needed.
There's a commented line that might indicate incomplete work:
// .with(\.$labels)Consider either removing this comment if it's no longer needed or implementing the functionality if it's still required.
src/swift-server/rules/rules.service.swift (2)
77-105: LGTM! Well-implemented deletion logic.
The deletion implementation is robust with:
- Proper transaction handling
- Child rule validation
- Cleanup of related entities
263-273: LGTM! Well-implemented pivot handling.
The helper method properly handles the pivot relationship by:
- Updating the link reason before detachment
- Using proper transaction context
| func addLabel( | ||
| labelId: UUID, | ||
| toRule ruleId: UUID, | ||
| for validGroupsIds: [UUID] | ||
| ) async throws -> AddLabelState { | ||
| let rule = try await Rule.query(on: db).filter( | ||
| \.$groupOwner.$id ~~ validGroupsIds | ||
| ).filter(\.$id == ruleId).first() | ||
| guard let rule else { | ||
| return .notFound | ||
| } | ||
|
|
||
| let label = try await Label.query(on: db).filter(\.$id == labelId).first() | ||
| guard let label else { | ||
| return .notFound | ||
| } | ||
|
|
||
| if rule.$groupOwner.id != label.$groupOwner.id { | ||
| return .invalidOwnerId | ||
| } | ||
|
|
||
| let labelAttached = try await rule.$labels.isAttached(to: label, on: db) | ||
| if !labelAttached { | ||
| try await rule.$labels.attach(label, on: db) | ||
| } | ||
|
|
||
| try await rule.$conditions.load(on: db) | ||
| try await rule.$labels.load(on: db) | ||
| return .ok(rule: rule) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap label addition in transaction.
While label removal uses a transaction, the label addition method should also use one for consistency and data integrity.
func addLabel(
labelId: UUID,
toRule ruleId: UUID,
for validGroupsIds: [UUID]
) async throws -> AddLabelState {
+ try await db.transaction { database in
- let rule = try await Rule.query(on: db)...
+ let rule = try await Rule.query(on: database)...
// ... rest of the implementation using database instead of db
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| let cursorData = try self.cursorHandler.parse(cursor) | ||
| if let idString = cursorData["id"], let id = UUID(uuidString: idString) { | ||
| query = query.filter(\.$id < id) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for cursor parsing.
The cursor parsing could fail silently if the cursor string is malformed. Consider adding proper error handling and validation.
- let cursorData = try self.cursorHandler.parse(cursor)
- if let idString = cursorData["id"], let id = UUID(uuidString: idString) {
- query = query.filter(\.$id < id)
- }
+ do {
+ let cursorData = try self.cursorHandler.parse(cursor)
+ guard let idString = cursorData["id"],
+ let id = UUID(uuidString: idString) else {
+ throw Abort(.badRequest, reason: "Invalid cursor format")
+ }
+ query = query.filter(\.$id < id)
+ } catch {
+ throw Abort(.badRequest, reason: "Failed to parse cursor: \(error)")
+ }📝 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.
| let cursorData = try self.cursorHandler.parse(cursor) | |
| if let idString = cursorData["id"], let id = UUID(uuidString: idString) { | |
| query = query.filter(\.$id < id) | |
| } | |
| } | |
| do { | |
| let cursorData = try self.cursorHandler.parse(cursor) | |
| guard let idString = cursorData["id"], | |
| let id = UUID(uuidString: idString) else { | |
| throw Abort(.badRequest, reason: "Invalid cursor format") | |
| } | |
| query = query.filter(\.$id < id) | |
| } catch { | |
| throw Abort(.badRequest, reason: "Failed to parse cursor: \(error)") | |
| } | |
| } |
There was a problem hiding this comment.
the idea is that a cursor string that's malformed is a non-existand cursor string.
There was a problem hiding this comment.
@Dracks, understood. Thank you for the clarification.
✏️ Learnings added
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/swift-server/rules/rules.service.swift:20-24
Timestamp: 2024-12-04T17:16:57.895Z
Learning: In the Swift file `src/swift-server/rules/rules.service.swift`, within the `getAll` method, if the cursor string is malformed, it should be treated as a nonexistent cursor. The pagination should proceed without throwing an error, as if no cursor was provided.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| func create(rule: RuleParam) async throws -> Rule { | ||
| let newRule = Rule( | ||
| groupOwnerId: rule.groupOwnerId, name: rule.name, | ||
| conditionsRelation: rule.relations, parentId: rule.parentId) | ||
| try await newRule.save(on: db) | ||
| try await newRule.$children.load(on: db) | ||
| try await newRule.$conditions.load(on: db) | ||
| try await newRule.$labels.load(on: db) | ||
| return newRule | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap creation in transaction and validate parent rule.
The rule creation process should validate the parent rule's existence and wrap all operations in a transaction for data consistency.
func create(rule: RuleParam) async throws -> Rule {
+ return try await db.transaction { database in
+ if let parentId = rule.parentId {
+ guard let _ = try await Rule.find(parentId, on: database) else {
+ throw Abort(.badRequest, reason: "Parent rule not found")
+ }
+ }
let newRule = Rule(
groupOwnerId: rule.groupOwnerId, name: rule.name,
conditionsRelation: rule.relations, parentId: rule.parentId)
- try await newRule.save(on: db)
- try await newRule.$children.load(on: db)
- try await newRule.$conditions.load(on: db)
- try await newRule.$labels.load(on: db)
+ try await newRule.save(on: database)
+ try await newRule.$children.load(on: database)
+ try await newRule.$conditions.load(on: database)
+ try await newRule.$labels.load(on: database)
return newRule
+ }
}📝 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 create(rule: RuleParam) async throws -> Rule { | |
| let newRule = Rule( | |
| groupOwnerId: rule.groupOwnerId, name: rule.name, | |
| conditionsRelation: rule.relations, parentId: rule.parentId) | |
| try await newRule.save(on: db) | |
| try await newRule.$children.load(on: db) | |
| try await newRule.$conditions.load(on: db) | |
| try await newRule.$labels.load(on: db) | |
| return newRule | |
| } | |
| func create(rule: RuleParam) async throws -> Rule { | |
| return try await db.transaction { database in | |
| if let parentId = rule.parentId { | |
| guard let _ = try await Rule.find(parentId, on: database) else { | |
| throw Abort(.badRequest, reason: "Parent rule not found") | |
| } | |
| } | |
| let newRule = Rule( | |
| groupOwnerId: rule.groupOwnerId, name: rule.name, | |
| conditionsRelation: rule.relations, parentId: rule.parentId) | |
| try await newRule.save(on: database) | |
| try await newRule.$children.load(on: database) | |
| try await newRule.$conditions.load(on: database) | |
| try await newRule.$labels.load(on: database) | |
| return newRule | |
| } | |
| } |
| func addCondition( | ||
| condition: Rule.BaseCondition, forRule ruleId: UUID, for validGroupsIds: [UUID] | ||
| ) async throws -> AddCodingState { | ||
| let rule = try await Rule.query(on: db).filter( | ||
| \.$groupOwner.$id ~~ validGroupsIds | ||
| ).filter(\.$id == ruleId).with(\.$children).first() | ||
| guard let rule else { | ||
| return .notFound | ||
| } | ||
|
|
||
| try await rule.$conditions.create( | ||
| .init(condition, for: try rule.requireID()), on: db) | ||
| try await rule.$conditions.load(on: db) | ||
| try await rule.$labels.load(on: db) | ||
| return .ok(rule: rule) | ||
| } | ||
|
|
||
| func updateCondition( | ||
| _ condId: UUID, condition updatedCondition: Rule.BaseCondition, | ||
| forRule ruleId: UUID, | ||
| for validGroupsIds: [UUID] | ||
| ) async throws -> AddCodingState { | ||
| let rule = try await Rule.query(on: db).filter( | ||
| \.$groupOwner.$id ~~ validGroupsIds | ||
| ).filter(\.$id == ruleId).with(\.$children).first() | ||
| guard let rule else { | ||
| return .notFound | ||
| } | ||
|
|
||
| let condition = try await Condition.query(on: db).filter(\.$id == condId).filter( | ||
| \.$rule.$id == ruleId | ||
| ).first() | ||
|
|
||
| guard let condition else { | ||
| return .notFound | ||
| } | ||
| condition.operation = updatedCondition.operation | ||
| condition.valueStr = updatedCondition.valueStr | ||
| condition.valueDouble = updatedCondition.valueDouble | ||
| try await condition.save(on: db) | ||
|
|
||
| try await rule.$conditions.load(on: db) | ||
| try await rule.$labels.load(on: db) | ||
| return .ok(rule: rule) | ||
| } | ||
|
|
||
| enum DeleteConditionState { | ||
| case deleted(rule: Rule) | ||
| case notFound | ||
| } | ||
|
|
||
| func deleteCondition( | ||
| _ condId: UUID, | ||
| forRule ruleId: UUID, | ||
| for validGroupsIds: [UUID] | ||
| ) async throws -> DeleteConditionState { | ||
| let rule = try await Rule.query(on: db).filter( | ||
| \.$groupOwner.$id ~~ validGroupsIds | ||
| ).filter(\.$id == ruleId).with(\.$children).first() | ||
| guard let rule else { | ||
| return .notFound | ||
| } | ||
|
|
||
| let condition = try await Condition.query(on: db).filter(\.$id == condId).filter( | ||
| \.$rule.$id == ruleId | ||
| ).first() | ||
|
|
||
| guard let condition else { | ||
| return .notFound | ||
| } | ||
| try await condition.delete(on: db) | ||
|
|
||
| try await rule.$conditions.load(on: db) | ||
| try await rule.$labels.load(on: db) | ||
| return .deleted(rule: rule) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor condition management methods for better maintainability.
- The condition management methods share similar rule querying logic that could be extracted.
- Operations should be wrapped in transactions.
+ private func findRule(id: UUID, in validGroupIds: [UUID], on database: Database) async throws -> Rule? {
+ return try await Rule.query(on: database)
+ .filter(\.$groupOwner.$id ~~ validGroupIds)
+ .filter(\.$id == id)
+ .with(\.$children)
+ .first()
+ }
func addCondition(
condition: Rule.BaseCondition, forRule ruleId: UUID, for validGroupsIds: [UUID]
) async throws -> AddCodingState {
+ try await db.transaction { database in
- let rule = try await Rule.query(on: db)...
+ guard let rule = try await findRule(id: ruleId, in: validGroupsIds, on: database) else {
+ return .notFound
+ }
// ... rest of the implementation
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| func updateRule(withId ruleId: UUID, rule inputRule: RuleParam, for validGroupsIds: [UUID]) | ||
| async throws -> UpdateRuleState | ||
| { | ||
| let rule = try await Rule.query(on: db).filter( | ||
| \.$groupOwner.$id ~~ validGroupsIds | ||
| ).filter(\.$id == ruleId).first() | ||
| guard let rule else { | ||
| return .notFound | ||
| } | ||
| rule.name = inputRule.name | ||
| rule.conditionsRelation = inputRule.relations | ||
| rule.$groupOwner.id = inputRule.groupOwnerId | ||
| rule.$parent.id = inputRule.parentId | ||
| try await rule.save(on: db) | ||
| try await rule.$children.load(on: db) | ||
| try await rule.$conditions.load(on: db) | ||
| try await rule.$labels.load(on: db) | ||
| return .ok(rule: rule) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction and parent rule validation to update method.
Similar to the create method, the update operation should validate the new parent rule and use a transaction.
func updateRule(withId ruleId: UUID, rule inputRule: RuleParam, for validGroupsIds: [UUID])
async throws -> UpdateRuleState
{
+ return try await db.transaction { database in
let rule = try await Rule.query(on: db).filter(
\.$groupOwner.$id ~~ validGroupsIds
).filter(\.$id == ruleId).first()
guard let rule else {
return .notFound
}
+ if let parentId = inputRule.parentId {
+ guard parentId != ruleId,
+ let _ = try await Rule.find(parentId, on: database) else {
+ throw Abort(.badRequest, reason: "Invalid parent rule")
+ }
+ }
rule.name = inputRule.name
rule.conditionsRelation = inputRule.relations
rule.$groupOwner.id = inputRule.groupOwnerId
rule.$parent.id = inputRule.parentId
- try await rule.save(on: db)
- try await rule.$children.load(on: db)
- try await rule.$conditions.load(on: db)
- try await rule.$labels.load(on: db)
+ try await rule.save(on: database)
+ try await rule.$children.load(on: database)
+ try await rule.$conditions.load(on: database)
+ try await rule.$labels.load(on: database)
return .ok(rule: rule)
+ }
}📝 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 updateRule(withId ruleId: UUID, rule inputRule: RuleParam, for validGroupsIds: [UUID]) | |
| async throws -> UpdateRuleState | |
| { | |
| let rule = try await Rule.query(on: db).filter( | |
| \.$groupOwner.$id ~~ validGroupsIds | |
| ).filter(\.$id == ruleId).first() | |
| guard let rule else { | |
| return .notFound | |
| } | |
| rule.name = inputRule.name | |
| rule.conditionsRelation = inputRule.relations | |
| rule.$groupOwner.id = inputRule.groupOwnerId | |
| rule.$parent.id = inputRule.parentId | |
| try await rule.save(on: db) | |
| try await rule.$children.load(on: db) | |
| try await rule.$conditions.load(on: db) | |
| try await rule.$labels.load(on: db) | |
| return .ok(rule: rule) | |
| } | |
| func updateRule(withId ruleId: UUID, rule inputRule: RuleParam, for validGroupsIds: [UUID]) | |
| async throws -> UpdateRuleState | |
| { | |
| return try await db.transaction { database in | |
| let rule = try await Rule.query(on: db).filter( | |
| \.$groupOwner.$id ~~ validGroupsIds | |
| ).filter(\.$id == ruleId).first() | |
| guard let rule else { | |
| return .notFound | |
| } | |
| if let parentId = inputRule.parentId { | |
| guard parentId != ruleId, | |
| let _ = try await Rule.find(parentId, on: database) else { | |
| throw Abort(.badRequest, reason: "Invalid parent rule") | |
| } | |
| } | |
| rule.name = inputRule.name | |
| rule.conditionsRelation = inputRule.relations | |
| rule.$groupOwner.id = inputRule.groupOwnerId | |
| rule.$parent.id = inputRule.parentId | |
| try await rule.save(on: database) | |
| try await rule.$children.load(on: database) | |
| try await rule.$conditions.load(on: database) | |
| try await rule.$labels.load(on: database) | |
| return .ok(rule: rule) | |
| } | |
| } |
There was a problem hiding this comment.
The parent rule is already check in the api-impl. I don't think a transaction here is useful.
There was a problem hiding this comment.
@Dracks, understood. Thank you for clarifying that the parent rule validation is already handled in the API implementation, and wrapping the updateRule method in a transaction is not necessary. I appreciate the explanation.
✏️ Learnings added
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/swift-server/rules/rules.service.swift:52-70
Timestamp: 2024-12-04T17:22:25.530Z
Learning: In the `RuleService`'s `updateRule` method in `src/swift-server/rules/rules.service.swift`, performing parent rule validation is unnecessary because it is already done in the API implementation (`src/swift-server/rules/rule-api-impl.swift`).
Learnt from: Dracks
PR: Dracks/mr-scrooge#30
File: src/swift-server/rules/rules.service.swift:52-70
Timestamp: 2024-12-04T17:22:25.530Z
Learning: Wrapping the `updateRule` method in a transaction within `src/swift-server/rules/rules.service.swift` is not required as per the current design.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary by CodeRabbit
Release Notes
New Features
Cacheclass for generic key-value storage with expiration management.ListWithCursorstructure for pagination support in API responses.Bug Fixes
Documentation
Refactor