Bug:throw error in authorizationChecker

[lp6moon]

If the authorizationChecker is a function that returns a rejected Promise, the client will not be able to respond.

my code:

authorizationChecker: async (action: Action, roles: string[]) => {
	let user = action.request.user;
	if (!user) throw new UnauthorizedError(); //bug is here

	if (user && !roles.length)
		return true;
	if (user && roles.find(role => user.roles.indexOf(role) !== -1))
		return true;

	return false;
}

[NoNameProvided]

Which version do you use? I dont know how authorization checker works behind the scenes, but if it's implemented as a non global middleware, then this issue was fixed in the latest release via #138.

[lp6moon]

version is 0.7.0-alpha.10. authorizationChecker is setup special routing controllers options to make @Authorized decorator.

[NoNameProvided]

Can you try with the latest 0.7.0-alpha.11 release?

[lp6moon]

There's still a bug like this, the source code should be:

checkResult.then(result => {
	if (!result) {
		return this.handleError(new AccessDeniedError(action), actionMetadata, action);

	} else {
		next();
	}
}).catch(error => {
	this.handleError(error, actionMetadata, action);	// right?
});

[NoNameProvided]

Hmm, yeah but then it should be prepared to catch synchronous errors as well, something like

try {
  const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
  if (isPromiseLike(checkResult)) {
    checkResult.then(result => {
      if (!result) {
        return this.handleError(new AccessDeniedError(action), actionMetadata, action);
      } else {
        next();
      }
    }).catch(error => this.handleError(error, actionMetadata, action));
});
  } else {
    if (!checkResult) {
      this.handleError(new AccessDeniedError(action), actionMetadata, action);
    } else {
      next();
    }
 }
} catch {
  this.handleError(new AccessDeniedError(action), actionMetadata, action);
}

I will open a PR for this in the evening, but feel free to do it if you feel so.

[MichalLytek]

What is the purpose of isPromiseLike and if-else? Why not use Promise.resolve and have unified code for both promise and objects?

[NoNameProvided]

@19majkel94 we expect an async function, but nothing guarantees that it will be an async function. So it's better to prepare for a sync authorizationChecker function as well.

[MichalLytek]

I know but Why not use Promise.resolve?

const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
Promise.resolve(checkResult).then(result => {
    if (!result) {
        return this.handleError(new AccessDeniedError(action), actionMetadata, action);
    } else {
        next();
    }
}).catch(error => this.handleError(error, actionMetadata, action));

[NoNameProvided]

I just kept it the same way as we check for errors in other parts of the app.

Your code is smaller, but wrapping checkResult in a Promise seems like a too big trade off on performance.

[NoNameProvided]

Why not use Promise.resolve?

Just made a quick perf test, even without waiting for the Promise to resolve (so just the overhead of creating a promise every time) the performance ratio of the two is ~ 1:30000.

The exact numbers are:

Type	Result
Using isPromiseLike	29,445,883 ops/s±1.05%
Wrapping in Promise	10,389 ops/s±3.24%

Feel free to run the test for yourself: https://esbench.com/bench/592b117799634800a03483bd

[MichalLytek]

1. Weird, I use Chrome with V8 as it's in node and my results are 31 551 588 vs 997 712, so just 1:30 which isn't really surprising about wrapping into promise vs simple if check.

2. I don't know how esbench works so I created my own test suite:

const next = () => { return; }
const authorizationCheckerSuccess = () => { return true };
const isPromiseLike = (value) => value.then !== undefined;
const ITERATION_LIMIT = 1000000;

(async () => {
    console.time("isPromiseLike")
    for (let i = 0; i < ITERATION_LIMIT; i++) {
        const checkResult = authorizationCheckerSuccess();

        if (isPromiseLike(checkResult)) {
            // this is not important
            next();
        } else {
            if (!checkResult) {
                throw new Error();
            } else {
                next();
            }
        }
    }
    console.timeEnd("isPromiseLike");
})();

(async () => {
    console.time("promise")
    for (let i = 0; i < ITERATION_LIMIT; i++) {
        const checkResult = authorizationCheckerSuccess();

        const result = await Promise.resolve(checkResult);
        if (!result) {
            throw new Error();
        } else {
            next();
        }
    }
    console.timeEnd("promise");
})();

Just comment one block to run one loop at once.
Results? 253.179ms vs 2361.842ms, so 1:10. What do you think, how frequent the code is called and how much it takes do to all the response logic? 1:300000 might not be enough 😉

3. Premature optimalization is root of all evil. I prefer have 0.02345% slower response time than repeated and error prone code in every place in codebase.

