Meddleware priority for router can preempt user middleware

[rragan]

I just figured out why my middleware is not being run. The kraken-js config/config.json has a priority of 120 for the router. I happened to assign 122 to my middleware so it was not run as the router took over. Consider giving router a much higher priority number and documenting the user priority range that should be used to avoid this. For now, I will drop below 120.

[totherik]

Middleware and their default priority settings are documented here. You also have control over the priority of all built-in middleware. For example, router can be reconfigured by specifying the desired override value in your configuration.

{
    "middleware": {
        "router": {
            "priority": 5000
        }
    }
}

[jasisk]

Additionally, the router doesn't have to be the end of your continuation. In fact, that's how the 404 handler works (if we've fallen through the router without error-ing, we likely just don't have any other routes to check). You could, for example, have a second router (perhaps to simply the route map).

Also useful for error-handling middleware.

[lmarkus]

I've also been bit by this. I'm going to argue that our config file can hack your brain into hitting this bug:

Here's the middleware section of a plain vanilla Kraken config.json file:

    "middleware": {

        "static": {
            "module": {
                "arguments": [ "path:./.build" ]
            }
        }

        ,"router": {
            "module": {
                "arguments": [{ "directory": "path:./controllers" }]
            }
        }

        ,"expressView": {
            "priority": 100,
            "enabled": true,
            "module": {
                "name": "engine-munger/lib/expressView"
            }
        }

        ,"fileNotFound": {
            "enabled": true,
            "priority": 130,
            "module": {
                "name": "kraken-js/middleware/404",
                "arguments": [ "errors/404" ]
            }
        }

        ,"serverError": {
            "enabled": true,
            "priority": 140,
            "module": {
                "name": "kraken-js/middleware/500",
                "arguments": [ "errors/500" ]
            }
        }   
    }

Notice the following subtle things: 1) Router has no visible priority (The 120 default is stashed away in the krakenJs's module default config)

2) The first visible priority is 100, then there is a double space between that middleware, and the last two. From a UI perspective, this hints at "Put your stuff here!"

3) The last two middewares have priorities of 130 and 140 which might suggest that increments of 10 is the way to go when adding new ones.

A naive user* might start adding stuff at 110 and incrementing from there. Even if you read the linked config once, by the time you add custom middleware 120 you probably forgot all about that.

Granted, it's a pretty easy issue to troubleshoot if you know what you're looking for, but we might want to consider raising that 120 safety barrier a bit :)

*(Me)

[aredridel]

I think increasing visibility and advertising that you can override it rather than arbitrarily raising the limit is the way to go.

[totherik]

I'm not disagreeing that this requires some thought to put together correctly. However, several issues and knowledge gaps are being conflated into a single problem here:

1. It was not known that order is important WRT middleware registration (and in this case relative to router specifically)
2. It was not known or obvious how to set the order.
3. It was not known or obvious how to ascertain the correct priority value.

The mechanism and default values are documented, both in the README and in code. Changing the range of values or blaming the choice of numbers only helps those that already know that numeric priority is even a thing. The problem isn't the default values, it's the use of arbitrary numbers in the first place, so just randomly changing numbers doesn't solve the problem. Especially if you're tripped up by whitespace or increments of ten.

Granted, it's not documented on GitHub, but I have maintained from day one that for some cases relative ordering might be preferable. e.g. instead of "priority": 121, you might use "after": "router". As I mentioned then, this can become even more confusing, creating configs that may be harder to read without curation, cycles that are hard to debug, and generally become a footgun; but a hybrid approach may add value. This hybrid can be achieved using both numeric ordering for built-ins and then allow relative ordering to inject middleware into the appropriate place.

Building a mechanism to do this and prove it out should be a relatively trivial exercise that would require no change to kraken-js or meddleware. One could create a module that sorts the config prior to passing to meddleware and hook it into app initialization via onconfig.

The 120 default is stashed away in the krakenJs's module default config

Per my previous comment, this is documented/stashed away in the README.

[aredridel]

I think you're right there. Relative ordering has some downsides, but I think it's got some merit.

[lmarkus]

What can I say... I'm a visual person :) I think the current system is sound. It just needs some minor tweaking.

Expecting users to read, understand and hold all the documentation in their heads is not realistic; and neither is expecting (most) people to deep dive into the code unless they have to.

A simple //Middleware that runs before routing goes here comment (Or something to that effect) can probably go a long way.

It's all about making the framework friendlier for first-time/novice users.

[rragan]

Documented it in a new section of sample-app in hopes of catching the new user’s attention.

https://github.com/krakenjs/meddleware#meddleware

Rich

From: Lenny Markus [mailto:notifications@github.com] Sent: Thursday, January 22, 2015 10:54 AM To: krakenjs/kraken-js Cc: Ragan, Richard Subject: Re: [kraken-js] Meddleware priority for router can preempt user middleware (#325)

What can I say... I'm a visual person :) I think the current system is sound. It just needs some minor tweaking.

Expecting users to read, understand and hold all the documentation in their heads is not realistic; and neither is expecting (most) people to deep dive into the code unless they have to.

A simple //Middleware that runs before routing goes here comment (Or something to that effect) can probably go a long way.

It's all about making the framework friendlier for first-time/novice users.

— Reply to this email directly or view it on GitHubhttps://github.com/krakenjs/kraken-js/issues/325#issuecomment-71076124.

[jasisk]

I think it may not be immediately clear to a novice why configuration-driven middleware loading is a benefit. I think it takes building larger applications to realize the benefit (but that's not to say there's no benefit for smaller applications, I just think it's not as immediately clear).

I'll add a more concrete explanation for why meddleware exists, and will quote it over in this repository. In that explanation, I'll address some of the ambiguity. I also think throwing a comment or two in the generator output (potentially directing the user to the meddleware explanation) would prove useful. I think those things will work best to address this issue for now and we can consider alternate load-order mechanisms later.

[lmarkus]

:+1: