Skip to content

Conversation

@MaggieKimani1
Copy link
Contributor

Fixes #799

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

I don't think we should be using the GetHashCode override for multiple reasons:

  • it's not its intended use, especially if caller want to store the hash somewhere for later comparison
  • it can yield equality between unequal objects
  • it will vary depending on chip instruction set (x86, 64, arm, arm64)
  • it requires also overriding the Equals method

I'd instead recommend using a proper hashing algorithm (configurable?) like sha256 or up.

see the documentation of get hash code.
https://docs.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-6.0#remarks

{
// select two random prime numbers e.g 1 and 3 and use them to compute hash codes
int hash = 1;
hash = hash * 3 + (Workspace == null ? 0 : Workspace.GetHashCode());
Copy link
Member

Choose a reason for hiding this comment

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

I think you're supposed to select different prime numbers.

@MaggieKimani1
Copy link
Contributor Author

MaggieKimani1 commented Aug 19, 2022

I don't think we should be using the GetHashCode override for multiple reasons:

  • it's not its intended use, especially if caller want to store the hash somewhere for later comparison
  • it can yield equality between unequal objects
  • it will vary depending on chip instruction set (x86, 64, arm, arm64)
  • it requires also overriding the Equals method

I'd instead recommend using a proper hashing algorithm (configurable?) like sha256 or up.

see the documentation of get hash code. https://docs.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-6.0#remarks

I leaned more towards overriding GetHashCode() and tried getting the hash value for each of the property values to avoid collision as I believed the goal was to perform a simple object lookup as opposed to encryption which comes with its own set of complications:

  • You have to convert the OpenApi document object into a byte array for hash computation
  • We have to mark all object model types, OpenApiAny and Expression types as either Serializable or NonSerialized for the hashing to take place..

@baywet
Copy link
Member

baywet commented Aug 19, 2022

le object lookup as opposed to encryption which comes with its own set of complications:

  • You have to convert the OpenApi document object into a byte array for hash computation
  • We have to mark all object model types, OpenApiAny and Expression types as Serializable for the hashing to take place..

Nitpicking: hashing and encryption are two completely different things :) I'm not suggesting to rely on any encryption algorithm, simply on standard hashing.

The nice additional thing with using industry standard hashing is that if we document the process correctly, other libs (from other languages) could interop with it and generate the exact same hash for the same document.

Serialize and convert: well that's the beauty of it, we already have the infrastructure to serialize the document to JSON/YAML, let's use that to produce a terse representation (so white-spacing doesn't impact the result), convert to binary, hash, and voilà! You don't even need to implement changes on the whole object model anymore, just on the document or root object.

What do you think? Also @darrelmiller, any objection with this approach?

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, a few minor comments.
We should also make sure the serialization is terse when we hash

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

thanks for making the changes, here are a few final remarks

@MaggieKimani1 MaggieKimani1 merged commit fb6ea09 into vnext Aug 26, 2022
@MaggieKimani1 MaggieKimani1 deleted the mk/enhancement-create-hash-code branch August 26, 2022 14:19
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.

Create hash of OpenAPI Document

3 participants