inversify / InversifyJS

A powerful and lightweight inversion of control container for JavaScript & Node.js apps powered by TypeScript.
http://inversify.io/
MIT License
11.34k stars 719 forks source link

As of 5.2.2 / 6.0.0 Inheriting controllers can lead to a crash instead of a 404. #854

Closed kbirger closed 3 years ago

kbirger commented 6 years ago

With the way that https://github.com/inversify/InversifyJS/issues/586 has been fixed, it has broken another use case in a very strange way. Prior to this, I was able to have a non abstract base controller, and another controller that extended its functionality by adding an additional endpoint. The desired effect is that the second controller has all of the endpoints of the first, plus additional ones. As of 5.2.2/6.0.0, this still works, but certain requests described below can crash the server.

As a side note, in the previous version, I had to override all of the base class methods and re-decorate them. I no longer have to do this due to the switch to getMetadata.

This works in the happy path. However, if I GET /stuff/private-things, it crashes the server. I suppose as a workaround I could implement this case differently, and have an abstract base, etc. But this seems like a natural way to do it, and we probably don't want bugs that can crash the server.

Expected Behavior

GET /stuff/private-things should return a 404 as per default behavior.

Current Behavior

GET /stuff/private-things crashes the server

Possible Solution

I'll have a deeper look at this. It seems like the metadata indicates that a controller method should exist on the base controller, but it actually doesn't. Perhaps all we need is a null check on the controller method to make sure it's really there.

Steps to Reproduce (for bugs)

@controller('stuff')
export class StuffController {
  constructor(/* deps */) { }

  @httpGet('/public-things')
  public getPublicThings() {
    return { things: [ 'a', 'b' ] }
  }
}

@controller('stuff-int')
export class PrivateStuffController extends StuffController {
  constructor(/* deps */) { 
    super(/* deps */);
  }

   // I can now remove this due to Reflect.getMetadata vs getOwnMetadata
  @httpGet('/public-things')
  public getPublicThings() {
    return super.getPublicThings();
  }
  @httpGet('/private-things')
  public getPrivateThings() {
    return { things: [ 'c', 'd' ] }
  }
}

GET /stuff/private-things

Context

To have a second controller has all of the endpoints of the first, plus additional ones.

Your Environment

Stack trace

Unhandled Promise rejection: Cannot read property 'apply' of undefined ; Zone: api ; Task: IncomingMessage.addListener:end ; Value: TypeError: Cannot read property 'apply' of undefined at InversifyExpressServer. (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:195:111) at step (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:32:23) at Object.next (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:13:53) at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:7:71 at new ZoneAwarePromise (C:\Dev\LabOrdersAndResultsUI\node_modules\zone.js\dist\zone-node.js:891:29) at __awaiter (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:3:12) at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:188:35 at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:211:19 at Layer.handle [as handle_request] (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\node_modules\express\lib\router\layer.js:95:5) at next (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\node_modules\express\lib\router\route.js:137:13) TypeError: Cannot read property 'apply' of undefined at InversifyExpressServer. (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:195:111) at step (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:32:23) at Object.next (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:13:53) at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:7:71 at new ZoneAwarePromise (C:\Dev\LabOrdersAndResultsUI\node_modules\zone.js\dist\zone-node.js:891:29) at __awaiter (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:3:12) at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:188:35 at C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\lib\server.js:211:19 at Layer.handle [as handle_request] (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\node_modules\express\lib\router\layer.js:95:5) at next (C:\Dev\LabOrdersAndResultsUI\node_modules\inversify-express-utils\node_modules\express\lib\router\route.js:137:13)

That seems to map to the handlerFactory method in server.ts, though I need to dig deeper into this stack since it's showing me the transpiled JS.

I assume it is the line that reads: let result = childContainer.getNamed<any>(TYPE.Controller, controllerName)[key](...args);

dcavanagh commented 6 years ago

@kbirger I'm having some trouble reproducing the error in the test code. Can you please take a look at the test case below? How were you able to produce the 404?

it("should support inherited methods", (done) => {
        let container = new Container();

        @controller("/api/test")
        class BaseController {
            @httpGet("/public-things")
            public getPublicThings() {
                return { things: ["a", "b"] };
            }

        }

        @controller("/api/test")
        class TestController extends BaseController {
            constructor(/* deps */) {
                super(/* deps */);
            }

            @httpGet("/private-things")
            public getPrivateThings() {
                return { things: ["c", "d"] };
            }
        }

        let server = new InversifyExpressServer(container);
        let app = server.build();

        supertest(app).get("/api/test/public-things")
            .expect("Content-Type", /json/)
            .expect(200)
            .then(response1 => {
                expect(response1.body.things[0]).to.equal("a");
            });

        supertest(app).get("/api/test/private-things")
            .expect("Content-Type", /json/)
            .expect(200)
            .then(response1 => {
                expect(response1.body.things[0]).to.equal("c");
                done();
            });

    });
kbirger commented 6 years ago

I can confirm that I was mostly correct. The line with childContainer.getNamed(...) is the source of the crash, but it is not the cause of the error.

The actual cause of the error is in the forEach of registerControllers. What seems to be happening is that both StuffController and PrivateStuffController are getting metadata for getPrivateThings, but obviously only one of them has the handler method.

I have a fix. It works. But I am not sure if it is the best way to fix the problem. I will keep looking, but maybe I can get an early opinion on the following diff. @remojansen since you were just in this code, what do you think about adding something like this to the registerControllers method?

if(controllerMetadata.target.prototype[metadata.key]) { 
    /* snip */
   this._router[metadata.method](/*snipping out existing params to the call for clarity*/)
}

It seems a little bit inelegant, yes, but I don't think there's a clear way to avoid that metadata getting picked up from the start since we are using getMetadata instead of getOwnMetadata now (which I definitely think is the way to go).

If this all sounds good, let me know and I can put in a PR with some tests to boot.

edit @AltekkeE I just saw your reply after submitting mine. Let me have a look at your test code and see if I can get a unit test that reproduces it. I'll try to do that this evening.

kbirger commented 6 years ago

@AltekkeE In my example the controllers have different base routes. If you change the TestController route to /api-int/test the tests you posted will throw the following error (which matches what I was seeing in live code).

(node:21116) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'apply' of undefined at InversifyExpressServer. (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:198:38) at step (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:32:23) at Object.next (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:13:53) at C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:7:71 at new Promise () at __awaiter (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:3:12) at C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:190:35 at C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:214:19 at Layer.handle [as handle_request] (C:\Users\kbirger\Documents\git\inversify-express-. ...SNIP.... at InversifyExpressServer. (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:105:29) at step (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:32:23) at Object.next (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:13:53) at fulfilled (C:\Users\kbirger\Documents\git\inversify-express-utils\src\server.js:4:58) at at process._tickCallback (internal/process/next_tick.js:188:7) (node:21116) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1) (node:21116) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

To add more context, I have an application whose single instance serves endpoints available to the internet, and another set of endpoints under a different route that is only available internally. This is handled by another appliance that does reverse proxying (Kubernetes stuff). This probably wouldn't be an issue if the routes were distinct, but I was asked to make the internal route contain all of the endpoints from the public one, to make it easier on internal services calling the one I am developing.

dcavanagh commented 6 years ago

@kbirger Thanks.. i missed that I will try it now

dcavanagh commented 6 years ago

@kbirger ~I'm still not getting the error.~ I got the error!!! Here is the refactored test case that produces the error.

import * as supertest from "supertest";
import { expect } from "chai";
import { InversifyExpressServer } from "../src/server";
import { httpGet, controller } from "../src/decorators";
import { Container } from "inversify";
import { cleanUpMetadata } from "../src";

describe("Unit Test: Issue 854", () => {

    beforeEach((done) => {
        cleanUpMetadata();
        done();
    });

    it("should support inherited methods", (done) => {
        let container = new Container();

        @controller("/api/test")
        class BaseController {
            @httpGet("/public-things")
            public getPublicThings() {
                return { things: ["a", "b"] };
            }
        }

        @controller("/api-int/test")
        class TestController extends BaseController {
            constructor(/* deps */) {
                super(/* deps */);
            }

            @httpGet("/private-things")
            public getPrivateThings() {
                return { things: ["c", "d"] };
            }
        }

        let server = new InversifyExpressServer(container);
        let app = server.build();

        supertest(app).get("/api-int/test/public-things")
            .expect("Content-Type", /json/)
            .expect(200)
            .then(response1 => {
                expect(response1.body.things[0]).to.equal("a");
            });

        supertest(app).get("/api-int/test/private-things")
            .expect("Content-Type", /json/)
            .expect(400)
            .then(response1 => {

                done();
            });

    });

});

edit Put full code from the bug test file Strike through Thanks for your patience!

kbirger commented 6 years ago

@dcavanagh , sorry I should have just posted a complete test file as you have. I've gone through and noted the differences between what you and I had separately.

import * as supertest from "supertest";
import { expect } from "chai";
import { InversifyExpressServer } from "../src/server";
import { httpGet, controller } from "../src/decorators";
import { Container } from "inversify";
import { cleanUpMetadata } from "../src";

