fix(adapter-tests): Add tests for pagination in multi updates#2472
fix(adapter-tests): Add tests for pagination in multi updates#2472daffl merged 8 commits intofeathersjs:dovefeathersjs/feathers:dovefrom fratzinger:adapter-tests-paginatefratzinger/feathers:adapter-tests-paginateCopy head branch name to clipboard
Conversation
[pull] dove from feathersjs:dove
- typing: add AdapterTestName
|
Sorry just revisiting this. It's actually a good question. I always looked at it that multi |
|
Following up again, the problem is that you can't paginate a request that actually modifies things. So it definitely should return everything. |
|
I agree that the adapters should patch/remove everything. I think I was the one that initially mentioned "Not patching everything is probably good practice, thats why pagination exists and the user can disable it if needed". But I also agree it is counter intuitive. |
|
It might make sense to allow turning it off. Problem is that you won't get real-time updates then. Just to confirm though, this PR should be good to merge now right? Also, nice addition on the adapter name typings! |
|
Yes, I think so. It's weird, that I changed the adapter-test from testing against default pagination to no pagination and the test is still passing for Other thing. I thought about backporting the two tests to crow adapter-tests so we can add the tests to all common adapters sooner. What do you think? |
Background
This PR is coming from feathersjs-ecosystem/feathers-sequelize#364 and feathersjs-ecosystem/feathers-sequelize#363 . It's a discussion between me, @daffl
and @DaddyWarbucks if, on a service with default pagination, a
patch:multiwithout any paginate inparamsshould patch only a subset of items (default pagination) or all items (paginate: false). The same applies toremove:multi.In my opinion there are two points:
service.patch(null, { isConfirmed: true }, { query: { canBeConfirmed: true, isConfirmed: false }}? All items that fulfill this criteria or a maximum of 10 items (most default pagination). Default pagination forfindmethod is a no-brainer, because the structure changes from{ data, skip, total }toany[]and you can check wetherdata.lengthis the same astotal. Forpatch:multiandremove:multiyou only haveany[]either way. You don't know, if you patched/removed all items or not. This is a point forpaginate: falseas default forpatch:multiandremove:multipatch:multiandremove:multisometimes you want to find all items that will be changed/removed. I need that in my applications for permissions, changelogs (need to know the old value before the change). So I would expect that afindand apatch/removereturn the exact same items. This is a point forpaginate.defaultas default forpatch.multiandremove:multiAs point 1 as for
paginate: false, point 2 is stronger to me. Either way I'll add a hook to myapp.hooks.jsfile that ensures the same pagination behavior forfind,patch:multiandremove:multiSolution
While the discussion for or against
paginate: falseas default forpatch:multiandremove:multiis somewhat ongoing., we should ensure that all adapters act the same. That's where this PR comes into play. I added a standardized adapter-test to check this behavior. Asfeathers-sequelizehaspaginate.defaultas defaultpatch:multiandremove:multi, the test exactly checks against this behavior. I added the test to the internal@feathersjs/memoryand it fails becausegetEntriesis used in_patchwhich usespaginate: falseby default. So the behavior offeathers-sequelizeandfeathers-memoryis different in this case.I also added typings for
@feathersjs/adapter-testswithAdapterTestNamereplacingstringto prevent typos.Todos:
paginate: falseorpaginate.defaultshould be the default behavior. @daffl@feathersjs/memory(currentlypaginate: falseoverwrites anything confirmed) @fratzingerAdd a note in docs fornot needed for no pagination - since it's intuitivepatch:multiandremove:multiso it's clear for everybody @fratzingerremove:multi@fratzingeradapter-testspackage @dafflfeathers-sequelize(currentlypaginate.defaultdoes not overwrite anything confirmed) @fratzingerfeathers-objection(currentlypaginate.defaultdoes not overwrite anything first glance) @fratzingerfeathers-knex(currentlypaginate: falseoverwrites anything first glance) @fratzingerfeathers-mongoose(currentlypaginate: falseoverwrites anything first glance) @fratzingerfeathers-mongodb(currentlypaginate: falseoverwrites anything first glance) @fratzingerfeathers-nedb(currentlypaginate: falseoverwrites anything first glance) @fratzingerAdd a hook tonot neededfeathers-hooks-commonto ensure the same behavior forfind,patch:multiandremove:multi@fratzinger