Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mongoose type "accessibleBy" returns an array of document after findOne #662

Closed
gterras opened this issue Aug 16, 2022 · 21 comments
Closed

Comments

@gterras
Copy link

gterras commented Aug 16, 2022

Model.accessibleBy from @casl/mongoose returns an array of document which makes it hard to deal with with operations such as findById and following:

        type CatDocument = Cat & Document

 	getById(id: MongooseTypes.ObjectId, ability: Ability): Promise<CatDocument> 
		return this.catModel
			.findById(id)
			.accessibleBy(ability, Action.Read)
                        // function return type now needs to be Promise<CatDocument[]>
			.exec()
	}

Update operations after find one (is there a better way to check ability before update?) trigger an error since it expects an array:

	async update(payload: UpdateCatInput, ability: Ability) {
		const doc = await this.catModel
			.findById(payload.id)
			.accessibleBy(ability, Action.Update)
			.exec()

		if (doc) return doc.set(payload).save() 
                // Error : Property 'set' does not exist on type '(Cat & Document<any, any, any> & { _id: ObjectId; })[]'
	}
@stalniy
Copy link
Owner

stalniy commented Aug 16, 2022

Just change the order of methods accessibleBy().findById()

@stalniy stalniy closed this as completed Aug 16, 2022
@stalniy
Copy link
Owner

stalniy commented Aug 16, 2022

Works as expected

@gterras
Copy link
Author

gterras commented Aug 16, 2022

Changing the order works with find:

return this.catModel
	.accessibleBy(ability, Action.Read)
	.find(filters)
	.exec()
	// OK

but triggers an error with findById

return this.catModel
	.accessibleBy(ability, Action.Update)
	^^^^^^^^^^^^ 	// Error
	.findById(id)
	.exec()
Property 'findById' does not exist on type 'QueryWithHelpers<(
Cat & Document<any, any, any> & { _id: ObjectId; })[], 
Cat & Document<any, any, any> & { _id: ObjectId; }, 
AccessibleRecordQueryHelpers<Cat & Document<...> & { ...; }, {}, {}, {}>, 
Cat & ... 1 more ... & { ...; }>'.

Model injected by NestJS with

private catModel: AccessibleRecordModel<CatDocument>

@stalniy
Copy link
Owner

stalniy commented Aug 16, 2022

works fine with pure mongoose and ts:
Screenshot 2022-08-16 at 22 04 34
Screenshot 2022-08-16 at 22 04 27

@stalniy
Copy link
Owner

stalniy commented Aug 16, 2022

Ah OK, there is no findById but this question should be addressed to mongoose.Query typings

@jitbasemartin
Copy link

I have the same problem, @gterras do you found a fix ?

@gterras
Copy link
Author

gterras commented Aug 18, 2022

I opened an issue at mongoose Automattic/mongoose#12286

As a workaround you can use any findOne*method since findById is just an alias.

@gterras
Copy link
Author

gterras commented Aug 22, 2022

FYI this has been fixed in mongoose Automattic/mongoose#12309

@stalniy
Copy link
Owner

stalniy commented Aug 23, 2022

Awesome! Thanks for pushing this!

@danielbayerlein
Copy link

danielbayerlein commented Oct 5, 2022

Does the problem exist with every .findById* method?

TypeError: _models.ExampleModel.accessibleBy(...).findByIdAndDelete is not a function

TypeError: _models.ExampleModel.accessibleBy(...).findByIdAndUpdate is not a function

TypeError: _models.ExampleModel.accessibleBy(...).findById is not a function

If I change the order (e.g. findByIdAndUpdate(..).accessibleBy(...)), it works.

package.json

"@casl/ability": "^6.3.0",
"@casl/mongoose": "^7.1.0",
"mongoose": "^6.6.4",

@stalniy
Copy link
Owner

stalniy commented Oct 5, 2022

Does it work if you do .find().findByIdAndDelete?

@danielbayerlein
Copy link

@stalniy Unfortunately no.

TypeError: _models.ExampleModel.accessibleBy(...).find(...).findByIdAndUpdate is not a function

@danielbayerlein
Copy link

The findOne* methods works fine.

@danielbayerlein
Copy link

@stalniy Should we reopen the issue?

@stalniy
Copy link
Owner

stalniy commented Oct 10, 2022

looks like it's not a casl issue but mongoose one. Types say this method exists but when I run:

Post.find().findByIdAndDelete();

I get TypeError: Post.find(...).findByIdAndDelete is not a function. This means mongoose.Query doesn't have this method anymore, either by mistake or by purpose

@danielbayerlein
Copy link

danielbayerlein commented Oct 10, 2022

Post.findByIdAndUpdate(..).accessibleBy(...) would work, but returns the wrong type. Is this a bug in casl?

@gterras
Copy link
Author

gterras commented Oct 10, 2022

accessbleBy will return an array of type, just switch the order and this will be fixed. As for findByIdAndDelete seems like it's missing a few types declaration https://github.com/Automattic/mongoose/blob/8ea0a44b8ee6e5580a18621762702033a1984d57/types/query.d.ts#L389 you can open a ticket there.

@danielbayerlein
Copy link

danielbayerlein commented Oct 10, 2022

@gterras The problem is not the type, see #662 (comment) and #662 (comment).

@stalniy
Copy link
Owner

stalniy commented Oct 10, 2022

@danielbayerlein do you have suggestion how to fix this?

Because this is how mongoose types designed, for example find explicitly returns an array of items without not carrying what was the return type of prev query. The same with findOne and co. That's why accessibleBy returns an array because then it's easy to convert to single record using findOne.

Want many use Post.accessibleBy(...) want one use Post.accessibleBy(...).findOne({ _id: ... }).

If you know how to make types work, so that they can rely on prev constructs, then awesome, let's fix this because I think it's impossible in how mongoose currently defines its types

@gterras
Copy link
Author

gterras commented Oct 10, 2022

find().findByIdAndDelete() is not found but find().findOneAndDelete() is. I would advice not using these findById shorthands as they often create problems.

@gterras
Copy link
Author

gterras commented Nov 14, 2022

any explanation why so? because I start facing this issue when I upgraded mongoose to it's latest.

Explained just above:

Because this is how mongoose types designed, for example find explicitly returns an array of items without not carrying what was the return type of prev query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants