Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: add Nest + NATS example #55

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Conversation

martin-aliverti
Copy link
Contributor

@martin-aliverti martin-aliverti commented Oct 2, 2023

What's this PR do?

This MR adds an example showcasing the use of NestJS alongside with NATS.

@ulises-jeremias @matiasz8

Summary by CodeRabbit

  • New Feature: Added a NestJS REST-based microservices project with NATS for inter-service communication.
  • New Feature: Implemented three services (service-a, service-b, service-c) each with their own Dockerfile and configuration.
  • New Feature: Service-a includes two resources (resource1 and resource2) with full CRUD operations.
  • New Feature: Service-b listens to events from service-a and logs them.
  • New Feature: Service-c responds to 'get_version' and 'get_hello' requests.
  • Documentation: Detailed README files added for the overall project and individual services, providing setup instructions, usage examples, and more.
  • Test: End-to-end tests and unit tests added for controllers and services in all three services.
  • Chore: ESLint and Prettier configurations added for code linting and formatting.

@coderabbitai
Copy link

coderabbitai bot commented Oct 2, 2023

Walkthrough

This pull request introduces a NestJS REST-based microservices project with NATS. It includes the setup of three services, each with its own Dockerfile, ESLint, and Prettier configurations. The services communicate using both event-based and request-response based communication. Each service has its own controllers, services, and modules, and they are tested using unit and end-to-end tests.

Changes

File(s) Summary
.../service-[a-c]/Dockerfile Adds Dockerfiles for each service, specifying Node.js as the base image and setting up the working directory, dependencies, and start command.
.../service-[a-c]/.eslintrc.js, .../service-[a-c]/.prettierrc Adds ESLint and Prettier configurations for each service to enforce consistent coding styles.
.../service-[a-c]/src/app.controller.ts, .../service-[a-c]/src/app.service.ts Adds AppController and AppService classes for each service, handling HTTP requests and business logic respectively.
.../service-[a-c]/src/main.ts Sets up the bootstrap function for each service, creating an instance of the application and listening on a specific port.
.../service-a/src/resource[1-2]/... Adds controllers, DTOs, entities, events, modules, and services for two resources in service A, including methods for creating, finding, updating, and removing resources.
.../service-b/src/incomingEvents/Resource1Created.event.ts Adds an event handler for when a resource of type 1 is created in service B.
.../service-c/src/app.controller.ts Adds methods to handle 'get_version' and 'get_hello' message patterns in service C.
.../service-[a-c]/README.md Adds README files for each service, providing setup instructions, running commands, and contact information.
.../service-[a-c]/test/app.e2e-spec.ts Adds end-to-end tests for the root endpoint of each service.

🐇 "In the land of code, where logic is king,
A rabbit hopped forth, changes to bring.
With Docker and Nest, a microservice was born,
And with NATS it spoke, from dusk until morn.
Through controllers and services, it danced and it twirled,
Whispering softly, 'Hello, World!'" 🌍


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.
  • Additionally, enter @coderabbitai pause to halt the bot's ongoing reviews as any issue or review comment. To
    resume reviews, enter @coderabbitai resume as any issue or review comment.

@github-actions
Copy link

github-actions bot commented Oct 2, 2023

Warnings
⚠️ This PR is changing more than 200 lines.
⚠️ This PR is changing more than 10 files.
Messages
📖 Thanks for updating docs! We ❤️ documentation!
📖

Thanks for updating tests! Only YOU can prevent production fires. 🔥🌲🐻

Generated by 🚫 dangerJS against 8909988

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 23