4. Better go and check promise performance if you are worried about promise.resolve overhead:
   https://jsperf.com/native-promise-vs-callback
   And then go back to callbacks 😆

[pleerock]

@19majkel94 everything is relative, so decision in "better if I do code better but avoid small performance optimization" depend on:

1. how much worth code will be and how many value in it
2. how much worth performance will be and how many value in it

having to change single line of code to callback is not a problem if we can performance optimizations in a places where its important for library users. And when we are talking about public routes that can be executed from hundred to million of times its important to make it performant as we can.

native-promise-vs-callback

yeah I hate callbacks as well and wont use them when I have promises. But question is about Promise.resolve right? And Promise.resolve is a real performance overhead because it requires for it to wait for a next tick to execute. Its the same as using timeout(() => {}, 0)

[NoNameProvided]

Weird, I use Chrome with V8 as it's in node and my results are 31 551 588 vs 997 712

Wow, thats surprisingly differ, I wonder if it's because benchmark.js messes up something or because our machines differ.

Results? 253.179ms vs 2361.842ms, so 1:10.

Making every part of the lib where it checks for promises 10x faster is not a "just" especially since this is a library. But this is just my two coin.

Premature optimization is root of all evil.

Yes in user land usually it doesn't make a difference, in libraries it does. Users of this project want to build (hopefully) performant applications, and we should give them the tools for that. They wont care at all, if we use Promise.resolve, or an if-else what they will care if it is fast or not.

repeated and error prone code in every place in codebase.

Uhm, I dont see why it's error prone, and Promise.resolve is repeated code as well. I think the things we have to consider here are:

• Do we want to make a trendy or a performant library? I vote for the latter one.
• Does adding an if-else to every promise check worth a 10x speed up in every part where we check for promises? I think yes.

And then go back to callbacks 😆

Yes we could go back to write Assembly as well, it should be performant. Putting the joke off, you compare promise vs callback, with the 2 line or 1 line problem. They are just simply not on the same level.

[MichalLytek]

having to change single line of code to callback is not a problem if we can performance optimizations in a places where its important for library users. And when we are talking about public routes that can be executed from hundred to million of times its important to make it performant as we can

optimizations in a places where its important

Exactly. So, I encourage you to do the real-case profiling to see all the bottlenecks and hot parts of the code. Don't make premature optimization everywhere 😉

And Promise.resolve is a real performance overhead because it requires for it to wait for a next tick to execute. Its the same as using timeout(() => {}, 0)

Nope, promises are executed in the same event loop cycle as the microtasks (in node.js/V8):
https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

Making every part of the lib where it checks for promises 10x faster is not a "just" especially since this is a library. But this is just my two coin.

10x faster, but the overall overhead on whole request-respons handling? 385.427ms vs 385.372ms? Don't bother oneself with this little things.

Putting the joke off, you compare promise vs callback, with the 2 line or 1 line problem.

My coin is violation of DRY - 2x the same logic for async and sync result:

try {
  const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
  if (isPromiseLike(checkResult)) {
    checkResult.then(result => {
      if (!result) {
        return this.handleError(new AccessDeniedError(action), actionMetadata, action);
      } else {
        next();
      }
    }).catch(error => this.handleError(error, actionMetadata, action));
});
  } else {
    if (!checkResult) {
      this.handleError(new AccessDeniedError(action), actionMetadata, action);
    } else {
      next();
    }
 }
} catch {
  this.handleError(new AccessDeniedError(action), actionMetadata, action);
}

vs.

const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
Promise.resolve(checkResult).then(result => {
    if (!result) {
        return this.handleError(new AccessDeniedError(action), actionMetadata, action);
    } else {
        next();
    }
}).catch(error => this.handleError(error, actionMetadata, action));

try-catch, then-catch, if-else... really a mess 😉

Promise.resolve is repeated code as well

Only this.handleError(error, actionMetadata, action), but you have one flow for async and sync result.

And Promise.resolve is a simple solution when the super-top perf isn't critical. Now you can do the wrapper like that:

const promiseHelper = (maybePromise, callback) => {
    if (isPromiseLike(maybePromise)) {
        return maybePromise.then(callback);
    } else {
        callback(maybePromise);
    }
}

So all you need is to pass a handler callback:

i = 0;
console.time("promise helper")
const callback = (result) => {
    if (!result) {
        throw new Error("Bad result");
    } else {
        next();
    }

    if (++i === ITERATION_LIMIT) {
        console.timeEnd("promise helper");
        throw new Error("Loop end!");
    }
}
while(true) {
    const checkResult = authorizationCheckerSuccess();
    promiseHelper(checkResult, callback);
}

The results for 10 000 000 iterations are:

isPromiseLike: 1747.457ms
promise helper: 1640.235ms

So it's faster than the simply solution 😆 You can use lambda syntax (create handler inline as param) but then it's 3000 ms time.

Yes we could go back to write Assembly as well

This is the future, core parts written is WebAssembly! 🎉 (ironic, WebAssembly in Node.js 😆)

[NoNameProvided]

Exactly. So, I encourage you to do the real-case profiling to see all the bottlenecks and hot parts

While a part which runs once on every request is not super hot like a helper which would run a hundred time / request, but it's definitely hot for me. I think you miss a point, that these things adds up. A lot of small things will add up, a lot of little bit slower piece of code will make the overall app slow, while a lot of small optimization will make the overall app fast.

But you are right about real-case profiling, and about the thing that this is a small part, maybe there are other parts which needs way more optimization than this one.

My coin is violation of DRY

Thats a valid point, I agree.

Now you can do the wrapper like that

Can you show me how would you use promiseHelper in the actual code to help to reach your desired code format? Maybe it's just late, but I don't see how will that simplify the error handling.

[MichalLytek]

I like simple things:

try {
    const checkResult = await this.authorizationChecker(action, actionMetadata.authorizedRoles);
    if (!checkResult) {
        this.handleError(new AccessDeniedError(action), actionMetadata, action);
    } else {
        next();
    }
} catch(error) {
    this.handleError(error, actionMetadata, action);
}

I think I should fork this project and do my vision of refactor and profiling instead of waisting time in the comments discussing about Promise.resolve or 2/4 space indent 😞

[NoNameProvided]

We want back to the beginning, with await you expect authorizationChecker to return a promise, which should be but not assured to be the case.

I think I should fork this project and do my vision of refactor and profiling instead of wasting time in the comments discussing about Promise.resolve or 2/4 space indent

I am sad to hear, that you feel as waste of time, when you have to discuss various topics with other people. I respect your ideas, I think thats the reason why we discuss these things, if you cannot see the value in other people opinion and ideas, maybe forking is the way to go. I am sure @pleerock will merge every PR which adds value back to the project.

[MichalLytek]

with await you expect authorizationChecker to return a promise, which should be but not assured to be the case

Nope, await works with promise and non promise too 😉 Try it in console/node REPL:

(async () => console.log(await 5))()

I am sad to hear, that you feel as waste of time, when you have to discuss various topics with other people

It's not always a waste of time but recently I spend too much time on discussion like about indenting than the ones about features, PR reviews and issue resolving. I prefer doing and coding than deliberating how much Promise.resolve will theoretically slow than endpoint response time without doing profiling.

I have many ideas and features proposals but existing codebase is a kinda mess which is hard to develop and I would like to rewrite everything from scratch. My code-style and goals are different than the pleerock's ones so it has to be a fork... but maybe one day we will join like io.js and node.js 😉

[NoNameProvided]

Nope, await works with promise and non promise too

Wow I have never tried that, great to know!

It's not always a waste of time but recently I spend too much time on discussion like about indenting than the ones about features

Well lets spend you time in the threads which are interesting to you, thats a good way to prevent burning out 😛 I can see why talking about code formatting is not too exciting, but it's an importan part to prevent the project to become messy.

I have many ideas and features proposals but existing codebase is a kinda mess which is hard to develop and I would like to rewrite everything from scratch.

I dont see why we cannot do this in this repo as well. Yes it's true that sometimes as we can see, we are not on the same page, but I think these discussions are necessary. Otherway we would just throw in and out features without thinking over what it will cause. Eg interceptors make no sense to you but it turned out it is pretty usefull for others.

For the code style, I don't like most of them either, but these are preferences, if we would fork every repo which has a different code style then the one I like, the I would have like a million fork 😄

With forking you split our resources, I am pretty sure we can make this repo a great self driving oss project. Yes it needs refactor, yes it needs better organization, and more documentation why things work the way they do, but it will be great in the end. So I hope we can stay on the same train and make routing controllers great again 😄 (hehe)

[MichalLytek]

Going back to described problem - is it related to #240? @NoNameProvided

[NoNameProvided]

Not #240 but #233

[MichalLytek]

Right, so we can close this issue as the PR with fixes has been merged.

[NoNameProvided]

We still need to fix it for Koa, but we can track that in the PR

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.