Bug: patch:multi only patches 10 items with default pagination

@daffl

Multi-patch without $limit in params.query results in an incomplete operation, because filterQuery 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 from filterQuery(params) if params.query.$limit is not the same. Not the nicest solution, but the quickest.
Question 2: Is this maybe worth a property for second options argument of filterQuery? 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 items

Actual behavior

patches 10 items and returns 10 items with default pagination:

"paginate": {
  "default": 10,
  "max": 100
},

System configuration

Module versions (especially the part that's not working):
"feathers-sequelize": "^6.2.0"

NodeJS version:
12

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.

@DaddyWarbucks

@DaddyWarbucks

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.

@daffl

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?

Maybe relevant: #405

This is completed in dove.

Navigation Menu

Repository navigation

Bug: patch:multi only patches 10 items with default pagination #363

Steps to reproduce

Expected behavior

Actual behavior

System configuration

@DaddyWarbucks

@daffl

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Bug: patch:multi only patches 10 items with default pagination #363

Description

Steps to reproduce

Expected behavior

Actual behavior

System configuration

Activity

DaddyWarbucks commented on Apr 7, 2021

fratzinger commented on Apr 7, 2021

DaddyWarbucks commented on Apr 7, 2021

daffl commented on Apr 7, 2021

fratzinger commented on Apr 7, 2021

@DaddyWarbucks

@daffl

DaddyWarbucks commented on Nov 16, 2022

fratzinger commented on Apr 3, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions