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

Prepare fastify integration #2922

Merged
merged 3 commits into from
Dec 7, 2024
Merged

Conversation

Romakita
Copy link
Collaborator

@Romakita Romakita commented Dec 7, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a new constant ANY_CONTENT_TYPE for handling wildcard content types.
    • Added PlatformContentTypeResolver and PlatformContentTypesContainer for improved content type management.
  • Bug Fixes

    • Enhanced error handling in the PlatformKoa middleware for improved robustness.
    • Added optional chaining in createContext to prevent runtime errors.
  • Refactor

    • Simplified useContext method signatures across PlatformExpress and PlatformKoa.
    • Refactored middleware instantiation in PlatformMulterMiddleware for clarity.
    • Streamlined the PlatformResponseFilter class by removing unnecessary properties and methods.
  • Chores

    • Updated method signatures in various classes to support asynchronous operations.

Copy link

coderabbitai bot commented Dec 7, 2024

Walkthrough

The changes in this pull request involve several modifications across multiple classes, primarily focusing on method signatures and enhancing asynchronous handling. The useContext method in both the PlatformExpress and PlatformKoa classes has been updated to return void, while the PlatformBuilder class has seen significant adjustments to its lifecycle methods to incorporate asynchronous behavior. Additionally, the PlatformMulterMiddleware class has been refactored for better clarity in middleware instantiation. The PlatformAdapter class has updated its useContext method to reflect asynchronous capabilities, and improvements in error handling and type safety have been made in the PlatformParams class. A new constant and several new exports have also been introduced in the response filter components.

Changes

File Path Change Summary
packages/platform/platform-express/src/components/PlatformExpress.ts Updated useContext method signature from this to void and removed return statement.
packages/platform/platform-http/src/common/builder/PlatformBuilder.ts Updated runLifecycle to use await for this.adapter.useContext(), modified callback for flexible arguments, added await to stop and ready methods.
packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts Refactored middleware instantiation into two steps for clarity and added conditional check for multer instance.
packages/platform/platform-http/src/common/services/PlatformAdapter.ts Updated useContext method signature from any to `Promise
packages/platform/platform-http/src/common/utils/createContext.ts Changed req.get("x-request-id") to req.get?.("x-request-id") to add optional chaining.
packages/platform/platform-koa/src/components/PlatformKoa.ts Updated useContext method signature from this to void, added error handling to mapHandler method.
packages/platform/platform-params/src/builder/PlatformParams.ts Updated getArg to be async and improved error handling; modified compile method's parameter type for scope to PlatformParamsScope<Context>.
packages/platform/platform-response-filter/src/constants/ANY_CONTENT_TYPE.ts Added new constant ANY_CONTENT_TYPE with value "*/*".
packages/platform/platform-response-filter/src/index.ts Added exports for PlatformContentTypeResolver and PlatformContentTypesContainer, removed export for getContentType.js.
packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts Introduced functionality for determining content types with getContentType and resolver functions, added types and injectable constants.
packages/platform/platform-response-filter/src/services/PlatformContentTypesContainer.ts Introduced a factory function for managing response filters based on content types.
packages/platform/platform-response-filter/src/services/PlatformResponseFilter.ts Removed constructor and properties, streamlined transform method to use contentTypeResolver directly.
packages/platform/platform-response-filter/src/utils/getContentType.ts Deleted file containing a utility function for determining content types.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant PlatformExpress
    participant PlatformBuilder

    Client->>PlatformExpress: Request
    PlatformExpress->>PlatformBuilder: useContext()
    PlatformBuilder->>PlatformBuilder: runLifecycle()
    PlatformBuilder-->>PlatformExpress: Context Established
    PlatformExpress-->>Client: Response
Loading
sequenceDiagram
    participant Client
    participant PlatformKoa

    Client->>PlatformKoa: Request
    PlatformKoa->>PlatformKoa: useContext()
    PlatformKoa-->>Client: Response
Loading

Possibly related PRs

  • fix(schema): add discriminator.mappings information on generated schema #2891: The changes in the main PR regarding the useContext method's return type in the PlatformExpress class are conceptually similar to the changes made in the PlatformKoa class, where the return type of useContext was also changed from this to void. Both PRs involve modifications to method signatures that clarify the intended use of the methods.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between 1204d06 and a0ea282.

📒 Files selected for processing (7)
  • packages/platform/platform-express/src/components/PlatformExpress.ts (1 hunks)
  • packages/platform/platform-http/src/common/builder/PlatformBuilder.ts (1 hunks)
  • packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts (1 hunks)
  • packages/platform/platform-http/src/common/services/PlatformAdapter.ts (1 hunks)
  • packages/platform/platform-http/src/common/utils/createContext.ts (1 hunks)
  • packages/platform/platform-koa/src/components/PlatformKoa.ts (1 hunks)
  • packages/platform/platform-params/src/builder/PlatformParams.ts (1 hunks)
🔇 Additional comments (8)
packages/platform/platform-params/src/builder/PlatformParams.ts (1)

67-68: LGTM! Type-safe parameter compilation

The improved type safety with PlatformParamsScope<Context> ensures better compile-time checks for context handling.

packages/platform/platform-http/src/common/middlewares/PlatformMulterMiddleware.ts (2)

42-48: 🛠️ Refactor suggestion

Improve type safety and error handling for multer middleware

While the null check is a good addition, there are a few improvements that could make the code more robust:

  1. The type annotation for middleware should be more specific than any
  2. The case where multer is undefined should throw a specific error rather than silently continuing

Consider applying these changes:

-      const multer = this.app.multer(settings);
+      const multer = this.app.multer(settings);
+      if (!multer) {
+        throw new Error("Failed to initialize multer middleware");
+      }
-      if (multer) {
-        const middleware: any = multer.fields(this.getFields({fields}));
-
-        return await middleware(ctx.getRequest(), ctx.getResponse());
-      }
+      const middleware = multer.fields(this.getFields({fields}));
+      return await middleware(ctx.getRequest(), ctx.getResponse());

Let's verify the multer types available in the codebase:


42-48: Verify error handling consistency across platform adapters

The error handling pattern here should be consistent with other platform adapters.

Let's check the error handling patterns in other adapters:

✅ Verification successful

Error handling is consistent with platform standards

The error handling implementation is appropriate and follows platform conventions:

  • Multer errors are properly wrapped in a MulterException that extends BadRequest
  • The error handling preserves the original error details (message, code) while mapping to HTTP-appropriate responses
  • The pattern matches other platform adapters which use try-catch blocks and throw typed exceptions
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar error handling patterns in platform adapters
rg -t ts "throw new \w+Exception|catch \(.*\) {" packages/platform/platform-*/src/

Length of output: 864


Script:

#!/bin/bash
# Search for error handling patterns in platform adapters with fixed regex
rg -t ts "throw new \w+Exception" packages/platform/platform-*/src/

# Search for catch blocks separately
rg -t ts "catch \(" packages/platform/platform-*/src/

# Look specifically for multer-related error handling
rg -t ts "multer.*error|error.*multer" packages/platform/platform-*/src/

Length of output: 4465


Script:

#!/bin/bash
# Let's examine the full MulterMiddleware implementation to understand the error handling context
ast-grep --pattern 'class PlatformMulterMiddleware {
  $$$
}'

# Also check for MulterException definition
rg -A 5 "class MulterException" packages/platform/platform-*/src/

Length of output: 1428

packages/platform/platform-http/src/common/builder/PlatformBuilder.ts (1)

193-193: Properly awaiting this.adapter.useContext() for asynchronous context initialization

The addition of await ensures that any asynchronous operations within useContext are completed before proceeding with the lifecycle processes, enhancing the reliability of the application's startup sequence.

packages/platform/platform-http/src/common/services/PlatformAdapter.ts (1)

45-45: Updated useContext method signature to support asynchronous operations

Changing the return type to Promise<void> | void allows for asynchronous context initialization in subclasses, providing greater flexibility for different platform implementations.

packages/platform/platform-koa/src/components/PlatformKoa.ts (1)

143-143: Adjusted useContext method to return void instead of this

Changing the return type to void and removing the return statement simplifies the method and aligns it with the updated useContext signature in the PlatformAdapter abstract class.

packages/platform/platform-express/src/components/PlatformExpress.ts (1)

178-178: Adjusted useContext method to return void instead of this

Changing the return type to void and removing the return statement simplifies the method and ensures consistency with the updated useContext signature in the PlatformAdapter abstract class.

packages/platform/platform-http/src/common/utils/createContext.ts (1)

10-10: Verify header access patterns across platform implementations.

Let's ensure this change aligns with how headers are accessed in different platform adapters.

✅ Verification successful

Based on the extensive code analysis, I can now provide a final response:

The header access pattern is consistent across platform implementations

The change to use req.get?.("x-request-id") is safe and aligns with the platform's architecture:

  • The base PlatformRequest class defines the standard header access pattern through the get() method, which all platform adapters inherit and implement
  • The Koa adapter properly inherits this behavior from the base class
  • The header access is abstracted through the platform's request interface, making it consistent across all implementations
  • The x-request-id header is handled uniformly throughout the codebase, as evidenced by the test cases and implementations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check how request headers are accessed across different platform implementations

# Look for header access patterns in platform-specific files
echo "Checking header access patterns in platform implementations..."
rg -A 2 "req\.(get|headers|header)" "packages/platform/**/Platform(Express|Koa|Fastify)*.ts"

# Look for x-request-id usage
echo "Checking x-request-id usage patterns..."
rg -A 2 "x-request-id" "packages/platform/**/*.ts"

Length of output: 530


Script:

#!/bin/bash
# Let's first find the platform adapter files
echo "Finding platform adapter files..."
fd "Platform.*\.ts$" packages/platform

# Then check header access patterns in all TypeScript files under platform
echo "Checking header access patterns..."
fd -e ts . packages/platform --exec rg -A 2 "req\.(get|headers|header)" {}

# Look for x-request-id usage in all TypeScript files under platform
echo "Checking x-request-id usage..."
fd -e ts . packages/platform --exec rg -A 2 "x-request-id" {}

Length of output: 11704


Script:

#!/bin/bash
# Let's specifically check the platform adapters' request handling implementations
echo "Checking Express adapter request handling..."
fd "PlatformExpressRequest\.ts$" packages/platform --exec cat {}

echo "Checking Koa adapter request handling..."
fd "PlatformKoaRequest\.ts$" packages/platform --exec cat {}

# Also check the base PlatformRequest implementation
echo "Checking base PlatformRequest implementation..."
fd "PlatformRequest\.ts$" packages/platform-http --exec cat {}

Length of output: 1504


Script:

#!/bin/bash
# Let's try with the correct path structure
echo "Checking Express adapter request handling..."
fd "PlatformExpressRequest\.ts$" . --exec cat {}

echo "Checking base PlatformRequest implementation..."
fd "PlatformRequest\.ts$" . --type f --exclude "*.spec.ts" --exec cat {}

# Also check for any header-related methods in platform adapters
echo "Checking header-related methods in platform code..."
rg -A 3 "headers|get\(" "packages/platform/platform-express/src/" "packages/platform/platform-koa/src/" "packages/platform/platform-http/src/"

Length of output: 67580

Comment on lines +73 to +77
async getArg<Context extends DIContext = DIContext>(
scope: PlatformParamsScope<Context>,
pipes: PipeMethods[],
param: JsonParameterStore
): Promise<any> {
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding input validation

While the implementation is solid, consider adding validation for the input parameters to ensure they're not undefined/null before processing.

 async getArg<Context extends DIContext = DIContext>(
   scope: PlatformParamsScope<Context>,
   pipes: PipeMethods[],
   param: JsonParameterStore
 ): Promise<any> {
+  if (!scope || !pipes || !param) {
+    throw new Error('Invalid arguments: scope, pipes, and param are required');
+  }
+  if (!Array.isArray(pipes)) {
+    throw new Error('pipes must be an array');
+  }
   try {
📝 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.

Suggested change
async getArg<Context extends DIContext = DIContext>(
scope: PlatformParamsScope<Context>,
pipes: PipeMethods[],
param: JsonParameterStore
): Promise<any> {
async getArg<Context extends DIContext = DIContext>(
scope: PlatformParamsScope<Context>,
pipes: PipeMethods[],
param: JsonParameterStore
): Promise<any> {
if (!scope || !pipes || !param) {
throw new Error('Invalid arguments: scope, pipes, and param are required');
}
if (!Array.isArray(pipes)) {
throw new Error('pipes must be an array');
}
try {

Comment on lines +78 to +88
try {
let value = scope;

try {
return await pipe.transform(value, param);
} catch (er) {
throw ParamValidationError.from(param, er);
for await (const pipe of pipes) {
value = await pipe.transform(value, param);
}
}, scope);

return value;
} catch (er) {
throw ParamValidationError.from(param, er);
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider adding debug logging

For better debugging capabilities, consider adding debug logging for pipe transformations and error cases.

   try {
     let value = scope;
+    const debug = require('debug')('tsed:platform:params');
 
     for await (const pipe of pipes) {
+      debug(`Transforming value through pipe ${pipe.constructor.name}`);
       value = await pipe.transform(value, param);
     }
 
     return value;
   } catch (er) {
+    debug(`Parameter validation failed: ${er.message}`);
     throw ParamValidationError.from(param, er);
   }

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +42 to +48
const multer = this.app.multer(settings);

return await middleware(ctx.getRequest(), ctx.getResponse());
if (multer) {
const middleware: any = multer.fields(this.getFields({fields}));

return await middleware(ctx.getRequest(), ctx.getResponse());
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider standardizing middleware initialization pattern

As this change is part of preparing for Fastify integration, consider establishing a consistent pattern for middleware initialization across different platform adapters (Express, Koa, Fastify).

You might want to:

  1. Create a common interface for middleware initialization
  2. Define clear error boundaries for middleware initialization failures
  3. Document the expected behavior when middleware initialization fails

Example interface pattern:

interface PlatformMiddlewareInitializer {
  initialize(settings: PlatformMulterSettings): Promise<Middleware | null>;
  validateInitialization(middleware: Middleware | null): void;
}

@@ -7,7 +7,7 @@ import {PlatformRequest} from "../services/PlatformRequest.js";
import {PlatformResponse} from "../services/PlatformResponse.js";

function defaultReqIdBuilder(req: any) {
return req.get("x-request-id") || v4().split("-").join("");
return req.get?.("x-request-id") || v4().split("-").join("");
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

LGTM! Good preparation for Fastify integration.

The addition of optional chaining (?.) makes the code more resilient across different HTTP frameworks, as not all frameworks implement the get method on their request objects. This change is particularly relevant for the upcoming Fastify integration.

For even better cross-platform compatibility, consider:

  1. Abstracting the header retrieval logic into platform-specific adapters
  2. Using a more generic method like req.headers["x-request-id"] which is commonly available across different HTTP frameworks

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.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI
Review profile: ASSERTIVE

📥 Commits

Reviewing files that changed from the base of the PR and between a0ea282 and 9699bcb.

📒 Files selected for processing (7)
  • packages/platform/platform-response-filter/src/constants/ANY_CONTENT_TYPE.ts (1 hunks)
  • packages/platform/platform-response-filter/src/index.ts (1 hunks)
  • packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts (1 hunks)
  • packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts (1 hunks)
  • packages/platform/platform-response-filter/src/services/PlatformContentTypesContainer.ts (1 hunks)
  • packages/platform/platform-response-filter/src/services/PlatformResponseFilter.ts (2 hunks)
  • packages/platform/platform-response-filter/src/utils/getContentType.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • packages/platform/platform-response-filter/src/utils/getContentType.ts
🔇 Additional comments (14)
packages/platform/platform-response-filter/src/services/PlatformResponseFilter.ts (5)

1-2: Dependencies imported correctly

The required dependencies for isSerializable, BaseContext, constant, inject, and injectable are correctly imported and will be used appropriately in the class.


6-7: New imports for content type resolution

The imports for PLATFORM_CONTENT_TYPE_RESOLVER and PLATFORM_CONTENT_TYPES_CONTAINER are correctly added to support the new content type resolution logic.


14-15: Dependencies injected properly

The container and contentTypeResolver properties are correctly injected using the inject function, ensuring that the class has access to the necessary services.


26-26: Utilize contentTypeResolver for determining content type

The use of this.contentTypeResolver(data, ctx) effectively replaces the previous method, streamlining content type determination based on the data and context.


30-30: Resolve response filter based on content type

The this.container.resolve(bestContentType) call appropriately retrieves the response filter corresponding to the best content type, enhancing flexibility in response handling.

packages/platform/platform-response-filter/src/constants/ANY_CONTENT_TYPE.ts (1)

1-1: Addition of 'ANY_CONTENT_TYPE' constant

Defining the ANY_CONTENT_TYPE constant as "*/*" provides a useful wildcard for content type handling, improving content negotiation scenarios.

packages/platform/platform-response-filter/src/index.ts (2)

4-4: Export 'ANY_CONTENT_TYPE' constant

The ANY_CONTENT_TYPE constant is correctly exported, making it available for use in other modules that require content type wildcards.


9-10: Export new services for content type handling

Exporting PlatformContentTypeResolver and PlatformContentTypesContainer ensures that these services are accessible throughout the application, facilitating content type resolution and response filtering.

packages/platform/platform-response-filter/src/services/PlatformContentTypesContainer.ts (2)

8-28: Factory function effectively manages response filters

The factory function correctly initializes and populates the containers map with response filter tokens. This design enhances the modularity and scalability of the response filter management.


30-31: Proper export of container type and constant

The PLATFORM_CONTENT_TYPES_CONTAINER type and constant are appropriately defined and exported, allowing for dependency injection and type safety across the platform.

packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts (3)

10-27: Content type determination logic is accurate

The getContentType function correctly identifies the appropriate content type based on the response and endpoint metadata, with suitable fallbacks for JSON data and views.


32-46: Content negotiation handled effectively

The resolver function properly uses the client's Accept header and available content types to determine the best content type, ensuring that wildcard types are appropriately filtered out.


51-54: Correct export of resolver type and constant

The PLATFORM_CONTENT_TYPE_RESOLVER type and constant are correctly defined and exported, enabling seamless integration and dependency injection of the content type resolver.

packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts (1)

29-32: LGTM! Test setup follows best practices

The test setup properly handles test isolation using beforeEach/afterEach hooks.

},
ctx
);
const result = contentTypeResolver(data, ctx);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix potential async handling issues

The contentTypeResolver calls are not awaited, which could lead to unreliable tests if the resolver is or becomes asynchronous.

Apply this pattern to all test cases:

- const result = contentTypeResolver(data, ctx);
+ const result = await contentTypeResolver(data, ctx);

Also applies to: 78-78, 95-95, 113-113

beforeEach(() => PlatformTest.create());
afterEach(() => PlatformTest.reset());

it("should return the content type (undefined)", () => {
it("should return the content type (undefined)", async () => {
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Improve test descriptions for better clarity

The test descriptions could be more specific about the scenarios they're testing.

Consider renaming the test cases to be more descriptive:

- it("should return the content type (undefined)"
+ it("should return undefined when no content type is specified"

- it("should return the content type (object - application/json)"
+ it("should return application/json for objects with explicit content type"

- it("should return the content type (string - application/json)"
+ it("should return application/json for strings with explicit content type"

- it("should return the content type (string - text/html)"
+ it("should return text/html when explicitly specified"

- it("should return the content type (string - view)"
+ it("should return text/html when using @View decorator"

Also applies to: 47-47, 65-65, 82-82, 99-99

Comment on lines +7 to +27
async function getTestFixture(contentTypes = ["application/json", "text/html"]) {
const contentTypeResolver = await PlatformTest.invoke<PLATFORM_CONTENT_TYPE_RESOLVER>(PLATFORM_CONTENT_TYPE_RESOLVER, [
{
token: PLATFORM_CONTENT_TYPES_CONTAINER,
use: {
contentTypes
}
}
]);

const data = {
test: "test"
};

const ctx = PlatformTest.createRequestContext();
return {
data,
ctx,
contentTypeResolver
};
}
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Consider enhancing the helper function with types and documentation

The helper function is well-structured, but could benefit from TypeScript types and JSDoc documentation for better maintainability.

Consider applying these improvements:

+ /** 
+  * Creates a test fixture with content type resolver and context
+  * @param contentTypes - Array of supported content types
+  * @returns Test fixture containing resolver, context and sample data
+  */
- async function getTestFixture(contentTypes = ["application/json", "text/html"]) {
+ async function getTestFixture(contentTypes: string[] = ["application/json", "text/html"]): Promise<{
+   data: Record<string, string>;
+   ctx: PlatformContext;
+   contentTypeResolver: PLATFORM_CONTENT_TYPE_RESOLVER;
+ }> {

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +29 to 116
class TestController {
@Get("/")
@(Returns(200).ContentType("application/json"))
get() {}
}

const ctx = PlatformTest.createRequestContext();
const {contentTypeResolver, ctx, data} = await getTestFixture();

ctx.endpoint = EndpointMetadata.get(TestController, "get");
ctx.response.getRes().statusCode = 200;

vi.spyOn(ctx.response, "getContentType").mockReturnValue("application/json");

const result = getContentType(
{
test: "test"
},
ctx
);
const result = contentTypeResolver(data, ctx);

expect(result).toEqual("application/json");
});
it("should return the content type (string - application/json)", () => {
it("should return the content type (string - application/json)", async () => {
class TestController {
@Get("/")
@(Returns(200).ContentType("application/json"))
get() {}
}

const ctx = PlatformTest.createRequestContext();
const {contentTypeResolver, ctx, data} = await getTestFixture();

ctx.endpoint = EndpointMetadata.get(TestController, "get");
ctx.response.getRes().statusCode = 200;
vi.spyOn(ctx.response, "getContentType").mockReturnValue("application/json");

const result = getContentType(
{
test: "test"
},
ctx
);
const result = contentTypeResolver(data, ctx);

expect(result).toEqual("application/json");
});
it("should return the content type (string - text/html)", () => {
it("should return the content type (string - text/html)", async () => {
class TestController {
@Get("/")
@(Returns(200).ContentType("text/html"))
get() {}
}

const ctx = PlatformTest.createRequestContext();
const {contentTypeResolver, ctx, data} = await getTestFixture();

ctx.endpoint = EndpointMetadata.get(TestController, "get");
ctx.response.getRes().statusCode = 200;
vi.spyOn(ctx.response, "getContentType").mockReturnValue("text/html");

const result = getContentType(
{
test: "test"
},
ctx
);
const result = contentTypeResolver(data, ctx);

expect(result).toEqual("text/html");
});
it("should return the content type (string - view)", () => {
it("should return the content type (string - view)", async () => {
class TestController {
@Get("/")
@Returns(200)
@View("view.html")
get() {}
}

const ctx = PlatformTest.createRequestContext();
const {contentTypeResolver, ctx, data} = await getTestFixture();

ctx.endpoint = EndpointMetadata.get(TestController, "get");
ctx.response.getRes().statusCode = 200;
ctx.view = "true";

const result = getContentType(
{
test: "test"
},
ctx
);
const result = contentTypeResolver(data, ctx);

expect(result).toEqual("text/html");
});
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Test coverage needs improvement for content type resolution

Based on the implementation file, the current test suite is missing coverage for these important scenarios:

  • Content type negotiation using Accept header
  • Fallback behavior when content type is ANY_CONTENT_TYPE
  • Multiple content types from the operation's getContentTypeOf method
  • Content type resolution when request.accepts returns multiple types

These scenarios are critical for the resolver's functionality as shown in the implementation but are not covered in the current test suite.

Example test cases to add:

  • Test with Accept header containing multiple content types
  • Test fallback when operation returns ANY_CONTENT_TYPE
  • Test content type filtering when accepts returns "/"
🔗 Analysis chain

Verify test coverage for edge cases

While the test suite covers the main scenarios, consider adding tests for:

  • Error cases (invalid content types)
  • Multiple content types in Accept header
  • Non-200 status codes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for existing error handling tests
rg -l "should.*error" "packages/platform/platform-response-filter/src/**/*spec.ts"

# Look for other content type related tests
rg -l "ContentType|content-type" "packages/platform/platform-response-filter/src/**/*spec.ts"

Length of output: 378


Script:

#!/bin/bash
# Let's first find the correct path using fd
fd -e spec.ts -e test.ts . packages/platform/platform-response-filter/

# Check implementation file to understand all scenarios that need testing
fd PlatformContentTypeResolver.ts . packages/platform/platform-response-filter/

Length of output: 663


Script:

#!/bin/bash
# Let's examine the implementation file to understand all scenarios
cat packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.ts

# Let's also check if there are any error tests in the test files we found
rg "should.*error|describe.*error" packages/platform/platform-response-filter/src/errors/TemplateRenderError.spec.ts packages/platform/platform-response-filter/src/services/PlatformContentTypeResolver.spec.ts packages/platform/platform-response-filter/src/services/PlatformResponseFilter.spec.ts packages/platform/platform-response-filter/src/utils/renderView.spec.ts

Length of output: 2222

@Romakita Romakita merged commit ee5d013 into production Dec 7, 2024
12 checks passed
@Romakita Romakita deleted the prepare-fastify-integration branch December 7, 2024 13:42
@Romakita
Copy link
Collaborator Author

Romakita commented Dec 8, 2024

🎉 This PR is included in version 8.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

1 participant