Accept limit and offset in enumerate API#6
Conversation
|
/gcbrun |
NilanjanDaw
left a comment
NilanjanDaw
left a comment
There was a problem hiding this comment.
Overall comment, the API is already published, so we need to make sure current behavior does not change with the addition f the limit and offset parameters. These should be additional optional parameters, and the API should work as-is without them without change in behavior.
| if limit == 0 { | ||
| limit = 100 // Default pagination limit | ||
| } | ||
| keys, _, err := s.keyProtectionService.EnumerateKEMKeys(int(limit), int(req.GetOffset())) |
There was a problem hiding this comment.
What if offset is not provided? The API will fail. Since this is a breaking API change we need to make it backward compatible.
There was a problem hiding this comment.
GetOffset returns 0 if offset is not provided.
| if err != nil { | ||
| return fmt.Errorf("failed to read request body: %w", err) | ||
| } | ||
| if len(body) == 0 { |
There was a problem hiding this comment.
What is this check for?
There was a problem hiding this comment.
If we do a get for /v1/keys protojson.Unmarshal fails as body is empty. Initially this would work but now that we have limit and offset we need to add this check if we want same behaviour.
| } | ||
| limit := req.GetLimit() | ||
| if limit == 0 { | ||
| limit = 100 // Default pagination limit |
There was a problem hiding this comment.
Additionally, this will change current API behavior limiting to 100 entries if no param is provided.
There was a problem hiding this comment.
What should the solution be here then? Set this limit to int_max?
| } | ||
| limit := req.GetLimit() | ||
| if limit == 0 { | ||
| limit = 100 // Default pagination limit |
There was a problem hiding this comment.
can we define a constant defaultEnumerateLimit for this?
atharvasaraf123
force-pushed
the
limit_and_offset
branch
from
9d8f5a5 to
6b0122b
Compare
atharvasaraf123
force-pushed
the
limit_and_offset
branch
from
bbc1cb3 to
a43e636
Compare
| writeError(w, "offset exceeds maximum allowed value", http.StatusBadRequest) | ||
| return | ||
| } | ||
| keys, _, err := s.keyProtectionService.EnumerateKEMKeys(r.Context(), int32(limit), int32(offset)) |
There was a problem hiding this comment.
we're discarding hasMore, the customer won't know if they're on the last page unless they look at the size of the response or call the next page and check if it has 0 entires. we should we sending hasMore in response.
But this would add to the currently shared signature. @NilanjanDaw your thoughts?
|
|
||
| message EnumerateKeysRequest { | ||
| // The maximum number of key infos to return. | ||
| uint64 limit = 1 [(buf.validate.field).uint64 = {gt: 0}]; |
There was a problem hiding this comment.
in workload_service/server.go we have logic to set defaultEnumerateLimit if limit isn't passed.
by default if limit isn't passed, it is considered 0, and it would fail due to this buf validation. we can remove this.
| if limit > math.MaxInt32 { | ||
| writeError(w, "limit exceeds maximum allowed value", http.StatusBadRequest) | ||
| return |
There was a problem hiding this comment.
this is dead code now
| // The maximum number of key infos to return. | ||
| uint64 limit = 1 [(buf.validate.field).uint64 = {gt: 0}]; | ||
| // The offset to start from. | ||
| uint64 offset = 2 [(buf.validate.field).uint64 = {gte: 0}]; |
There was a problem hiding this comment.
uint64 is already gte 0 and won't allow invalid values, so not sure if buf validation is useful here.
| @@ -325,6 +325,10 @@ type Server struct { | |||
| } | |||
|
|
|||
| var ( | |||
There was a problem hiding this comment.
not related to the PR but should be const
| } | ||
| } | ||
|
|
||
| func TestHandleEnumerateKeysPagination(t *testing.T) { |
There was a problem hiding this comment.
need to test:
- Empty body → should use default limit (100)
- {"limit": 2000} → should be capped at 1000
- Malformed JSON body → should return 400
3 participants
This review fixes the issue - google/go-tpm-tools#693
Changes: