fix(memory/mongodb): $select as only property & force 'id' in '$select'

[fratzinger]

Quick fix for @feathersjs/memory.

First commit is the test which should work but fails without the second commit.

The problem was, that select(...) only was called if there was $sort, $limit or $skip in the query.

While being at it:

‼️ diffing behaviour of $select also returning id, also if not added explicitely.

• memory: has a test to not select id automatically, which is crucial for crud! https://github.com/feathersjs/feathers/blob/dove/packages/memory/test/index.test.ts#L193 I think this was introduced by recent changes from @DaddyWarbucks
• knex: adds id automatically, if it's not provided: https://github.com/feathersjs/feathers/blob/dove/packages/knex/src/adapter.ts#L128
• mongodb: same as knex: https://github.com/feathersjs/feathers/blob/dove/packages/mongodb/src/adapter.ts#L203
• I can confirm, that feathers-sequelize also adds id

I think that should be tested by @feathersjs/adapter-tests. We have it in:

• .find + $select not included ❌ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/syntax.ts#L129
• .get + $select: included ✅ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/methods.ts#L32
• .create + $select: not included ❌ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/methods.ts#L620
• .patch + $select: not included ❌ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/methods.ts#L329
• .update + $select: not included ❌ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/methods.ts#L247
• .remove + $select: not included ❌ https://github.com/feathersjs/feathers/blob/dove/packages/adapter-tests/src/methods.ts#L97

This PR adds explicit tests to @feathersjs/adapter-tests and makes sure, that all tests pass for these linked tests.

[fratzinger]

This came up while porting feathers-graph-populate to dove (cc @marshallswain). Also for feathers-trigger.

This stops me from releasing dove versions of these two packages.

[daffl]

I think that makes sense. It looks like the virtual property resolver test needs to be updated accordingly. I'm also wondering if this could be considered a breaking change 🤔

[DaddyWarbucks]

Sorry gents! Glad yall got it worked out.

[fratzinger]

It looks like the virtual property resolver test needs to be updated accordingly.

The test expects two properties (I guess 'user' and 'text', but it's not clear). In reality it only returns 'text' (I double checked that). To get that fixed, I check now, if $select has virtual properties and if so then skip $select for adapter entirely. In some way this is, how the test was 'working' before.

I'm also wondering if this could be considered a breaking change

No, not at all. This was how every adapter was working in v4, even feathers-memory. Only @feathersjs/memory introduced a test to explicitly not adding service.id.