Commits Files that changed from the base of the PR and between 5b7bd93 and 872d5a1.
Files ignored due to filter (20)
  • examples/nest-nats-microservices/docker-compose.yaml
  • examples/nest-nats-microservices/images/logo1.svg
  • examples/nest-nats-microservices/service-a/nest-cli.json
  • examples/nest-nats-microservices/service-a/package.json
  • examples/nest-nats-microservices/service-a/test/jest-e2e.json
  • examples/nest-nats-microservices/service-a/tsconfig.build.json
  • examples/nest-nats-microservices/service-a/tsconfig.json
  • examples/nest-nats-microservices/service-a/yarn.lock
  • examples/nest-nats-microservices/service-b/nest-cli.json
  • examples/nest-nats-microservices/service-b/package.json
  • examples/nest-nats-microservices/service-b/test/jest-e2e.json
  • examples/nest-nats-microservices/service-b/tsconfig.build.json
  • examples/nest-nats-microservices/service-b/tsconfig.json
  • examples/nest-nats-microservices/service-b/yarn.lock
  • examples/nest-nats-microservices/service-c/nest-cli.json
  • examples/nest-nats-microservices/service-c/package.json
  • examples/nest-nats-microservices/service-c/test/jest-e2e.json
  • examples/nest-nats-microservices/service-c/tsconfig.build.json
  • examples/nest-nats-microservices/service-c/tsconfig.json
  • examples/nest-nats-microservices/service-c/yarn.lock
Files selected for processing (52)
  • examples/nest-nats-microservices/README.md (1 hunks)
  • examples/nest-nats-microservices/service-a/.eslintrc.js (1 hunks)
  • examples/nest-nats-microservices/service-a/.gitignore (1 hunks)
  • examples/nest-nats-microservices/service-a/.prettierrc (1 hunks)
  • examples/nest-nats-microservices/service-a/Dockerfile (1 hunks)
  • examples/nest-nats-microservices/service-a/README.md (1 hunks)
  • examples/nest-nats-microservices/service-a/src/app.controller.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/app.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/app.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/app.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/main.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/dto/create-resource1.dto.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/dto/update-resource1.dto.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/entities/resource1.entity.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/events/created.event.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.controller.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/dto/create-resource2.dto.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/dto/update-resource2.dto.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/entities/resource2.entity.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.controller.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.service.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/test/app.e2e-spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/.eslintrc.js (1 hunks)
  • examples/nest-nats-microservices/service-b/.gitignore (1 hunks)
  • examples/nest-nats-microservices/service-b/.prettierrc (1 hunks)
  • examples/nest-nats-microservices/service-b/Dockerfile (1 hunks)
  • examples/nest-nats-microservices/service-b/README.md (1 hunks)
  • examples/nest-nats-microservices/service-b/src/app.controller.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/app.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/app.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/app.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/incomingEvents/Resource1Created.event.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/main.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/test/app.e2e-spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/.eslintrc.js (1 hunks)
  • examples/nest-nats-microservices/service-c/.gitignore (1 hunks)
  • examples/nest-nats-microservices/service-c/.prettierrc (1 hunks)
  • examples/nest-nats-microservices/service-c/Dockerfile (1 hunks)
  • examples/nest-nats-microservices/service-c/README.md (1 hunks)
  • examples/nest-nats-microservices/service-c/src/app.controller.spec.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/app.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/app.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/app.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/main.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/test/app.e2e-spec.ts (1 hunks)
Files skipped from review due to trivial changes (27)
  • examples/nest-nats-microservices/README.md
  • examples/nest-nats-microservices/service-a/.eslintrc.js
  • examples/nest-nats-microservices/service-a/.gitignore
  • examples/nest-nats-microservices/service-a/.prettierrc
  • examples/nest-nats-microservices/service-a/Dockerfile
  • examples/nest-nats-microservices/service-a/README.md
  • examples/nest-nats-microservices/service-a/src/app.module.ts
  • examples/nest-nats-microservices/service-a/src/main.ts
  • examples/nest-nats-microservices/service-a/src/resource1/dto/update-resource1.dto.ts
  • examples/nest-nats-microservices/service-a/src/resource1/entities/resource1.entity.ts
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.spec.ts
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.ts
  • examples/nest-nats-microservices/service-a/src/resource2/dto/create-resource2.dto.ts
  • examples/nest-nats-microservices/service-a/src/resource2/entities/resource2.entity.ts
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.service.spec.ts
  • examples/nest-nats-microservices/service-b/.eslintrc.js
  • examples/nest-nats-microservices/service-b/.gitignore
  • examples/nest-nats-microservices/service-b/.prettierrc
  • examples/nest-nats-microservices/service-b/README.md
  • examples/nest-nats-microservices/service-b/src/app.module.ts
  • examples/nest-nats-microservices/service-b/test/app.e2e-spec.ts
  • examples/nest-nats-microservices/service-c/.eslintrc.js
  • examples/nest-nats-microservices/service-c/.gitignore
  • examples/nest-nats-microservices/service-c/.prettierrc
  • examples/nest-nats-microservices/service-c/README.md
  • examples/nest-nats-microservices/service-c/src/app.module.ts
  • examples/nest-nats-microservices/service-c/src/app.service.ts
Additional comments (Suppressed): 10
examples/nest-nats-microservices/service-a/src/resource1/events/created.event.ts (1)
  • 1-9: The Resource1Created class is well defined and follows the standard pattern for defining events in a microservices architecture using NATS. The event has a version (v) which is good for handling changes to the event structure over time. The pattern constant is used to identify the event type, and the data property holds the payload of the event. This code is clean, maintainable, and adheres to best practices.
examples/nest-nats-microservices/service-a/src/app.controller.ts (1)
  • 1-12: The new hunk introduces a getVersion() method in the AppController class which returns the version of the application by calling the getVersion() method from the AppService. This is a good practice as it separates concerns between the controller and service. The controller is only responsible for handling the HTTP request and response, while the service contains the business logic.
examples/nest-nats-microservices/service-a/src/resource2/dto/update-resource2.dto.ts (1)
  • 1-4: The UpdateResource2Dto class extends PartialType(CreateResource2Dto), which means it will have all the properties of CreateResource2Dto but they will be optional. This is a good practice for update DTOs as it allows partial updates to the resource.
examples/nest-nats-microservices/service-a/test/app.e2e-spec.ts (1)
  • 1-24: The end-to-end test for the root endpoint in service-a looks good. It correctly sets up a testing module, initializes an application instance, and tests the GET request to the root endpoint. The expected status code and response are also correctly specified.
examples/nest-nats-microservices/service-a/src/resource2/resource2.controller.ts (1)
  • 20-22: The id parameter is being converted to a number using the unary plus operator (+id). This could lead to unexpected behavior if non-numeric strings are passed in. If id is expected to be a string, there's no need for conversion. If it should be a number, consider adding error handling to ensure that only valid numbers are processed.
examples/nest-nats-microservices/service-c/Dockerfile (1)
  • 1-6: The Dockerfile seems to be well structured and follows best practices. However, it's important to note that running the application in development mode (yarn start:dev) inside a Docker container might not be ideal for production environments due to potential performance issues and lack of optimization. Consider creating a separate Dockerfile for production or using a multi-stage build to compile your TypeScript code into JavaScript and run the compiled code instead.
- CMD ["yarn", "start:dev"]
+ CMD ["yarn", "start:prod"]

Also, ensure that the start:prod script is defined in your package.json.

examples/nest-nats-microservices/service-b/src/app.controller.ts (2)
  • 14-17: The getHello() method is a simple endpoint that returns a string. It's good to see that the business logic is separated into the service layer (appService.getHello()), which promotes separation of concerns.

  • 19-24: The handleResource1Created method listens for the Resource1CreatedPattern event and uses the Resource1CreatedValidationPipe to validate incoming data. This is a good use of NestJS's microservices module and validation pipes. However, it's important to ensure that the console.log statement on line 22 is intended for production use. If it's only for debugging or development purposes, consider removing it or replacing it with a proper logging solution.

  handleResource1Created(data: Resource1Created) {
-    console.log('received Resource1', data);
+    // replace with a proper logging solution if needed
+    this.appService.log('received Resource1', data);
    this.appService.handleResource1Created(data);
  }

