refactor record validation #17
Conversation
Instead of re-implementing the key splitting logic everywhere, we might as well expose it.
When generalizing routing, I noticed a couple of issues: 1. Passing around separate validators/selectors is annoying. All records should support both validation and selection. 2. We already need validators with state (e.g., access to the peerstore) so we might as well define them in terms of an interface. 3. Not all record stores will use the record protobuf type and tying validation to it is kind of annoying. 4. Using the same interface for the namespaced validator and the sub-validators is really convenient. The cost of having to re-split the key is minimal.
ghost
assigned 
| // Validate validates the given record, returning an error if it's | ||
| // invalid (e.g., expired, signed by the wrong key, etc.). | ||
| Validate(key string, value []byte) error | ||
|
|
There was a problem hiding this comment.
Is it worth making the Select function look more like the less parameter to Sort's Slice function?
Otherwise every Select implementation will need to implement similar functionality to iterate over the values, return an error if the list is empty, or if there is no value found etc
There was a problem hiding this comment.
IMO, that can be implemented as a wrapper. Defining Select as we have here allows us to halve the number of times we have to deserialize the values when comparing multiple values.
Wrapper:
func Less(key string, values [][]byte, v Validator) func(i, j int) bool { /* ... */ }
There was a problem hiding this comment.
@Stebalien agreed. this is less a full sort and more a min function. unless there is use for a full priority sorted list of entries, i think this approach is fine.
|
|
||
| // SplitKey takes a key in the form `/$namespace/$path` and splits it into | ||
| // `$namespace` and `$path`. | ||
| func SplitKey(key string) (string, string, error) { |
There was a problem hiding this comment.
could simplify this a bit i think!
parts := strings.SplitN(key, "/", 3)
if len(parts) < 3 || len(parts[0]) > 0 || len(parts[1]) == 0 || len(parts[2]) == 0 {
return "", "", ErrInvalidRecordType
}
return parts[1], parts[2], nildno if this matters tho!
There was a problem hiding this comment.
When I first wrote that function, I chose not to use SplitN to save on allocations. Basically, I want to make that function as fast as possible so we can call it with impunity.
There was a problem hiding this comment.
sgtm. impl is strong as stands, then.
validator.go
Outdated
|
|
||
| // ErrBadRecord is returned any time a dht record is found to be | ||
| // incorrectly formatted or signed. | ||
| var ErrBadRecord = errors.New("bad dht record") |
There was a problem hiding this comment.
perhaps for purposes of more specific logging, we could separate into
var ErrBadRecord = errors.New("bad dht record: improperly formatted")
var ErrBadSignature = errors.New("bad dht record: improperly signed")There was a problem hiding this comment.
Actually, it looks like ErrBadRecord isn't even used. Validators return different errors depending on what went wrong. I'll just remove that error.
| // Validate validates the given record, returning an error if it's | ||
| // invalid (e.g., expired, signed by the wrong key, etc.). | ||
| Validate(key string, value []byte) error | ||
|
|
There was a problem hiding this comment.
@Stebalien agreed. this is less a full sort and more a min function. unless there is use for a full priority sorted list of entries, i think this approach is fine.
Stebalien
force-pushed
the
feat/routing-refactor
branch
from
fa9cc59 to
b3dd0d6
Compare
When generalizing routing, I noticed a couple of issues: