feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.04k stars 751 forks source link

Mongo .update with query #3363

Closed DaddyWarbucks closed 4 months ago

DaddyWarbucks commented 10 months ago

The update method is currently breaking when using data that affects the query.

For example,

const dave = await app.service('people').create({ name: 'Dave' })

// Now mutate AND query the name property.
const result = await app
  .service('people')
  .update(dave._id, { name: 'Marshal' }, { query: { name: 'Dave' } })

The problem originates here: https://github.com/feathersjs/feathers/blob/fe072a26b64eaab1d659bc94ba98a404de663e1f/packages/mongodb/src/adapter.ts#L376

We have updated the data, but then this following _findOrGet is using the original query. Because our data mutated a value in that query, the request throws a NotFound (even though the record was updated); This likely needs to be handled similarly to patch/remove.

I am currently working on a mongoJoinQuery for a project that will ultimately end up in feathers-fletching. So I am taking a deep dive in the new Mongo adapter. I will eventually create a PR for this problem and any others I stumble across.

DaddyWarbucks commented 10 months ago

Furthermore, this should probably be a test in the common adapters test suite.

daffl commented 10 months ago

Ah, same problem we had with .patch where it now uses a list of original ids instead. Since update doesn't support multiple updates, should it just remove the query?

return this._findOrGet(id, {
  ...params,
  query: {}
}).catch(errorHandler)
DaddyWarbucks commented 10 months ago

I like the query because it throws a NotFound actually. I often use this for some security benefits where I stub on some extra params in the query serverside.

// Serverside hook after auth hook
const permissionsHook = (context) => {
  context.params.query.organizationId = context.params.user.organizationId
}
// This will throw a NotFound it item with id: 1 is not in my organization
const item = await app.service('items').get(1)
DaddyWarbucks commented 10 months ago

I am taking a deep dive on this new version of the adapter because I need some fixes/features for a particular project. See: https://github.com/feathersjs/feathers/compare/dove...DaddyWarbucks:feathers:mongodb/aggregate

So I will open a PR soon with

DaddyWarbucks commented 4 months ago

Closed via https://github.com/feathersjs/feathers/pull/3366