Fix bug with LegacyRowVersionNullBehavior

[cheenamalhotra]

Fixes #1175 as reported.

[Wraith2]

Can you add the covering tests the user provided please.

[Wraith2]

What should the new behaviour be for GetFieldValue<byte[]> on a null RowVersion value? It can't return an INullable but because byte[] can be null we could return a null.

[cheenamalhotra]

What should the new behaviour be for GetFieldValue<byte[]> on a null RowVersion value? It can't return an INullable but because byte[] can be null we could return a null.

Addressed that too.

[Wraith2]

Ok, I'm fine with returning null for NULL there, just wondering singl ei was converting the new tests for the current behaviour to make sure it was well defined.

I think I prefer my goto fix code better but I'm fine with approving this version if you prefer it.

[cheenamalhotra]

Ok, I'm fine with returning null for NULL there, just wondering singl ei was converting the new tests for the current behaviour to make sure it was well defined.

I decided to revert this back to throw exception - as ideally this is "expected" with the breaking change we have when LegacyRowVersionNullBehavior is not enabled. As changing it to give null can cause other types to avoid exception too when cast cannot be done for null column value.

I think I prefer my goto fix code better but I'm fine with approving this version if you prefer it.

I don't mind yours either, the one I have now is a less impactful and less complex to understand.. I'll leave it as is.

[Wraith2]

My only complaint is that the goto version avoids checking the same variable twice. On each path you only check what is needed. In the if-based construction you have to access the legacy switch twice and use isnull twice. If you and the team prefer this version for code clarity that's ok though.

[JRahnama]

LGTM, plus I liked the approach to C# 9's new features.

[DavoudEshtehari]

Could you extend the tests and include Timestamp too?

[cheenamalhotra]

Could you extend the tests and include Timestamp too?

Timestamp is already included when we run this query in tests:
"select cast(null as rowversion) rv";

I'll take care of other improvements.

[roji]

Should this be considered for backporting to 3.0.1?

[ErikEJ]

@roji most important if 3.0 is LTS, I guess?

[roji]

@ErikEJ I think it isn't though, no? Isn't 4.0 the LTS?

[DavoudEshtehari]

This is one of the candidates for backporting to 3.0.1.

[cheenamalhotra]

Hi @roji @ErikEJ

v3.0 is not an LTS but we'll continue to hotfix it till it stays in support. The next LTS version is not yet determined.