Also, make sure that the handleResource1Created method in AppService properly handles any errors that might occur during processing. Unhandled errors could cause the service to crash or behave unexpectedly.

examples/nest-nats-microservices/service-b/src/main.ts (1)
  • 1-19: The code looks good and follows the NestJS microservices pattern. However, it's important to ensure that the NATS server is running at the specified address (nats://nats:4222) and that the queue channel1 is correctly set up. Also, consider moving these configurations to a separate configuration file or environment variables for better maintainability and security.
examples/nest-nats-microservices/service-c/test/app.e2e-spec.ts (1)
  • 1-24: The end-to-end test for the root endpoint in service-c looks good. It correctly sets up a testing module, initializes an application instance, and tests the GET request to the root endpoint. The expected status code and response body are also correctly specified.

Comment on lines +1 to +4
export class CreateResource1Dto {
public id: string;
public isActive: boolean;
}
Copy link

@coderabbitai coderabbitai bot Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CreateResource1Dto class is well defined with public properties. However, it's not a good practice to include the id field in the creation DTO. The id should be generated by the server when creating a new resource to ensure uniqueness and prevent potential security issues. Consider removing the id field from this DTO.

export class CreateResource1Dto {
- 2:   public id: string;
  public isActive: boolean;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I think we want to include the id as part of the DTO in this case 😊

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulises-jeremias While it's possible to include the id in the DTO, it's generally not recommended for a few reasons:

  1. Uniqueness: The server is usually responsible for generating unique IDs. If clients provide the ID, there's a risk of collision if two clients provide the same ID.

  2. Security: Allowing clients to set their own IDs can lead to potential security issues. For example, a malicious client could potentially overwrite data by providing an existing ID.

