AdoNet - be more explicit when extracting DateTime from IDataRecord

[mjameson-se]

Context:
In my project, we are using Npgsql, NodaTime and Npgsql.NodaTime. As a result of this combination, the postgres date/time types (such as those used in the AdoNet membership table) get mapped to NodaTime types instead of BCL types by default, unless you explicitly request the column as DateTime via IDataRecord.GetDateTime(...) or similar.

Without the changes here, I am getting an invalid cast exception at runtime because the Instant type returned by Npgsql is being cast to DateTime by the Orleans AdoNet code.

[dnfadmin]

All CLA requirements met.

[veikkoeeva]

@mjameson-se Have you run the database tests for other providers? I think all should be fine as nothing should change except be explicit about the type and not use a generic that the connector then makes the inference with, but I just want to make sure. I think I can do that but it may be next Monday evening or maybe Sunday evening, unfortunately.

Edit: If you have time and energy, copying comments from other functions as appropriate is fine. Useful in that case would to be to add in <remarks> something about this problem. As in

An explicit function like this is needed in cases where to connector infers a type that is undesirable. An example here is Npgsql.NodaTime, which makes Npgsql return Noda type and consequently Orleans is not able to use it since it expects .NET DateTime.

(It's a clunky explanation, but to give a gist). No need for this, though. I may add myself later as this is a good piece of information to keep in code, I believe.

[dave-b-code]

Have been running into issues also with NodaTime (primarily due to Npgsql/EfCore global typemapping https://github.com/npgsql/efcore.pg/blob/f6e9789fc6b6e16e1ca700c3d311bc23e27a7b91/src/EFCore.PG.NodaTime/Extensions/NpgsqlNodaTimeDbContextOptionsBuilderExtensions.cs#L22) and this PR looks like it could solve them. Wrt to the comments from @veikkoeeva , I don't have experience with Azure Storage or DynamoDb but it doesn't appear their libraries use global type mapping the same way AdoNet does so changes shouldn't be necessary there afaik. Are there any other further blockers on this getting merged? (P.S tried to look at failing tests but Azure Devops is giving me a 'Build not found' error, would it be possible for someone to trigger a rerun? Thanks).

[veikkoeeva]

@dave-b-code I have a faint memory of running tests and not remembering blockers.

The database tests need to be run locally, I think. That is, cloning the repository, compiling and then maybe in VS Unit Test Window selecting either all or a subset of AdoNet tests. The configuration on supported databases is hardcoded currently to default connection string values and assume a server is running.

To be on the safe side, I can run the tests sometime this week, I believe, if no one else does.

[dave-b-code]

Merged in the current 3.x branch and ran through Tester.AdoNet (.net 6), had some flakiness on the MySqlMembershipTableTests, never used MySql locally before so assuming I've set it up on the wrong transaction isolation level or something like that (MembershipTable_MySql_ReadRow_Insert_Read failed repeatedly with "InsertRow should have failed - duplicate entry"). PersistenceGrainTests_Sql tests all failed due to "Unable to resolve service for type 'Orleans.IMembershipTable' while attempting to activate 'Orleans.Runtime.MembershipService.MembershipTableManager'", though this seems unrelated to this current PR. Thanks for the assistance!

[veikkoeeva]

@dave-b-code Looks good for the PR!

For SQL Server, I take a guess the problem is Asynchronous Processing = True; in the connection string and the tests throw in initialization. It appears one has to periodically make a PR to remove it (it keeps on appearing). I think another one is TestDb.mdf that could've appread too. I remember it doesn't do anything and even if it did, I'd say better to "program" SQL server by telling it to create a database in SQL. (Maybe there are needed for .NET 4.5 compatibility? In any event, throws on new .NET).

MySQL ones should not have failed. I think the isolation level ought to be set in scripts to be a correct one. I think warrants further looking into, but of course not in this PR.

About isolation levels: https://www.percona.com/blog/2021/02/11/various-types-of-innodb-transaction-isolation-levels-explained-using-terminal/. It should work with repeatable reads, I believe.

[ReubenBond]

Many thanks, @mjameson-se, @dave-b-code, and @veikkoeeva!