-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add DID static validation without tests #291
Conversation
Signed-off-by: Andrew Nikitin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a rocket, really. Just a few changes to apply
match := SplitDIDRegexp.FindStringSubmatch(did) | ||
if len(match) > 0 { | ||
return match[1], match[3], match[4] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's throw an error otherwise
return ErrStaticDIDBadMethod.Wrap(method) | ||
} | ||
// check namespaces | ||
if !DidNamespaceRegexp.MatchString(namespace) || !utils.Contains(allowedNamespaces, namespace) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move regexp matches to the split method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So validate will only compare namespace / method with passed parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be cool to throw the exception while splitting the DID? From my point of view it's a phase of validation.
return ErrStaticDIDNamespaceNotAllowed.Wrap(namespace) | ||
} | ||
// check unique-id | ||
err := ValidateUniqueId(unique_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one too
return err == nil | ||
} | ||
|
||
// SplitDID panics if did is not valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't panic :) Return an error
"regexp" | ||
) | ||
|
||
var SplitDIDRegexp, _ = regexp.Compile(`did:([^:]+?)(:([^:]+?))?:([^:]+)$`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var SplitDIDRegexp, _ = regexp.Compile(`did:([^:]+?)(:([^:]+?))?:([^:]+)$`) | |
var SplitDIDRegexp, _ = regexp.Compile(`^did:([^:]+?)(:([^:]+?))?:([^:]+)$`) |
|
||
// SplitDIDUrl panics if did cannot be splitted properly | ||
func SplitDIDUrl(didUrl string) (did string, path string , query string, fragment string) { | ||
match := SplitDIDURL.FindStringSubmatch(didUrl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's check for matches count
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we check for matches count?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It matches only once for Golang regexps... I checked it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, The last changes in regexp doesn't match in case of fragment being #key123???#####
return err | ||
} | ||
// Validate path | ||
err = ValidatePath(path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And let's move those checks to the split method as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we need to mix validation and splitting?
Signed-off-by: Andrew Nikitin [email protected]