feat: add rate limiting policy support#428
Conversation
| Id string `uri:"id"` | ||
| } | ||
|
|
||
| type ListRateLimitingPoliciesRequest struct { |
There was a problem hiding this comment.
Not 100% sure I'm doing this the right way, as while there are some examples (e.g. live status) that are similar with requests and responses, there seem to be numerous different patterns at play in this repo. This seems the most strongly aligned with our target architecture though, and aligns with the C# client too.
68f0fbc to
24905ca
Compare
24905ca to
8f57a45
Compare
Review (from Claude)I reviewed this PR for correctness, conformance with recent SDK patterns (using Alignment with the C# PR
Everything else aligns well: the core field set, the Conformance with SDK patterns
Minor
Nice work overall — the test coverage (marshal round-trips, invalid-enum, and end-to-end modify/error cases) is thorough. |
| package ratelimitingpolicies | ||
|
|
||
| type RateLimitingPolicy struct { | ||
| Id string `json:"Id"` |
There was a problem hiding this comment.
Searching for json:"Id" finds that we are inconsistent, sometimes the Go field name is ID and other times Id.
ID seems to be winning on numbers though, and the Go style guide says "ID" so we should use that here.
There was a problem hiding this comment.
Will do. What about something like GetRateLimitingPolicyByIdRequest? Should I change that to GetRateLimitingPolicyByIDRequest? I wonder if we ever start code-genning these things, will we still try to do ID or allow it to remain Id to match the C# types 🤔
There was a problem hiding this comment.
To help decide here, I did a case-sensitive find-in-files for ById and a second for ByID
There are 44 ById and 968 ByID.
Combined with the google style guide saying ID, I think that's a pretty clear push to GetRateLimitingPolicyByIDRequest
| type RateLimitingPolicyScopeType int | ||
|
|
||
| const ( | ||
| Unauthenticated RateLimitingPolicyScopeType = iota |
There was a problem hiding this comment.
Just checking, this serializes across the JSON API as a string, doesn't it?
There was a problem hiding this comment.
Yep! TestRateLimitingPolicyScopeTypeJsonMarshal goes back and forth to confirm
There was a problem hiding this comment.
TIL about this enumer code generator
borland
left a comment
There was a problem hiding this comment.
Apart from the casing of ID, LGTM. Approved so you can merge when you're ready

Adds support for an upcoming rate limiting feature, currently behind
RateLimitingV2FeatureToggle. Three methods are added (pseudo-signatures:)GetByID(GetRateLimitingPolicyByIdRequest) -> RateLimitingPolicyList(ListRateLimitingPoliciesRequest) -> ListRateLimitingPoliciesResponseModify(ModifyRateLimitingPolicyCommand) -> RateLimitingPolicy