Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with LegacyRowVersionNullBehavior #1182

Merged
merged 6 commits into from
Aug 19, 2021

Conversation

cheenamalhotra
Copy link
Member

Fixes #1175 as reported.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 21, 2021

Can you add the covering tests the user provided please.

@Wraith2
Copy link
Contributor

Wraith2 commented Jul 22, 2021

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
Copy link
Member Author

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
Copy link
Contributor

Wraith2 commented Jul 22, 2021

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
Copy link
Member Author

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.

@cheenamalhotra cheenamalhotra added this to the 4.0.0-preview1 milestone Jul 22, 2021
@Wraith2
Copy link
Contributor

Wraith2 commented Jul 22, 2021

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
Copy link
Contributor

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

Copy link
Contributor

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you extend the tests and include Timestamp too?

@cheenamalhotra
Copy link
Member Author

cheenamalhotra commented Aug 17, 2021

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.

@cheenamalhotra cheenamalhotra merged commit ce5a1f7 into dotnet:main Aug 19, 2021
Kaur-Parminder added a commit to Kaur-Parminder/SqlClient that referenced this pull request Aug 23, 2021
@roji
Copy link
Member

roji commented Aug 25, 2021

Should this be considered for backporting to 3.0.1?

@ErikEJ
Copy link
Contributor

ErikEJ commented Aug 25, 2021

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

@roji
Copy link
Member

roji commented Aug 25, 2021

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

@DavoudEshtehari
Copy link
Contributor

This is one of the candidates for backporting to 3.0.1.

@cheenamalhotra
Copy link
Member Author

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.

JRahnama pushed a commit that referenced this pull request Sep 14, 2021
* Fix Async thread blocking on SqlConnection open for AAD modes

Backporting #1182 for 3.0.1-servicing

* Adding missed TdsParser.cs from Bacporting 1182 issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LegacyRowVersionNullBehavior causes InvalidCastException when using SqlDataReader.GetFieldValue[T]
6 participants