Skip to content

Fixes #1332: Deep Comparison for Array-based Concurrency tokens. - #1334

Merged
gathogojr merged 9 commits into
OData:release-8.xfrom
anasik:fix/EtagsArrays
Jan 30, 2025
Merged

gathogojr merged 9 commits into
OData:release-8.xfrom
anasik:fix/EtagsArrays

Conversation

@anasik

@anasik anasik commented Oct 28, 2024 •

Copy link
Copy Markdown
Contributor

Fixes #1332

A popular convention today for concurrency check fields is to use a byte[] field called RowVersion with a Timestamp or ConcurrencyCheck attribute.

However, when using such a field, the queryOptions.ifNoneMatch.ApplyTo call was having absolutely no effect. That's because ApplyTo was internally using Expression.Equal which is perfect for value types but doesn't fare very well against reference types or particularly Arrays.

I fixed this by adding a case for arrays that, through a helper method, generates an expression that performs a deep comparison.

@anasik
anasik marked this pull request as ready for review October 28, 2024 21:12
@anasik

anasik commented Nov 4, 2024

Copy link
Copy Markdown
Contributor Author

@WanjohiSammy , may I interest you in this?

@anasik

anasik commented Nov 13, 2024

Copy link
Copy Markdown
Contributor Author

@xuzhg, it's been a little quiet here. Maybe I can help with something?

@anasik

anasik commented Nov 27, 2024

Copy link
Copy Markdown
Contributor Author

@xuzhg , @WanjohiSammy, I see you guys were busy with the 8.2.6 release. Just bumping this in case it got lost or forgotten.

@anasik

anasik commented Jan 15, 2025

Copy link
Copy Markdown
Contributor Author

@WanjohiSammy, bumping this up just in case.

Comment thread test/Microsoft.AspNetCore.OData.E2E.Tests/ETags/DerivedETagTests.cs Outdated
@WanjohiSammy

Copy link
Copy Markdown
Member

@anasik tests are missing. Can you add several tests for this Deep Comparison feature

@anasik

anasik commented Jan 16, 2025

Copy link
Copy Markdown
Contributor Author

@anasik tests are missing. Can you add several tests for this Deep Comparison feature

@WanjohiSammy,
All I did was prove that an existing test was failing under certain circumstances and then I passed that test by adding the deep comparison feature. Since there's already a test that passes only when the feature exists, I am not sure what an additional test would look like?

If there's something specific you're looking for in terms of tests, please let me know and I'll see what I can do?

@anasik
anasik requested a review from WanjohiSammy January 16, 2025 17:39

@WanjohiSammy WanjohiSammy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I want to test and make sure that this is not a breaking change

@anasik

anasik commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

I want to test and make sure that this is not a breaking change

@WanjohiSammy, makes perfect sense. Let me know if any tests are failing and I'll take a look.

@anasik

anasik commented Jan 23, 2025

Copy link
Copy Markdown
Contributor Author

@WanjohiSammy are all existing build tests passing?

@anasik

anasik commented Jan 28, 2025

Copy link
Copy Markdown
Contributor Author

@WanjohiSammy , are you waiting on me for something here?

WanjohiSammy
WanjohiSammy previously approved these changes Jan 28, 2025
@WanjohiSammy

Copy link
Copy Markdown
Member

@habbes @gathogojr @marabooy please assist looking at this PR.

Comment thread src/Microsoft.AspNetCore.OData/Query/ETag.cs
@gathogojr
gathogojr merged commit fa871bd into OData:release-8.x Jan 30, 2025
@gathogojr

Copy link
Copy Markdown
Contributor

Thank you @anasik for your contribution

@anasik
anasik deleted the fix/EtagsArrays branch January 30, 2025 19:39
@anasik

anasik commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

Thank you @anasik for your contribution

@gathogojr , it was a pleasure. I hope to contribute more.

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.

4 participants