Enforce lookup limit for storage #312
Conversation
Pull Request Test Coverage Report for Build 3335
💛 - Coveralls |
|
I think that semantically "no limit" is a good zero value for limit. |
storage/listkeys.go
Outdated
| // TODO: this shouldn't be probably the case | ||
| limit = unlimited | ||
| if limit > LookupLimit { | ||
| return nil, ErrLimitExceeded |
There was a problem hiding this comment.
Wouldn't it better to override the user-defined limit with the internal LookupLimit? This proposal is just for the list methods, not for GetAll.
There was a problem hiding this comment.
Not feeling in any particular direction.
pkg/pointerdb/pointerdb.go
Outdated
| @@ -30,7 +30,7 @@ var ( | |||
| // ListPageLimit is the maximum number of items that will be returned by a list | |||
| // request. | |||
| // TODO(kaloyan): make it configurable | |||
| const ListPageLimit = 1000 | |||
| const ListPageLimit = storage.LookupLimit | |||
There was a problem hiding this comment.
I wonder if we still need this const here and the check in the List method, if this limit check is now handled in the DB.
There was a problem hiding this comment.
I was thinking about it, I also thought about how to configure it. There are a few roads we could take:
- Remove the limit from KeyValueStore and enforce it at service level.
- Add a wrapper around KeyValueStore that enforces such limits.
As usual with these things, since the cost of defensive programming here isn't high, it can be duplicated in both places.
There was a problem hiding this comment.
Ok, I'll remove it for now and let's revisit the problem when making it configurable.
|
@kaloyan-raev do you approve as . well? |
|
pr 318 fixes the integration failure - once that is fixed, integration should pass here. I'll leave my approve, assuming the fix will be merged first. |
| @@ -19,6 +19,9 @@ var ErrKeyNotFound = errs.Class("key not found") | |||
| // ErrEmptyKey is returned when an empty key is used in Put | |||
| var ErrEmptyKey = errors.New("empty key") | |||
|
|
|||
| // ErrLimitExceeded is returned when request limit is exceeded | |||
| var ErrLimitExceeded = errors.New("limit exceeded") | |||
There was a problem hiding this comment.
so i know we had this discussion in the golang channel (so feel free to ignore) if you wanted to add a . class here; jt mentions, if it seems right, he prefers zeebo/errs
There was a problem hiding this comment.
yeah, my preferences are just preferences. i would prefer to not have to grep to find where "limit exceeded" is in the codebase if i hit this error to understand what went wrong, but i can be convinced that specific error cases like this are fine
| @@ -10,6 +10,7 @@ import ( | |||
| "storj.io/storj/storage" | |||
|
|
|||
| "github.com/google/go-cmp/cmp" | |||
Enforces lookup limits for store implementations.
I'm not sure whether to change the
limit = 0means as much as possible behavior. I think the reason for this approach is that when you don't specify a value in API requests it defaults to 0, and you are unlikely to want 0 items.This change is