Use another hash code for projects #734
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jvican
added
bug
alexarchambault
approved these changes
Nov 23, 2018
| @@ -59,7 +59,8 @@ final case class Project( | |||
| } | |||
|
|
|||
| override def toString: String = s"$name" | |||
| override val hashCode: Int = origin.hash | |||
| val uniqueId = s"${origin.path}#${name}" | |||
| override val hashCode: Int = uniqueId.hashCode | |||
There was a problem hiding this comment.
Not a change of this PR, but are you sure you want to rely on hashCode to test for equality just below? String hash code in Java can collide sometimes.
There was a problem hiding this comment.
Object.hashCode isn't really a crypto secure hash.
There was a problem hiding this comment.
Addressed in the latest commit, the likelihood of this happening is quite small for us because at max we'll only have 200 projects in one bloop server, but it's nonetheless good advice and it's better be safe than sorry!
The previous `hashCode` of `Project` was using the hash of the serialized file. At the time of doing that, I did not realize the use case that this commit fixes would be affected. We're now using a unique ID composed of the name and the configuration path a file comes from. Whenever that is different, it must be a different project. We use `ByteHasher` instead of `hashCode` to avoid hash code collisions. The probability of a hash code collision is very slow, especially because the server should only have around 100 or 200 projects maximum, but it's better be safe than sorry. Fixes #733
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous
hashCodeofProjectwas using the hash of theserialized file. At the time of doing that, I did not realize the use
case that this commit fixes would be affected.
We're now using a unique ID composed of the name and the configuration
path a file comes from. Whenever that is different, it must be a
different project.
Fixes #733