Access core in hooks

[louy]

Correct me if I'm wrong, but I think there's no way to access Core (and therefore any Model) from hooks. If I want to use the instance db that I create by calling new Core(...) I'd have a circular dependency as models must be defined before Core. (I have them in separate files)

My suggested solution is to pass core as the last argument to all hooks. Like this:

interface Hooks<TDocument, TInstance, TCore> {
  onCreating? (document: TDocument, core: TCore)
}

[louy]

Use-cases for this:

• When changing a User's password, I want to remove all Sessions linked to that user.
• When creating a new Session, I want to remove all previous Sessions which have an identical UDID attribute.

[notheotherben]

Hi @louy,

You're not wrong, however you should be aware that hooks are intentionally incapable of performing blocking async operations (a heavy handed way of preventing people from adversely affecting performance accidentally).

Depending on your architecture, this could be best handled by simply providing a changePassword method which explicitly handles this - or if it really is something you feel strongly about, we can certainly discuss incorporating the functionality again.

[louy]

I understand what you mean, but to be honest, I feel this is what hooks are supposed to do. All the major ORMs are allowing async hooks. node-orm, mongoose and sequelize to name a few. I feel that wrapping everything with methods is more like not having hooks at all.

Moreover, I prefer to unify logic for all my models in controllers. I wouldn't want to call a method like createSessionAndRemoveOldOnes() in a controller when I can just say new Controller('/sessions', db.Session) and all my routes get generated automatically.

[louy]

Anyway, I guess this issue is about accessing core which is kind of irrelevant to async hooks, so let's continue this discussion in #19.

[notheotherben]

Back onto this topic, adding a core parameter is probably the cleanest approach - but it does represent a change to the external API (so in other words, major version bump) while also not carrying through the TCore properly (there is currently no way to convey this information to the hook implementation without creating the circular dependency you mentioned).

Come to think of it, I can't find a particularly nice way of solving this particular problem using hooks - since the approach taken to implement them just doesn't really gel well with this particular use case. Perhaps a better solution would be to implement a custom Instance base for your models which has overridable preSave() and postSave() methods - something I can probably make easier for you with some changes to the current Instance implementation, without making any backwards incompatible changes to the API.

Let me think on it a little bit more and I'll get back to you

[notheotherben]

Coming back to this following #22 - the use case you're referring to here is very much a relational one. While there are undoubtedly situations in which you need to operate relationally, I feel that (at least in your case) it would probably make more sense to simply embed the user sessions within your user document.

For example, this is a rough structure for a user document which would make use of a synchronous onSaving hook to ensure that all user sessions are invalidated whenever you change the password.

export interface UserDocument {
  _id: string;
  email: string;
  passwordHash: string;
  passwordSalt: string;
  sessions: {
    id: string;
    expires: Date;
  }[];
}

@Iridium.Collection("user")
@Iridium.Index({ email: 1 }, { unique: 1 })
export class User extends Iridium.Instance<UserDocument, User> implements UserDocument {
  /*
   * @Iridium.Property(...) entries for each schema property
   */

  static onCreating(document: UserDocument) {
    document.sessions = document.sessions || [];
  }

  static onSaving(instance: User, changes: any) {
    if(changes.$set && changes.$set.passwordHash) {
      changes.$set.sessions = [];
    }
  }
}

Still just trying to avoid any potentially breaking changes to the API since it is rather disruptive to the user base as a whole - will keep trying to think on a way to provide that sort of functionality without breaking other stuff - but a quick solution, assuming you don't have more than one Iridium.Core instance in your app, would be to add the following to your models:

export class MyModel<TDocument, MyModel> extends Iridium.Instance<TDocument, MyModel> {
  constructor(model, document, isNew, isPartial) {
    super(model, document, isNew, isPartial);
    MyModel.model = model;
  }

  static model: Iridium.Model<TDocument, TInstance> = null;

  static onReady(instance: MyModel) {
    MyModel.model.core.MyOtherModel.remove();
  }
}

[louy]

So yesterday I did create a patch to solve this issue, but I wasn't able to. You'll always end up with a circular dependency. (Not that it's not working, but I don't like it) I was also experimenting with accessing the "model" object and I wasn't able to, but now you've just shown me how. What I ended up doing is really just importing core back to the model. It worked and I got the type correctly. However to be honest, I found the way models/interfaces/documents work a bit confusing. I suggest unifying Iridium.Model and Iridium.Instance into one class, using static methods for Iridium.Model and prototype methods for Iridium.Instance. Should make things way much clearer. Maybe in the next major release?