  3. Consistency: It's common practice for servers to generate IDs to ensure consistency across all resources.

If you have a specific use case where the client needs to provide the ID, please consider these points and make sure to handle potential issues appropriately. If the client-provided ID is meant to be a secondary identifier, consider renaming it to something like clientProvidedId or externalId to avoid confusion with the primary, server-generated ID.

Comment on lines 1 to 16
import { NestFactory } from '@nestjs/core';
import { Transport } from '@nestjs/microservices';
import { AppModule } from './app.module';

async function bootstrap() {
const app = await NestFactory.create(AppModule);
app.connectMicroservice({
transport: Transport.NATS,
options: {
servers: ['nats://nats:4222'],
queue: 'channel2',
},
});
await app.startAllMicroservices();
await app.listen(3001);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good overall, but there are a few points to consider:

  1. The NATS server URL is hardcoded in the bootstrap function. This could be problematic if the server's address changes or if you want to use different servers for different environments (development, testing, production, etc.). Consider moving this value to an environment variable or configuration file.

  2. There is no error handling in case the connection to the NATS server fails. It would be beneficial to add some error handling logic to ensure that the service can recover or at least log the error and exit gracefully in case of a failure.

  3. The queue name 'channel2' is hardcoded. If you plan to have multiple instances of this service, they will all listen to the same queue. If you want each instance to listen to its own queue, you should generate a unique queue name for each instance.

Here is a refined version of your code with these considerations:

import { NestFactory } from '@nestjs/core';
import { Transport } from '@nestjs/microservices';
import { AppModule } from './app.module';

async function bootstrap() {
  const app = await NestFactory.create(AppModule);
  try {
    app.connectMicroservice({
      transport: Transport.NATS,
     options: {
       servers: [process.env.NATS_SERVER || 'nats://nats:4222'],
       queue: process.env.QUEUE_NAME || 'channel2',
     },
   });
   await app.startAllMicroservices();
 } catch (error) {
   console.error('Failed to connect to NATS server:', error);
   process.exit(1);
 }
 await app.listen(process.env.PORT || 3001);
}

bootstrap();

Copy link
Member

@ulises-jeremias ulises-jeremias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-aliverti thank you for your contribution! since this is a big PR there are a lot of suggestions. You can apply some of the changes and then request me a code review again 👌🏻

examples/nest-nats-microservices/docker-compose.yaml Outdated Show resolved Hide resolved
Comment on lines +18 to +20
it('should return "Hello World!"', () => {
expect(appController.getHello()).toBe('Hello World!');
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-aliverti feel free to choose whether to use this suggestion or not. I don't have any strong opinion on this 😅

Comment on lines +1 to +4
export class CreateResource1Dto {
public id: string;
public isActive: boolean;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai I think we want to include the id as part of the DTO in this case 😊

Comment on lines 28 to 31
@Get(':id')
findOne(@Param('id') id: string) {
return this.resource1Service.findOne(+id);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-aliverti why are we casting to a positive number here?

Comment on lines 33 to 39
@Patch(':id')
update(
@Param('id') id: string,
@Body() updateResource1Dto: UpdateResource1Dto,
) {
return this.resource1Service.update(+id, updateResource1Dto);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-aliverti why are we casting to a positive number here?

@martin-aliverti
Copy link
Contributor Author

@ulises-jeremias the string-to-number casting is added by default by Nest when it generates the CRUD operations. I can remove them, but it is irrelevant to the sample

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between 872d5a1 and d4b677c.
Files ignored due to filter (1)
  • examples/nest-nats-microservices/compose.yaml
Files selected for processing (4)
  • examples/nest-nats-microservices/README.md (1 hunks)
  • examples/nest-nats-microservices/service-a/README.md (1 hunks)
  • examples/nest-nats-microservices/service-b/README.md (1 hunks)
  • examples/nest-nats-microservices/service-c/README.md (1 hunks)
Files skipped from review due to trivial changes (4)
  • examples/nest-nats-microservices/README.md
  • examples/nest-nats-microservices/service-a/README.md
  • examples/nest-nats-microservices/service-b/README.md
  • examples/nest-nats-microservices/service-c/README.md

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 4

Commits Files that changed from the base of the PR and between d4b677c and bb46c90.
Files selected for processing (10)
  • examples/nest-nats-microservices/service-a/src/app.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.module.ts (1 hunks)
  • examples/nest-nats-microservices/service-a/src/resource2/resource2.service.ts (1 hunks)
  • examples/nest-nats-microservices/service-b/src/main.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/app.controller.ts (1 hunks)
  • examples/nest-nats-microservices/service-c/src/main.ts (1 hunks)
Files skipped from review due to trivial changes (1)
  • examples/nest-nats-microservices/service-a/src/resource1/resource1.service.ts
Additional comments (Suppressed): 8
examples/nest-nats-microservices/service-b/src/main.ts (1)
  • 1-18: The code looks good and follows the NestJS microservices pattern. However, it's important to ensure that the environment variable NATS_SERVER is properly set in your deployment environment, otherwise, it will default to 'nats://nats:4222'. If this default value doesn't match your NATS server address, it could lead to connection issues.
examples/nest-nats-microservices/service-a/src/app.service.ts (1)
  • 1-8: The getVersion() method retrieves the version from environment variables and defaults to '0.0.1' if not set. This is a good practice as it allows for easy configuration changes without code modifications. However, ensure that the environment variable VERSION is properly set in your deployment environment to avoid using the default value unintentionally.
examples/nest-nats-microservices/service-a/src/resource2/resource2.controller.ts (1)
  • 1-23: The new hunk introduces a controller for resource2 in service-a. It includes three routes: a POST route at the base URL for creating a new resource, a GET route at the base URL for finding a hello message, and a GET route with an id parameter for finding a specific resource. The code looks clean and follows NestJS best practices. However, ensure that the findHelloMessage() method in the Resource2Service returns a meaningful response or is used appropriately as its purpose isn't clear from this context.
examples/nest-nats-microservices/service-a/src/resource2/resource2.module.ts (1)
  • 1-21: The code looks good overall. However, it's important to ensure that the environment variable NATS_SERVER is properly set in your deployment environment. If it's not set, the default value 'nats://nats:4222' will be used which might not be the desired behavior in a production environment. Consider adding error handling or a configuration check at startup to ensure that all necessary environment variables are set.
        options: {
          servers: [process.env.NATS_SERVER || 'nats://nats:4222'],
        },
examples/nest-nats-microservices/service-c/src/app.controller.ts (1)
  • 14-17: The getHello() method is correctly delegating its functionality to the appService.getHello(). This is a good example of separation of concerns, where the controller is only responsible for handling the request/response cycle, and the service is responsible for the business logic.
examples/nest-nats-microservices/service-a/src/resource1/resource1.controller.ts (1)
  • 14-45: The new hunk introduces a controller for resource1 in service-a. The controller has five methods: create, findAll, findOne, update, and remove. Each method corresponds to a specific HTTP verb (POST, GET, PATCH, DELETE) and uses the Resource1Service to perform operations.

The create method takes a CreateResource1Dto object from the request body and passes it to the service's create method.

The findAll method doesn't take any parameters and calls the service's findAll method.

The findOne, update, and remove methods all take an id parameter from the route and call the corresponding service method. Additionally, the update method also takes an UpdateResource1Dto object from the request body and passes it to the service's update method.

Overall, the code looks clean and follows NestJS best practices for controllers. However, there is no error handling present. While NestJS handles many errors out of the box, it might be beneficial to add custom error handling for more specific cases, such as when a resource is not found or an update fails.

examples/nest-nats-microservices/service-a/src/resource2/resource2.service.ts (2)
  • 9-11: The create method currently returns a static string. Based on the context, it seems like this method should be creating a new instance of resource2 using the provided CreateResource2Dto. Please verify if this is the intended behavior.
-  create(createResource2Dto: CreateResource2Dto) {
-    return 'This action adds a new resource2';
-  }
+  create(createResource2Dto: CreateResource2Dto) {
+    // Implement logic to create a new resource2
+  }
  • 17-19: Similar to the create method, the findOne method also returns a static string. It seems like this method should be retrieving a specific instance of resource2 based on the provided id. Please verify if this is the intended behavior.
-  findOne(id: string) {
-    return `This action returns a #${id} resource2`;
-  }
+  findOne(id: string) {
+    // Implement logic to retrieve a specific resource2
+  }

Comment on lines +9 to +12
@MessagePattern('get_version')
getVersion() {
return process.env.VERSION || '0.0.21';
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method getVersion() is directly accessing the environment variable VERSION. While this may not be an issue, it's generally a good practice to use a configuration service for managing environment variables. This way, you can centralize all your application's configuration logic and improve maintainability.

-    return process.env.VERSION || '0.0.21';
+    return this.appService.getVersion();

In the appService, you can have a getVersion method that retrieves the version from the environment variables.

Comment on lines +1 to +21
import { Module } from '@nestjs/common';
import { ClientsModule, Transport } from '@nestjs/microservices';
import { Resource1Controller } from './resource1.controller';
import { Resource1Service } from './resource1.service';

@Module({
imports: [
ClientsModule.register([
{
name: 'NATS',
transport: Transport.NATS,
options: {
servers: [process.env.NATS_SERVER || 'nats://nats:4222'],
},
},
]),
],
controllers: [Resource1Controller],
providers: [Resource1Service],
})
export class Resource1Module {}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good overall, but there's a potential issue with the NATS server URL. It's directly using process.env.NATS_SERVER without checking if it's defined or not. If NATS_SERVER is not set in the environment variables, it will default to 'nats://nats:4222'. This might be fine for local development, but could cause issues in production if the NATS server is not at that address.

Consider adding a check to ensure that NATS_SERVER is defined and throw an error if it's not. This would make it easier to debug issues related to the NATS server connection.

Here's how you can do it:

        options: {
          servers: [process.env.NATS_SERVER || 'nats://nats:4222'],
        },

Change to:

        options: {
+           servers: [process.env.NATS_SERVER ? process.env.NATS_SERVER : throw new Error('NATS_SERVER is not defined')],
        },

Comment on lines +1 to +17
import { NestFactory } from '@nestjs/core';
import { Transport } from '@nestjs/microservices';
import { AppModule } from './app.module';

async function bootstrap() {
const app = await NestFactory.create(AppModule);
app.connectMicroservice({
transport: Transport.NATS,
options: {
servers: [process.env.NATS_SERVER || 'nats://nats:4222'],
},
});
await app.startAllMicroservices();
await app.listen(3001);
}

bootstrap();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good overall, but there's a potential issue with the NATS server address. It's being fetched from an environment variable NATS_SERVER and if it's not set, it defaults to 'nats://nats:4222'. This could lead to unexpected behavior if the environment variable is not set correctly or if the default value is not the correct address for the NATS server in the deployment environment.

Consider adding error handling or validation to ensure that the NATS server address is valid and reachable. Also, consider using a configuration service to manage your application's configuration in a centralized manner. This would make it easier to validate and use configuration values throughout your application.

+ import { ConfigService } from '@nestjs/config';
...
async function bootstrap() {
  const app = await NestFactory.create(AppModule);
+   const configService = app.get(ConfigService);
+   const natsServer = configService.get('NATS_SERVER');
+   if (!natsServer) {
+     throw new Error('NATS_SERVER is not defined');
+   }
  app.connectMicroservice({
    transport: Transport.NATS,
    options: {
      servers: [natsServer],
    },
  });
  await app.startAllMicroservices();
  await app.listen(3001);
}

Comment on lines +13 to +15
findHelloMessage() {
return this.natsClient.send('get_hello', {});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The findHelloMessage method sends a 'get_hello' message pattern to NATS but does not handle any potential errors that might occur during this process. Consider adding error handling here to ensure that your application can gracefully handle any issues that might arise when interacting with NATS.

-  findHelloMessage() {
-    return this.natsClient.send('get_hello', {});
-  }
+  async findHelloMessage() {
+    try {
+      return await this.natsClient.send('get_hello', {}).toPromise();
+    } catch (error) {
+      // Handle error
+    }
+  }


@Get(':id')
findOne(@Param('id') id: string) {
return this.resource1Service.findOne(id);

Check failure

Code scanning / CodeQL

Reflected cross-site scripting High

Cross-site scripting vulnerability due to a
user-provided value
.

@Get(':id')
findOne(@Param('id') id: string) {
return this.resource2Service.findOne(id);

Check failure

Code scanning / CodeQL

Reflected cross-site scripting High

Cross-site scripting vulnerability due to a
user-provided value
.
@ulises-jeremias ulises-jeremias self-requested a review October 2, 2023 20:18
Copy link
Member

@ulises-jeremias ulises-jeremias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR is great! I'll ignore the suggestions made by @coderabbitai for now since this is just a reference examples 👌🏻

@ulises-jeremias ulises-jeremias merged commit 9a66fb2 into main Oct 2, 2023
6 checks passed
@ulises-jeremias ulises-jeremias deleted the nest-nats-example branch October 2, 2023 20:19
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between bb46c90 and 8909988.
Files selected for processing (1)
  • examples/nest-nats-microservices/README.md (1 hunks)
Files skipped from review due to trivial changes (1)
  • examples/nest-nats-microservices/README.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants