Skip to content

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

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

Fixes #1332: Deep Comparison for Array-based Concurrency tokens.#1334
gathogojr merged 9 commits intoOData:release-8.xfrom
anasik:fix/EtagsArrays

Conversation

@anasik
Copy link
Copy Markdown
Contributor

@anasik anasik commented Oct 28, 2024

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

anasik commented Nov 4, 2024

@WanjohiSammy , may I interest you in this?

@anasik
Copy link
Copy Markdown
Contributor Author

anasik commented Nov 13, 2024

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

@anasik
Copy link
Copy Markdown
Contributor Author

anasik commented Nov 27, 2024

@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
Copy link
Copy Markdown
Contributor Author

anasik commented Jan 15, 2025

@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
Copy link
Copy Markdown
Contributor Author

anasik commented Jan 16, 2025

@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
Copy link
Copy Markdown
Member

@WanjohiSammy WanjohiSammy left a comment

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

anasik commented Jan 20, 2025

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

anasik commented Jan 23, 2025

@WanjohiSammy are all existing build tests passing?

@anasik
Copy link
Copy Markdown
Contributor Author

anasik commented Jan 28, 2025

@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
Copy link
Copy Markdown
Contributor Author

anasik commented Jan 31, 2025

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