Pg: Feature Request: Add hooks (BeforeUpdate/AfterInsert, etc...)

[ryanbyyc]

Are hooks on the roadmap? I can understand if they're omitted for performance concerns, but it would be handy if I had a standard way to perform things like expiring redis cache, updating row counts, or re-indexing elasticsearch results.

I had looked at Notifications, but disregarded those because of the Not thread-safe. text.

[vmihailenco]

If you mean https://godoc.org/gopkg.in/pg.v4#Listener then using it with sync.Mutex or chan is not too hard (though API can be improved). And I think that is the most reliable approach.

Are hooks on the roadmap?

They are not on roadmap, because I don't find them very useful. E.g. you can't use hooks to set data that comes from http.Request and/or context.Context and/or Redis if Redis connection is not global:

func (a *Article) PreSave() error {
    a.EditedBy = req.FormValue("user_id") // req is not available
}

So far my opinion is that hooks have very limited usability, but that is not the final decision.

[ryanbyyc]

Alright, thanks for the feedback. I would love to see more work done on the Listener's API, but regarding the Feature request I'll accept your decision. Thanks again!

[heynemann]

@vmihailenco I respectfully disagree with you. Even though there are some valid use cases where context would be needed (in the use cases you pointed), there are many valid scenarios where hooks would be wonderful.

Right now the pain point for me in adopting PG is precisely this issue. I'll explain some use cases I have:

• Automatically populate created_at and updated_at with the proper unix timestamps (due to constraints between systems we must use unix timestamps instead of actual dates);
• We have some int fields that must be parsed to string values (something akin to an enumeration). I have all the values, I just do not want to repeat the code that parses the value, maps it in the configuraton and sets the string value. There are many fields in my struct that fit this scenario;
• PreInsert and PreUpdate I can centralize the validation of my fields. Sometimes the Database validation is not enough. i.e.: validating email or URL fields. I know I can validate them in the methods I'm using to create my model instances, but that definitely makes my models weaker as in people can abuse them unwittingly.

I hope those three scenarios are enough to warrant at least the reopening of this ticket. I can assure you that PG would be a much better ORM simply by supporting these two hooks for each event (pre and post for insert, update and delete).

If it is something you'd consider maybe we can come up with an implementation.

What I had in mind is something akin to what gorp does:

type Model struct {
    TableName struct{} `sql:"models"`

    ID int `sql:"id, pk"`
    CreatedAt int64 `sql:"created_at"`
    UpdatedAt int64 `sql:"updated_at"`
}

//PreInsert populates fields before inserting a new model
func (m *Model) PreInsert(db *pg.DB) error {
    // Handle JSON fields
    g.CreatedAt = util.NowMilli()
    g.UpdatedAt = g.CreatedAt

    return db.Update(m)
}

That way you can call the methods on the model being saved if it implements the PreInsertModel interface with a simple type assertion:

pre, ok := m.(PreInsertModel)
if ok {
    err := pre.PreInsert(db)
    //Check error...
}

What do you think?

[vmihailenco]

I will try to do something about this, but so far I don't have a good understanding of this feature.,, Your example

func (m *Model) PreInsert(db *pg.DB) error {
    // Handle JSON fields
    g.CreatedAt = util.NowMilli()
    g.UpdatedAt = g.CreatedAt

    return db.Create(m)
}

looks really cool for single model, but

• what if we create multiple models in 1 call, e.g. db.Create(&book1, &book2) - should we do batch insert or call PreInsert for each model?
• what about SelectOrCreate?
• what about updates? what about updates that update few columns like db.Model(&book).Set("name = ?name").Update()?
• should we support soft-deletes, i.e. overloading Delete with updates that sets deleted_at?

Most likely I will start with really simple pre-insert/post-insert hooks, because it can get complicated very fast...

[heynemann]

First, thanks a LOT for your reply and for reopening the issue.

Let's try and separate the cases you talk about: bulk insert, select or create, update some fields and delete. I'll also try to come up with a spec of sorts that make sense for all these scenarios and we can reason about it. Once more, thank you a lot for your time and patience.

Just pointing out an error in my previous code, you should not have to call db.Create as the event implies create will be done afterwards by pg. You should just return nil as in:

func (m *Model) PreInsert(db *pg.DB) error {
    // Handle JSON fields
    m.CreatedAt = time.Now().Unix()
    m.UpdatedAt = m.CreatedAt

    return nil // instance will get created with the proper values
}

Edge Cases

Bulk Insert

I don't think this should be a special case at all, just a different event. In keeping with Go's explicit culture I think bulks should have their own events:

