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

Fixed the equality contract on Metrics type #7733

Merged
merged 3 commits into from Jul 10, 2023

Conversation

YohDeadfall
Copy link
Contributor

HashSet<T> doesn't override GetHashCode. Therefore, the default provided algorithm for reference types is used which doesn't take fields into account as it happens for value types. That breaks the equality contracts on Metrics.

Ideally HashCode type should be used for more correct computations with less number of collisions, but that requires increasing the minimal target framework.

SetEqual is available since .NET Fx 3.5 and is destined for equality comparison of two different hash set instances. Since both instances of the Metrics type has the same default comparer, the method works as a sugar over a length check followed by iteration over one set and probing the second one for containing the current element through the Contains method.

`SetEqual` is available since .NET Fx 3.5 and is destined for equality
comparison of two different hash set instances. Since both instances of
the `Metrics` type has the same default comparer, the method works as
a sugar over a length check followed by iteration over one set and
probing the second one for containing the current element through
the `Contains` method.
`HashSet<T>` doesn't override `GetHashCode`. Therefore, the default
provided algorithm for reference types is used which doesn't take
fields into account as it happens for value types. That breaks the
equality contracts on Metrics.

Ideally `HashCode` type should be used for more correct computations
with less number of collisions, but that requires increasing the
minimal target framework.
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@technige
Copy link

Thank you for your submission. We will review this in due course as and when someone is available on the team to do so.

@flobernd flobernd added bug 8.x Relates to 8.x client version labels Jul 5, 2023
@flobernd
Copy link
Member

flobernd commented Jul 5, 2023

Thanks as well for this contribution!

It might be worth adding a conditional reference to Microsoft.Bcl.HashCode for the net462 and netstandard2.0 targets. There are around ~150 custom GetHashCode implementations in the project (a lot of them are auto-generated) that could benefit from using HashCode. I will check internally, if this is something we might consider.

For now I'm going to approve the PR to have this fix included in the next patch release. The HashCode optimization can be done later.

@flobernd flobernd removed the v8.1.2 label Jul 10, 2023
@flobernd flobernd merged commit 85146f0 into elastic:main Jul 10, 2023
12 checks passed
@YohDeadfall YohDeadfall deleted the hash-set-equals-fix branch July 10, 2023 12:45
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
flobernd added a commit that referenced this pull request Jul 11, 2023
* DoubleWithFractionalPortionConverter and FloatWithFractionalPortionConverter should not fall through to always throw a JSONException for non NETCore builds (#7753)

* Complted buckets JSON converter (#7738)

* Boosted non-exhaustive enum deserialization (#7737)

* Optimized `FieldConverter` (#7736)

* Removed unused JsonIgnore (#7735)

* Fixed the equality contract on Metrics type (#7733)

* No allocations in ResponseItem IsValid prop (#7731)

* Refactoring and tiny behavior fix for Ids (#7730)

---------

Co-authored-by: Florian Bernd <git@flobernd.de>
Co-authored-by: Karl-Johan Sjögren <karl.sjogren@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to 8.x client version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants