add promise support

[brysgo]

See #7

• use async await this time

[chrismatix]

looks good to me. @makinde ?

[makinde]

Only took a glimpse. The code looks like it works.

The issue I'm concerned about is that right now the code calls getAuthLevels liberally without regard for how expensive the function is. For instance, if you do Model.find with the permissions option, you might end up calling getAuthLevels 5 or 6 times. If someone puts a query in there, that's gonna be really wasteful.

I think the way you'd want to do this is potentially adding another method that is preparePayload which would do all the queries once and then can be cached for the rest of that query. But I'm not convinced that extra complexity is worth it considering there's a reasonable alternative (below).

Could we move this to a 2.1 milestone? The way to do this right now is to do all the needed data fetching and put that info into the authPaylod before calling Model.find. in my use case I have a helper function that takes in an Express req object and produces the options dictionary the should be passed to all queries.

[brysgo]

It would make more sense to push the optimization to 2.1 since this would be a breaking change.

In my app we are using dataloader, so the optimization is handled on our end.

The solution described above presupposes you know all the information to authorize all the resources. This is not an assumption we can make, so we need to lazy load our authorization requirements.

[makinde]

Is you use case that you need to fetch more data based on the doc?

I'm supposing that simpkying allowing asynchronous functions is not the right API. that if we want to allow folks to do things asynchronously, it should be done as an explicit step. Of we allow/encourage folks to do queries in that method, their performance will be exexptectedly horrible for find queries . Imagine you did a find query that returned 50 objects, but then our framework did an extra 250 queries to handle authorization. It seems like setting that up as a possibility is really bad.

If you could share more about your use case, that'd be helpful.

[brysgo]

@makinde - I understand the concern, but it isn't like we are giving them an API where it fetches once asynchronously, the turning it into something that fetches 5 times. Simply providing a promise API does not mean that the user needs to run a query to get that Promise. A Promise simply represents an asynchronous action. I agree that consolidating our calls is the best course of action, but it is an implementation detail, not an API change. This change-set represents an API change, optimizations can come later.

The use case I have uses dataloaders, which are essentially lazyloaded database model calls. You can call them any number of times with the same id and they will give you the same model. They must still return a promise though, since they still need to get that model from the database in the first place.

[makinde]

Right, it makes sense that a promise isn't necessarily a DB call. That makes sense:

I agree that consolidating our calls is the best course of action, but it is an implementation detail, not an API change.

I'm suggesting a different API if we are going to consolidate calls. I'm suggesting that the developer implement another method in addition to getAuthLevel (perhaps prepareAuthPayload) and we'd only call that method once per doc.

The use case I have uses dataloaders

For the data loaders, do those depend on the specific document being checked? Or are those just based on the content of the authPayload?

[makinde]

Thanks for putting the time into this. I can see that it was a lot of code to pour through.

I too will probably need this very soon. Considering how much tweaking is required throughout the whole library to make a small function at the core asynchronous, I want to make sure to get the API right so it's not a PITA to maintain, so that's why i'm discussing this so much...and part of me want to see how far we can get before really opening this can of worms.

[makinde]

Why the approach of changes lines 7-17 as opposed to adding await just to this line?

[makinde]

otherwise it's ambiguous what exactly is in the authLevelsIn variable. it could be an array, or it could be a promise.

[brysgo]

A few reasons, I'm casting it right after and throwing away authLevelsIn, adding await Promise.resolve(...) to each line is not super easy to read, and last but not least, the first iteration was using .then() so this would have been a lot more challenging.

[makinde]

I see. Two things. You don't need an await in the first case. authLevels is just looking up a key in an object. Second, you don't need await Promise.resolve(foo)k...you can just write await foo;

[brysgo]

@makinde learned something new! await works on non-promise values

[makinde]

I don't think anything's a promise by this point.

[brysgo]

Good catch, probably changed my mind about the approach mid way through writing it.

[makinde]

You can break this up into separate statements so it's easier to read and less nested.

[brysgo]

I was just trying to change as little as possible, I don't usually use underscore/lodash. Happy to break it up though.

[makinde]

These functions would all need to async (and use await) as well, right? This is a little off since they use the next() function as well. Mongoose 5.x (I think) will see that a promise is returned and move along when the promise is resolved. But the code is written to wait for next() to be called. And Mongoose 4.x doesn't support promises from middleware, so there might be some inconsistent behavior.

[brysgo]

This should work as is, because we are just forwarding next it doesn't care about the promises.

[makinde]

You've got me thinking on overdrive here. I think the issue is that if one of the lower level functions throws an exception, then we'll get an UnresolvedPromiseRejection since the promise isn't tied all the way up. removeQuery is an async function that will return a promise, and that promise isn't handled.

You could poke at this and test it. I suspect you might need to do

removeQuery(this, next).catch(err => next(err));

It's a little jank since the non-error path passes the next function down, but the error path is handled up here as a promise. This actually might be a problem in the existing code, so I'm open to you highlighting that and us creating a new task to properly handle exceptions.

[brysgo]

I really appreciate the thought around the error path here. I think it is important and it is something I hadn't considered.

[brysgo]

I'm not sure I fully understand the problem you are outlining, but I would love to learn more.

[brysgo]

I think I see what you are saying, does mongoose do .catch() on returned promises?

[makinde]

