feathersjs-ecosystem / feathers-hooks-common

Useful hooks for use with FeathersJS services.
https://hooks-common.feathersjs.com
MIT License
193 stars 89 forks source link

Hook so create acts as an upsert. #99

Closed eddyystop closed 2 years ago

eddyystop commented 7 years ago

Idea from a discussion on #general

"Say you get a CREATE request for IDs [1, 2, 3, 4, 5]. You call FIND with those IDs which tells you that IDs 3 and 5 already exist. You'd then call PATCH with [3, 5] and CREATE with [1, 2, 4]"

camstuart commented 7 years ago

+1 I agree with this idea.

But I would recommend a hook argument that describes the param used to determine if we are doing a create or a patch.

So, for example,

module.exports = {
  before: {
    all: [],
    find: [],
    get: [],
    create: [upsertServiceRecordBykey({ updateByKey: 'someField' })], // or typically: upsertServiceRecordBykey({ updateByKey: '_id' })
    update: [],
    patch: [],
    remove: []
  },

  after: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  },

  error: {
    all: [],
    find: [],
    get: [],
    create: [],
    update: [],
    patch: [],
    remove: []
  }
}

This way you can tell the hook what to use in it's where clause..... even better yet, perhaps an array of naturalKeys. so a compound where cleause can detrmine if we need to update, or insert a new record.

I came to this issue while looking for a common hook that behaves this way.

I'll have a go at writing my own now, and keep you posted here

camstuart commented 7 years ago

This is what I have so far:

module.exports = function (options = {}) { // eslint-disable-line no-unused-vars
  return function (hook) {
    // Hooks can either return nothing or a promise
    // that resolves with the `hook` object for asynchronous operations

    const query = {}
    query[options.updateByKey] = hook.data[options.updateByKey]

    return hook.app.service(options.service)
      .find({ query }).then(page => {
        if (page.total === 0) {
          console.info(`No existing record found in service: ${options.service} with key: '${options.updateByKey}:${hook.data[options.updateByKey]}'`)
          return Promise.resolve(hook)
        } else {
          console.info(`Existing record found in service: ${options.service} with key: '${options.updateByKey}:${hook.data[options.updateByKey]}'`)
          hook.app.service(options.service).update(page.data[0]._id, hook.data)
          // How do I stop the create from carrying on?
        }
      })
  }
}

Called like so: upsertServiceRecordBykey({ service: 'stocks', updateByKey: 'code' })

But how do I stop the create from carrying on?

eddyystop commented 7 years ago

Read this https://docs.feathersjs.com/guides/step-by-step/basic-feathers/writing-hooks.html#returning-a-result

camstuart commented 7 years ago

Makes sense @eddyystop. Ended up with this working example:

module.exports = function (options = {}) {
  return function (hook) {
    if (options.updateByKey && options.updateByKey) {
      const query = {}
      query[options.updateByKey] = hook.data[options.updateByKey]

      return hook.app.service(options.service)
        .find({ query }).then(page => {
          if (page.total === 0) {
            console.info(`No existing record found in service: ${options.service} with key: '${options.updateByKey}:${hook.data[options.updateByKey]}'`)
          } else {
            console.info(`Existing record found in service: ${options.service} with key: '${options.updateByKey}:${hook.data[options.updateByKey]}, updating...'`)
            hook.app.service(options.service).update(page.data[0]._id, hook.data)
            hook.result = page.data[0]
          }
          return Promise.resolve(hook)
        })
    } else {
      return Promise.resolve(hook)
    }
  }
}

I'd like to improve this in two ways;

Thoughts?

eddyystop commented 7 years ago
camstuart commented 7 years ago

Thanks for the pointers @eddyystop !

Here is the latest implementation that determines the primary key's field name and allows for multiple lookup keys:

Hook usage:

Context, I have a service called stocks that has the fields: code, name, sector

When a stock is posted, I only want a new record when there are no the records with the matching fields code & name, otherwise, an update is required.

In stock.hooks.js

before: {
    all: [],
    find: [],
    get: [],
    create: [upsertServiceRecord({ service: 'stocks', updateByKeys: ['code', 'name'] })],
    update: [],
    patch: [],
    remove: []
  },

The Hook itself:

/*
 * A hook to update a record instead of creating a new one, based on an array of keys
 */
module.exports = function (options = {}) { // eslint-disable-line no-unused-vars
  return function (hook) {
    if (options.updateByKeys && options.updateByKeys.length > 0) {
      const primaryKeyField = hook.app.service(options.service).id

      const query = {}
      for (const updateByKey of options.updateByKeys) {
        query[updateByKey] = hook.data[updateByKey]
      }

      return hook.app.service(options.service)
        .find({ query }).then(page => {
          if (page.total === 0) {
            console.info(`No existing record found in service: ${options.service} with key(s): '${options.updateByKeys.join(', ')}'`)
          } else {
            console.info(`Existing record found in service: ${options.service} with keys(s): '${options.updateByKeys.join(', ')}' updating...`)
            hook.app.service(options.service).update(page.data[0][primaryKeyField], hook.data)
            hook.result = page.data[0]
          }
          return Promise.resolve(hook)
        })
    } else {
      return Promise.resolve(hook)
    }
  }
}

The body of a post request:

{
    "code": "BHP",
    "name": "BHP BILLITON LIMITED",
    "sector": "Materials"
}

When the above json is posted, a new record is created. If it is posted again, and update is performed.

I'd really like to get better at writing tests for feathers apps, is there more examples/tutorials or docs to help me with this?

Once tested further, would the feathersjs team consider accepting a PR of this hook?

eddyystop commented 7 years ago

feathers-community-hooks exists and will be mentioned in the next monthly Feathers newsletter. We hope many useful hooks such as yours will be contributed. I hope you get yours in in time for the newsletter so readers can see it.

gabrielstuff commented 5 years ago

This upsert service is nice. A little issue when the pagination is not set but it works fine. Do you think it will be added to the plus/commons ? I id not find it there: https://feathers-plus.github.io/v1/feathers-hooks-common/

eddyystop commented 5 years ago

You can propose a PR

gabrielstuff commented 5 years ago

What is the best way to cover this hook with a test ? Any direction to point me to? In order to make sure the PR will be clean and accepted ?

G Le mar. 18 déc. 2018 à 19:55, Eddyystop notifications@github.com a écrit :

You can propose a PR

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/feathers-plus/feathers-hooks-common/issues/99#issuecomment-448330200, or mute the thread https://github.com/notifications/unsubscribe-auth/AARZaZZl5DSI3rm0gxOVq4b7KQTCyDFaks5u6TohgaJpZM4Ldvls .

-- 👌

eddyystop commented 5 years ago

There are existing DB tests, for example stash-before.test.js. The docs are in feathers-plus/docs.

jcwren commented 5 years ago

Being rather new to the FeatherJS ecosystem, I'm sure I'm doing something wrong, but using the upsert function in https://github.com/feathers-plus/feathers-hooks-common/issues/99#issuecomment-314963532, I'm getting both the created and updated events firing. I was hoping that only one or the other would fire. Is both events firing expected, and any advice on how to work around that?