For now, I think I can create a quick PR to pass core to hooks without breaking anything (probably setting this.core = model.core in constructor). But as there's a workaround for this right now I can wait for the next major release. Thanks for your effort by the way. Great work 😊

[notheotherben]

Unfortunately that is the case, the best solution is (if you're happy to work with a singleton) to move the model (which has a reference to core) into the static domain. That being said, it is fragile, since you can very quickly lose access to a core if you instantiate more than one within your app domain.

The reason that Model and Instance are separate is that they represent separate concepts (though perhaps the naming could be better). Model is probably more akin to the MongoDB collection concept, while Instance represents a single document - they are also kept separate so that it is possible to replace the Instance type with whatever you like should you require any custom logic or behaviour.

Will definitely look at maybe doing some renaming in the next major release, since it can get a bit confusing for people who are new to Iridium - will need to see what other users think.

Keep in mind that setting this.core in the Instance constructor won't actually make that property available in your hooks, since the hooks execute in a different context to the Instance (i.e. this in the hooks is undefined). You have to pass it through as a static property, which requires that you implement it on each and every model you create, otherwise there will be collision between them. Of course, setting core minimizes the effect of this provided you only have a single Iridium.Core instance at a time.

[louy]

Oh. You're right. Well then I guess the only thing we can do is pass it as the last argument to hooks. This won't be a breaking change AFAIK. The ModelHandlers class receives the model object already, so maybe we can grab core there and store it as a private property inside ModelHandlers, and then pass it to hooks?

I get the idea behind Instance & Model, but even mongoose uses the same class for both (which is probably why I got confused). Anyway, I personally think using an Instance class for two models is a bad idea and shouldn't be supported. I've never had a case where I needed two collections of the same model in the same application. However, the part about replacing Instance with something else is a good thing (as someone might want to follow a different approach here instead and replace Iridium.Instance with Object.)

[louy]

Here's a common use case for this, #20 & #23: Auto-increment fields. In mongoose what you'd do is create a new collection called "counters" which is an object of the shape:

interface ICounter {
  collection: string;
  seq: number;
}

Then you set id field of each document onCreating and increment collection seq value onCreated.

[louy]

By the way, this is the officially recommended way to implement auto-increment.

However, there's another issue. In onCreating hook you have no way of knowing which "collection" you're hooked to, and so you won't be able to inherit from a class implementing auto-increment.

[notheotherben]

While I can certainly see where you're going with that, the official recommendation also clearly states that:

Generally in MongoDB, you would not use an auto-increment pattern for the _id field, or any field, because it does not scale for databases with large numbers of documents. Typically the default value ObjectId is more ideal for the _id.

With Iridium the idea was always to avoid placing an emphasis on higher-order abstractions, especially for suboptimal patterns, and definitely not to try and emulate the functionality of a SQL database (which ORMs like node-orm2 do) - instead focussing on providing the lightest weight wrapper over the MongoDB client while still offering productivity enhancements and the ability to keep an API model in your head (i.e. I didn't want something which required you to consult the documentation each time you wanted to do something).

As far as the post-operation hooks you mention in #23 - it was something considered in v4, but subsequently removed as they didn't add any functionality unavailable in v5 and v6 with good schema design. Specifically, it made more sense to ensure that the hooks were triggered in a predictable and logical order, such that their side effects could have a meaningful effect on the outcome of your query should you require it.

For example, the onCreating hook is triggered before the document is committed to the database, allowing you to configure default values or compute aggregates. The onSaving hook is triggered after the diff has been generated (if necessary) and allows you to enforce certain constraints like lastChanged without requiring additional queries. onRetrieved executes after a document is received from the database, allowing you to perform some preprocessing and finally onReady is executed after the received document has been converted into a TInstance type using the constructor.

Remember that any situation in which you are using hooks to execute database changes generally means your schema is too-normalized and will result in performance degradation as you scale - far better to denormalize your data structures so that you can perform operations in a single query to the database, operating on a single document.

To answer your implied question regarding knowing which collection you are hooked into, you should be aware of that information as the collection and onCreating hook are both defined on the model - so you can either hard code the value, or if you are using an inheritance scheme to provide that functionality, simply access the ModelType.collection static property (ModelType is available as TInstance).

Sorry if it comes across as a rather opinionated approach, I guess in many ways it is, but my idea was always to keep things as simple as possible by eliminating the "fluff" which so regularly accompanies other ORMs. As always, I'm very open to continuing the discussion on the need for #23 - so let's see if there's a use case which can't be satisfied using the current approach (and which does truly benefit from using post-operation hooks instead of adopting another approach) and if so, I'll be certain to add the functionality. :smile: