Add validation to Set(peers ...string) #142
Open
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
…h `peers` input value is a valid base URL with scheme as required and as noted in the func-level comments. The prior version of `Set()` did not include any validation of the input value(s) even though the func-level comments noted "Each peer value should be a valid base URL, for example 'http://example.net:8000'". This could lead to confusion when a cluster of peers cannot communicate. For example, if a user called `Set("localhost:8080")` this may look correct but the host on which `Set()` was called will not be able to communicate with the peer provided via the address `localhost:8080`. This is compounded by the fact no error is returned, nor is anything logged, leading the user to believe the error is elsewhere besides with the inputted address. To solve the above noted issue/confusion, `Set()` was modified to check if each input value is a valid URL and return an error when an input value is invalid. This is done by checking if the input URL has the required scheme (http or https since groupcache only uses http based communication at this point) and if a host was provided. Modifying the `Set()` func to return an `error` should not break anything since you can ignore return values from funcs. The lines of code within the `Set()` func were reordered so that validation of input peer address could be done before setting of any pool data.
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.
Check if each
peersinput value toSet(peers ...string)is a valid base URL as required and as noted in the func-level comments.The prior version of
Set()did not include any validation of the input value(s) even though the func-level comments noted "Each peer value should be a valid base URL, for example 'http://example.net:8000'". This could lead to confusion when a cluster of peers cannot communicate. For example, if a user calledSet("localhost:8080")this may look correct but the host on whichSet()was called will not be able to communicate with the peer provided via the addresslocalhost:8080. This is compounded by the fact no error is returned, nor is anything logged, leading the user to believe the error is elsewhere besides with the inputted address.To solve the above noted issue/confusion,
Set()was modified to check if each input value is a valid URL and return an error when an input value is invalid. This is done by checking if the input URL has the required scheme (http or https since groupcache only uses http based communication at this point) and if a host was provided. Modifying theSet()func to return anerrorshould not break anything since you can ignore return values from funcs.The lines of code within the
Set()func were reordered so that validation of input peer address could be done before setting of any pool data.