Closed fratzinger closed 1 year ago
Does setting paginate: false
work?
this.service('posts').patch(
null,
{ title: 'test' },
{
query: { id: { $in: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] } },
paginate: false
}
);
Thanks for your quick response! Yep, it does. I've added this in a before-hook for multi-patches.
Do you think this should stay as it is? for find
the pagination is totally necessary, but for multi-patches I would say it's counter intuitive.
If you think we should leave this as is, maybe we should add a note to the readme?:
patch: [
context => {
if (context.id) { return context; }
context.params.paginate = false;
return context;
}
],
If setting paginate:false
works as expected, then I do think it should stay as is. But it should also be mentioned in the README. Good catch!
It's primarily a security concern IMO. Otherwise someone can overload the server/database by patching 1 million+ rows.
For example,
this.service('posts').patch(
null,
{ title: 'test' },
{
query: { created_at: { $gt: new Date('1/1/1900') } },
paginate: false
}
);
Be careful with that hook you mentioned in your comment, because it is susceptible to the "attack" above. Anything that allows the client to potentially disable pagination is a security risk.
Hm, I was expecting this to work as intended (patching/removing all items) because the _getOrFind
explicitly always sets paginate: false
: https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L114 but as pointed out, the filterQuery
is causing the problem here. I wonder if other adapters do this differently and how a test for this would look like.
It's totally a security issue. Maybe it's something for disablePagination
from feathers-hooks-common
(add 'patch'
to checkContext
)?
I understand the concerns. But patching the default amount of items and leave the rest as is leaves you behind with headaches. Maybe a better way would be to throw a BadRequest
if no $limit
and no paginate: false
is set and the idList.length
is larger than the default $limit
?
I also thought like @daffl because _getOrFind
sets paginate: false
. The idList
(https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L232) has the full list of ids (for the 1 million+ rows). So if it should stay as is, I think it's worth considering to integrate the filter to the request of idList
.
A generic test would start with more than 10 items. The problem is, that @feathersjs/adapter-tests
takes the servicePath. One option would be to fail, if the service doesn't have a options.paginate
property. Another option would be to clone the service and add the options.paginate
property to the cloned instance. But I haven't looked into @feathersjs/adapter-tests
that much.
Since the patch
returns all items that were patched, we could compare the length of items.
What do you think @daffl? Should I come up with a PR for @feathersjs/adapter-tests
?
This is completed in dove.
Multi-patch without
$limit
inparams.query
results in an incomplete operation, becausefilterQuery
adds the default$limit: -1
to the request. Not sure if this problem exists in multi-remove as well.I'll add this to
feathers-sequelize
and do a hacky PR for now. But the problem goes deeper, I think. I have something in mind for@feathersjs/adapter-tests
and@feathersjs/adapter-commons
:Question 1: Is this something we should add to
@feathersjs/adapter-tests
? I saw there are tests for multiple items (count = 2) but not for many items, at least > 10.I find this behavior of
filterQuery
from@feathersjs/adapter-commons
quite confusing, although I understand, why it is like it is. In another project I ended up deleting the default$limit: 10
fromfilterQuery(params)
ifparams.query.$limit
is not the same. Not the nicest solution, but the quickest. Question 2: Is this maybe worth a property for secondoptions
argument offilterQuery
? Something like:filterQuery(params, { addDefaultLimit: false }
?I will look into this in
feathers-sequelize
for now. @daffl if you answered one of the two bold questions with "yes", I would be happy to help. Just let me know!Steps to reproduce
The error comes from this line: https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L235
filterQuery
adds$limit: 10
for default pagination, although no$limit
is defined in params.idList
from https://github.com/feathersjs-ecosystem/feathers-sequelize/blob/master/lib/index.js#L232 has the full list of all items.Expected behavior
this.service('posts').patch(null, { title: 'test' }, { query: { id: { $in: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] } } })
should patch 12 items and return 12 itemsActual behavior
patches 10 items and returns 10 items with default pagination:
System configuration
Module versions (especially the part that's not working):
"feathers-sequelize": "^6.2.0"
NodeJS version: 12