fix(crud): Custom route decorators applied twice

[eugenio165]

fixes #230 Reflect.decorate was being called twice on the routes, causing metadata decorators to be added twice. In the Reflect.defineProperty call, Reflect.decorate has to be evaluated first either way to be passed down to the function. So i call Reflect.decorate above, and pass it down so it isn't called again. I've tested it out here with a few personal decorators, and others like @Override, all seems to be working well.

In my case, my auth guard decorator was being called twice.

Excellent work on this package! Thanks!

[eugenio165]

It is working on method, but not property

there are properties https://github.com/nestjsx/crud/blob/master/packages/crud/src/crud/crud-routes.factory.ts#L203-L243

Sorry, i don't think i understand completely. Please correct me if i'm missing something

Methods and properties on a prototype both return a property descriptor, the main difference between both is that a method has a function in its value field. If the member (property or method) exists on the prototype, that code will work for both, if the decorator is appropriate for that type of member (explanation below). The line will only return undefined if the member does not exist on the prototype. You can test this yourself, defining a property on the prototype.

Also, in the specific case for the function setDecorators, it is called only on the route methods (getManyBase, getOneBase, etc) created by the CrudRoutesFactory on the target.prototype, which is the controller you use @Crud() on. That means the only possible values for the name parameter is (getManyBase, getOneBase, etc) which have already been created previously on the target.prototype, which means it will always be called on method members.

If you look at the typescript handbook for property decorators the expression to apply the property decorator is called with the:

• prototype of the class for an instance member (or constructor of the class for a static member)
• name of the member

which means the ProperyDescriptor can be undefined and it will still apply the decorator (for properties). If you check the Reflect.decorate function you will also see that the last parameter (PropertyDescriptor) is an optional parameter.

For now ateast, the main responsability of the setDecorators function is to apply method decorators on the route methods created on the prototype of the target you use the @Crud() decorator on, so that isnt really a concern. Althought the code will work for both cases (properties or methods).

Also, in the previous code the Reflect.decorate method would have to be evaluated first before being passed into the Reflect.defineProperty method, so even if it were undefined, thats how the code was working before also, i have only removed 1 unecessary function call.

Nice work, and thanks for the quick response!

[eugenio165]

If theres something ive missed, tell me how to reproduce it and ill check it out! Thanks again

[Diluka]

I made a test, but it failed on property like below:

import 'reflect-metadata';

class A {
    p2 = ()=>"test"
}

const p2 = Reflect.decorate([Reflect.metadata('test', 'OK!')], A.prototype, 'p2')
Reflect.defineProperty(A.prototype, 'p2', p2)

console.log(a.p2(), Reflect.getMetadata('test', a, 'p2'))

Reflect.defineProperty(A.prototype, 'p2', p2)
        ^
TypeError: Property description must be an object: undefined

well, it is pretty good on method

[eugenio165]

The problem with that test is that the p2 property is not defined on the prototype. Like i said earlier that code will only fail if the property is not defined on the prototype, which is the case in your test. If you add console.log(Reflect.getOwnPropertyDescriptor(A.prototype, 'p2')); after the class A definition, you will see it is undefined. In the code of this package, the routes (getManyBase, getOneBase, etc) are created on the prototype, like this:

this.targetProto[name] = function createOneBase(req: CrudRequest, dto: any) {
      return this.service.createOne(req, dto);
};

That means the properties are always defined (be it a method or not). Also, check out this gist i made that shows that it works correctly on both properties and methods on the prototype, using the exact same code i just changed.

Also, the main point i am making is: I have not changed how the code works, like i said before, if you take the PREVIOUS code, the Reflect.decorate function is called inside the Reflect.defineProperty function, and then called again after. I have only removed the uncessary call after the defineProperty. It works EXACTLY the same, because the Reflect.decorate call has to be evaluated FIRST before going inside the Reflect.defineProperty function as a parameter.

My change is the same as removing the second Reflect.decorate call. It is already being called before the Reflect.defineProperty

Also, in the gist i have proven that it works for both properties and methods on the prototype. Obviously, like they are now, the properties have to be defined before the decorator, on the prototype.

[Diluka]

I understand

[epver]

I also found this bug, still haven't fixed merge

[eugenio165]

@vuesets It hasn't been released yet. The bug is still in the latest version available on npm.

[Varkal]

@eugenio165 Do you know if a release is planned soon ?