Make conversion of RevisionType explicit to avoid failure using Npgsql

[patearl]

This failure was mentioned by somebody else on the mailing list. Our internal tests prove that this trivial change eliminates the exception.

[RogerKratz]

Actually I was about to setup npgsql for other reasons on a local build machine here. Is this the only thing needed to get a green build?

Why converting to a byte? Any particular reason an int cast isn't enough?

[RogerKratz]

EnumUserType in NH Core seems to do a simple
(int)value
?

[patearl]

I made it a byte for maximum expected compatibility. The column type for the user type is DbType.Byte, which is an unsigned byte, so I figured the most likely thing to work on all providers is sending the same unsigned byte in.

[patearl]

We are struggling with a strange TypeLoadException (declaration cannot be final) that is unrelated to this particular change (is related to NHE-163), but after that it appears we might end up with green tests for Npgsql. I will post another comment once that is confirmed.

[patearl]

In case somebody stumbles upon this thread, the TypeLoadException looks like a bug in the NHibernate ProxyFactory stuff. The name and/or MethodAttributes of the methods implementing the interfaces explicitly need to be set differently to not conflict with base implementations. We will submit a PR to NHibernate for this and replace the NHibernate.Envers proxy with something custom in the interim.

I have yet to confirm that the parameter fix is all that is needed for Npgsql. We are working on replacing the proxy and will comment back.

[patearl]

With this Convert.ToByte fix and the unrelated fix to NHibernate for the TypeLoadException, everything is green for us. A speedy release of this would naturally be appreciated. :)

[hazzik]

@patearl could you please make a PR to NHibernate for this "unrelated fix"? We might be able to include it into the next release, which is already overdue.

[RogerKratz]

I have setup a local env for Npgsql now. Will merge your fix, thank you!
Before I make a release though, I just want to check with you...
After your fix, I still have some red tests

• ForceInitializeTest - is this the NH Proxy bug you refer to above? If so, I ignore it for npgsql.
• OutsideTransactionTest & SessionClosedWhenCommitingTest - do you see the same red tests? Seems to be related to distributed transactions. Do you happen to know anything about these (my knowledge in postgres is minimal...)

[patearl]

We only ran our local suite of tests; sorry for the confusion.

The NH proxy bug manifests itself as a TypeLoadException mentioning a final declaration. Is that what you are seeing?

We don't use transaction enlistment or distributed transactions, so I wouldn't have any immediate insight into that area.

[RogerKratz]

The tests that are still red on postgres were not related to nor "TypeLoadException" or this fix.

Do you want me to do a release now or do you have to wait for the NH fix mentioned above anyhow (if so, I'll wait with a release until next NH Core release is made)?