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.

Sign up for GitHub

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

feat(di): add support chain interceptor on the same method #2915

Merged
merged 1 commit into from
Dec 4, 2024
Merged

feat(di): add support chain interceptor on the same method #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 49 additions & 4 deletions packages/di/src/common/decorators/intercept.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import {catchError} from "@tsed/core";
import {DITest} from "../../node/index.js";
import {InterceptorContext} from "../interfaces/InterceptorContext.js";
import {InterceptorMethods} from "../interfaces/InterceptorMethods.js";
import {InjectorService} from "../services/InjectorService.js";
import {getInterceptorOptions, Intercept} from "./intercept.js";
import {Interceptor} from "./interceptor.js";
import {Service} from "./service.js";
Expand All @@ -14,7 +13,17 @@ class MyInterceptor implements InterceptorMethods {
const r = typeof context.args[0] === "string" ? undefined : new Error(`Error message`);
const retValue = context.next(r);

return `${retValue} - ${context.options || ""} - intercepted`;
return `${retValue} - ${context.options || ""} - intercepted 1`;
}
}

@Interceptor()
class MyInterceptor2 implements InterceptorMethods {
intercept(context: InterceptorContext<any>) {
const r = typeof context.args[0] === "string" ? undefined : new Error(`Error message`);
const retValue = context.next(r);

return `${retValue} - ${context.options || ""} - intercepted 2`;
}
Comment on lines +20 to 27
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 validation in MyInterceptor2

MyInterceptor2 duplicates error checking logic from MyInterceptor. Consider extracting common validation into a base class or utility function.

// Example base class or utility
class BaseInterceptor implements InterceptorMethods {
  protected validateInput(args: any[]): Error | undefined {
    return typeof args[0] === "string" ? undefined : new Error("Error message");
  }
}

@Interceptor()
class MyInterceptor2 extends BaseInterceptor {
  intercept(context: InterceptorContext<any>) {
    const error = this.validateInput(context.args);
    const retValue = context.next(error);
    return `${retValue} - ${context.options || ""} - intercepted 2`;
  }
}

}

Expand All @@ -24,6 +33,18 @@ class ServiceTest {
exec(param: string) {
return `Original data - ${param}`;
}

@Intercept(MyInterceptor, "options data")
@Intercept(MyInterceptor2, "options data")
chained(param: string) {
return `Chained - ${param}`;
}
Romakita marked this conversation as resolved.
Show resolved Hide resolved

@Intercept(MyInterceptor2, "options data")
@Intercept(MyInterceptor, "options data")
reverseChained(param: string) {
return `Chained - ${param}`;
}
}

@Service()
Expand All @@ -49,7 +70,31 @@ describe("@Intercept", () => {
expect(getInterceptorOptions(ServiceTest, "exec")).toEqual("options data");

// THEN
expect(result).toEqual("Original data - param data - options data - intercepted");
expect(result).toEqual("Original data - param data - options data - intercepted 1");
});
it("should chained interceptor", async () => {
// GIVEN
const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest)!;

// WHEN
const result = serviceTest.chained("param data");

expect(getInterceptorOptions(ServiceTest, "chained")).toEqual("options data");

// THEN
expect(result).toEqual("Chained - param data - options data - intercepted 2 - options data - intercepted 1");
});
it("should chained interceptor (reversed)", async () => {
// GIVEN
const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest)!;

// WHEN
const result = serviceTest.reverseChained("param data");

expect(getInterceptorOptions(ServiceTest, "reverseChained")).toEqual("options data");

// THEN
expect(result).toEqual("Chained - param data - options data - intercepted 1 - options data - intercepted 2");
Comment on lines +75 to +97
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage for interceptor chaining

While the basic chaining tests are good, consider adding:

  1. Tests with more than two interceptors
  2. Tests for error scenarios in the middle of the chain
  3. Tests with different options for each interceptor

Example additional test:

it("should handle errors in middle of chain", async () => {
  // GIVEN
  const serviceTest = await DITest.invoke<ServiceTest>(ServiceTest, [
    {
      token: MyInterceptor2,
      use: {
        intercept: vi.fn().mockImplementation(() => {
          throw new Error("Chain error");
        })
      }
    }
  ]);

  // WHEN
  const error = catchError(() => serviceTest.chained("param data"));

  // THEN
  expect(error?.message).toEqual("Chain error");
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 77-77: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

[error] 89-89: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

});
it("should intercept the method and mock interceptor", async () => {
// GIVEN
Expand Down Expand Up @@ -88,7 +133,7 @@ describe("@Intercept", () => {
const result = serviceTest.exec("param data");

// THEN
expect(result).toEqual("Original data - param data - options data - intercepted");
expect(result).toEqual("Original data - param data - options data - intercepted 1");
});
it("should intercept the method and throw error", async () => {
// GIVEN
Expand Down
11 changes: 8 additions & 3 deletions packages/di/src/common/decorators/intercept.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {classOf, decorateMethodsOf, DecoratorParameters, decoratorTypeOf, DecoratorTypes, Store, Type} from "@tsed/core";
import {classOf, decorateMethodsOf, DecoratorParameters, decoratorTypeOf, DecoratorTypes, nameOf, Store, Type} from "@tsed/core";

import {DI_INTERCEPTOR_OPTIONS, DI_INVOKE_OPTIONS} from "../constants/constants.js";
import {inject} from "../fn/inject.js";
Expand All @@ -19,7 +19,7 @@ export function bindIntercept(target: any, propertyKey: string | symbol, token:

Store.fromMethod(klass, propertyKey).set(DI_INTERCEPTOR_OPTIONS, options);

descriptor!.value = function (...args: any[]) {
function newMethod(...args: any[]) {
const next = (err?: Error) => {
if (!err) {
return originalMethod.apply(this, args);
Expand Down Expand Up @@ -50,7 +50,12 @@ export function bindIntercept(target: any, propertyKey: string | symbol, token:
},
next
);
};
}

descriptor!.value = newMethod;

Reflect.deleteProperty(klass.prototype, propertyKey);
Reflect.defineProperty(klass.prototype, propertyKey, descriptor!);

Comment on lines +55 to +58
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider preserving method properties during replacement

The current implementation might lose method metadata during property replacement. Consider using Object.getOwnPropertyDescriptor to preserve all property attributes.

- Reflect.deleteProperty(klass.prototype, propertyKey);
- Reflect.defineProperty(klass.prototype, propertyKey, descriptor!);
+ const originalDescriptor = Object.getOwnPropertyDescriptor(klass.prototype, propertyKey);
+ Reflect.defineProperty(klass.prototype, propertyKey, {
+   ...originalDescriptor,
+   value: newMethod
+ });
📝 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
descriptor!.value = newMethod;
Reflect.deleteProperty(klass.prototype, propertyKey);
Reflect.defineProperty(klass.prototype, propertyKey, descriptor!);
descriptor!.value = newMethod;
const originalDescriptor = Object.getOwnPropertyDescriptor(klass.prototype, propertyKey);
Reflect.defineProperty(klass.prototype, propertyKey, {
...originalDescriptor,
value: newMethod
});
🧰 Tools
🪛 Biome (1.9.4)

[error] 55-55: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

[error] 58-58: Forbidden non-null assertion.

(lint/style/noNonNullAssertion)

return descriptor;
}
Expand Down
Loading