The issue is that removeQuery returns a promise, right? It is an async function. If that promise rejects (let's say because an error gets thrown), node will stop because the promise rejects, but there's no .catch() on the promise. Try it out. just insert a throw in one of the helper functions. You'll get the following when you run npm test.

helpers.test.js
(node:46340) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): fpp
(node:46340) [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.

This sorta the challenge of this change. By adding an async function at the root, we're forced to change every function in the library to handle promises properly when the old code was mostly synchronous.

[makinde]

Any reason the return statements were shifted to their own lines?

[makinde]

Also, perhaps an easy way to save the result of resolveAuthLevel would be to save it in the options object as options.authLevel. Then when the function is called subsequent times, the value is cached already and it won't call the async function again.

I also wonder if there's a way to (in index.js to check if there's no authLevel provided AND there's a getAuthLevel. If so call the async function once. That way, the rest of the code can keep operating synchronously and not have to be async/awaited everywhere. Should make the code simpler and solve the caching concern.

@brysgo, what do you think?

[brysgo]

@makinde - that sounds like a good idea, we would still need to add code to every function though, and that code would need to be awaited.

As for the returns, returning the output of next creates ambiguity in the usage of the function. I wanted to be more clear that the return value wasn't being used so that the reader knows that it is purely a middleware function that passes a callback.

[makinde]

I just realized that what I purposed won't work. The result of getAuthLevel depends on the doc. Options is shared across all doc's.

In case there's a similar idea that would work...the idea is that the top level functions in index.js would compute the authLevels once, and then store it somewhere. That first call would need to use a promise. And then after that, everywhere else would just reference the stored version, which wouldn't need to involve promises. So them most of the code could avoid the need for promises.

As for the returns

Makes sense. Though I'm a little sad the the code gets longer :( Make sure to run eslint . as well.

[makinde]

@brysgo question still stands. Do the async data loaders you're working with depend on the doc?

[brysgo]

@makinde - they absolutely depend on the doc! This was one of the coolest parts of this library, I could get fine grained permissioning on a per doc basis for collections. It is a huge enabler.

Basically they check what role the user (from the authPayload info) has when operating on the given doc.

[makinde]

This is a really tough one for me. Thanks for taking so many passes at it.

I came across the memoizee library that could be useful here. Specifically, I think the weakMap configuration would skip the need for us need to clear the cache all the time. It also seems to support async functions, which is nice. _We'd need to figure out if the function is async, which is a little dicey in this situation. It's possible to detect async functions, but then that would leave out functions that return a promise, which feels strange. I think the mongoose way is to add a config flag where people specify if their callback is async.

We could either wrap getAuthLevel with the memoize function and stick it back into the schema (replacing the original function). Another approach is to wrap our own resolveAuthLevel function and leave

[brysgo]

@makinde - If we create a memoized function, we still need to pass it down. I honestly don't see what the problem is. I think we could do without the optimization personally, but if we have to do it, I don't see what the problem is with having to clear it out.

[brysgo]

Sorry if I came off as a little short, I just think there may be some element of over-engineering here.

The goal is to have an incredibly secure, easy to use API for validating things. I would be totally happy to offload performance concerns if you don't want to have calls to reset the cache, but I don't think it is something we need to kill ourselves over.

[brysgo]

I have to admit though, my ulterior motive is to have this library come out as something I can use at clients. It would be much easier to be able to point to an existing NPM module rather than having to maintain one myself.

[makinde]

Most definitely, I hear you. A lot of work is going into what seems like a small tweak here.

I'm trying to make sure we have reasonable performance for how people will likely write code. If someone puts a query in their async function (which is going to be my use case very soon), it would really suck to call it 5 times for every object returned on a find query.

I think the reason is doesn't matter much in your use case is that your data loaders are already cached. So as the client, you are doing your own memoization. Having every developer think about that seems like a bad API.

My last suggestion is a simpler alternative to the last commit you added. The rough additional code to helpers.js would be the following, which I think is shorter and easier to maintain than your last commit.

const memoizee = require('memoizee/weak');
const resolveAuthLevel = memoizee(
  (schema, options, doc) => {
    ...
  },
  { promise: true }
);

That's the point I was making. No clearing needed. No extra code inside index.js.

I totally realize it seems like I'm being a stick in the mud. If this library is going to be something I put effort into maintaining, my priorities are to make sure the code is easy to reason about/follow, and that we're adding to the API in a way that will be work for the general use case. It's a slow process because I'm not sure how to do this in a way that isn't slow in the common case.

I'm stuck at the airport right now so I'll put some thought into this and perhaps submit a diff. Hold tight.

[brysgo]

So this is no different than what I am doing, the difference is that this will leak memory like hell. The only reason I am clearing the cache is to prevent memory leaks.

TIL weak maps are garbage collected differently

This sounds like a great idea. You are awesome @makinde !

[brysgo]

I did this because weakmaps only support objects, I'm open to alternatives.

[makinde]

Okay, so I just landed from my flight. I took a pass at this and rejiggered the module to only call resolveAuthLevel once per query/operation. There are some places where this resulted in slightly more code, but overall it ends up being slightly simpler since not every function needs to be async.

I got my git all sorts of tangled, so I'll figure out how to send you some code to review and see what you think.

The other major thing is that I upgraded the min peerDepenency for mongoose to 5.x. Lmk if that's too much of a breaking change. It keeps the actual functions that get attached to the hooks a bit simpler.

[brysgo]

@makinde - I'm okay with limiting to mongoose 5

[chrismatix]

@makinde I am, too, but we should explicitly mention this in the README.md. Maybe in a "What's new in 2.0.0 section"

[brysgo]

Once the tests pass we should go with #28

[makinde]

@xChrisPx Updating the docs would be great. I'm also very open to reviewing pull requests. :) There's a lot of tests and docs to write, and there's where I could use the most support from you guys. New PR's on devcolor/mongoose-authz would be great!

[brysgo]

Closing this in favor of #28