{"action":"created","issue":{"url":"https://api.github.com/repos/Starcounter/Starcounter/issues/2760","labels_url":"https://api.github.com/repos/Starcounter/Starcounter/issues/2760/labels{/name}","comments_url":"https://api.github.com/repos/Starcounter/Starcounter/issues/2760/comments","events_url":"https://api.github.com/repos/Starcounter/Starcounter/issues/2760/events","html_url":"https://github.com/Starcounter/Starcounter/issues/2760","id":86559256,"number":2760,"title":"Thoughts on Commit hooks 2.0","user":{"login":"miyconst","id":6435556,"avatar_url":"https://avatars.githubusercontent.com/u/6435556?v=3","gravatar_id":"","url":"https://api.github.com/users/miyconst","html_url":"https://github.com/miyconst","followers_url":"https://api.github.com/users/miyconst/followers","following_url":"https://api.github.com/users/miyconst/following{/other_user}","gists_url":"https://api.github.com/users/miyconst/gists{/gist_id}","starred_url":"https://api.github.com/users/miyconst/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/miyconst/subscriptions","organizations_url":"https://api.github.com/users/miyconst/orgs","repos_url":"https://api.github.com/users/miyconst/repos","events_url":"https://api.github.com/users/miyconst/events{/privacy}","received_events_url":"https://api.github.com/users/miyconst/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/Starcounter/Starcounter/labels/enhancement","name":"enhancement","color":"84b6eb"},{"url":"https://api.github.com/repos/Starcounter/Starcounter/labels/question","name":"question","color":"02d7e1"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2015-06-09T11:16:35Z","updated_at":"2015-06-10T07:52:18Z","closed_at":null,"body":"After short play with the Commit hooks I have some thoughts.\r\n\r\n1. The `Hook<T>.OnDelete` should also pass an object, but not its ID. Commit hook with an ID makes it useless. The object is deleted by that time and there is no way to know what other entity or file was associated with this object.\r\n2. The `Hook<T>.OnXxxx` should be called for every object of type `T` or `X : T`. Lets say I have a class `Image`, in `OnDelete` action I remove the physical file. Another app which uses same db has a class `Photo : Image` with some custom behaviour. The `OnDelete` hook should also be invoked for `Photo` objects.\r\n3. Non of the hooks fires for objects created/updated inside a hook action. Not sure it should work, differently, just noting a fact.\r\n4. Application fails with an exception `The operation failed because of an invalid object access.` if I delete just inserted object inside an `OnInsert` hook. This is probably correct, just noting another fact.\r\n5. The hook names are misleading a bit and there is no description available. If the call back executes after an action, then the names should be `OnDeleted`, `OnInserted`, `OnUpdated`, if before the action then it should be `OnBeforeDelete`, `OnBeforeInsert`, `OnBeforeUpdate`. I think there should be all of those hooks.\r\n6. How to detach a hook? Native c# `event` supports operators `+=` and `-=`, why not use it?\r\n7. What if I need a hook for a single object only? I would rather implement an `IHookable` interface in my db class and then subscribe to the object's events. Some code to explain what I mean:\r\n```csharp\r\npublic interface IHookable {\r\n    void OnBeforeDelete();\r\n    void OnBeforeUpdate();\r\n    void OnBeforeInsert();\r\n}\r\n\r\npublic class Product : IHookable {\r\n    public event EventHandler BeforeInsert;\r\n    public event EventHandler BeforeUpdate;\r\n    public event EventHandler BeforeDelete;\r\n\r\n    public string Name;\r\n    public DateTime CreateDate;\r\n    public DateTime ChangeDate;\r\n\r\n    public void OnBeforeDelete() {\r\n        if (this.BeforeDelete != null) {\r\n            this.BeforeDelete(this, EventArgs.Empty);\r\n        }\r\n    }\r\n\r\n    public void OnBeforeUpdate() {\r\n        this.CreateDate = DateTime.Now;\r\n\r\n        if (this.BeforeUpdate != null) {\r\n            this.BeforeUpdate(this, EventArgs.Empty);\r\n        }\r\n    }\r\n\r\n    public void OnBeforeInsert() {\r\n        this.CreateDate = this.ChangeDate = DateTime.Now;\r\n\r\n        if (this.BeforeInsert != null) {\r\n            this.BeforeInsert(this, EventArgs.Empty);\r\n        }\r\n    }\r\n}\r\n\r\npublic class StarcounterInternal {\r\n    public void CommitTransaction() {\r\n        foreach (var entity in this.EntitiesToInsert) {\r\n            IHookable hook = entity as IHookable;\r\n\r\n            if (hook != null) {\r\n                hook.OnBeforeInsert();\r\n            }\r\n\r\n            this.InsertObject(entity);\r\n        }\r\n    }\r\n}\r\n\r\npublic class Program {\r\n    static void Main() {\r\n        Db.Transact(() => {\r\n            var p = new Product() { \r\n                Name = \"I am a special product!\"\r\n            };\r\n\r\n            p.BeforeInsert += (s, a) => {\r\n                (s as Product).Name = \"This is a hook for only one object, not entire set of products\";\r\n            };\r\n        });\r\n    }\r\n}\r\n```"},"comment":{"url":"https://api.github.com/repos/Starcounter/Starcounter/issues/comments/110641187","html_url":"https://github.com/Starcounter/Starcounter/issues/2760#issuecomment-110641187","issue_url":"https://api.github.com/repos/Starcounter/Starcounter/issues/2760","id":110641187,"user":{"login":"per-samuelsson","id":2902045,"avatar_url":"https://avatars.githubusercontent.com/u/2902045?v=3","gravatar_id":"","url":"https://api.github.com/users/per-samuelsson","html_url":"https://github.com/per-samuelsson","followers_url":"https://api.github.com/users/per-samuelsson/followers","following_url":"https://api.github.com/users/per-samuelsson/following{/other_user}","gists_url":"https://api.github.com/users/per-samuelsson/gists{/gist_id}","starred_url":"https://api.github.com/users/per-samuelsson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/per-samuelsson/subscriptions","organizations_url":"https://api.github.com/users/per-samuelsson/orgs","repos_url":"https://api.github.com/users/per-samuelsson/repos","events_url":"https://api.github.com/users/per-samuelsson/events{/privacy}","received_events_url":"https://api.github.com/users/per-samuelsson/received_events","type":"User","site_admin":false},"created_at":"2015-06-10T07:52:18Z","updated_at":"2015-06-10T07:52:18Z","body":"> 2) The Hook<T>.OnXxxx should be called for every object of type T or X : T. Lets say I have a class Image, in OnDelete action I remove the physical file. Another app which uses same db has a class Photo : Image with some custom behaviour. The OnDelete hook should also be invoked for Photo objects.\r\n\r\nWe have referencing to this as inheritable hooks, and the current implementation is by design: we choose to have registration per class/table, rather than propagating it down to childs. There were several reasons why.\r\n\r\nThe main reason was that we want to keep the \"low-level\" implementation as **simple** as possible, and the described behaviour can be achieved on a higher, application level, even though it would require some work from developers. What I mean is, that since we support an overload of every ```Hook<T>.OnXXX``` that allows a runtime type to be passed, developers can register basically every type they which at runtime.\r\n\r\nSome pseudo code to show what I mean:\r\n\r\n```C#\r\nvoid Main() {\r\n  foreach (var t in GetLoadedDatabaseTypes()) {\r\n    Hook<object>.OnInsert(t, o => {\r\n      Console.Write(\"An object {0}/{1} was inserted\", o.GetObjectId(), o.GetType().FullName);\r\n    }\r\n  }\r\n}\r\n```\r\n\r\nConnecting back to your example, just replace ```object``` with ```Image```, and register ```Photo``` (and other images on that hook), something like:\r\n\r\n```C#\r\nvoid Main() {\r\n  foreach (var t in GetLoadedDatabaseTypes()) {\r\n    if (typeof(Image).IsAssignableFrom(t)) {\r\n      Hook<Image>.OnInsert(t, i => {\r\n        Console.Write(i.Resolution);\r\n      }\r\n    } \r\n  }\r\n}\r\n```\r\n\r\nA second reason was that since hooks are quite expensive in terms of performance, we wouldn't want them to be active if not absolutly neccessary. Hence, per table/class registration by default minimize the risk of that.\r\n\r\nFinally, in 1.x we supported a notion of \"filterable hooks\", that was basically a built-in high level API built on top of hooks, that supported inherited registration/invocation like you suggest, but we haven't ported anything such to the current version."}}