Improve bounds checking for RenewFileset #10350
Conversation
Addresses MLDM-56 and MLDM-45.
size-limit report 📦
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10350 +/- ##
==========================================
+ Coverage 62.40% 62.44% +0.03%
==========================================
Files 1218 1218
Lines 87608 87609 +1
Branches 1820 1820
==========================================
+ Hits 54675 54707 +32
+ Misses 32259 32227 -32
- Partials 674 675 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/internal/storage/server.go
Outdated
| } | ||
| if ttl > maxTTL { | ||
| return nil, errors.Errorf("ttl (%d) exceeds max ttl (%d)", ttl, maxTTL) | ||
| var ttl time.Duration |
There was a problem hiding this comment.
Nits, ttl could just be declared where it is set and I don't think the comment is really necessary. The implicit structure should just be validate then process with the expectation that the inputs are valid at that point.
src/internal/storage/server_test.go
Outdated
| @@ -419,3 +420,28 @@ func checkFilesetCDR(ctx context.Context, t *testing.T, c storage.FilesetClient, | |||
| } | |||
| require.Equal(t, 0, len(expected)) | |||
| } | |||
|
|
|||
| func TestRenewFileset(t *testing.T) { | |||
There was a problem hiding this comment.
This seems like it should indicate it is testing the validation. Maybe in the test name, or just a subtest such that we could add other types of subtests later.
| require.True(t, strings.Contains(err.Error(), "exceeds max ttl")) | ||
| _, err = c.RenewFileset(ctx, &storage.RenewFilesetRequest{ | ||
| FilesetId: id, | ||
| TtlSeconds: 1800000000000, |
There was a problem hiding this comment.
This test case seems unnecessary, and would require maintenance if we changed the max ttl. Also, it looks like there are duplicate max ttl error checks below this.
There was a problem hiding this comment.
I’ve removed the duplicated error check.
This test case is deliberately trying to test that a value much greater than the max TTL but much less than the maximum int64 is invalidated.
Addresses MLDM-56 and MLDM-45.