Move to typescript and mongodb#67
Conversation
WalkthroughThis update introduces a comprehensive TypeScript-based refactor and modernization of a state machine service. It adds new core, infrastructure, and presentation modules, implements a MongoDB-backed repository, replaces legacy SQL backends, migrates from Mocha to Jest for testing, updates Docker and CI/CD configurations, and introduces extensive new test suites and TypeScript type definitions. Changes
Sequence Diagram(s)sequenceDiagram
participant API as Fastify API Server
participant DI as Dependency Injection Container
participant Logic as Logic Layer
participant Repo as StateMachineRepo (Mongo)
participant Queue as Queue Service Client
participant Worker as Worker Process
API->>DI: Resolve logic, repo, queue dependencies
API->>Logic: list/create/update/get/remove/invoke state machine
Logic->>Repo: Persist/retrieve state machine/execution/operation
Logic->>Queue: Enqueue execution message (invoke)
Worker->>Queue: Poll for execution messages
Worker->>Repo: Retrieve operation and state machine definition
Worker->>Worker: Run operation (Task/Wait/Choice/etc.)
Worker->>Repo: Update operation/execution state
Worker->>Queue: Enqueue next operation if needed
API->>Repo: Fetch execution details (status, operations)
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 52
🔭 Outside diff range comments (3)
src-old/operations/choice.js (1)
113-114:⚠️ Potential issueMissing executionId parameter in updateOperation call
While other
updateOperationcalls in this file have been updated to include theexecutionIdparameter, this instance is still using the old signature without it. This appears to be an oversight and could lead to inconsistent behavior.- .updateOperation(operationId, enums.OP_STATUS.Executing) + .updateOperation(operationId, executionId, enums.OP_STATUS.Executing)bin/entrypoint (1)
76-76: 🛠️ Refactor suggestionAdd error handling for async launcher functions
The launcher functions are async but called without proper error handling, which can lead to unhandled promise rejections.
const launcher = commandLookup[cmd]; if (launcher) { - launcher(); + launcher().catch((err) => { + logger.error({ err }, `Error launching ${cmd}`); + process.exit(1); + }); } else {src-old/operations/wait.js (1)
52-52:⚠️ Potential issueFix incorrect path reference
The code is using
SecondsPathwhen it should be usingTimestampPathto extract the timestamp from the path.- ts = new Date(getValueFromPath(that.input, that.SecondsPath)); + ts = new Date(getValueFromPath(that.input, that.TimestampPath));
🧹 Nitpick comments (100)
tsconfig.json (2)
7-21: Optimize build performance by skipping library type checks.
Consider adding"skipLibCheck": trueundercompilerOptionsto speed up compilation and reduce build times without impacting type safety for your code.
18-20: Clarify or remove the temporary inline comment.
The comment// Disable laterlacks context. Either remove it or specify which setting is intended to be disabled and at what stage.src/core/operations/DEVELOPERS.md (1)
1-1: Convert bare URL to hyperlink.
Markdownlint rule MD034 discourages bare URLs. Consider embedding the AWS Step Functions documentation link as a proper markdown link for better readability, e.g.:[Workflow state types documentation](https://docs.aws.amazon.com/step-functions/latest/dg/workflow-states.html)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Bare URL used
null(MD034, no-bare-urls)
docs/Data_Model.md (1)
1-1: Remove trailing punctuation from heading.
Markdownlint MD026 flags punctuation in headings. Drop the period and refine phrasing for clarity:-# NOTE: this is out of date as the data model is not using a document store. +# NOTE: this data model is out of date as the system no longer uses a document store🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: Trailing punctuation in heading
Punctuation: '.'(MD026, no-trailing-punctuation)
src/core/errors/not-found.ts (1)
1-1: Custom error class looks good, consider adding constructor.The
NotFounderror class is correctly implemented by extending the Error class. This approach allows for specific error type identification throughout the application usinginstanceofchecks.Consider adding a constructor for better error messages:
-export class NotFound extends Error {} +export class NotFound extends Error { + constructor(message = 'Resource not found') { + super(message); + this.name = 'NotFound'; + // Maintains proper stack trace in Node.js + if (Error.captureStackTrace) { + Error.captureStackTrace(this, NotFound); + } + } +}src/presentation/schemas/index.ts (1)
1-3: Consider setting a timeline for the TODO item.The TODO comment about moving schemas into the controllers area for versioning purposes is valid. However, it would be helpful to establish a timeline or create a dedicated issue/ticket for this refactoring to ensure it doesn't get forgotten.
Otherwise, the barrel file approach for centralizing schema exports is a good practice that simplifies imports throughout the application.
config/test.js (2)
8-9: Fix typo in the comment.There's a typo in the comment describing the
oridProviderKeyproperty.- // The provider element for all ORIDs created or consumed. USed int he validation process. + // The provider element for all ORIDs created or consumed. Used in the validation process.
1-10: Consider using ES module syntax for consistency with TypeScript migration.Since you're migrating to TypeScript, consider using ES module syntax instead of CommonJS for better consistency across the codebase.
-module.exports = { +export default { fastifyOptions: { logger: { level: 'fatal', }, }, // The provider element for all ORIDs created or consumed. Used in the validation process. oridProviderKey: 'testIssuer', };🧰 Tools
🪛 ESLint
[error] 1-1: 'module' is not defined.
(no-undef)
DEV_NOTES.md (2)
38-42: Standardize Markdown formatting for consistency.For consistency with the rest of the document, consider using asterisks for unordered lists and specifying the language in code blocks.
-## Docker cleanup commands +## Docker cleanup commands -- Remove dangling images +* Remove dangling images -``` +```bash docker rmi $(docker image ls -f dangling=true -q --no-trunc)<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 38-38: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 40-40: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `44-48`: **Standardize Markdown formatting for the second list item.** Apply the same formatting standards to the second list item and code block. ```diff -- Remove previously built MDS Serverless Functions images +* Remove previously built MDS Serverless Functions images -``` +```bash docker image ls | grep mds-sf- | awk '{print $3}' | xargs docker rmi<details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 44-44: Unordered list style Expected: asterisk; Actual: dash (MD004, ul-style) --- 46-46: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> </blockquote></details> <details> <summary>src/presentation/routes/v1/v1-router.ts (1)</summary><blockquote> `4-6`: **Consider adding a comment describing the router's purpose.** A brief comment explaining what API endpoints are provided by the state machine controller would improve documentation. ```diff export async function v1Router(app: FastifyInstance) { + // Register state machine endpoints for managing machines and their executions await app.register(stateMachineController, { prefix: '/' }); }__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.ts (1)
3-12: Consider using ES module syntax instead of CommonJSSince the project is moving to TypeScript, consider using ES module syntax (export const MdsSdk = {...}) instead of CommonJS module.exports for consistency with the rest of the TypeScript codebase.
-module.exports = { - MdsSdk: { +export const MdsSdk = { getQueueServiceClient: jest .fn() .mockReturnValue(Promise.resolve(mockQueueClient)), getServerlessFunctionsClient: jest .fn() .mockReturnValue(Promise.resolve(mockFunctionsClient)), - }, };src/presentation/__tests__/di-container-init.test.ts (1)
14-30: Consider testing other container registrationsThe test effectively verifies that the logic dependency can be resolved, but consider expanding it to check other dependencies registered in the container (logger, mdsAuthManager, mongoClient, stateMachineRepo) for complete verification of the DI setup.
- const logic = diContainer.resolve('logic'); - - // Assert - expect(logic).not.toBeNull(); + // Act & Assert + expect(diContainer.resolve('logic')).not.toBeNull(); + expect(diContainer.resolve('logger')).not.toBeNull(); + expect(diContainer.resolve('mdsAuthManager')).not.toBeNull(); + expect(diContainer.resolve('mongoClient')).not.toBeNull(); + expect(diContainer.resolve('stateMachineRepo')).not.toBeNull();src/core/operations/fail.test.ts (1)
6-6: Fix the error message in test description.The test description mentions "when definition type is not choice" but this test is for the
Failoperation, notChoice. This appears to be a copy-paste error.- it('should throw exception when definition type is not choice', () => { + it('should throw exception when definition type is not fail', () => {Dockerfile.old (1)
1-18: Consider cleaning up commented code in the Dockerfile.The Dockerfile contains a commented line for removing test files. Since this is marked as the "old" Dockerfile and is likely kept for reference, consider either removing the commented line or adding a comment explaining why it's preserved.
-# RUN rm -r ./**/*.test.jssrc/presentation/functions/__tests__/make-orid.test.ts (1)
9-9: Consider using a mock for config.The test is currently accessing the actual configuration value from the config module. For unit tests, consider mocking the config to isolate the test from external configuration changes.
- const issuer = config.get<string>('oridProviderKey'); + // Mock config in the test setup + jest.mock('config', () => ({ + get: jest.fn().mockImplementation((key) => { + if (key === 'oridProviderKey') return 'test-provider'; + return undefined; + }), + })); + const issuer = 'test-provider';Also applies to: 26-26
src-old/operations/task.js (1)
85-85: TODO comment extension updateThe TODO comment now references a TypeScript file extension (.ts), reflecting the migration to TypeScript.
Consider addressing this TODO as part of the migration or creating a specific issue to track it. The comment suggests retry logic implementation is needed.
src/core/operations/succeed.test.ts (2)
66-99: Inconsistent test descriptionThe test title doesn't match what the test is actually verifying.
- it('should succeed operation and fail execution', async () => { + it('should succeed operation and execution', async () => {The test is correctly verifying that both the operation and execution are updated to 'Succeeded' status, not 'Failed' as the title suggests.
78-83: Variable name doesn't match classThe variable name
failis used for aSucceedclass instance, which is confusing.- const fail = new Succeed( + const succeed = new Succeed(src/presentation/controllers/health-check-controller.ts (1)
10-12: Generic type parameter could be more specificThe request body is currently typed as
any.- app.get<{ - Body: any; - }>( + app.get(For a GET request without a body, you could simply omit the generic type parameter since there's no request body to type.
src/presentation/schemas/stateMachines/invoke-state-machine-response-body-schema.ts (1)
3-5: Consider adding documentation to the schemaThe schema would benefit from additional documentation for better API documentation generation.
export const InvokeStateMachineResponseBodySchema = Type.Object({ - orid: Type.String(), + orid: Type.String({ + description: 'Object Resource Identifier for the state machine execution', + examples: ['sm:exec:12345'] + }), });.eslintrc.js (1)
19-20: Consider addressing the TODO comment about explicit any types.While disabling the
no-explicit-anyrule is reasonable during a transition to TypeScript, it's worth planning to address this in the future for improved type safety.Consider creating a ticket to revisit this rule once the migration is complete to improve type safety throughout the codebase.
src/presentation/controllers/__tests__/health-check-controller.test.ts (1)
5-48: Consider adding more test cases for different health check scenarios.The test suite only covers the "happy path" scenario where the health check returns a specific status. In real-world applications, health checks might return different statuses based on various conditions.
Consider adding additional test cases to cover scenarios such as:
- Database being unavailable (dbStatus = "DOWN")
- Server having issues (serverStatus = "DOWN")
- Error handling paths
These additional tests would improve the robustness of your test suite.
src/presentation/server.ts (3)
25-27: Consider using the logger consistently.The error handling in the catch block uses
console.errorinstead ofapp.log. For consistency with the rest of the application, consider using the Fastify logger.} catch (err) { - console.error(err); + app.log.error(err); process.exit(1); }
14-28: Consider implementing graceful shutdown.The current error handling immediately terminates the process. Consider implementing a more graceful shutdown mechanism that closes existing connections and performs cleanup before exiting.
try { const address = await app.listen({ port, host: '::' }); app.log.info( app.printRoutes({ includeHooks: false, includeMeta: ['metaProperty'], }), ); app.log.info(`Server listening at ${address}`); } catch (err) { console.error(err); + // Attempt to close the server gracefully + await app.close(); process.exit(1); }
15-15: Consider cross-platform compatibility for the host binding.Using
::for IPv6 unspecified address may not work consistently across all environments. Consider making this configurable or using0.0.0.0as a more universally compatible alternative.- const address = await app.listen({ port, host: '::' }); + // Get host from config with fallback, or use '0.0.0.0' for broader compatibility + const host = config.has('apiHost') ? config.get<string>('apiHost') : '0.0.0.0'; + const address = await app.listen({ port, host });src/presentation/hooks/validate-request-orid-param.ts (2)
9-10: Simplify boolean conversion.The boolean conversion using
!!on the result of accessing theoridproperty is unnecessary and can be simplified.- const hasOridInRequest = !!(request.params as { orid: string | undefined }) - .orid; + const hasOridInRequest = Boolean((request.params as { orid: string | undefined }).orid);
16-29: Align error messages.The error message in the
donecallback ("Missing or malformed ORID") is different from what's sent to the client ("Resource not understood"). Consider aligning these messages for consistency and better debugging.if (!parsedOrid) { reply.status(400); reply.header('content-type', 'text/plain'); - reply.send('Resource not understood'); + const errorMessage = 'Missing or malformed ORID'; + reply.send(errorMessage); request.log.debug( { orid, parsedOrid, }, - 'Resource not understood', + errorMessage, ); - done(new Error('Missing or malformed ORID')); + done(new Error(errorMessage)); return; }src/presentation/hooks/__tests__/validate-request-orid-param.test.ts (4)
25-46: Add mock for request.log.debug in the first test.The first test doesn't include a mock for
request.log.debug, which is used in the hook function. While it's not being called in this test case, it's good practice to have consistent mock objects across all test cases.const request: { parsedToken: IdentityJwt | undefined; params: Record<string, string>; + log: { + debug: (arg: unknown) => void; + }; } = { parsedToken: undefined, params: {}, + log: { + debug: jest.fn(), + }, };
48-71: Add mock for request.log.debug in the second test.Similar to the first test, the second test doesn't include a mock for
request.log.debug. This should be added for consistency with the third test.const request: { parsedToken: IdentityJwt | undefined; params: Record<string, string>; + log: { + debug: (arg: unknown) => void; + }; } = { parsedToken: undefined, params: { orid: 'orid:1:testIssuer:::1234:qs:5678', }, + log: { + debug: jest.fn(), + }, };
73-116: Improve test case description.The test description "raises error and replies with bad request when invalid orid in params" accurately describes what happens, but could be more specific about what makes the ORID invalid (format/structure rather than authorization).
- it('raises error and replies with bad request when invalid orid in params', () => { + it('raises error and replies with bad request when orid has invalid format', () => {
25-116: Add test for edge case with empty ORID string.There's no test for the edge case where the ORID parameter is an empty string. Consider adding this test to ensure proper handling of all possible inputs.
it('raises error and replies with bad request when orid is an empty string', () => { // Arrange const request: { parsedToken: IdentityJwt | undefined; headers: Record<string, string>; params: Record<string, string>; log: { debug: (arg: string) => void; }; } = { parsedToken: undefined, headers: { token: 'testToken' }, params: { orid: '', }, log: { debug: jest.fn(), }, }; const reply: { status: () => void; header: (key: string, value: string) => void; send: (arg: unknown) => void; } = { status: jest.fn(), header: jest.fn(), send: jest.fn(), }; const done = jest.fn(); // Act validateRequestOridParam( request as FastifyRequest, reply as unknown as FastifyReply, done, ); // Assert expect(done).toHaveBeenCalledTimes(1); expect(done).toHaveBeenCalledWith(new Error('Missing or malformed ORID')); expect(reply.status).toHaveBeenCalledWith(400); expect(reply.header).toHaveBeenCalledWith('content-type', 'text/plain'); expect(reply.send).toHaveBeenCalledWith('Resource not understood'); });src/presentation/hooks/__tests__/validate-can-access-orid-param.test.ts (4)
25-46: Add mock for request.log.debug in the first test.The first test doesn't include a mock for
request.log.debug, which might be used in the hook function. While it's not being called in this test case, it's good practice to have consistent mock objects across all test cases.const request: { parsedToken: IdentityJwt | undefined; params: Record<string, string>; + log: { + debug: (arg: unknown) => void; + }; } = { parsedToken: undefined, params: {}, + log: { + debug: jest.fn(), + }, };
48-75: Add mock for request.log.debug in the second test.Similar to the first test, the second test doesn't include a mock for
request.log.debug. This should be added for consistency.const request: { parsedToken: IdentityJwt | undefined; params: Record<string, string>; + log: { + debug: (arg: unknown) => void; + }; } = { parsedToken: { payload: { accountId: '1234', }, } as IdentityJwt, params: { orid: 'orid:1:testIssuer:::1234:qs:5678', }, + log: { + debug: jest.fn(), + }, };
77-104: Add mock for request.log.debug in the third test.Similar to the previous tests, the third test doesn't include a mock for
request.log.debug. This should be added for consistency.const request: { parsedToken: IdentityJwt | undefined; params: Record<string, string>; + log: { + debug: (arg: unknown) => void; + }; } = { parsedToken: { payload: { accountId: '1', }, } as IdentityJwt, params: { orid: 'orid:1:testIssuer:::1234:qs:5678', }, + log: { + debug: jest.fn(), + }, };
149-149: Check for response message in the forbidden case.The test verifies that
reply.sendis called but doesn't check what message is being sent. Consider adding an assertion to verify the content of the response.expect(reply.status).toHaveBeenCalledWith(403); - expect(reply.send).toHaveBeenCalledWith(); + expect(reply.send).toHaveBeenCalledWith('Forbidden');This assumes that the implementation sends 'Forbidden' as the response body. If the actual implementation sends a different message or no message at all, adjust this assertion accordingly.
src/presentation/schemas/stateMachines/invoke-state-machine-request-body-schema.ts (1)
3-7: Consider adding basic structure validation instead of using Type.Unknown()Using
Type.Unknown()allows any data without validation, which may be overly permissive. If the state machine invocation parameters have some expected structure, consider defining at least a basic schema to validate the shape of the data.If there's truly no consistent structure for invocation requests, document this decision with a comment to clarify the intentional flexibility.
- export const InvokeStateMachineRequestBodySchema = Type.Unknown(); + // Using Type.Unknown() to allow flexible payload structures for different state machine types + export const InvokeStateMachineRequestBodySchema = Type.Unknown();config/default.js (2)
1-1: Consider converting to TypeScript with ES module syntaxThe codebase appears to be using TypeScript with ES modules, but this file uses CommonJS syntax. This inconsistency could lead to issues with TypeScript's module resolution.
Consider converting this file to a TypeScript file with ES module syntax for consistency:
- module.exports = { + export default {🧰 Tools
🪛 ESLint
[error] 1-1: 'module' is not defined.
(no-undef)
36-39: Consider using environment variables for MongoDB connectionHardcoded database connection details may not be suitable for different environments (development, testing, production).
Consider using environment variables with defaults:
mongo: { - url: 'mongodb://localhost:27017', - db: 'mdsCloudStateMachine', + url: process.env.MONGO_URL || 'mongodb://localhost:27017', + db: process.env.MONGO_DB || 'mdsCloudStateMachine', },src/core/operations/index.test.ts (1)
10-78: Add a negative test case for unknown state typesThe test suite thoroughly tests each supported state type but doesn't verify behavior with unknown or invalid state types. Adding a negative test would improve robustness.
Consider adding a test case for an unknown state type:
it('should throw an error for unknown state type', () => { // Arrange const invalidDefinition = { ...testDefinition, States: { ...testDefinition.States, Invalid: { Type: 'InvalidType', }, }, }; const metadata = { ...baseMetadata, stateKey: 'Invalid', }; // Act & Assert expect(() => getOperation( invalidDefinition, metadata, mockLogger, mockRepo, )).toThrow(); });src/presentation/types/identity-jwt.ts (1)
7-12: Consider adding JSDoc comments for better developer experience.These interfaces represent critical authentication components. Adding JSDoc comments would enhance IDE tooltips and documentation.
+/** + * Extends JwtPayload with MDS Cloud identity-specific claims + */ export interface IdentityJwtPayload extends JwtPayload { + /** The account ID of the authenticated user */ accountId: string; + /** The user ID of the authenticated user */ userId: string; + /** The human-readable name of the authenticated user */ friendlyName: string; + /** Optional field indicating if this token represents an impersonated user session */ impersonatedBy?: string; }jest-reporters/emit-only-failures.js (3)
1-4: Consider converting this file to TypeScript like the rest of the codebase.While I understand there's a Jest issue (referenced in line 3) blocking the TypeScript conversion, consider adding a TODO with a link to the issue or a tracking task to revisit this file when the Jest issue is resolved.
🧰 Tools
🪛 ESLint
[error] 2-2: Definition for rule 'import/no-extraneous-dependencies' was not found.
(import/no-extraneous-dependencies)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: 'require' is not defined.
(no-undef)
14-15: Consider using rest parameters instead of the arguments object.The ESLint disable comment indicates you're aware of this, but using the spread syntax directly would be cleaner:
- // eslint-disable-next-line prefer-rest-params - super.printTestFileHeader(...arguments); + super.printTestFileHeader(_testPath, config, result);
42-50: Remove unnecessary await in Promise.all array.The Promise.all is already awaited, so the additional awaits inside the array are redundant:
await Promise.all([ - await this.#repo.updateOperation( + this.#repo.updateOperation( this.#operationId, this.#executionId, 'Succeeded', this.#output, ), - await this.#repo.updateExecution(this.#executionId, 'Succeeded'), + this.#repo.updateExecution(this.#executionId, 'Succeeded'), ]);src/core/operations/succeed.ts (2)
1-3: Standardize import ordering.Consider organizing imports with third-party imports first, followed by local imports:
+import { BaseLogger } from 'pino'; import { StateMachineRepo } from '../interfaces/state-machine-repo'; -import { BaseLogger } from 'pino'; import { SucceedState } from '../types/state-machine-definition';
39-55: Remove redundant awaits in Promise.all array.The Promise.all is already awaited, so the additional awaits inside the array are unnecessary and redundant:
await Promise.all([ - await this.#repo.updateOperation( + this.#repo.updateOperation( this.#operationId, this.#executionId, 'Succeeded', this.#output, ), - await this.#repo.updateExecution(this.#executionId, 'Succeeded'), + this.#repo.updateExecution(this.#executionId, 'Succeeded'), ]);src/presentation/schemas/stateMachines/get-execution-response-body-schema.ts (1)
1-15: Add more specific types for string fields.Consider using more specific string types for fields that have constrained values:
export const GetExecutionResponseBodySchema = Type.Object({ orid: Type.String(), - status: Type.String(), + status: Type.Union([ + Type.Literal('Running'), + Type.Literal('Succeeded'), + Type.Literal('Failed'), + // Add any other valid status values + ]), operations: Type.Array( Type.Object({ created: Type.String({ format: 'date-time' }), stateKey: Type.String(), - status: Type.String(), + status: Type.Union([ + Type.Literal('Running'), + Type.Literal('Succeeded'), + Type.Literal('Failed'), + // Add any other valid status values + ]), input: Type.Unknown(), output: Type.Unknown(), }), ), });src/core/operations/fail.ts (3)
1-3: Standardize import ordering.Consider organizing imports with third-party imports first, followed by local imports:
+import { BaseLogger } from 'pino'; import { StateMachineRepo } from '../interfaces/state-machine-repo'; -import { BaseLogger } from 'pino'; import { FailState } from '../types/state-machine-definition';
39-55: Remove redundant awaits in Promise.all array.The Promise.all is already awaited, so the additional awaits inside the array are redundant:
await Promise.all([ - await this.#repo.updateOperation( + this.#repo.updateOperation( this.#operationId, this.#executionId, 'Succeeded', this.#output, ), - await this.#repo.updateExecution(this.#executionId, 'Failed'), + this.#repo.updateExecution(this.#executionId, 'Failed'), ]);
5-37: Consider extracting common base class for state operations.The Fail and Succeed classes share almost identical structures and functionality. Consider creating a base class for common functionality to reduce code duplication and improve maintainability.
src/presentation/hooks/validate-can-access-orid-param.ts (3)
4-31: ORID access validation looks solid but could benefit from early returnsThe hook correctly validates that the authenticated user has access to the requested ORID by comparing account IDs, with a special case for admin account '1'.
Consider refactoring to use early returns for cleaner control flow:
export function validateCanAccessOridParam( request: FastifyRequest, reply: FastifyReply, done: (err?: any) => void, ) { const logger = request.log; const hasOridInRequest = !!(request.params as { orid: string | undefined }) .orid; - if (hasOridInRequest) { - // NOTE: Orid should be validated by other hook at this point. - const orid = (request.params as { orid: string | undefined }).orid; - const parsedOrid = v1.parse(orid!); - - const tokenAccountId = request.parsedToken?.payload.accountId; - if (parsedOrid.custom3 !== tokenAccountId && tokenAccountId !== '1') { - logger.debug( - { tokenAccountId, requestAccount: parsedOrid.custom3 }, - 'Insufficient privilege for request', - ); - reply.status(403); - reply.send(); - done(new Error('Insufficient privilege for request')); - return; - } + if (!hasOridInRequest) { + done(); + return; + } + + // NOTE: Orid should be validated by other hook at this point. + const orid = (request.params as { orid: string | undefined }).orid; + const parsedOrid = v1.parse(orid!); + + const tokenAccountId = request.parsedToken?.payload.accountId; + if (parsedOrid.custom3 === tokenAccountId || tokenAccountId === '1') { + done(); + return; } + + logger.debug( + { tokenAccountId, requestAccount: parsedOrid.custom3 }, + 'Insufficient privilege for request', + ); + reply.status(403); + reply.send(); + done(new Error('Insufficient privilege for request')); }
10-15: Consider using a type-safe approach for parameter extractionThe current approach uses type casting which could be error-prone.
Consider defining a proper interface for request parameters:
+interface OridParams { + orid?: string; +} export function validateCanAccessOridParam( request: FastifyRequest, reply: FastifyReply, done: (err?: any) => void, ) { const logger = request.log; - const hasOridInRequest = !!(request.params as { orid: string | undefined }) - .orid; + const params = request.params as OridParams; + const hasOridInRequest = !!params.orid; if (hasOridInRequest) { // NOTE: Orid should be validated by other hook at this point. - const orid = (request.params as { orid: string | undefined }).orid; + const orid = params.orid;
18-18: Consider extracting admin account ID to a constantThe hardcoded '1' as admin account ID could benefit from being a named constant for clarity and maintainability.
+const ADMIN_ACCOUNT_ID = '1'; export function validateCanAccessOridParam( request: FastifyRequest, reply: FastifyReply, done: (err?: any) => void, ) { // ... const tokenAccountId = request.parsedToken?.payload.accountId; - if (parsedOrid.custom3 !== tokenAccountId && tokenAccountId !== '1') { + if (parsedOrid.custom3 !== tokenAccountId && tokenAccountId !== ADMIN_ACCOUNT_ID) {jest.config.ts (1)
23-23: Custom Jest reporter may need documentationThe custom reporter helps filter test output, but its exact behavior isn't self-documenting.
Consider adding a brief comment explaining what the custom reporter does:
- reporters: ['<rootDir>/jest-reporters/emit-only-failures.js'], + reporters: [ + // Only emits output for failing tests to reduce console noise + '<rootDir>/jest-reporters/emit-only-failures.js' + ],src/presentation/controllers/v1/__tests__/state-machine-controller.test.ts (5)
7-7: Unused importThe
mockLoggeris imported but never used in this test file.Remove the unused import:
-import { mockLogger } from '../../../../test-utilities';🧰 Tools
🪛 ESLint
[error] 7-7: 'mockLogger' is defined but never used.
(@typescript-eslint/no-unused-vars)
23-23: Unused mock declarationThe
mockValidateTokenis declared but never used in the tests.Remove the unused variable:
-const mockValidateToken = jest.mocked(validateToken);🧰 Tools
🪛 ESLint
[error] 23-23: 'mockValidateToken' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
36-37: Type assertion with ts-expect-errorUsing
@ts-expect-errorto bypass TypeScript's type checking for the mock may be necessary but could hide issues.Consider creating a proper type or interface for the mock to avoid the need for
@ts-expect-error:+// Create a type that matches only the public interface we need to mock +type LogicMock = Pick<Logic, 'createStateMachine' | 'getStateMachine' | + 'updateStateMachine' | 'listStateMachines' | 'removeStateMachine' | + 'invokeStateMachine' | 'getExecutionDetails'>; const logicMock = { createStateMachine: jest.fn(), getStateMachine: jest.fn(), updateStateMachine: jest.fn(), listStateMachines: jest.fn(), removeStateMachine: jest.fn(), invokeStateMachine: jest.fn(), getExecutionDetails: jest.fn(), - // @ts-expect-error - Mocking private attribute - } satisfies Logic; + } satisfies LogicMock;
142-142: URL mismatch between test and implementationThis test is using
/v1/machines/while other tests use/v1/machine/(singular).Ensure consistency in URL paths:
const response = await makeRequest({ method: 'GET', - url: `/v1/machines/${testStateMachineOrid}`, + url: `/v1/machine/${testStateMachineOrid}`, headers: { authorization: 'Bearer test-token', }, });
91-93: Consider extracting repeated test values to constantsThe authorization header with "Bearer test-token" is repeated in all test requests.
Extract repeated values to constants for better maintainability:
+ const AUTH_HEADER = 'Bearer test-token'; + const CONTENT_TYPE_JSON = 'application/json'; function makeRequest(overrides: InjectOptions = {}) { return app.inject({ ...({ url: '/', method: 'GET', + headers: { + authorization: AUTH_HEADER, + }, } satisfies InjectOptions), ...overrides, }); } // Then in tests: const response = await makeRequest({ method: 'GET', url: '/v1/machines', - headers: { - authorization: 'Bearer test-token', - }, }); // For requests with content-type: const response = await makeRequest({ method: 'POST', url: '/v1/machine', headers: { - authorization: 'Bearer test-token', - 'content-type': 'application/json', + 'content-type': CONTENT_TYPE_JSON, }, payload: newDefinition, });Also applies to: 117-119, 144-145, 184-185, 213-215, 271-272, 339-341, 376-378
src/core/operations/index.ts (1)
24-29: Function lacks return type annotationAdding an explicit return type would make the API more clear and help catch type errors.
Add a return type annotation to the function:
export function getOperation( definition: StateMachineDefinition, metadata: Metadata, logger: BaseLogger, repo: StateMachineRepo, -) { +): Task | Succeed | Fail | Choice | Wait | undefined {src/presentation/schemas/stateMachines/list-state-machines-response-body-schema.ts (1)
10-12: Consider adding a ticket reference to the TODO commentThe TODO comment mentions removing snake_case properties once verified they're no longer used, but doesn't specify when or how that verification will happen.
- // TODO: Remove snake case items once it has been verified they are no longer used + // TODO: [TICKET-123] Remove snake case items once it has been verified they are no longer usedsrc/presentation/hooks/validate-token.ts (1)
22-27: Simplify nested await patternThe double await pattern can be simplified for better readability.
- const publicSignature = await ( - await MdsSdk.getIdentityServiceClient() - ).getPublicSignature(); + const identityClient = await MdsSdk.getIdentityServiceClient(); + const publicSignature = await identityClient.getPublicSignature();bin/entrypoint (2)
15-16: Missing semicolon after getQueueServiceClient callThere's a missing semicolon which could lead to unexpected behavior in some JavaScript environments.
- const client = await mdsSdk.getQueueServiceClient() + const client = await mdsSdk.getQueueServiceClient();
12-25: Extract queue names to configurationThe hardcoded queue names should be moved to configuration for better maintainability.
+const SYSTEM_QUEUES = { + IN_FLIGHT: 'mds-sm-inFlightQueue', + PENDING: 'mds-sm-pendingQueue', +}; const ensureSystemQueuesExist = async () => { // mds qs create --env localAdmin mds-sm-inFlightQueue && mds qs create --env localAdmin mds-sm-pendingQueue - const queueNames = ['mds-sm-inFlightQueue', 'mds-sm-pendingQueue']; + const queueNames = Object.values(SYSTEM_QUEUES); const client = await mdsSdk.getQueueServiceClient(); const existingQueues = await client.listQueues();src-old/operations/wait.js (2)
62-63: Remove unnecessarythisaliasThe aliasing of
thistothatis unnecessary and can be simplified.- const that = this; - const { operationId, executionId, output, Next } = this; + const { operationId, executionId, output, Next } = this;Or using an arrow function to preserve
thiscontext:-Wait.prototype.run = function run() { +Wait.prototype.run = async function() { this.output = this.input; - const that = this; const { operationId, executionId, output, Next } = this;🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: This aliasing of this is unnecessary.
Arrow functions inherits
thisfrom their enclosing scope.
Safe fix: Use this instead of an alias.(lint/complexity/noUselessThisAlias)
64-86: Consider using async/await patternThe code uses promise chains which can be harder to read than async/await. Consider refactoring to use modern async/await syntax for better readability and error handling.
Wait.prototype.run = async function run() { this.output = this.input; const { operationId, executionId, output, Next } = this; const opDetails = await repos.getOperation(operationId, executionId); if (!opDetails.waitUntilUtc || opDetails.waitUntilUtc > globals.toEpoch(new Date())) { const afterUtc = opDetails.waitUntilUtc || computeWaitTimestamp(this); logger.trace({ operationId, afterUtc }, 'Task entering waiting state.'); await repos.delayOperation(operationId, executionId, afterUtc); return null; } const nextOpId = globals.newUuid(); logger.trace({ operationId }, 'Task finished waiting.'); await repos.updateOperation(operationId, executionId, enums.OP_STATUS.Succeeded); await repos.createOperation(nextOpId, executionId, Next, output); return { nextOpId, output, next: Next, }; };src/presentation/schemas/stateMachines/update-state-machine-response-body-schema.ts (2)
9-13: Structure looks good, but consider more specific typing for StatesThe schema is well-defined, but
Type.Any()for the States record values sacrifices type safety. If the structure of state objects is known, consider creating a more specific type definition to improve type checking and documentation.definition: Type.Object({ Name: Type.String(), StartAt: Type.String(), - States: Type.Record(Type.String(), Type.Any()), + States: Type.Record(Type.String(), Type.Object({ + Type: Type.String(), + // Add other common state properties here + // You can use Type.Optional() for non-required fields + })), }),
20-33: Remove or formalize the commented code sectionThe commented section appears to be documenting the full structure of a state machine object, but including it as a comment in production code isn't ideal.
Consider one of these approaches:
- Remove the comment if it's just for reference during development
- Convert it to a proper TypeScript interface with documentation comments
- Move it to a separate documentation file
-/* -id: string -accountId: string -name: string -isDeleted: boolean -activeVersion: string -versions: { - id: string - definition: StateMachineDefinition -}[] -orid: string -definition: StateMachineDefinition -versions: StateMachineDefinition[] - */ +/** + * Full StateMachine object structure for reference: + * @property {string} id + * @property {string} accountId + * @property {string} name + * @property {boolean} isDeleted + * @property {string} activeVersion + * @property {Array<{id: string, definition: StateMachineDefinition}>} versions + * @property {string} orid + * @property {StateMachineDefinition} definition + */src/core/operations/task.test.ts (1)
58-76: Test looks good, but consider more specific error assertionsThis test correctly verifies exception propagation when the task's definition omits a catch handler, but could be more specific in error assertions.
Consider enhancing your assertions to verify the error is properly logged before propagating:
// Act & Assert await expect(task.run()).rejects.toThrow(mockInvokeError); + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to invoke resource'), + mockInvokeError + ); expect(mockInvokeFunction).toHaveBeenCalledWith( 'some-resource-uri', JSON.stringify(baseMetadata.input), );src/presentation/hooks/__tests__/validate-token.test.ts (1)
38-71: Verify debugging output in success caseThe test verifies that the token is set on the request object, but doesn't check if debug logging occurs.
Consider adding a debug property to the request mock and asserting it's called:
const request: { parsedToken: IdentityJwt | undefined; headers: Record<string, string>; + log: { + debug: jest.fn(); + }; } = { parsedToken: undefined, headers: { token: 'testToken' }, + log: { + debug: jest.fn(), + }, }; // After Act section + expect(request.log.debug).toHaveBeenCalledWith( + expect.stringContaining('Token validated successfully') + );Dockerfile (1)
18-19: Consider clarifying the commented gRPC proto lineThe commented line about copying gRPC proto files creates ambiguity about whether these files are needed.
Either uncomment the line if the protos are needed, or add a comment explaining why it's commented out:
COPY --from=builder /usr/src/app/dist . -# COPY --from=builder /usr/src/app/src/infrastructure/grpc/protos/*.proto ./infrastructure/grpc/protos/ +# Uncomment if gRPC functionality is enabled: +# COPY --from=builder /usr/src/app/src/infrastructure/grpc/protos/*.proto ./infrastructure/grpc/protos/src-old/workers/internal.js (1)
154-171: Potential burst of concurrent I/O – consider throttlingPromise.allon delayed ops.
Promise.all(allDelayed.map(async …))will fire all updates & queue writes in parallel.
IfallDelayedis large (e.g. thousands of ops after a long outage) MongoDB / the queue service may get hammered, increasing latency or causing throttling.Ideas:
- Chunk the array (e.g. batches of 100) and
awaiteach chunk sequentially.- Use a simple concurrency limiter such as
p-limit.This keeps memory use predictable and provides back-pressure to the datastore.
src/presentation/di-container-init.ts (2)
26-33:loggercan be registered as a value instead of a factory.The factory is executed once, but
asValue(server.log)is more explicit and skips an extra function allocation.-logger: asFunction( - () => { - return server.log; - }, - { lifetime: Lifetime.SINGLETON }, -), +logger: asValue(server.log),
77-78:Promise.resolve()is redundant – make the functionasyncor returnvoid.
diContainerInitperforms only synchronous registration; returning an already-resolved promise adds noise and may mislead callers into unnecessaryawaits.src/core/operations/wait.test.ts (1)
10-18: Reset mocks between tests to avoid state leakage.
jest.restoreAllMocks()only affects spies created withjest.spyOn.
mockRepo’s jest.fn() fields retain call history across tests, forcing you tomockReset()manually inside tests.Add a blanket reset in
afterEach:afterEach(() => { jest.useRealTimers(); jest.restoreAllMocks(); + Object.values(mockRepo).forEach((fn) => fn?.mockReset?.()); });This keeps each test independent and removes the need for ad-hoc
mockReset()calls.src/worker/index.test.ts (1)
124-126: Avoiddeleteonprocess.env; assignundefinedinsteadUsing the
deleteoperator onprocess.envincurs a de-opt in V8 and is flagged by most linters. Prefer assigningundefined, which semantically achieves the same result without the performance penalty.-delete process.env.PENDING_QUEUE_NAME; +process.env.PENDING_QUEUE_NAME = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/worker/entry-point.ts (1)
65-71: Unhandled rejections when creating missing queues
Promise.allSettledhides individual rejections which might mask mis-provisioned system queues.
Consider logging rejected promises so operational issues surface quickly.- await Promise.allSettled( + const results = await Promise.allSettled( queueNames.map(async (queueName) => { if (!existingQueues.find((queue) => queue.name === queueName)) { return client.createQueue(queueName); } return Promise.resolve(); }), ); + results + .filter((r) => r.status === 'rejected') + .forEach((r) => + logger.error({ err: (r as PromiseRejectedResult).reason }, `Failed to create queue`), + );src/worker/index.ts (1)
31-33:QUEUE_INTERVALparsing lacks validation
parseIntwill returnNaNfor non-numeric values, leading to an infinite loop withsetTimeout(NaN) → 0. Guard the result and fall back to a sane default.return Number.isNaN(Number(process.env.QUEUE_INTERVAL)) ? 1000 : parseInt(process.env.QUEUE_INTERVAL as string, 10);src/test-utilities.ts (1)
40-55: Provide typed Jest mocks to satisfy async return contractsSeveral repo methods (e.g.
listStateMachines,getExecution) are specified to returnPromise<…>but the currentjest.fn()default returnsundefined, which can cause tests to pass incorrectly or type-checking to degrade whenstrictmode is on.- listStateMachines: jest.fn(), + listStateMachines: jest.fn<jest.MockedFunction< + typeof mockRepo.listStateMachines + >>().mockResolvedValue([]),Doing this for each async method makes unit tests safer and gives clear, predictable defaults.
package.json (1)
9-15: Make scripts cross-platform by usingcross-envSetting
NODE_ENV=localdevdirectly works on *nix but breaks on Windows shells.- "dev:server": "NODE_ENV=localdev ts-node-dev --respawn --pretty --transpile-only src/presentation/server.ts", - "dev:worker": "NODE_ENV=localdev ts-node-dev --respawn --pretty --transpile-only src/worker/entry-point.ts", + "dev:server": "cross-env NODE_ENV=localdev ts-node-dev --respawn --pretty --transpile-only src/presentation/server.ts", + "dev:worker": "cross-env NODE_ENV=localdev ts-node-dev --respawn --pretty --transpile-only src/worker/entry-point.ts",Add
cross-envtodevDependenciesif not already present.src/presentation/index.ts (1)
34-48: Swagger plugins are loaded in production as well – consider guarding withNODE_ENV !== 'production'or a config default
enableSwaggeris fetched directly from configuration. If the value is accidentally left true in production you will expose an extra route surface (and a live OpenAPI UI) that might reveal implementation details.- if (config.get<boolean>('enableSwagger')) { + if (process.env.NODE_ENV !== 'production' && config.get<boolean>('enableSwagger')) {src/core/types/state-machine-definition.ts (2)
42-48:NextonWaitStateis also optionalA
Waitstate can be terminal (when followed byEnd: true). Consider:- Next: string; + Next?: string; + End?: boolean;
54-60: Export the union type and follow naming conventions
allStatesis camel-case and not exported, yet used outside this file. Rename toState(or similar) and export to improve DX.-type allStates = TaskState | ChoiceState | WaitState | FailState | SucceedState; +export type State = + | TaskState + | ChoiceState + | WaitState + | FailState + | SucceedState;src/presentation/controllers/v1/state-machine-controller.ts (2)
215-226: Nit: spelling in description
'Create a new execution fromm a state machine'→from.
271-279: 404 vs 400: ambiguous rider handlingIf the user omits the rider, the code returns 400 after parsing an invalid composite ORID.
Consider validating beforeoridV1.parseto avoid unnecessary parsing errors.src-old/repos/index.js (2)
33-52: Avoid duplicate camelCase / snake_case fields in API response.Lines 40-49 build an object that duplicates every property with two casings. This balloons payload size and invites consumers to pick the “wrong” key permanently.
Recommend returning only one canonical style and bumping the downstream SDKs in lock-step; alternatively remove the TODO if intentional.
171-188: Possible race when$addToSeton unindexedid.If two workers insert the same operation id concurrently there’s still a small window before the array is materialised where duplicates sneak in. Add a unique compound index on
operations.idor move operations to a dedicated collection withunique: trueindex.src/core/operations/task.ts (1)
47-84:Catchhandling only supportsStates.ALL.The spec also allows catching specific error names. The current loop ignores them, so any non-catch-all will act as an unhandled failure. Consider matching on the constructor name /
err.name.src/core/operations/choice.test.ts (3)
1-1: Unnecessary eslint disable.
/* eslint-disable jest/expect-expect */is no longer needed—the tests containexpectassertions. Removing it lets ESLint catch accidental missing expectations.
7-10:uuidmock could leak across suites.You clear mocks but never restore the original implementation. Safer to move the mock inside each test or add
jest.unmock('uuid')in anafterAll.
211-223: Large “TODO” test groups risk becoming stale.The “And / Or / Not operator” placeholders are valuable but easily forgotten. Consider creating skipped tests (
it.skip) to surface them in Jest output or filing issues to track.src/core/operations/wait.ts (2)
55-65: Consider extractinggetValueFromPathinto a shared utility and add basic type-safety
getValueFromPathis now duplicated (seechoice.ts). Moving it intosrc/core/utils/json-path.ts(or similar) avoids code drift and allows you to:
- Add unit tests once, not twice.
- Expose a narrow, typed contract – e.g.
getValueFromPath<T = unknown>(src: unknown, path: string): T | undefined.That also makes it easier later to support array indices (
$.foo[0]) or stricter error handling.
110-124: Potentially redundant recomputation ofwaitUntilUtcWhen
waitUntilUtcis not yet set we compute it and persist it.
On subsequent invocations (e.g. worker restarts) the branchopDetails.waitUntilUtc > nowis taken and we immediately call
delayOperation()again – even though the timestamp is unchanged.This is harmless but results in an unnecessary database round-trip per poll.
You could short-circuit when the value is already present:-if (!opDetails.waitUntilUtc || opDetails.waitUntilUtc > toEpoch(new Date())) +if (!opDetails.waitUntilUtc) + ... +else if (opDetails.waitUntilUtc > toEpoch(new Date())) ...src/core/operations/choice.ts (2)
7-18: Duplicate helper; extract & add array-index supportSame duplication comment as in
wait.ts; additionally, neither implementation supports paths with array indices ($.items[0]). Consolidating into one helper (see previous comment) will make that enhancement straightforward.
167-176:awaiting a non-promise may mask synchronous exceptions
this.#output = await this.#input;If
#inputis not aPromiseLike, wrapping it inawaitconverts sync exceptions into rejected promises, altering the stack trace and making debugging harder. Unless you intentionally support promises here, consider removingawait.src/infrastructure/repos/state-machine-repo-mongo.ts (2)
66-76:listStateMachinesreturns soft-deleted recordsYou set
isDeletedtotrueinremoveStateMachine, yetlistStateMachines()filters only byaccountId. Either filter them out:- .find({ accountId }) + .find({ accountId, isDeleted: false })or document why callers are expected to handle deleted items.
329-355:getDelayedOperationsperforms client-side filtering – exploit MongoDB projectionsThe nested loops can become expensive as the
executionscollection grows.
Using the positional$operator and an index onoperations.waitUntilUtclets Mongo return only the matching sub-documents:db.executions.find( { 'operations.waitUntilUtc': { $lte: waitUntilUtc }, 'operations.status': 'Waiting' }, { 'operations.$': 1 } )This removes the need for application-level iteration and yields smaller payloads.
src/core/interfaces/state-machine-repo.ts (3)
22-30: Consider using more specific types for input/output parameters.While using
unknownforinputandoutputprovides flexibility, it shifts type checking to runtime. If the structure of these fields follows patterns, consider using more specific types or generics.export type OperationData = { id: string; created: string; stateKey: string; status: Status; - input: unknown; - output: unknown | null; + input: Record<string, any>; // Or a more specific type if known + output: Record<string, any> | null; // Or a more specific type if known waitUntilUtc?: number; // EPOCH seconds };
32-39: Remove or address the TODO comment.The TODO comment on line 38 appears unnecessary as the operations field is already typed as
OperationData[].export type ExecutionData = { id: string; created: string; status: Status; stateMachine: string; // StateMachineId version: string; // VersionId - operations: OperationData[]; // TODO: define the type + operations: OperationData[]; };
1-104: Consider adding JSDoc comments for improved documentation.Adding JSDoc comments to interface methods and type definitions would make the code more maintainable and improve developer experience, especially for teams implementing this interface.
Example:
/** * Repository interface for managing state machines, executions, and operations. */ export interface StateMachineRepo { /** * Creates a new state machine for an account. * * @param accountId - The ID of the account that owns the state machine * @param definition - The state machine definition * @returns The created state machine data */ createStateMachine( accountId: string, definition: StateMachineDefinition, ): Promise<StateMachineData>; // Add JSDoc for other methods... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (105)
.dockerignore(1 hunks).envrc(0 hunks).eslintignore(1 hunks).eslintrc.js(2 hunks).gitignore(1 hunks).mds-log-pumprc(1 hunks).mocharc.json(0 hunks).nycrc.json(0 hunks)DEV_NOTES.md(1 hunks)Dockerfile(1 hunks)Dockerfile.old(1 hunks)Jenkinsfile(0 hunks)__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.ts(1 hunks)bin/entrypoint(3 hunks)config/default.js(1 hunks)config/test.js(1 hunks)docs/Data_Model.md(1 hunks)docs/fnProject/Getting_Started.md(0 hunks)envrc.sample(1 hunks)jest-reporters/emit-only-failures.js(1 hunks)jest.config.ts(1 hunks)other/postman/fnStateMachine.postman_collection.json(0 hunks)package.json(1 hunks)package.json-old(1 hunks)renovate.json(1 hunks)scripts/mysql-initial.sql(0 hunks)src-old/globals.js(1 hunks)src-old/handlers/app_shutdown.js(0 hunks)src-old/handlers/handler-helpers.js(1 hunks)src-old/handlers/index.js(3 hunks)src-old/handlers/index.test.js(3 hunks)src-old/operations/choice.js(2 hunks)src-old/operations/choice.test.js(6 hunks)src-old/operations/fail.js(1 hunks)src-old/operations/fail.test.js(5 hunks)src-old/operations/succeed.js(1 hunks)src-old/operations/succeed.test.js(5 hunks)src-old/operations/task.js(4 hunks)src-old/operations/task.test.js(6 hunks)src-old/operations/wait.js(1 hunks)src-old/repos/index.js(1 hunks)src-old/workers/internal.js(5 hunks)src-old/workers/internal.test.js(2 hunks)src/core/classes/logic.ts(1 hunks)src/core/errors/not-found.ts(1 hunks)src/core/interfaces/state-machine-repo.ts(1 hunks)src/core/operations/DEVELOPERS.md(1 hunks)src/core/operations/choice.test.ts(1 hunks)src/core/operations/choice.ts(1 hunks)src/core/operations/fail.test.ts(1 hunks)src/core/operations/fail.ts(1 hunks)src/core/operations/index.test.ts(1 hunks)src/core/operations/index.ts(1 hunks)src/core/operations/succeed.test.ts(1 hunks)src/core/operations/succeed.ts(1 hunks)src/core/operations/task.test.ts(1 hunks)src/core/operations/task.ts(1 hunks)src/core/operations/wait.test.ts(1 hunks)src/core/operations/wait.ts(1 hunks)src/core/types/health-check-result.ts(1 hunks)src/core/types/state-machine-definition.ts(1 hunks)src/infrastructure/repos/state-machine-repo-mongo.ts(1 hunks)src/presentation/__tests__/di-container-init.test.ts(1 hunks)src/presentation/__tests__/presentation.test.ts(1 hunks)src/presentation/controllers/__tests__/health-check-controller.test.ts(1 hunks)src/presentation/controllers/health-check-controller.ts(1 hunks)src/presentation/controllers/index.ts(1 hunks)src/presentation/controllers/v1/__tests__/state-machine-controller.test.ts(1 hunks)src/presentation/controllers/v1/index.ts(1 hunks)src/presentation/controllers/v1/state-machine-controller.ts(1 hunks)src/presentation/di-container-init.ts(1 hunks)src/presentation/fastify.ts(1 hunks)src/presentation/functions/__tests__/make-orid.test.ts(1 hunks)src/presentation/functions/make-orid.ts(1 hunks)src/presentation/hooks/__tests__/validate-can-access-orid-param.test.ts(1 hunks)src/presentation/hooks/__tests__/validate-request-orid-param.test.ts(1 hunks)src/presentation/hooks/__tests__/validate-token.test.ts(1 hunks)src/presentation/hooks/validate-can-access-orid-param.ts(1 hunks)src/presentation/hooks/validate-request-orid-param.ts(1 hunks)src/presentation/hooks/validate-token.ts(1 hunks)src/presentation/index.ts(1 hunks)src/presentation/routes/root-router.ts(1 hunks)src/presentation/routes/v1/v1-router.ts(1 hunks)src/presentation/schemas/health/get-health-response-schema.ts(1 hunks)src/presentation/schemas/health/index.ts(1 hunks)src/presentation/schemas/index.ts(1 hunks)src/presentation/schemas/stateMachines/create-state-machine-request-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/get-execution-response-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/index.ts(1 hunks)src/presentation/schemas/stateMachines/invoke-state-machine-request-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/invoke-state-machine-response-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/list-state-machines-response-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/update-state-machine-request-body-schema.ts(1 hunks)src/presentation/schemas/stateMachines/update-state-machine-response-body-schema.ts(1 hunks)src/presentation/server.ts(1 hunks)src/presentation/types/identity-jwt.ts(1 hunks)src/repos/index.js(0 hunks)src/repos/mysql.js(0 hunks)src/repos/sqlite3.js(0 hunks)src/test-utilities.ts(1 hunks)src/worker/entry-point.ts(1 hunks)src/worker/index.test.ts(1 hunks)src/worker/index.ts(1 hunks)tsconfig.build.json(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (11)
- .envrc
- src-old/handlers/app_shutdown.js
- docs/fnProject/Getting_Started.md
- .nycrc.json
- .mocharc.json
- other/postman/fnStateMachine.postman_collection.json
- Jenkinsfile
- src/repos/sqlite3.js
- src/repos/index.js
- src/repos/mysql.js
- scripts/mysql-initial.sql
🧰 Additional context used
🧬 Code Graph Analysis (20)
src/presentation/fastify.ts (2)
src/presentation/types/identity-jwt.ts (1)
IdentityJwt(3-5)src/core/classes/logic.ts (1)
Logic(7-92)
__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.ts (1)
src/test-utilities.ts (2)
mockQueueClient(8-27)mockFunctionsClient(29-38)
src/presentation/controllers/health-check-controller.ts (1)
src/presentation/schemas/health/get-health-response-schema.ts (1)
GetHealthResponseSchema(4-7)
src/core/operations/fail.test.ts (2)
src/core/types/state-machine-definition.ts (1)
FailState(50-50)src/test-utilities.ts (2)
mockRepo(40-55)mockLogger(57-66)
src/presentation/__tests__/di-container-init.test.ts (1)
src/presentation/di-container-init.ts (1)
diContainerInit(17-78)
src/core/operations/index.ts (2)
src/core/types/state-machine-definition.ts (4)
StateMachineDefinition(56-60)TaskState(11-15)ChoiceState(17-40)WaitState(42-48)src/core/interfaces/state-machine-repo.ts (1)
StateMachineRepo(41-104)
src/presentation/hooks/__tests__/validate-token.test.ts (1)
src/presentation/types/identity-jwt.ts (1)
IdentityJwt(3-5)
src-old/operations/wait.js (1)
src/core/operations/wait.ts (1)
computeWaitTimestamp(67-94)
src/core/operations/index.test.ts (2)
src/core/types/state-machine-definition.ts (1)
StateMachineDefinition(56-60)src/test-utilities.ts (2)
mockLogger(57-66)mockRepo(40-55)
src/worker/index.test.ts (2)
src/test-utilities.ts (3)
mockQueueClient(8-27)mockRepo(40-55)mockLogger(57-66)src/worker/index.ts (1)
Worker(25-301)
src/core/operations/succeed.test.ts (2)
src/core/types/state-machine-definition.ts (1)
SucceedState(52-52)src/test-utilities.ts (2)
mockRepo(40-55)mockLogger(57-66)
src-old/workers/internal.js (1)
src/worker/index.ts (1)
parseInt(31-33)
src/presentation/index.ts (3)
src/presentation/di-container-init.ts (1)
diContainerInit(17-78)src/presentation/routes/root-router.ts (1)
rootRouter(5-8)src/core/classes/logic.ts (1)
Logic(7-92)
src/presentation/hooks/validate-token.ts (1)
src/presentation/types/identity-jwt.ts (1)
IdentityJwt(3-5)
src/test-utilities.ts (1)
src/core/interfaces/state-machine-repo.ts (1)
StateMachineRepo(41-104)
src/core/classes/logic.ts (3)
src/core/interfaces/state-machine-repo.ts (1)
StateMachineRepo(41-104)src/core/types/state-machine-definition.ts (1)
StateMachineDefinition(56-60)src/core/errors/not-found.ts (1)
NotFound(1-1)
src/core/operations/succeed.ts (2)
src/core/interfaces/state-machine-repo.ts (1)
StateMachineRepo(41-104)src/core/types/state-machine-definition.ts (1)
SucceedState(52-52)
src/presentation/controllers/v1/state-machine-controller.ts (10)
src/presentation/hooks/validate-request-orid-param.ts (1)
validateRequestOridParam(4-33)src/presentation/hooks/validate-can-access-orid-param.ts (1)
validateCanAccessOridParam(4-31)src/presentation/schemas/stateMachines/list-state-machines-response-body-schema.ts (1)
ListStateMachinesResponseBodySchema(3-14)src/presentation/schemas/stateMachines/create-state-machine-request-body-schema.ts (2)
CreateStateMachineRequestBody(9-11)CreateStateMachineRequestBodySchema(3-7)src/presentation/schemas/stateMachines/update-state-machine-request-body-schema.ts (1)
UpdateStateMachineRequestBody(9-11)src/presentation/schemas/stateMachines/update-state-machine-response-body-schema.ts (1)
UpdateStateMachineResponseBodySchema(3-14)src/presentation/schemas/stateMachines/invoke-state-machine-request-body-schema.ts (2)
InvokeStateMachineRequestBody(5-7)InvokeStateMachineRequestBodySchema(3-3)src/presentation/schemas/stateMachines/invoke-state-machine-response-body-schema.ts (1)
InvokeStateMachineResponseBodySchema(3-5)src/core/errors/not-found.ts (1)
NotFound(1-1)src/presentation/schemas/stateMachines/get-execution-response-body-schema.ts (1)
GetExecutionResponseBodySchema(3-15)
src/core/operations/choice.ts (3)
src/core/operations/wait.ts (1)
getValueFromPath(55-65)src/core/types/state-machine-definition.ts (1)
ChoiceState(17-40)src/core/interfaces/state-machine-repo.ts (1)
StateMachineRepo(41-104)
src/core/interfaces/state-machine-repo.ts (1)
src/core/types/state-machine-definition.ts (1)
StateMachineDefinition(56-60)
🪛 markdownlint-cli2 (0.17.2)
src/core/operations/DEVELOPERS.md
1-1: Bare URL used
null
(MD034, no-bare-urls)
docs/Data_Model.md
1-1: Trailing punctuation in heading
Punctuation: '.'
(MD026, no-trailing-punctuation)
DEV_NOTES.md
38-38: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
40-40: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
44-44: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
46-46: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 Biome (1.9.4)
tsconfig.build.json
[error] 10-10: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
src-old/operations/wait.js
[error] 62-62: This aliasing of this is unnecessary.
Arrow functions inherits this from their enclosing scope.
Safe fix: Use this instead of an alias.
(lint/complexity/noUselessThisAlias)
src/worker/index.test.ts
[error] 124-124: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/worker/index.ts
[error] 25-301: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 44-44: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 52-52: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 77-77: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 86-86: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 98-98: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 105-105: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 106-106: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 110-110: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 115-115: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 122-122: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 124-124: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 130-130: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 133-133: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 141-141: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 147-147: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 152-152: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 154-154: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 167-167: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 171-171: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 172-172: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 174-174: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 188-188: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 196-196: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 204-204: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 205-205: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 205-205: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 206-206: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 211-211: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 222-222: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 233-233: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 234-234: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 234-234: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 235-235: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 240-240: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 241-241: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 264-264: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 268-268: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 277-277: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 278-278: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 280-280: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 282-282: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 283-283: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 286-286: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 294-294: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 298-298: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 298-298: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 298-298: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 299-299: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 299-299: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 299-299: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 ESLint
config/default.js
[error] 1-1: 'module' is not defined.
(no-undef)
jest-reporters/emit-only-failures.js
[error] 2-2: Definition for rule 'import/no-extraneous-dependencies' was not found.
(import/no-extraneous-dependencies)
[error] 4-4: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 4-4: 'require' is not defined.
(no-undef)
[error] 21-21: 'module' is not defined.
(no-undef)
src/presentation/controllers/v1/__tests__/state-machine-controller.test.ts
[error] 7-7: 'mockLogger' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 23-23: 'mockValidateToken' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
config/test.js
[error] 1-1: 'module' is not defined.
(no-undef)
.eslintrc.js
[error] 40-41: Delete ;⏎
(prettier/prettier)
src/presentation/controllers/v1/state-machine-controller.ts
[error] 15-15: 'UpdateStateMachineRequestBodySchema' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 46-46: 'reply' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 75-75: 'reply' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (93)
.eslintignore (1)
1-3: Verified ignore patterns.
The updated.eslintignorecorrectly excludes generateddist/files, ZIP archives, and legacysrc-old/code from linting.renovate.json (1)
1-12: Confirm Renovate rules accuracy.
The newrenovate.jsoncorrectly extendsconfig:baseand automerges minor, patch, pin, and digest updates for npm packages, while leaving major updates for manual review.tsconfig.json (1)
3-6: Verify base configuration extension.
Extending@tsconfig/node16/tsconfig.jsonensures consistent Node.js 16 settings. Thets-nodesection with"files": trueis appropriate for running TypeScript scripts directly.src/presentation/controllers/v1/index.ts (1)
1-1: Simple and effective barrel file pattern.This barrel file appropriately exports all functionality from the state machine controller, which promotes modularity and organized access to the v1 controllers.
src/presentation/controllers/index.ts (1)
1-1: Proper use of barrel pattern for health check controller.This export statement follows the same modular pattern as seen in other parts of the codebase, promoting consistent organization of controllers.
src/presentation/schemas/health/index.ts (1)
1-1: The export organization looks good.Simple and clean barrel file for exporting health-related schemas. This approach provides good code organization by centralizing exports.
src-old/handlers/handler-helpers.js (1)
7-7: Good refactoring of environment variable access.Replacing direct environment variable access with a centralized utility function improves configuration management, makes testing easier, and provides a consistent way to access environment variables throughout the application.
Also applies to: 11-11, 16-16
src/presentation/schemas/stateMachines/index.ts (1)
1-1: Export organization looks good.Clean barrel file approach for state machine schemas, consistent with the pattern used in other schema exports. This promotes good organization and simplifies imports.
.mds-log-pumprc (1)
1-8: Configuration looks good but consider documenting port requirements.The log pump configuration appropriately sets up Pino logging with Logstash HTTP output. The echo option is enabled for development visibility.
Verify that port 6002 is the standard port for your Logstash HTTP input and that it's properly documented elsewhere in the project.
src/core/types/health-check-result.ts (1)
1-5: Excellent health check status enum implementation.The enum has appropriate values that clearly communicate health status. The explicit string assignments ensure type safety and readability.
src/presentation/routes/v1/v1-router.ts (1)
1-6: Clean router implementation with appropriate async/await pattern.The router implementation follows best practices for Fastify plugin registration with the async/await pattern.
src-old/handlers/index.test.js (2)
10-10: Good improvement for test maintainabilityUsing
__filenamein the describe block instead of hardcoding the test description makes the test suite more maintainable and self-documenting.
66-66: Consistent ORID issuer valuesUpdating ORID strings to use 'testIssuer' matches the stubbed value returned by
getIssuer()(line 12), improving test consistency.Also applies to: 96-96
src-old/operations/fail.js (1)
21-26: Updated to match new repository interfaceThe addition of
executionIdas a parameter toupdateOperationproperly aligns with the repository interface changes as part of the migration to MongoDB.src-old/operations/task.test.js (3)
13-13: Improved test maintainabilityUsing
__filenamein the describe block makes the test suite more maintainable and self-documenting.
90-90: Updated test expectations to match new repository interfaceTests now correctly expect the
executionIdparameter inupdateOperationcalls, aligning with the updated repository interface for MongoDB.Also applies to: 140-144, 210-215, 282-286, 353-357
151-151: Improved object handling in testsChanged from JSON stringified forms to raw object forms in expected outputs and result assertions, which better aligns with how MongoDB handles object data compared to SQL databases.
Also applies to: 222-222, 293-293, 364-364, 165-165, 236-236, 307-307, 378-378
src-old/workers/internal.test.js (2)
113-113: Improved test case descriptionRenamed test to "updates operation as expected" which better describes the test's purpose.
139-139: Updated assertion to match new repository interfaceTest now correctly expects three parameters including 'testExecution' in the
updateOperationcall, aligning with the MongoDB repository implementation..dockerignore (1)
4-13: Docker build optimization looks good!The added entries properly exclude development, test, and IDE-specific files from the Docker build context. This ensures smaller, cleaner Docker images by preventing unnecessary files from being copied into the container.
src/presentation/routes/root-router.ts (1)
1-8: Clean router implementation with proper modularization!The router structure follows best practices for Fastify by:
- Using async/await with the register method
- Organizing routes with prefix-based modularization
- Separating health checks from versioned API endpoints
This approach provides a solid foundation for API versioning and maintainability.
src/presentation/__tests__/presentation.test.ts (1)
1-25: Well-structured test with proper lifecycle management!The test properly:
- Verifies the application's bootstrapping functionality
- Includes appropriate cleanup in afterEach and afterAll hooks
- Uses Jest idioms correctly
- Maintains type safety with FastifyInstance
This provides good coverage for the core application initialization.
tsconfig.build.json (1)
1-11: Good build configuration setupThe build-specific TypeScript configuration properly:
- Extends the base configuration
- Enables source maps for debugging
- Excludes test files from production builds
This is a good practice for maintaining separate configs for development and production builds.
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
src/presentation/fastify.ts (1)
7-14: LGTM: Well-structured type augmentation for FastifyThe module augmentation for Fastify follows TypeScript best practices, providing type safety for accessing the JWT token and logic service in request handlers.
This approach ensures request handler implementations have proper type checking for authentication data and service access, supporting the dependency injection pattern used throughout the application.
src-old/operations/succeed.test.js (5)
8-8: Good practice: Using dynamic test descriptionsUsing
__filenameinstead of a hardcoded string provides better test output and maintainability.
61-66: Updated parameter order for updateOperationThe test has been correctly updated to include the executionId as the second parameter to the updateOperation call, aligning with the repository method signature changes in the MongoDB implementation.
95-100: Consistent parameter updates across test casesThis test case has been correctly updated to match the new parameter structure for updateOperation.
129-134: Consistent parameter updates across test casesThis test case has been correctly updated to match the new parameter structure for updateOperation.
165-165: Consistent parameter updates for object input caseThe final test case has been correctly updated to include the executionId parameter.
__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.ts (1)
1-2:Details
✅ Verification successful
Verify import path correctness
The import path seems to go up two directory levels (
../../src/test-utilities) from the mock directory. Ensure this correctly resolves to the test-utilities file in your project structure.
🏁 Script executed:
#!/bin/bash # Verify the test-utilities file exists and is accessible from the mock path # Check if the test-utilities.ts file exists echo "Checking for test-utilities.ts file..." if find src -name "test-utilities.ts" | grep -q .; then echo "✅ Found test-utilities.ts file" find src -name "test-utilities.ts" else echo "❌ test-utilities.ts file not found" fi # Check relative path from mock directory to src directory echo "Checking relative path from mock directory..." MOCK_DIR=$(dirname "__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.ts") echo "Mock directory: $MOCK_DIR" echo "Path to src from mock directory would be: $MOCK_DIR/../../src"Length of output: 734
✅ Import path verified
The import path
../../src/test-utilitiesin__mocks__/@maddonkeysoftware/mds-cloud-sdk-node.tscorrectly resolves tosrc/test-utilities.ts. No changes are needed.src-old/operations/succeed.js (1)
20-26: Good change - Adding executionId parameter to updateOperationThe addition of the
executionIdparameter to theupdateOperationcall is consistent with the broader refactoring effort to include execution context in repository operations throughout the codebase.src/presentation/functions/make-orid.ts (1)
1-23: Well-structured ORID generator implementationThe
makeOridfunction is well-implemented with proper TypeScript typings, clear parameter definitions, and effective use of the external ORID generation library. The function correctly retrieves the provider key from configuration and creates ORIDs with a consistent format.src-old/operations/choice.js (3)
119-125: Good - Consistent use of executionId parameterThe addition of the
executionIdparameter to thisupdateOperationcall is consistent with the broader refactoring to include execution context in repository operations.
127-127: Good - Added executionId parameterProperly updated the
updateOperationcall to include theexecutionIdparameter.
142-142: Good - Added executionId parameter in error handlerCorrectly updated the
updateOperationcall in the catch block to include theexecutionIdparameter, ensuring consistent error handling.src/core/operations/fail.test.ts (2)
66-94: The test successfully verifies core Fail operation behavior.The test properly verifies that when a Fail operation runs, it correctly:
- Marks the operation as 'Succeeded'
- Marks the execution as 'Failed'
- Calls repository methods exactly once with the right parameters
This aligns with the expected behavior of a terminal Fail state in a state machine.
5-95: Well-structured tests with good coverage.The tests follow best practices by:
- Using the AAA (Arrange-Act-Assert) pattern with clear section comments
- Testing both positive cases and error conditions
- Verifying constructor validation for invalid inputs
- Properly mocking dependencies for isolated unit testing
- Using proper TypeScript typing throughout
This provides good test coverage for the Fail operation class.
src-old/handlers/index.js (3)
14-27: Good encapsulation by moving oridBase inside the function.Moving
oridBasefrom module scope to function scope improves encapsulation and prevents unintended modifications from other parts of the code.
71-71: Correct logger reference fixed.This change fixes an inconsistency by using the locally defined
loggervariable instead ofglobals.logger, ensuring consistent logging behavior.
139-139: Correctly updated repository call to include version information.The update to
repos.createExecution()adds themachine.activeVersionparameter, which is necessary to align with the updated repository interface. This change supports proper version tracking in executions.Dockerfile.old (1)
1-18:Details
❓ Verification inconclusive
Verify if Dockerfile.old is needed in the repository.
Since you're migrating to a new Dockerfile with a multi-stage build process, consider whether keeping this old version in the repository is necessary. If it's kept for historical reference, add a comment at the top explaining its purpose.
🏁 Script executed:
#!/bin/bash # Check if there are references to Dockerfile.old in the codebase or documentation rg -i "dockerfile\.old" --type-not dockerfileLength of output: 82
I’ll verify if
Dockerfile.oldis referenced anywhere in the repo by searching for its name (case-insensitive):
🏁 Script executed:
#!/bin/bash # Search for any references to Dockerfile.old in the entire codebase rg -ni 'dockerfile\.old'Length of output: 26
Please confirm the necessity of Dockerfile.old
I don’t see any references toDockerfile.oldin the codebase or docs. If you’re keeping it purely for historical context, add a brief comment at the top of the file explaining its purpose. Otherwise, consider removing it to avoid confusion.src/presentation/functions/__tests__/make-orid.test.ts (2)
5-19: Well-structured test for ORID generation without rider.The test clearly verifies that the ORID is generated in the correct format when no rider is provided. It follows best practices by using the AAA pattern and making explicit assertions.
21-39: Well-structured test for ORID generation with rider.The test properly verifies that the ORID correctly includes the rider parameter when provided. The assertion checks the exact format with the rider appended after a slash separator.
src-old/operations/task.js (5)
36-41: Repository method signature updateThe
updateOperationmethod now acceptsexecutionIdas a second parameter, which is consistent with the PR's goal of migrating to MongoDB and enhancing context tracking in operations.
57-57: Updated method signature for consistencyCorrectly updated this
updateOperationcall to include theexecutionIdparameter, maintaining consistency with other calls.
64-64: Changed output handlingThe result object is now used directly as the output rather than potentially being JSON stringified. This change aligns with MongoDB's ability to store complex objects directly.
67-72: Repository method signature updateProperly updated with
executionIdparameter.
98-102: Repository method signature updateCorrectly updated with
executionIdparameter.src/core/operations/succeed.test.ts (3)
1-4: Good test imports and setupThe test file properly imports the module under test and necessary dependencies, including mock objects for the repository and logger.
5-24: Test for invalid definition typeWell-structured test that verifies the correct error handling when an invalid definition type is provided.
26-64: Thorough null/undefined metadata handling testsGood coverage of edge cases with null and undefined metadata. The tests ensure robust error handling.
src/presentation/schemas/stateMachines/invoke-state-machine-response-body-schema.ts (1)
1-9: Well-defined schema with type exportsThe schema is correctly defined using TypeBox and properly exports both the schema object and a TypeScript type. This provides good type safety for the API response.
src-old/operations/fail.test.js (1)
61-66:Details
❓ Verification inconclusive
Modified parameter list looks good but consider migrating legacy tests.
The changes correctly add the execution ID as a second parameter to the
updateOperationcalls, which is consistent with the migration to MongoDB where execution context is explicitly required in repository methods.Note that this file is in the
src-olddirectory, suggesting it might be legacy code being maintained during a transition period. Do you have a plan to eventually migrate these tests to TypeScript and Jest to align with the new architecture?Also applies to: 95-100, 129-134, 165-165
🏁 Script executed:
#!/bin/bash # Check if there are any other test files in the src-old directory echo "Checking for test files in src-old directory..." find src-old -name "*.test.js" | sortLength of output: 530
Consider Migrating Legacy Tests to TypeScript & Jest
I see that we’re still maintaining a suite of 12
.test.jsfiles undersrc-old/(e.g.operations/fail.test.js,operations/choice.test.js,handlers/index.test.js, etc.). While the parameter-list update forupdateOperationcalls looks good:
- Tests correctly assert the new
executionIdparameter.- Coverage remains in place for all operation outcomes.
To keep our testing strategy aligned with the new architecture (TypeScript + Jest):
• Do you have a roadmap for gradually porting these legacy tests into the main test suite?
• Consider setting up atests/directory (or similar) and convertingsrc-old/**/*.test.jsto.test.tswith Jest, ensuring consistent fixtures and helpers.This will improve maintainability and allow us to retire the
src-old/folder once fully migrated.src/presentation/schemas/health/get-health-response-schema.ts (1)
1-9: Well-structured health check response schemaThe schema is well-designed with appropriate typing for health status responses. The use of TypeBox's
Statictype for deriving the TypeScript interface from the schema is a good practice that ensures runtime validation and compile-time type safety are in sync.config/default.js (1)
30-34: Document or provide default values for SDK configurationThe SDK configuration values are explicitly set to
undefined, which could cause runtime errors if these values are required but not provided through environment variables or other means.Consider either providing sensible defaults, documenting how these values should be provided in production, or adding runtime validation to check for these values before using them.
src/core/operations/index.test.ts (2)
12-45: Well-structured test data with comprehensive state typesThe test definition includes all supported state types (Task, Wait, Choice, Fail, Succeed) with appropriate configurations, providing good coverage for the
getOperationfunction.
53-76: Effective use of parameterized testsUsing
it.eachfor parameterized testing is an excellent approach that reduces code duplication while maintaining clear test cases for each state type..gitignore (3)
1-7: Well-structured log patterns for Node.js projects.The expanded log patterns follow best practices by including npm and yarn-specific log files. This will help keep the repository clean during development.
39-41: TypeScript declaration files properly excluded.Adding TypeScript-specific patterns aligns with the migration from JavaScript to TypeScript mentioned in the PR description.
65-69: Migration strategy reflected in ignore patterns.The addition of
old-src/,dist/, andconfig/local*.jsto the ignore list supports the gradual migration approach. This allows for maintaining legacy code while developing new TypeScript implementations.src-old/operations/choice.test.js (4)
9-9: Dynamic test description using filename.Using
__filenamein the describe block creates more maintainable tests, as the test description will automatically update if the file is renamed.
84-87: Updated function signature with executionId parameter.The
updateOperationfunction call now includes theexecutionIdparameter, which aligns with the repository interface changes mentioned in the PR summary.
136-136: Consistent application of updated function signature.The
updateOperationfunction call consistently includes theexecutionIdparameter throughout the codebase, maintaining compatibility with the new TypeScript interface.
233-275: Improved test organization for BooleanEquals operator.The refactored BooleanEquals operator tests now separate top-level and nested key matching into distinct test cases, improving test clarity and coverage. This refactoring makes tests more maintainable and errors easier to diagnose.
src/presentation/types/identity-jwt.ts (1)
1-12: Well-structured TypeScript interfaces for JWT handling.The interfaces provide strong typing for JWT tokens used in authentication, which improves type safety throughout the application. This is a good practice for TypeScript migration.
jest-reporters/emit-only-failures.js (1)
6-19: The Reporter class implementation is clean and elegant.This custom Jest reporter effectively suppresses console output for passing tests, making the test output more concise and focusing only on failures.
src/core/operations/succeed.ts (1)
5-37: Constructor implementation looks good.The class makes good use of TypeScript features:
- Private fields with the
#syntax- Proper validation of inputs
- Clear type definitions for parameters
The constructor correctly validates its inputs and initializes all necessary fields.
src/presentation/schemas/stateMachines/get-execution-response-body-schema.ts (1)
17-19: Excellent use of TypeScript type inference.The type alias using
Static<typeof Schema>is an elegant way to derive TypeScript types from the runtime schema definition.src/core/operations/fail.ts (1)
42-50: Verify if operation status should be 'Succeeded' in a Fail state.There's a potential inconsistency: in a Fail operation, the operation status is set to 'Succeeded' (line 46) while the execution is set to 'Failed' (line 49). This might be intentional if the operation itself succeeded in setting the execution to the failed state, but it's worth double-checking that this is the intended behavior.
jest.config.ts (3)
15-22: Strict coverage thresholds are great for project qualitySetting high coverage thresholds (90%) for all metrics is excellent for maintaining code quality throughout the migration to TypeScript and MongoDB.
28-33: Performance optimizations with thorough documentationGood job documenting the performance optimizations with links to relevant issues and StackOverflow discussions.
8-8:Details
✅ Verification successful
Documented hack for Jest-Prettier compatibility issue
The comment clearly indicates this is a temporary workaround for a known issue.
🌐 Web query:
Is Jest issue #14305 with Prettier still open?💡 Result:
Jest issue #14305, which tracks the incompatibility between Jest and Prettier v3 (specifically, Jest's failure to update or create inline snapshots when Prettier v3 is installed), is still open as of May 2025[1][4][2].
Prettier v3 is not yet supported by Jest, and the official Jest documentation explicitly states this limitation, offering workarounds such as setting
prettierPath: nullto disable Prettier integration, or using Prettier v2 specifically for Jest snapshot formatting[4]. Various projects and tools (such as skuba) have implemented their own workarounds while waiting for an official fix, confirming the issue remains unresolved[2].There has been discussion about resolving the problem in an upcoming major Jest release (Jest 30), but as of the latest available updates, native support for Prettier v3 has not yet been shipped, and the tracking issue remains active[1][4].
Citations:
- 1: jestjs/jest#14305
- 2: https://seek-oss.github.io/skuba/CHANGELOG.html
- 3: prettier/prettier#16930
- 4: https://jestjs.io/docs/configuration
- 5: https://www.benmvp.com/blog/prettier-eslint
- 6: https://kittygiraudel.com/2024/06/01/from-eslint-and-prettier-to-biome/
- 7: https://dev.to/teyim/effortless-testing-setup-for-react-with-vite-typescript-jest-and-react-testing-library-1c48
Keep Jest–Prettier workaround until upstream fix is released
The hack in jest.config.ts (line 8) is still required—Jest issue #14305 remains open and Jest does not yet support Prettier v3.
No changes needed; retainprettierPath: nullas a temporary workaround and remove it once Jest adds native Prettier v3 support.src/presentation/controllers/v1/__tests__/state-machine-controller.test.ts (3)
82-106: Comprehensive test for listing state machinesThis test thoroughly validates the functionality of the list machines endpoint, including appropriate HTTP status code and response format.
108-151: Well-structured tests for getting machine detailsThe tests cover both successful retrieval and 404 not found scenarios, which is excellent.
153-201: Complete test coverage for all controller endpointsThe tests thoroughly cover create, update, delete, invoke state machine, and get execution details endpoints, including both success and error cases.
Also applies to: 203-258, 260-296, 298-348, 350-412
src/core/operations/index.ts (1)
23-23: TODO comment should be addressedThere's a TODO comment about moving the function to the worker module.
Is moving this function to the worker module still planned? If so, you could either implement this change now or create a tracking issue to handle it in a future PR.
src/presentation/schemas/stateMachines/list-state-machines-response-body-schema.ts (1)
3-14: Well-structured schema with backward compatibilityThe schema correctly defines state machine objects with both camelCase and snake_case variations to ensure backward compatibility.
src-old/operations/wait.js (1)
67-70: Improved epoch time comparisonThe code correctly uses epoch time format for timestamp comparisons, which is more efficient and accurate than string-based ISO date comparisons.
src/core/operations/task.test.ts (1)
139-146: Test is good, but verify execution flow is working correctlyThe test verifies repository calls and return values, but doesn't explicitly check if the task execution flow is correct.
The test case is well-structured and thoroughly checks the important aspects of the Task operation. The assertions on repository calls and return values are comprehensive.
src/presentation/hooks/__tests__/validate-token.test.ts (2)
7-8: Good mock setup, ensure proper type safetyThe jest.mock setup looks good, but consider using more explicit typing with the jest.mocked utility.
193-199: Good error message validationThis test thoroughly validates the error response format for missing tokens, including status code, content type header, and response body.
The detailed verification of the error response ensures consistent error handling for missing tokens.
Dockerfile (3)
1-8: Good multi-stage build setupThe builder stage is well structured with proper dependency installation and build process.
Using a multi-stage build is a best practice to minimize the final image size by excluding build tools and dependencies.
15-15: Explicitly copying configuration is good practiceExplicitly copying the configuration file ensures it's available in the runtime image.
This approach is better than copying all files and helps keep the image smaller.
22-39: Excellent documentation of architecture and extension pointsThe comments clearly explain the architecture and provide examples for extending the command to ship logs.
These comments are valuable for understanding how to use and extend the container, especially for shipping logs to an ELK stack.
src/core/classes/logic.ts (1)
89-91:accountIdparameter is ignored – verify intention
getExecutionDetails(accountId, resourceId)never usesaccountId, so authorisation or multi-tenant filtering is bypassed. Either:
- Remove the unused parameter, or
- Call a repo method that validates the execution really belongs to
accountId.Please double-check the contract you need here.
src/presentation/index.ts (1)
72-76: Overwritingrequest.logmay drop bindings added by other pluginsCreating a child logger is great, but re-assigning may mask additional bindings (e.g.
reqId) that Fastify adds later. Prefer:- request.log = request.log.child({ traceId: request.headers['request-id'] }); + request.log = request.log.child({ + traceId: request.headers['request-id'], + // retain existing bindings automatically + });src-old/repos/index.js (2)
1-260: Legacy module should be removed or marked “deprecated”.Everything under
src-old/appears to be dead code after the TypeScript/Mongo refactor. Keeping it in the repository increases cognitive load and the risk of somebody importing the wrong implementation. Either delete the folder or, at minimum, move it behind a build-ignore / export-nothing façade.
[ suggest_essential_refactor ]
105-114: Soft-delete leaves TTL implementation incomplete.You set
removeAtbut the TTL index mentioned in the TODO is never created. Until the index exists, documents will live forever, defeating the purpose.Action: add a
createIndex({ removeAt: 1 }, { expireAfterSeconds : 0 })migration or in-code once-only initializer.src/core/operations/task.ts (1)
127-134: Serialising objects may double-encode JSON.You stringify any object, but
invokeFunctionmay itself JSON-stringify. Passing raw objects and letting the SDK handle encoding is safer unless the API contract explicitly expects a string.src/core/interfaces/state-machine-repo.ts (4)
3-13: Well-structured state machine data type with versioning support.The
StateMachineDatatype correctly models a state machine entity with versioning capabilities, allowing multiple definitions to coexist while maintaining an active version reference. This will enable proper version control and backward compatibility.
15-20: Good use of union type for status values.Using a union type for
Statusensures type safety by restricting values to only the valid statuses defined. This prevents bugs from typos or invalid status values.
60-61: LGTM for listStateMachines method.The method signature correctly returns a Promise of an array of state machine data objects filtered by account ID.
62-67: Well-designed repository methods with appropriate Promise return types.The repository methods provide a comprehensive API for managing state machines, executions, and operations with consistent naming and proper Promise-based return types for asynchronous operations.
Also applies to: 68-69, 70-71, 72-75, 76-82, 83-89, 90-95, 96-99, 101-104
|
|
||
| /** | ||
| * JS uses milliseconds where epoch is defined in seconds | ||
| */ | ||
| toEpoch: (ts) => Math.floor(ts.getTime() / 1000), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve documentation for toEpoch.
The function correctly converts Date objects to Unix epoch seconds, but it doesn't handle invalid inputs. Since this appears to be part of a legacy codebase (src-old), consider adding safeguards for robustness.
/**
- * JS uses milliseconds where epoch is defined in seconds
+ * Converts a JavaScript Date object to Unix epoch time (seconds since Jan 1, 1970)
+ * @param {Date} ts - The Date object to convert
+ * @returns {number} Unix epoch time in seconds
*/
-toEpoch: (ts) => Math.floor(ts.getTime() / 1000),
+toEpoch: (ts) => {
+ if (!(ts instanceof Date) || isNaN(ts.getTime())) {
+ throw new Error('Invalid Date object provided to toEpoch');
+ }
+ return Math.floor(ts.getTime() / 1000);
+},📝 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.
| /** | |
| * JS uses milliseconds where epoch is defined in seconds | |
| */ | |
| toEpoch: (ts) => Math.floor(ts.getTime() / 1000), | |
| /** | |
| * Converts a JavaScript Date object to Unix epoch time (seconds since Jan 1, 1970) | |
| * @param {Date} ts - The Date object to convert | |
| * @returns {number} Unix epoch time in seconds | |
| */ | |
| toEpoch: (ts) => { | |
| if (!(ts instanceof Date) || isNaN(ts.getTime())) { | |
| throw new Error('Invalid Date object provided to toEpoch'); | |
| } | |
| return Math.floor(ts.getTime() / 1000); | |
| }, |
| - Remove dangling images | ||
|
|
||
| ``` | ||
| docker rmi $(docker image ls -f dangling=true -q --no-trunc) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety checks to Docker cleanup commands.
The current commands might fail or produce errors if no matching images are found. Consider adding safety checks.
For the first command:
-docker rmi $(docker image ls -f dangling=true -q --no-trunc)
+docker image ls -f dangling=true -q --no-trunc | xargs -r docker rmiFor the second command:
-docker image ls | grep mds-sf- | awk '{print $3}' | xargs docker rmi
+docker image ls | grep mds-sf- | awk '{print $3}' | xargs -r docker rmiThe -r flag for xargs prevents it from running the command if there are no input arguments.
Also applies to: 47-47
| { | ||
| "compilerOptions": { | ||
| "sourceMap": true | ||
| }, | ||
| "extends": "./tsconfig.json", | ||
| "exclude": [ | ||
| "node_modules", | ||
| "**/*.test.ts", | ||
| "**/*.spec.ts", | ||
| ] | ||
| } |
There was a problem hiding this comment.
Fix JSON syntax error with trailing comma
There's a syntax error in your tsconfig.build.json file - the last item in the exclude array has an unnecessary trailing comma, which is invalid in JSON.
"exclude": [
"node_modules",
"**/*.test.ts",
- "**/*.spec.ts",
+ "**/*.spec.ts"
]📝 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.
| { | |
| "compilerOptions": { | |
| "sourceMap": true | |
| }, | |
| "extends": "./tsconfig.json", | |
| "exclude": [ | |
| "node_modules", | |
| "**/*.test.ts", | |
| "**/*.spec.ts", | |
| ] | |
| } | |
| { | |
| "compilerOptions": { | |
| "sourceMap": true | |
| }, | |
| "extends": "./tsconfig.json", | |
| "exclude": [ | |
| "node_modules", | |
| "**/*.test.ts", | |
| "**/*.spec.ts" | |
| ] | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Expected an array, an object, or a literal but instead found ']'.
Expected an array, an object, or a literal here.
(parse)
| # Remove this comment, rename file to ".envrc", and update the relevant passwords | ||
| export MDS_SM_PROVIDER_KEY=mdsCloud | ||
| export FN_SM_DB_URL="mongodb://dbuser:somepass@127.0.0.1:27017" | ||
| export ORID_PROVIDER_KEY="mdsCloud" | ||
| export PENDING_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-pendingQueue" | ||
| export IN_FLIGHT_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-inFlightQueue" | ||
| export MDS_IDENTITY_URL="http://127.0.0.1:8079" | ||
| export MDS_SM_QS_URL="http://127.0.0.1:8083" | ||
| export MDS_SM_SF_URL="http://127.0.0.1:8085" | ||
| export MDS_SM_SYS_USER="mdsCloud" | ||
| export MDS_SM_SYS_ACCOUNT="1" | ||
| export MDS_SM_SYS_PASSWORD="sysPass" | ||
| export APP_PORT="8086" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security guidance for sensitive environment variables
While the file provides a good template for required environment variables, it contains default passwords that could pose security risks if not properly updated. Consider adding more detailed instructions about security best practices and the purpose of each variable.
-# Remove this comment, rename file to ".envrc", and update the relevant passwords
+# SECURITY IMPORTANT: Rename this file to ".envrc", update ALL passwords/credentials below with strong values
+# Never commit the actual .envrc file containing real credentials to version control
+
+# Provider and service configuration
export MDS_SM_PROVIDER_KEY=mdsCloud
-export FN_SM_DB_URL="mongodb://dbuser:somepass@127.0.0.1:27017"
+export FN_SM_DB_URL="mongodb://dbuser:STRONG_PASSWORD_HERE@127.0.0.1:27017"
export ORID_PROVIDER_KEY="mdsCloud"
+
+# Queue configuration
export PENDING_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-pendingQueue"
export IN_FLIGHT_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-inFlightQueue"
+
+# Service URLs
export MDS_IDENTITY_URL="http://127.0.0.1:8079"
export MDS_SM_QS_URL="http://127.0.0.1:8083"
export MDS_SM_SF_URL="http://127.0.0.1:8085"
+
+# System user credentials - MUST be updated for production use
export MDS_SM_SYS_USER="mdsCloud"
export MDS_SM_SYS_ACCOUNT="1"
-export MDS_SM_SYS_PASSWORD="sysPass"
+export MDS_SM_SYS_PASSWORD="STRONG_PASSWORD_HERE"
+
+# Application configuration
export APP_PORT="8086"📝 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.
| # Remove this comment, rename file to ".envrc", and update the relevant passwords | |
| export MDS_SM_PROVIDER_KEY=mdsCloud | |
| export FN_SM_DB_URL="mongodb://dbuser:somepass@127.0.0.1:27017" | |
| export ORID_PROVIDER_KEY="mdsCloud" | |
| export PENDING_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-pendingQueue" | |
| export IN_FLIGHT_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-inFlightQueue" | |
| export MDS_IDENTITY_URL="http://127.0.0.1:8079" | |
| export MDS_SM_QS_URL="http://127.0.0.1:8083" | |
| export MDS_SM_SF_URL="http://127.0.0.1:8085" | |
| export MDS_SM_SYS_USER="mdsCloud" | |
| export MDS_SM_SYS_ACCOUNT="1" | |
| export MDS_SM_SYS_PASSWORD="sysPass" | |
| export APP_PORT="8086" | |
| # SECURITY IMPORTANT: Rename this file to ".envrc", update ALL passwords/credentials below with strong values | |
| # Never commit the actual .envrc file containing real credentials to version control | |
| # Provider and service configuration | |
| export MDS_SM_PROVIDER_KEY=mdsCloud | |
| export FN_SM_DB_URL="mongodb://dbuser:STRONG_PASSWORD_HERE@127.0.0.1:27017" | |
| export ORID_PROVIDER_KEY="mdsCloud" | |
| # Queue configuration | |
| export PENDING_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-pendingQueue" | |
| export IN_FLIGHT_QUEUE_NAME="orid:1:mdsCloud:::1:qs:mds-sm-inFlightQueue" | |
| # Service URLs | |
| export MDS_IDENTITY_URL="http://127.0.0.1:8079" | |
| export MDS_SM_QS_URL="http://127.0.0.1:8083" | |
| export MDS_SM_SF_URL="http://127.0.0.1:8085" | |
| # System user credentials - MUST be updated for production use | |
| export MDS_SM_SYS_USER="mdsCloud" | |
| export MDS_SM_SYS_ACCOUNT="1" | |
| export MDS_SM_SYS_PASSWORD="STRONG_PASSWORD_HERE" | |
| # Application configuration | |
| export APP_PORT="8086" |
| async (request, response) => { | ||
| response.status(200); | ||
| response.send({ | ||
| serverStatus: HealthCheckResult.OK, | ||
| dbStatus: HealthCheckResult.INDETERMINATE, | ||
| }); | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Database status is hardcoded as INDETERMINATE
The health check returns a hardcoded database status rather than performing an actual check.
Consider implementing a real database connection check to provide a meaningful health status. The repository could be injected into the controller to perform this check:
export function healthCheckController(
app: FastifyInstance,
opts: FastifyPluginOptions,
+ repo: StateMachineRepo,
done: (err?: Error) => void,
) {
// ...
async (request, response) => {
+ let dbStatus = HealthCheckResult.INDETERMINATE;
+ try {
+ // Perform a lightweight DB operation to verify connection
+ await repo.someLightweightOperation();
+ dbStatus = HealthCheckResult.OK;
+ } catch (error) {
+ dbStatus = HealthCheckResult.ERROR;
+ }
response.status(200);
response.send({
serverStatus: HealthCheckResult.OK,
- dbStatus: HealthCheckResult.INDETERMINATE,
+ dbStatus,
});
},
// ...
}📝 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.
| async (request, response) => { | |
| response.status(200); | |
| response.send({ | |
| serverStatus: HealthCheckResult.OK, | |
| dbStatus: HealthCheckResult.INDETERMINATE, | |
| }); | |
| }, | |
| export function healthCheckController( | |
| app: FastifyInstance, | |
| opts: FastifyPluginOptions, | |
| repo: StateMachineRepo, | |
| done: (err?: Error) => void, | |
| ) { | |
| // ... | |
| async (request, response) => { | |
| // perform a real check instead of hard-coding | |
| let dbStatus = HealthCheckResult.INDETERMINATE; | |
| try { | |
| // Perform a lightweight DB operation to verify connection | |
| await repo.someLightweightOperation(); | |
| dbStatus = HealthCheckResult.OK; | |
| } catch (error) { | |
| dbStatus = HealthCheckResult.ERROR; | |
| } | |
| response.status(200); | |
| response.send({ | |
| serverStatus: HealthCheckResult.OK, | |
| dbStatus, | |
| }); | |
| }, | |
| // ... | |
| } |
| computeWaitTimestamp() { | ||
| let ts; | ||
| if (this.#seconds) { | ||
| ts = new Date(); | ||
| ts.setSeconds(ts.getSeconds() + Number(this.#seconds)); | ||
| } | ||
|
|
||
| if (!ts && this.#secondsPath) { | ||
| ts = new Date(); | ||
| ts.setSeconds( | ||
| ts.getSeconds() + | ||
| Number(Wait.getValueFromPath(this.#input, this.#secondsPath)), | ||
| ); | ||
| } | ||
|
|
||
| if (!ts && this.#timestamp) { | ||
| ts = new Date(this.#timestamp); | ||
| } | ||
|
|
||
| if (!ts && this.#timestampPath) { | ||
| ts = new Date( | ||
| String(Wait.getValueFromPath(this.#input, this.#timestampPath)), | ||
| ); | ||
| } | ||
|
|
||
| if (!ts) throw new Error('Could not compute timestamp.'); | ||
| return toEpoch(ts); | ||
| } |
There was a problem hiding this comment.
Validate dynamic values when computing the wait timestamp
computeWaitTimestamp() silently coerces any value found via SecondsPath/TimestampPath into a number / Date.
If the JSONPath resolves to undefined, null, or an unparsable string, Number(undefined) becomes NaN, yielding an “Invalid Date” that is quietly accepted until the final toEpoch call throws.
A quick guard prevents difficult-to-trace runtime errors:
+const secs = Number(Wait.getValueFromPath(this.#input, this.#secondsPath));
+if (Number.isNaN(secs)) {
+ throw new Error(`SecondsPath ${this.#secondsPath} did not resolve to a number`);
+}
ts.setSeconds(ts.getSeconds() + secs);Similar validation for Timestamp and TimestampPath would improve robustness.
Committable suggestion skipped: line range outside the PR's diff.
| await Promise.all([ | ||
| this.#repo.updateOperation( | ||
| this.#operationId, | ||
| this.#executionId, | ||
| 'Failed', | ||
| undefined, | ||
| ), | ||
| this.#repo.updateExecution(this.#executionId, 'Failed'), | ||
| ]); | ||
| } | ||
|
|
||
| return { | ||
| nextOpId, | ||
| output: this.#output, | ||
| next: nextStateKey, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return object is misleading when the choice fails
When no branch matches, the operation is marked Failed, yet the method still resolves to
{ nextOpId, output: this.#output, next: nextStateKey /* undefined */ }Down-stream callers might treat the presence of nextOpId as success.
Recommend returning null (similar to the Wait op) or throwing to make the failure explicit.
| switch (operation) { | ||
| case 'BooleanEquals': | ||
| case 'StringEquals': | ||
| case 'NumericEquals': | ||
| case 'TimestampEquals': | ||
| return getValueFromPath(input, choice.Variable) === choice[operation]; | ||
|
|
||
| case 'NumericGreaterThan': | ||
| case 'StringGreaterThan': | ||
| case 'TimestampGreaterThan': | ||
| // @ts-expect-error Porting code from node and will revisit once time permits | ||
| return getValueFromPath(input, choice.Variable) > choice[operation]; | ||
|
|
||
| case 'NumericGreaterThanEquals': | ||
| case 'StringGreaterThanEquals': | ||
| case 'TimestampGreaterThanEquals': | ||
| // @ts-expect-error Porting code from node and will revisit once time permits | ||
| return getValueFromPath(input, choice.Variable) >= choice[operation]; | ||
|
|
||
| case 'NumericLessThan': | ||
| case 'StringLessThan': | ||
| case 'TimestampLessThan': | ||
| // @ts-expect-error Porting code from node and will revisit once time permits | ||
| return getValueFromPath(input, choice.Variable) < choice[operation]; | ||
|
|
||
| case 'NumericLessThanEquals': | ||
| case 'StringLessThanEquals': | ||
| case 'TimestampLessThanEquals': | ||
| // @ts-expect-error Porting code from node and will revisit once time permits | ||
| return getValueFromPath(input, choice.Variable) <= choice[operation]; | ||
|
|
||
| default: | ||
| throw Error(`Condition ${operation} not yet implemented.`); | ||
| } |
There was a problem hiding this comment.
Missing implementation for And, Or, and Not leads to runtime failure
performTest()’s switch never handles the logical operators yet #processChoice includes them in the evaluation set.
If a state machine definition uses And, Or, or Not, the code will throw Condition <op> not yet implemented. and mark the execution as failed.
At minimum, short-circuit unsupported operators before invoking performTest, or (preferably) implement them:
-case 'And':
-// TODO implement
+case 'And':
+ return (choice.And ?? []).every((sub) =>
+ Object.entries(sub).every(([op]) =>
+ performTest(op as Operations, sub as any, input),
+ ),
+ );Failing fast during constructor validation (detecting unsupported operators) would also improve DX.
Committable suggestion skipped: line range outside the PR's diff.
| get #mongoClient() { | ||
| try { | ||
| return this.#mongoClientInternal | ||
| .connect() | ||
| .then(() => this.#mongoClientInternal); | ||
| } catch (err) { | ||
| this.#logger.warn(err, 'Error connecting to mongo'); | ||
| throw new Error('Error connecting to mongo'); | ||
| } | ||
| } |
There was a problem hiding this comment.
MongoClient.connect() is executed on every call – memoise the promise
MongoClient.connect() is not idempotent; calling it repeatedly spawns new connections and can throw “MongoClient must be connected before…” errors. Cache the promise the first time:
- get #mongoClient() {
- try {
- return this.#mongoClientInternal.connect().then(() => this.#mongoClientInternal);
- } catch (err) {
+ #connectionPromise?: Promise<MongoClient>;
+ get #mongoClient() {
+ if (!this.#connectionPromise) {
+ this.#connectionPromise = this.#mongoClientInternal.connect();
+ }
+ return this.#connectionPromise.catch((err) => {
this.#logger.warn(err, 'Error connecting to mongo');
throw new Error('Error connecting to mongo');
- }
+ });
}📝 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.
| get #mongoClient() { | |
| try { | |
| return this.#mongoClientInternal | |
| .connect() | |
| .then(() => this.#mongoClientInternal); | |
| } catch (err) { | |
| this.#logger.warn(err, 'Error connecting to mongo'); | |
| throw new Error('Error connecting to mongo'); | |
| } | |
| } | |
| #connectionPromise?: Promise<MongoClient>; | |
| get #mongoClient() { | |
| if (!this.#connectionPromise) { | |
| this.#connectionPromise = this.#mongoClientInternal.connect(); | |
| } | |
| return this.#connectionPromise.catch((err) => { | |
| this.#logger.warn(err, 'Error connecting to mongo'); | |
| throw new Error('Error connecting to mongo'); | |
| }); | |
| } |
| resourceId: string, | ||
| ): Promise<StateMachineData | null>; | ||
|
|
||
| removeStateMachine(accountId: string, resourceId: string): unknown; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent return type for removeStateMachine method.
Other repository methods consistently return Promise-based types, but removeStateMachine returns unknown. Consider updating it to return a Promise for consistency.
- removeStateMachine(accountId: string, resourceId: string): unknown;
+ removeStateMachine(accountId: string, resourceId: string): Promise<void>;📝 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.
| removeStateMachine(accountId: string, resourceId: string): unknown; | |
| removeStateMachine(accountId: string, resourceId: string): Promise<void>; |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
src/core/operations/task.test.ts (1)
79-129: 🛠️ Refactor suggestionRename 'temp' test to clearly indicate its purpose
The test name 'temp' is non-descriptive and suggests temporary code. Since this test verifies error handling with a catch block, rename it to something more descriptive like 'should catch exceptions and transition to the catch state'.
- it('temp', async () => { + it('should catch exceptions and transition to the catch state', async () => {src/worker/index.ts (3)
302-304:⚠️ Potential issue
#delaypromises are fired-and-forgotten – unhandled rejections propagateIf either
#processMessagesor#enqueueDelayedMessagesthrows synchronously, the resulting promise rejection is unobserved, potentially terminating the Node process in future versions. Capture errors to keep the worker alive.-this.#delay(this.#queueInterval).then(() => this.#processMessages()); -this.#delay(this.#queueInterval).then(() => this.#enqueueDelayedMessages()); +this.#delay(this.#queueInterval) + .then(() => this.#processMessages()) + .catch((err) => this.#logger.error({ err }, 'processMessages loop failed')); +this.#delay(this.#queueInterval) + .then(() => this.#enqueueDelayedMessages()) + .catch((err) => this.#logger.error({ err }, 'enqueueDelayedMessages loop failed'));🧰 Tools
🪛 Biome (1.9.4)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
228-229:⚠️ Potential issueWrong fallback label: copy-paste typo
The fallback string references
PENDING_QUEUE_NAMEeven though we are enqueuing to the in-flight queue. This will confuse error triage.-process.env.IN_FLIGHT_QUEUE_NAME || - 'PENDING_QUEUE_NAME environment variable not set', +process.env.IN_FLIGHT_QUEUE_NAME || + 'IN_FLIGHT_QUEUE_NAME environment variable not set',
173-177:⚠️ Potential issueDeleting the queue message before processing risks message loss
If
#processMessagethrows afterdeleteMessage, the work item is irretrievably lost, breaking at-least-once delivery guarantees.Move
deleteMessageafter successful processing (or implement a visibility-timeout style pattern).-const message = await this.#queueClient.fetchMessage(queueOrid); -if (message) { - // TODO: Deleting the message here may not be the best idea. - // Entertain moving it to processMessage. - await this.#queueClient.deleteMessage(queueOrid, message.id); - void this.#processMessage(message); +const message = await this.#queueClient.fetchMessage(queueOrid); +if (message) { + try { + await this.#processMessage(message); + await this.#queueClient.deleteMessage(queueOrid, message.id); + } catch (err) { + this.#logger.error({ err, message }, 'Failed to process message'); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 176-176: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🧹 Nitpick comments (6)
src/core/operations/task.test.ts (4)
81-81: Avoid unnecessary type assertionThe type assertion
as unknown as TaskStateis redundant here since baseDefinition is already typed as TaskState.- const definition = baseDefinition as unknown as TaskState; + const definition = baseDefinition;
132-141: Add type annotation to inputMapper functionThe inputMapper function lacks explicit return type annotation, which would improve type safety and readability.
- const inputMapper = (value: unknown) => { + const inputMapper = (value: unknown): string | null | undefined => {
155-155: Consider testing more complex response structuresThe current test only verifies a simple response object. Consider adding tests with more complex nested response structures to ensure proper handling of various response formats.
// Example of a test with complex response structure mockInvokeFunction.mockResolvedValue({ userResult: { nested: { data: [1, 2, 3], metadata: { timestamp: new Date().toISOString(), source: 'test' } } } });
1-202: Add test for ResultPath functionalityThe AWS Step Functions Task state supports a ResultPath parameter that determines where in the state input the result is placed. Consider adding tests that verify this functionality if it's supported by your Task implementation.
it('should apply ResultPath when specified', async () => { // Arrange const definition = { ...baseDefinition, ResultPath: '$.taskResult', } as TaskState; mockInvokeFunction.mockResolvedValue({ result: 'value' }); (v4 as jest.Mock).mockReturnValue('nextOpId'); const task = new Task(definition, baseMetadata, mockRepo, mockLogger); // Act const result = await task.run(); // Assert expect(mockRepo.updateOperation).toHaveBeenCalledWith( baseMetadata.id, baseMetadata.execution, 'Succeeded', { someData: 'value', taskResult: { result: 'value' } } ); // Additional assertions... });src/worker/index.ts (1)
294-298: Consider adding MongoDB client optionsThere's a TODO comment about potentially needed MongoDB client options. While the current implementation might work, some options could improve reliability and performance.
Consider adding these commonly used MongoDB options:
new StateMachineRepoMongo({ mongoClient: new MongoClient(config.get<string>('mongo.url'), { - // TODO: Do we need any options here? - // useNewUrlParser: true, - // useUnifiedTopology: true, + connectTimeoutMS: 5000, + socketTimeoutMS: 30000, + maxPoolSize: 10, + retryWrites: true }), logger: this.#logger, });src/core/classes/__tests__/logic.test.ts (1)
235-276: LGTM! Proper test for default queue name fallbackThe test correctly verifies that invokeStateMachine uses a default queue name when the environment variable is not set.
However, use
process.env.PENDING_QUEUE_NAME = undefinedinstead ofdeletefor better performance.-// Remove env var to test default -delete process.env.PENDING_QUEUE_NAME; +// Remove env var to test default +process.env.PENDING_QUEUE_NAME = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 264-264: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/commit-checks.yml(1 hunks)src/core/classes/__tests__/logic.test.ts(1 hunks)src/core/operations/index.test.ts(1 hunks)src/core/operations/index.ts(1 hunks)src/core/operations/task.test.ts(1 hunks)src/infrastructure/repos/__tests__/state-machine-repo-mongo.test.ts(1 hunks)src/test-utilities.ts(1 hunks)src/worker/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/core/operations/index.test.ts
- src/test-utilities.ts
- src/core/operations/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/core/classes/__tests__/logic.test.ts (4)
src/core/classes/logic.ts (1)
Logic(7-92)src/test-utilities.ts (2)
mockQueueClient(8-28)mockRepo(41-56)src/core/types/state-machine-definition.ts (1)
StateMachineDefinition(56-60)src/core/errors/not-found.ts (1)
NotFound(1-1)
src/infrastructure/repos/__tests__/state-machine-repo-mongo.test.ts (4)
src/infrastructure/repos/state-machine-repo-mongo.ts (1)
StateMachineRepoMongo(62-356)src/test-utilities.ts (1)
mockLogger(58-67)src/core/interfaces/state-machine-repo.ts (3)
StateMachineData(3-13)Status(15-20)ExecutionData(32-39)src/core/types/state-machine-definition.ts (1)
StateMachineDefinition(56-60)
🪛 Biome (1.9.4)
src/core/classes/__tests__/logic.test.ts
[error] 264-264: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
src/worker/index.ts
[error] 25-306: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 44-44: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 52-52: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 100-100: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 107-107: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 108-108: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 113-113: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 125-125: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 127-127: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 133-133: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 136-136: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 144-144: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 155-155: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 156-156: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 159-159: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 172-172: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 176-176: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 179-179: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 193-193: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 201-201: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 209-209: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 211-211: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 221-221: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 227-227: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 238-238: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 239-239: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 239-239: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 240-240: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 245-245: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 246-246: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 269-269: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 273-273: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 282-282: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 283-283: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 287-287: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 288-288: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 291-291: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 299-299: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🪛 actionlint (1.7.4)
.github/workflows/commit-checks.yml
14-14: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (33)
src/core/operations/task.test.ts (3)
143-149: Great test coverage for different input typesThe test suite thoroughly covers various input scenarios including undefined, null, string, and object inputs. This comprehensive approach ensures that the Task class handles inputs correctly across different data types.
169-174: Good verification of operation state transitionsThe test properly verifies that the operation state transitions from 'Executing' to 'Succeeded', which is essential for tracking the lifecycle of operations in the state machine.
Also applies to: 175-182
183-192: Good verification of next operation creationThe test correctly validates that a new operation is created with the expected ID, execution context, state name, and output data. This ensures proper state transitions in the state machine.
.github/workflows/commit-checks.yml (1)
18-18: LGTM! Test script update for new test frameworkThe script update from
test-covtotest:covaligns with the migration from Mocha to Jest.src/core/classes/__tests__/logic.test.ts (11)
1-7: LGTM! Well-structured test imports and setupThe imports are well-organized, including the core Logic class, required types, and test utilities.
8-18: LGTM! Appropriate mocking of external dependenciesThe mocking of UUID and the MdsSdk correctly isolates the test from external dependencies.
20-38: LGTM! Good test setup with before/after hooksThe test setup is clean, with appropriate initialization of test data and cleanup after tests.
40-52: LGTM! Clear test for listStateMachines methodThis test effectively verifies that the listStateMachines method calls the repository with the correct arguments and returns the expected result.
54-79: LGTM! Comprehensive test for createStateMachine methodThe test properly validates that createStateMachine passes the correct arguments to the repository and returns the expected result.
81-112: LGTM! Well-structured test for updateStateMachine methodThe test thoroughly checks that updateStateMachine correctly passes all parameters to the repository and returns the expected result.
114-130: LGTM! Clear verification of getStateMachine methodThe test effectively verifies that getStateMachine passes the correct arguments to the repository and returns the expected result.
132-147: LGTM! Proper test for removeStateMachine methodThe test correctly verifies that removeStateMachine calls the repository with the expected arguments and returns the expected result.
149-213: LGTM! Thorough test for invokeStateMachine success pathThe test comprehensively verifies the success path for invokeStateMachine, including:
- UUID generation
- State machine retrieval
- Execution creation
- Operation creation
- Message enqueuing
- Expected result
215-233: LGTM! Good error handling test for invokeStateMachineThe test properly verifies that invokeStateMachine throws a NotFound error when the state machine doesn't exist and doesn't call any other methods.
279-292: LGTM! Clear test for getExecutionDetails methodThe test effectively verifies that getExecutionDetails calls the repository with the correct arguments and returns the expected result.
src/infrastructure/repos/__tests__/state-machine-repo-mongo.test.ts (18)
1-12: LGTM! Well-structured importsThe imports cover all necessary dependencies for testing MongoDB operations, including mocking of external dependencies like UUID and Luxon.
13-19: LGTM! Proper mocking of external dependenciesThe mocking of the config module, UUID, and Luxon packages isolates the tests from external dependencies.
20-74: LGTM! Comprehensive test setupThe test setup is thorough, with proper mocking of MongoDB client, logger, and collections. The beforeEach hook ensures a clean test environment for each test.
76-100: LGTM! Effective test for listStateMachinesThe test correctly verifies that listStateMachines passes the right query parameters to MongoDB and returns the expected results.
102-148: LGTM! Thorough test for createStateMachineThe test comprehensively verifies that createStateMachine correctly assembles the document and inserts it into MongoDB with proper parameters.
150-185: LGTM! Good test coverage for getStateMachineThe tests cover both successful retrieval and not-found scenarios for getStateMachine, with proper assertions.
187-251: LGTM! Comprehensive test for updateStateMachineThe test thoroughly verifies that updateStateMachine correctly handles document updates with MongoDB operators like $set and $addToSet.
253-275: LGTM! Proper test for removeStateMachineThe test verifies that removeStateMachine correctly marks a state machine as deleted with a future removal date.
277-299: LGTM! Clear test for createExecutionThe test effectively verifies that createExecution correctly assembles the execution document with proper initial values.
301-317: LGTM! Straightforward test for updateExecutionThe test correctly verifies that updateExecution updates only the status field with the right MongoDB update operator.
319-352: LGTM! Good test coverage for getExecutionThe tests cover both successful retrieval and not-found scenarios for getExecution, with proper assertions.
354-446: LGTM! Comprehensive tests for getStateMachineDefinitionForExecutionThese tests thoroughly verify:
- Successful retrieval of a state machine definition
- Error handling when execution is not found
- Error handling when version is not found
448-480: LGTM! Proper test for createOperationThe test correctly verifies that createOperation updates an execution document by adding an operation with the right structure and initial values.
482-507: LGTM! Clear test for updateOperationThe test verifies that updateOperation correctly updates both status and output fields using MongoDB's positional operator.
509-528: LGTM! Proper test for delayOperationThe test effectively verifies that delayOperation sets both the Waiting status and the waitUntilUtc timestamp correctly.
530-591: LGTM! Comprehensive tests for getOperationThese tests thoroughly verify:
- Successful retrieval of an operation with execution context
- Error handling when execution is not found
- Error handling when operation is not found
593-683: LGTM! Thorough tests for getDelayedOperationsThe tests cover both:
- Successful filtering of operations that are waiting and past their wait time
- Proper handling when no delayed operations are found
685-694: LGTM! Simple test for handleAppShutdownThe test effectively verifies that handleAppShutdown correctly closes the MongoDB connection.
| { | ||
| someData: 'value', | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify error details in repository update
The test is checking that the operation is marked as failed, but it's not verifying that any error details are included in the repository update. This is important for debugging failed executions.
expect(mockRepo.updateOperation).toHaveBeenCalledWith(
baseMetadata.id,
baseMetadata.execution,
'Failed',
{
someData: 'value',
+ error: expect.stringContaining('Failed to invoke resource')
},
);📝 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.
| { | |
| someData: 'value', | |
| }, | |
| ); | |
| expect(mockRepo.updateOperation).toHaveBeenCalledWith( | |
| baseMetadata.id, | |
| baseMetadata.execution, | |
| 'Failed', | |
| { | |
| someData: 'value', | |
| error: expect.stringContaining('Failed to invoke resource') | |
| }, | |
| ); |
| static async #handleOpCompleted( | ||
| operationId: string, | ||
| executionId: string, | ||
| runData: any, // TODO: Define runData type |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define type for runData parameter
The TODO comment indicates a pending task to define a proper type for the runData parameter.
Based on the usage in the code, consider defining a type for runData:
type RunData = {
output?: unknown;
nextOpId?: string;
next?: boolean;
};Then update the function signature:
- static async #handleOpCompleted(
- operationId: string,
- executionId: string,
- runData: any, // TODO: Define runData type
- ) {
+ static async #handleOpCompleted(
+ operationId: string,
+ executionId: string,
+ runData: RunData | null,
+ ) {| export class Worker { | ||
| static #queueClient: QueueServiceClient; | ||
| static #stateMachineRepo: StateMachineRepo; | ||
| static #running: boolean = false; | ||
| static #logger: BaseLogger; | ||
|
|
||
| static get #queueInterval(): number { | ||
| return parseInt(process.env.QUEUE_INTERVAL ?? '1000', 10); | ||
| } | ||
|
|
||
| static #delay(timeout: number) { | ||
| return new Promise((resolve) => { | ||
| setTimeout(() => { | ||
| resolve(null); | ||
| }, timeout); | ||
| }); | ||
| } | ||
|
|
||
| static handleAppShutdown() { | ||
| this.#running = false; | ||
| } | ||
|
|
||
| static async #handleOpCompleted( | ||
| operationId: string, | ||
| executionId: string, | ||
| runData: any, // TODO: Define runData type | ||
| ) { | ||
| this.#logger.trace( | ||
| { operationId, executionId, runData: runData || 'N/A' }, | ||
| 'Handling operation completed', | ||
| ); | ||
|
|
||
| if (runData) { | ||
| const { output, nextOpId, next } = runData; | ||
| this.#logger.trace({ operationId, output }, 'Operation completed'); | ||
| try { | ||
| await this.#stateMachineRepo.updateOperation( | ||
| operationId, | ||
| executionId, | ||
| 'Succeeded', | ||
| output, | ||
| ); | ||
|
|
||
| if (next) { | ||
| const message = { executionId, operationId: nextOpId }; | ||
| await this.#queueClient.enqueueMessage( | ||
| process.env.IN_FLIGHT_QUEUE_NAME ?? | ||
| 'IN_FLIGHT_QUEUE_NAME environment variable not set', | ||
| message, | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| /* istanbul ignore next */ | ||
| this.#logger.warn({ err }, 'Set up of next operation failed'); | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| static async #buildOperationDataBundle(metadata: ExecutionMetadata) { | ||
| const definition = | ||
| await this.#stateMachineRepo.getStateMachineDefinitionForExecution( | ||
| metadata.execution, | ||
| ); | ||
| return { metadata, definition }; | ||
| } | ||
|
|
||
| static async #invokeOperation(data: { | ||
| metadata: ExecutionMetadata; | ||
| definition: StateMachineDefinition | null; | ||
| }) { | ||
| const { definition, metadata } = data; | ||
| /* istanbul ignore if */ | ||
| if (!definition) { | ||
| this.#logger.warn({ metadata }, 'No definition found for operation id'); | ||
| return; | ||
| } | ||
|
|
||
| const t = getOperation( | ||
| definition, | ||
| metadata, | ||
| this.#logger, | ||
| this.#stateMachineRepo, | ||
| ); | ||
|
|
||
| /* istanbul ignore if */ | ||
| if (!t) { | ||
| this.#logger.warn({ metadata }, 'No operation found for operation id'); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await this.#stateMachineRepo.updateOperation( | ||
| metadata.id, | ||
| metadata.execution, | ||
| 'Executing', | ||
| undefined, | ||
| ); | ||
| const runData = await t.run(); | ||
| await this.#handleOpCompleted(metadata.id, metadata.execution, runData); | ||
| } catch (err) { | ||
| this.#logger.warn({ err }, 'Failed to execute operation'); | ||
| } | ||
| } | ||
|
|
||
| static async #processMessage(message: { message: string }) { | ||
| try { | ||
| this.#logger.trace('Processing message'); | ||
| const event = JSON.parse(message.message); | ||
| if (event.fromInvoke) { | ||
| await this.#stateMachineRepo.updateExecution( | ||
| event.executionId, | ||
| 'Executing', | ||
| ); | ||
| } | ||
|
|
||
| // TODO: Check to see if execution is cancelled. | ||
|
|
||
| const operation = await this.#stateMachineRepo.getOperation( | ||
| event.operationId, | ||
| event.executionId, | ||
| ); | ||
|
|
||
| /* istanbul ignore if */ | ||
| if (!operation) { | ||
| this.#logger.warn({ message }, 'Operation not found'); | ||
| return; | ||
| } | ||
|
|
||
| const data = await this.#buildOperationDataBundle(operation); | ||
| return this.#invokeOperation(data); | ||
| } catch (err) { | ||
| /* istanbul ignore next */ | ||
| this.#logger.warn({ err }, 'process event failed'); | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| static async #pullMessageFromQueue( | ||
| queueOrid: string, | ||
| limit: number, | ||
| runningData: RunningData | undefined, | ||
| ): Promise<RunningData> { | ||
| const data: RunningData = runningData || { count: 0, needMore: false }; | ||
| try { | ||
| if (data.count < limit) { | ||
| const message = await this.#queueClient.fetchMessage(queueOrid); | ||
| if (message) { | ||
| // TODO: Deleting the message here may not be the best idea. | ||
| // Entertain moving it to processMessage. | ||
| await this.#queueClient.deleteMessage(queueOrid, message.id); | ||
| void this.#processMessage(message); | ||
| data.count += 1; | ||
| return this.#pullMessageFromQueue(queueOrid, limit, data); | ||
| } | ||
| data.needMore = true; | ||
| } | ||
| } catch (err) { | ||
| // TODO: MdsSdk does not handle the service not being available gracefully | ||
| // For now we ignore errors. Once this is resolved we should remove this try catch. | ||
| data.needMore = true; | ||
| } | ||
|
|
||
| return data; | ||
| } | ||
|
|
||
| static async #processMessages() { | ||
| const data = await this.#pullMessageFromQueue( | ||
| process.env.IN_FLIGHT_QUEUE_NAME || | ||
| 'IN_FLIGHT_QUEUE_NAME environment variable not set', | ||
| BATCH_PROCESSING_SIZE, | ||
| undefined, | ||
| ); | ||
|
|
||
| if (data.needMore) { | ||
| await this.#pullMessageFromQueue( | ||
| process.env.PENDING_QUEUE_NAME || | ||
| 'PENDING_QUEUE_NAME environment variable not set', | ||
| BATCH_PROCESSING_SIZE, | ||
| data, | ||
| ); | ||
| } | ||
|
|
||
| if (this.#running) { | ||
| await this.#delay(this.#queueInterval); | ||
| this.#processMessages(); | ||
| } | ||
| } | ||
|
|
||
| static async #enqueueDelayedMessages() { | ||
| const allDelayed = await this.#stateMachineRepo.getDelayedOperations( | ||
| toEpoch(new Date()), | ||
| ); | ||
| await Promise.all( | ||
| allDelayed.map(async (delayed) => { | ||
| await this.#stateMachineRepo.updateOperation( | ||
| delayed.id, | ||
| delayed.execution, | ||
| 'Pending', | ||
| undefined, | ||
| ); | ||
| await this.#queueClient.enqueueMessage( | ||
| process.env.IN_FLIGHT_QUEUE_NAME || | ||
| 'PENDING_QUEUE_NAME environment variable not set', | ||
| { | ||
| executionId: delayed.execution, | ||
| operationId: delayed.id, | ||
| }, | ||
| ); | ||
| }), | ||
| ); | ||
|
|
||
| if (this.#running) { | ||
| await this.#delay(this.#queueInterval); | ||
| this.#enqueueDelayedMessages(); | ||
| } | ||
| } | ||
|
|
||
| static enqueueMessage(message: unknown) { | ||
| this.#logger.trace({ message }, 'Enqueuing message'); | ||
| return this.#queueClient.enqueueMessage( | ||
| process.env.PENDING_QUEUE_NAME || | ||
| 'PENDING_QUEUE_NAME environment variable not set', | ||
| message, | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Initializes required items and starts the workers internal processes. | ||
| * @param args - Optional arguments used to override defaults for testing. | ||
| * @param args.queueClient - Optional queue client to use. | ||
| * @param args.stateMachineRepo - Optional state machine repo to use. | ||
| * @param args.logger - Optional logger to use. | ||
| */ | ||
| static async startWorker({ | ||
| queueClient, | ||
| stateMachineRepo, | ||
| logger, | ||
| }: { | ||
| queueClient?: QueueServiceClient; | ||
| stateMachineRepo?: StateMachineRepo; | ||
| logger?: BaseLogger; | ||
| } = {}) { | ||
| if (this.#running) { | ||
| return; | ||
| } | ||
|
|
||
| this.#logger = | ||
| logger ?? | ||
| pino({ | ||
| ...config.get<{ | ||
| level: string; | ||
| mixin: (obj: object) => object; | ||
| }>('workerOptions.logger'), | ||
| }); | ||
|
|
||
| this.#logger.info('Starting worker'); | ||
| this.#running = true; | ||
| try { | ||
| this.#queueClient = queueClient || (await MdsSdk.getQueueServiceClient()); | ||
| } catch (err) { | ||
| this.#logger.error({ err }, 'Failed to initialize queue client'); | ||
| this.#running = false; | ||
| return; | ||
| } | ||
| this.#stateMachineRepo = | ||
| stateMachineRepo ?? | ||
| new StateMachineRepoMongo({ | ||
| mongoClient: new MongoClient(config.get<string>('mongo.url'), { | ||
| // TODO: Do we need any options here? | ||
| // useNewUrlParser: true, | ||
| // useUnifiedTopology: true, | ||
| }), | ||
| logger: this.#logger, | ||
| }); | ||
|
|
||
| // TODO: Start processing | ||
| this.#delay(this.#queueInterval).then(() => this.#processMessages()); | ||
| this.#delay(this.#queueInterval).then(() => this.#enqueueDelayedMessages()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to avoid static-only class
This class is implemented entirely with static methods and properties, which goes against TypeScript best practices. Static-only classes are generally discouraged as they make testing harder, reduce code reusability, and prevent dependency injection through normal class instantiation.
Consider refactoring this to a regular class with instance methods:
-export class Worker {
- static #queueClient: QueueServiceClient;
- static #stateMachineRepo: StateMachineRepo;
- static #running: boolean = false;
- static #logger: BaseLogger;
+export class Worker {
+ #queueClient: QueueServiceClient;
+ #stateMachineRepo: StateMachineRepo;
+ #running: boolean = false;
+ #logger: BaseLogger;
- static get #queueInterval(): number {
+ get #queueInterval(): number {
return parseInt(process.env.QUEUE_INTERVAL ?? '1000', 10);
}
- static #delay(timeout: number) {
+ #delay(timeout: number) {
return new Promise((resolve) => {
setTimeout(() => {
resolve(null);
}, timeout);
});
}
- static handleAppShutdown() {
+ handleAppShutdown() {
this.#running = false;
}
// Continue refactoring other methods...
- static async startWorker({
+ async startWorker({
queueClient,
stateMachineRepo,
logger,
}: {
queueClient?: QueueServiceClient;
stateMachineRepo?: StateMachineRepo;
logger?: BaseLogger;
} = {}) {
// ...
}
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 25-306: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 44-44: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 52-52: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 61-61: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 70-70: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 78-78: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 100-100: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 107-107: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 108-108: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 113-113: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 125-125: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 127-127: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 133-133: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 136-136: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 144-144: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 151-151: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 155-155: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 156-156: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 159-159: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 172-172: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 176-176: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 179-179: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 193-193: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 201-201: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 209-209: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 210-210: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 211-211: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 216-216: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 221-221: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 227-227: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 238-238: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 239-239: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 239-239: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 240-240: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 245-245: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 246-246: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 269-269: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 273-273: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 282-282: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 283-283: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 285-285: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 287-287: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 288-288: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 291-291: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 299-299: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 303-303: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 304-304: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| // TODO: Deleting the message here may not be the best idea. | ||
| // Entertain moving it to processMessage. | ||
| await this.#queueClient.deleteMessage(queueOrid, message.id); | ||
| void this.#processMessage(message); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use await instead of void for proper error propagation
Using void with an async method call ignores all errors and doesn't block execution. This makes debugging harder and might cause unexpected behavior.
-void this.#processMessage(message);
+await this.#processMessage(message).catch(err =>
+ this.#logger.error({ err, message }, 'Failed to process message')
+);📝 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.
| void this.#processMessage(message); | |
| await this.#processMessage(message).catch(err => | |
| this.#logger.error({ err, message }, 'Failed to process message') | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 177-177: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
| - name: Use Node 18.x | ||
| uses: actions/setup-node@v2 | ||
| with: | ||
| node-version: '14.x' | ||
| node-version: '18.x' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update GitHub Actions setup-node action to a newer version
The runner for "actions/setup-node@v2" is outdated according to GitHub Actions standards.
Update to a newer version of the action:
- - name: Use Node 18.x
- uses: actions/setup-node@v2
+ - name: Use Node 18.x
+ uses: actions/setup-node@v3
with:
node-version: '18.x'📝 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.
| - name: Use Node 18.x | |
| uses: actions/setup-node@v2 | |
| with: | |
| node-version: '14.x' | |
| node-version: '18.x' | |
| - name: Use Node 18.x | |
| uses: actions/setup-node@v3 | |
| with: | |
| node-version: '18.x' |
🧰 Tools
🪛 actionlint (1.7.4)
14-14: the runner of "actions/setup-node@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
|
Many comments here will be addressed/re-addressed when the refactor happens after the next release. |
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores
Tests
Revert