//PreBulkInsert populates fields before inserting new models
func (m *Model) PreBulkInsert(db *pg.DB, instances []*Model) error {
        for _, instance := range instances {
            // Handle JSON fields
            instance.CreatedAt = util.NowMilli()
            instance.UpdatedAt = instance.CreatedAt
        }
        return nil
}

Select or Create

This should be relatively straightforward IMHO. If you are selecting, you don't need to call the PreInsert hook, otherwise it's just a regular PreInsert hook. Does that make sense?

Update some fields

It's actually great that we're discussing this, because it gives us an opportunity to say what data each event will get. The Update hooks should get what was updated if it's not terribly expensive.

Something like:

//PreUpdate populates fields before updating existing models
func (m *Model) PreUpdate(
    db *pg.DB,
    changedFieldsPreviousValues map[string]interface{},
    changedFieldsNewValues map[string]interface{},
    instances []*Model) error {
        for _, instance := range instances {
            // Handle JSON fields
            instance.CreatedAt = time.Now().Unix()
            instance.UpdatedAt = instance.CreatedAt
            if isDeleted, ok := changedFieldsNewValues["IsDeleted"]; ok && isDeleted {
                        instance.DeletedAt = time.Now().Unix()
            }
        }
        return nil
}

This way users can validate, as well as inspect values that were changed. I believe this scenario works for regular updates as well as for selected fields updates.

Delete

I think the delete hooks are pretty straightforward and can be GREAT if you want to do some clean-up after a deleted instance. It's one last shot of using the instance values (not in the DB anymore in the case of the PostDelete hook).

The implementation should be as simple as:

//PreDelete populates fields before deleting new models
func (m *Model) PreDelete(db *pg.DB, instance *Model) error {
        if (instance.IsAdmin) {
                return fmt.Errorf("Can't delete admin users. Please demote user first.")
        }
        return nil
}

Contract

I think the events we should support (considering the above scenarios) should be:

• PreInsert (access to the instance without ID);
• PostInsert (access to the saved instance with ID);
• PreUpdate (access to the updated fields values before and after the update, as well as the instance before updating in the DB);
• PostUpdate (access to the updated fields values after the update, as well as the instance after updating in the DB);
• PreDelete (access to instance before deleting it);
• PostDelete (access to instance that no longer exists in DB);
• PreBulkInsert (access to all the instances without IDs);
• PostBulkInsert (access to all the instances with IDs).

All the Pre-* hooks should check for returned errors and stop the operation if an error is returned (that is a way to cancel the operation from inside the hook).

Conclusion

I think this would add a significant improvement to PG and one that is advertised as an important feature in most ORMs. I also think it's entirely feasible to implement it in a way that's straightforward and explicit and won't impact the performance in any significant way.

What do you guys think @ryanbyyc @vmihailenco?

[vmihailenco]

I've implemeneted before create hook with following signature:

func (b *Book) BeforeCreate(c context.Context) error {
    if b.CreatedAt.IsZero() {
        b.CreatedAt = time.Now()
    }
    return nil
}

It uses Go 1.7 context which can be set using db.WithContext(context.Background().WithValue("your_key", "your_value")). With context we probably should pass:

• ctx.Value("db") - returns *pg.DB
• ctx.Value("tx") - return *pg.Tx or nil

I am not sure if I want to support BeforeBulkCreate :)

PreUpdate (access to the updated fields values before and after the update, as well as the instance before updating in the DB);

I doubt I am going to support list of updated fields. But if someone has a good implementation we can add it later to Context :)

[vmihailenco]

I also have a version without context:

func (b *Book) BeforeCreate(db orm.DB) error {
    if b.CreatedAt.IsZero() {
        b.CreatedAt = time.Now()
    }
    return nil
}

I like it more because context is only available in Go 1.7.

[ryanbyyc]