describe.only("Unit Test: Issue 854 - kbirger", () => {

    beforeEach((done) => {
        cleanUpMetadata();
        done();
    });

    it("should support inherited methods", (done) => {
        let container = new Container();

        @controller("/api/test")
        class BaseController {
            @httpGet("/public-things")
            public getPublicThings() {
                return { things: ["a", "b"] };
            }
        }

        @controller("/api-int/test")
        class TestController extends BaseController {
            constructor(/* deps */) {
                super(/* deps */);
            }

            @httpGet("/private-things")
            public getPrivateThings() {
                return { things: ["c", "d"] };
            }
        }

        let server = new InversifyExpressServer(container);
        let app = server.build();

        // diff1 - this succeeds
        // instead of /api-int/test/public-things, /api/test/public-things
        supertest(app).get("/api/test/public-things")
            .expect("Content-Type", /json/)
            .expect(200)
            .then(response1 => {
                expect(response1.body.things[0]).to.equal("a");
            });

        // diff2 - this fails with the reported stack
        // instead of /api-int/test/private-things, /api/test/private-things
        supertest(app).get("/api/test/private-things")
            .expect("Content-Type", /html/)
            .expect(404) // diff3 - this should be a 404
            .then(response1 => {
                done();
            });

    });

});
remojansen commented 6 years ago

Hi guys, I think I know what is happening but I'm not sure about being able to support it. I'm also not sure about it being a good idea even if we can support it.

Basically only derived controllers can be decorated with @controller. You can do the following:

@injectable()
class BaseController {
    public getPublicThings() {
        return { things: ["a", "b"] };
    }
}

@controller("/api/test")
class PublicController  extends BaseController {
    constructor(/* deps */) {
        super(/* deps */);
    }
    @httpGet("/public-things")
    public getPublicThings() {
        return { things: ["a", "b"] };
    }
}

@controller("/api-int/test")
class TestController extends BaseController {
    constructor(/* deps */) {
        super(/* deps */);
    }

    @httpGet("/private-things")
    public getPrivateThings() {
        return { things: ["c", "d"] };
    }
}

I think we should investigate how difficult is to support both parent and dserived controllers to define routes. If the complexity is too high then we should avoid implementing it and throw an error that helps users to understand that they cannot do that.

kbirger commented 6 years ago

Thanks for the workaround @remojansen. Do you have any thought on the patch I proposed to registerControllers in server.ts above? I'm not sure if it would cause any other issues, but it does seem to address this particular care.

kbirger commented 6 years ago

@remojansen I decided to try the workaround you suggested. Were you able to get it to work? For me it responds to /api/test/private-things, though it shouldn't. Oddly enough if I print all of the bindings that one doesn't show up. Could be a related issue.

justintime4tea commented 4 years ago

We've now ran into this issue as well. Our issue description probably doesn't give any more helpful information. I wanted to mainly comment on our use-case in the hopes that it is helpful feedback in some way. I would hope more people have common base classes and aren't just duplicating code all willy nilly all over the place :) I suppose composition is an alternative approach to inheritance however I haven't really thought through how we would refactor to not need a base class and just inject "special services".

Our use case We have a RestfulHttpController<T> abstract class which is derived from from BaseHttpController. It provides all the standard RESTful interface/interaction for example GET /:id, POST :id, etc... Most all of our resources are interacted with in accordance with standard REST practices for example PUT being replace like it should be, PATCH being "update" using patch ops, etc... The majority of the functionality of our controllers are to route standard REST calls to CRUD operations.

We have controllers like BananaHttpController extends RestfulHttpController<Banana>. Most of our resources simply call super methods. The point of extending is mainly so we can perform various nuanced behaviors directly in the controllers but also so we can decorate the controllers with OpenAPI decorators and generate really cool client SDKs at will :) Following the OpenAPI spec all of our methods in each controller are also different for example getApple, getBanana, replaceApple, replaceBanana, etc...

Our issue This all works wonderfully when we keep it simple with GET, POST, PUT, PATCH, DEL to routes like / and /:id. It even works fine with /:id/image for those resources which have images; getAppleImage. We wanted to add GET /defaults/global (not a fan of this route to begin with) to two of our controllers. We again use different method names like getAppleDefaults and getBananaDefaults but all of a sudden it's now trying to call getBananaDefaults when we're making a request to the apple controller and it's throwing the TypeError: Cannot read property 'apply' of undefined ... It's driving me bananas :) Further still... if we only declare it in one of the controllers then, depending on order of import, it will recognize the route but try calling the method on the controller which doesn't provide that method...

Our workaround For now we've just bloated our base class with needless "getSpecificResourceThing" with a default return of 404. Any controller which doesn't implement simply returns 404 and the ones that do work fine. This sucks pretty bad because now the base class is temporally coupled to every controller. Luckily it's just 2 methods... for now :)