This repository has been archived by the owner on Apr 12, 2024. It is now read-only.
Fix UUID equality check bug #45
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
Signed-off-by: Josh Kim <kjosh@vmware.com>
UUID is used throughout the entire codebase of Transport and is especially important in filtering messages based on their destination. While implementing basic WASM bridge for the bus I realized that UUID comparison was failing, i.e. returning true for the equality check between two dififerent UUID instances. Turns out the use of .ID() method on the UUID object only returns the first 4 bytes of the underlying 16-byte slice. This means as long as the first 8 hexadecimal characters matched between two UUID instances, they would come out as equal, and it would therefore mean a significantly higher chance of UUID collision.. The fix introduced in this PR is to use the string comparison of the full UUID in places where .ID() was used. Signed-off-by: Josh Kim <kjosh@vmware.com>
Codecov Report
@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 87.56% 87.60% +0.03%
==========================================
Files 52 52
Lines 2695 2695
==========================================
+ Hits 2360 2361 +1
+ Misses 335 334 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
daveshanley
reviewed
Dec 7, 2021
| config := &bridge.BrokerConnectorConfig{ | ||
| Username: "guest", | ||
| Password: "guest", | ||
| ServerAddr: "appfabric.vmware.com", |
There was a problem hiding this comment.
I have seen a couple of these old references in here still. can we get rid of them?
daveshanley
approved these changes
Dec 7, 2021
|
@daveshanley For that purpose I explicitly split this work into two commits. By choosing the latest commit you'll be able to see the changes only pertaining to this PR. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
UUID is used throughout the entire codebase of Transport and is
especially important in filtering messages based on their destination.
While implementing basic WASM bridge for the bus I realized that UUID
comparison was failing, i.e. returning true for the equality check
between two dififerent UUID instances. Turns out the use of .ID()
method on the UUID object only returns the first 4 bytes of the
underlying 16-byte slice. This means as long as the first 8
hexadecimal characters matched between two UUID instances, they would
come out as equal, and it would therefore mean a significantly higher
chance of UUID collision.. The fix introduced in this PR is to use
the string comparison of the full UUID in places where .ID() was used.
Also performs
gofmtacross all source files, which is done in a separate committo make reviewing the PR easier.
Signed-off-by: Josh Kim kjosh@vmware.com