NH-3724 - Added support for notification handlers #363
Conversation
| @@ -20,6 +20,12 @@ public abstract class DriverBase : IDriver, ISqlParameterFormatter | |||
| private int commandTimeout; | |||
| private bool prepareSql; | |||
|
|
|||
| public virtual void AddNotificationHandler(IDbConnection con, Delegate handler) | |||
There was a problem hiding this comment.
Probably better to throw not supported exception
- Base driver throws exception if a handler is passed to it - Drivers for .NET BCL classes just cast parameters to the expected types and will throw an exception if the type is wrong
|
Since many providers seems to follow the "InfoMessage" convention set by the providers provided by MS in ADO.NET, and since we're using lots of reflection anyway, I think DriverBase should look for this event to avoid all the duplication. If a handler is set and the InfoMessage event isn't found, DriverBase should throw NotSupportedException, as Alex said. I didn't count, but I think much fewer drivers would need to override. As I understand this, the application must set a handler of a type that matches the currently used provider. We don't make an effort to abstract to a common delegate definition? Perhaps that is ok. So the value of this is that the application can configure NH to set the delegate on all connections, without having to override the application or implement a connection provider? |
|
For PostgreSQL there are two events. I havent' researched so I'm just gonna ask if you are sure you picked the correct one, or if we should handle both, or if we should pick one depending on type of the configured handler? |
Exception is thrown if event is not found
|
@oskarb I updated the pull request as mentioned. As for for PostgreSQL, I think it is the Notification event that we're interested at - I saw lots of examples mentioning it in the same context. For the delegate, I don't think we can abstract it with any class other than Delegate. |
src/NHibernate/Driver/DriverBase.cs
Outdated
|
|
||
| if (prop == null) | ||
| { | ||
| throw new NotSupportedException("Current driver does not support notifications"); |
There was a problem hiding this comment.
Please add the dot at the end of the message (as recommended by MS).
|
Please remove the using-declarations that was added by this patch but which are no longer required. |
…ed . at the end of the exception message
|
Done. |
|
@rjperes can you please update titles of you PRs including title from JIRA? Like "NH-3724 - Handle Information Messages From Server" |
|
Replaced by #2113 |
Added support for notification handlers (InfoMessage event in most ADO.NET drivers)