I can honestly say that I like the convenience that these hooks are going to add. I think that especially having AfterSelect methods will make my job much easier (for instance, getting some view data from https://github.com/go-redis/redis and hydrating it into the model automatically).

As a Go 1.7 user, I would love to see context usage supported as well as without.

Overall, it's nice to see some discussion on this.

[heynemann]

Actually context was available already, just under a different package, isn't that right?

Other than that, love the new hooks @vmihailenco! I just think if you are not going to pass context, then you must at least supply both DB and TX, right?

I wholeheartedly agree with @ryanbyyc. Having these hooks will change my experience with Go ORMs move from OK to enjoyable. It will also eliminate a bunch of boilerplate code spread all over my codebase.

Why do you think the BeforeBulk is not a good idea @vmihailenco? It seems to me a matter of passing in all instances and that should be it, shouldn't it?

[vmihailenco]

I've released v4.9.0 with BeforeCreate/AfterCreate and AfterSelect hooks. Docs are missing, but you can take a look at https://github.com/go-pg/pg/blob/v4/hook_test.go#L20-L33 for example.

Other hooks may come later though the way go-pg works BeforeUpdate and BeforeDelete hooks are almost useless if your code looks like this:

db.Model(&Model{}).Set("count = count + 1").Where("id = ?", 1).Update()
db.Model(&Model{}).Where("id = ?", 1).Delete()

At least that is how I use go-pg.

Actually context was available already, just under a different package, isn't that right?

Yes, but you can't use mix golang.org/x/net/context and context, because Go treats interfaces defined in different packages as different types...

I just think if you are not going to pass context, then you must at least supply both DB and TX, right?

Now I pass orm.DB which can be *pg.DB or *pg.Tx. Later I can add context support to *pg.DB and *pg.Tx so you can set context using db.WithContext(ctx) and obtain it using db.Context().

Why do you think the BeforeBulk is not a good idea @vmihailenco? It seems to me a matter of passing in all instances and that should be it, shouldn't it?

Yeah, but calling a method on nil model does not look idiomatic and adds some complexity to the code. Besides that I am not sure that bulk version is very useful.

[vmihailenco]

Another interesting idea is a hook for queries, e.g.

func (m *Model) BeforeUpdateQuery(q *orm.Query) *orm.Query {
    return q.Where("locked IS FALSE")
}

But to be really useful orm.Query should support inspection of internal state.

[heynemann]

First of all, thank you so much for this. It's definitely a LEAP forward IMHO. I'd like to address a couple points that I didn't understand before (thanks for clarifying!).

BeforeUpdate and BeforeDelete

BeforeDelete seems to be very useful to allow people to do some cleanup (caching and search comes to mind), but I can definitely live without it since most of my code does not delete anything at all (soft deletes).

Now the BeforeUpdate hook is actually very useful, since it must do the same validations that creating a new instance does. Updating is actually even more important in my scenario, since I do a lot of updates and few inserts.

What you proposed in the BeforeUpdateQuery got me thinking that we can have two separate events and be very explicit about it:

func (b *Book) BeforeUpdate(db orm.DB) error {
    b.UpdatedAt = time.Now().Unix()
    if (b.Rating == 5) {
         b.ShowInFrontPage = true
    }
    return nil
}

func (b *Book) BeforeUpdateFields(db orm.DB, newValues map[string]interface{}) error {
    newValues["UpdatedAt"] = time.Now().Unix()
    if rating, ok := newValues["rating"]; ok && rating == 5 {
         newValues["ShowInFrontPage"] = true
    }
    return nil
}

This way I can still keep my models Rock Solid! :) Even if you do not want to support the second scenario, at least support the first one allowing users to go that way if they want to ensure some computed property values or validation.

BeforeUpdateQuery

I like that A LOT, but as you said, I would need to see a test case to really understand how that would be used.

Context

You are absolutely right. The only way to support it forward would be to have an interface for it that both net/http/context and context implement. Is that feasible?

BeforeBulk

The BeforeBulk method can be a class method, instead of an instance method if that make it more idiomatic (I'm relatively new to go, ~5mo). If you still think it's not a good idea, I'll respect your decision.

Thanks once more for your hard work! I appreciate it a lot!

[vmihailenco]

It simply not possible to use BeforeUpdate in a sensible manner now. For example with query like

db.Model(&entry).Set("rating = ?rating").Update()

and hook

func (e *Entry) BeforeUpdate(db orm.DB) error {
    e.Rating = 0 // updated entries lose rating
    return nil
}

it currently will not work, because Set("rating = ?rating") is evaluated before hook is called. It is possible to fix it by implementing lazy evaluation, but that is a big change. Update/Delete hooks will be supported eventually, but they require more work to be useful.

The only way to support it forward would be to have an interface for it that both net/http/context and context implement. Is that feasible?

Unfortunately not, see https://github.com/golang/go/issues/8082

The BeforeBulk method can be a class method

Golang does not have class methods, does it? :) Anyway if you have a real use case - please post it here and I will consider supporting it.

[heynemann]

You are right on both accounts! Thank you very much for the explanation.

Thanks for the hard work again. I'll just hang tight for the BeforeUpdate method and keep on updating manually 'til then.

I'm ok with the BeforeBulk waiting til someone can come up with an actual use case, too.

[vmihailenco]

BeforeUpdate & BeforeDelete were added in v5. Closing.