Add ctx. isNewInstance to "before save" and "after save" hooks

[pungoyal]

Extend before save and after save hooks and add a new property ctx.isNewInstance (or similar).

This new property will have one of the following values for Model.updateOrCreate:

• before save:
  • true when the data has not id and the method falls back to a regular create
  • true/false when a non-atomic implementation is used
  • undefined when an atomic implementation is used
• after save:
  • true when the connector reported that a record was created
  • false when the connector reported that a record was updated
  • undefined when the connector did not report anythin

A thing to figure out: how to handle atomic findOrCreate, as it may fire the before save hook even if the model already exists?

---

The original issue description

Does it make sense to have a created event on model? Currently we have changed, but that fires for any changes to the model.

[clark0x]

I have same question. Currently, I fire a created event in afterCreate hook and an updated event in afterUpdate hook in all my models.

I lookup the PR history, this loopbackio/loopback-datasource-juggler#65 introduces changed, deleted, deletedAll by @ritch .

To @ritch so, what do you think?

[ritch]

+1 for created event

[bajtos]

This should be a reasonably easy change, would you (@pungoyal or @clarkorz) mind to contribute it?

[glesage]

A state other than beforeCreate and afterCreate ? (which already exists)

[bajtos]

It is probably better to defer this enhancement until loopbackio/loopback-datasource-juggler#367 is implemented.

[bajtos]

The new Operation hooks implemented by loopbackio/loopback-datasource-juggler#367 provide after save hook which does not distinguish between created and updated. One of the reasons is that this information may not be available from updateOrCreate (a.k.a upsert and HTTP PUT) implemented by connectors as an atomic operation.

However, if you are able to come up with a way how to distinguish between CREATE and UPDATE that works in all connectors, then I am ok with adding a new property to the ctx object passed to after save hook and use this property to decide which event to fire.

Another alternative is to let updateOrCreate() trigger always updated event, even in the case where a new model instance was created.

Thoughts?

[clark0x]

@bajtos loopback-connector-mongodb implements updateOrCreate by using findAndModify. The result that findAndModify command responses could be use to distinguish FIND, CREATE, and UPDATE.

For loopback-connector-mysql, I am not familiar with mysql, but I do find a hint in mysql document:

With ON DUPLICATE KEY UPDATE, the affected-rows value per row is 1 if the row is inserted as a new row, 2 if an existing row is updated, and 0 if an existing row is set to its current values.

I don't study all others connectors, but can we tolerate that some connectors could distinguish between CREATE and UPDATE while others couldn't?

[bajtos]

@clarkorz thank you for making the research.

Based on what you have described above, I am proposing the following implementation: extend after save hook and add a new property ctx.wasCreated (or ctx.isNewInstance or similar).

This new property will have one of the following values for Model.updateOrCreate:

1. true when the connector reported that a record was created
2. false when the connector reported that a record was updated
3. undefined when the connector did not report anything

The benefit of such solution is that a simple check if (!ctx.isNewInstance) will catch both 2 & 3, while it is still possible to distinguish between 2 and 3 if needed.

Of course, methods that already know what kind of write is being performed (Model.create, Model.updateAll, etc.) will set this flag independently on the connector implementation.

Thoughts?

/cc @raymondfeng @ritch

[clark0x]

LGTM

Besides the new hooks, should we complete the event system in advance? To specific, should we replace changed event with created and updated events, or pass a similar flag (as you describe above) when emitting changed event?

[bajtos]

We need to preserve the changed event for backwards compatibility.

The choice is then changed with flag, or emit both changed and created/updated. I slightly prefer the flag as it is consistent with after save, but I don't really mind either option.

[kidwm]

Sorry, but I have a newbie question, since then how do old model hook afterCreate work?

[bajtos]

how do old model hook afterCreate work?

It is called only when you invoke Model.create or when you invoke Model.updateOrCreate without model id. When you call updateOrCreate with an id that does not exist, a new model instance is created but the hook afterCreate is not called at all. This is one of the bugs in the old hooks that is fixed by the new Operation hooks.

[bajtos]

See #1148 for an example of multiple different approaches for converting the old "beforeCreate" hook into the new "before save" hook.

[fabien]

@bajtos I would go with ctx.isNewInstance and the three values you proposed: true, false, undefined

[fabien]

@bajtos could you please summarize which context property name and values you expect? I think I'll need this soon as well.

[bajtos]

@bajtos could you please summarize which context property name and values you expect? I think I'll need this soon as well.

I am not sure what you are asking me. Your previous comment described exactly what I would like to see implemented:

I would go with ctx.isNewInstance and the three values you proposed: true, false, undefined

I am ok with a partial implementation that does not include necessary changes in all connectors, as long as the behaviour is reasonable in the case when the connector does not provide the flag yet. It's also ok to implement this flag for one of the hooks only, e.g. "before save", and leave the other hook for later.

Last but not least, there is no need to implement this for the "changed" event, since it has been already deprecated in favour of the new Operation hooks.

[notbrain]

As part of this I would love to see the operation hooks expanded to include 'after create' and 'after update' that would hide the implementation of isNewInstance and the checking that it entails. If you need specific detail you can use 'after save' and then inspect ctx.isNewInstance for the values but straight up usage for the simple difference between create and update would be +1 in my book. Even if they are just aliases to 'after save' with the requisite checking.

[bajtos]

I have landed loopbackio/loopback-datasource-juggler#515 which added ctx.isNewInstance property and implemented support for that in the memory connector. Now we need implement this feature in all connectors, any volunteers?

[bajtos]

Connectors supported:

• Memory (built-in)
• MongoDB via Support ctx.isNewInstance loopbackio/loopback-connector-mongodb#121
• MySQL via Return isNewInstance from upsert loopbackio/loopback-connector-mysql#91

I am closing this issue are solved, we will keep track of the remaining work in connector-specific issues.