client: Expose rate limit extensions in response #12
Conversation
philipgough
force-pushed
the
expose-limit-extensions
branch
from
824e376 to
ffa7c9f
Compare
|
While this looks good for responses, there is another response we are very interested in, which is the one you get when you specify the |
|
@unleashed Yep sure. I will take a look at that. |
|
@philipgough 3scale/apisonator#75 - we haven't documented it yet, so it's a good moment to do so (we can talk doubts in the chat). |
|
@Unleased I found that and then deleted my comment but you were too quick :) |
philipgough
force-pushed
the
expose-limit-extensions
branch
4 times, most recently
from
0e844ca to
219d10e
Compare
|
@unleashed - this pr grew a bit from the original plan. Regardless, support and tests have now been added for limit and hierarchy extension and ready for review. |
philipgough
force-pushed
the
expose-limit-extensions
branch
from
219d10e to
22e535e
Compare
client/client.go
Outdated
|
|
||
| if limReset := resp.Header[limitResetHeaderKey][0]; limReset != "" { | ||
| resetLimit, _ := strconv.Atoi(limReset) | ||
| authRepRes.limitReset = &resetLimit |
There was a problem hiding this comment.
this does not look good if this is meant to be an integer - why not do this:
authRepRes.limitReset = strconv.Atoi(limReset)(same thing applies just above)
There was a problem hiding this comment.
Well it returns two values, one of which is an error which we can safely ignore here. Can probably move this to one line. 👍
There was a problem hiding this comment.
Can we safely ignore it? I mean, if we do, what would be the contents of the int variable? 0? I think I'd prefer to have it nil'ed if so.
There was a problem hiding this comment.
Well I personally don't like to ignore errors. Bt in this case I thought it was safe to assume that backend will return a value that can be cast to an int meaning we dont have to check the error. However happy to go the more verbose path if you think its safer.
There was a problem hiding this comment.
Yep, backend won't do that ever, because it's officially bug-free, but you know, sometimes some ops guys tries to modify live production code or the odd contribution ends up affecting backend in the future...
So this is my proposal: don't turn them into pointers, use a struct to contain both ints as normal variables, and use a pointer to the struct to signal it is available. Now, if the parsing fails, you could use a non-sensical value, perhaps a negative value. Wdyt?
client/types.go
Outdated
| Success bool | ||
| StatusCode int | ||
| limitRemaining *int | ||
| limitReset *int |
There was a problem hiding this comment.
Why are these pointers? You seldom see pointers to (machine) word-sized integers because it is more efficient to just store them in the same space a pointer would consume, plus you save the indirection caused by a second access to memory.
There was a problem hiding this comment.
Because the zero value for integers is 0 and I did not want it to be confusing. If someone calls the function and the extension is not being used, prefer to return nil than zero, wdyt?
There was a problem hiding this comment.
I think then I'd prefer to have a LimitRemaining struct with both ints so that you'd only need to check one pointer for nil, and if it exists, then you can read the ints without further indirection nor checks.
There was a problem hiding this comment.
Ok so you mean to wrap the limits in some struct, maybe limitExtensions or something and assume its safe to do a single check for nil?
There was a problem hiding this comment.
yep, if the header was parsed (even if the Atoi failed, which you can deal with separately) you then fill the two integers in this struct and set a pointer to it in the response (otherwise it just stays with nil).
philipgough
force-pushed
the
expose-limit-extensions
branch
from
22e535e to
44137aa
Compare
test: Updates extension test for changes to limit extension api
…his simplifies the interface for the caller and avoids duplicate nil checks
philipgough
force-pushed
the
expose-limit-extensions
branch
from
44137aa to
7398eb2
Compare
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
==========================================
+ Coverage 81.81% 82.97% +1.15%
==========================================
Files 5 5
Lines 242 276 +34
==========================================
+ Hits 198 229 +31
+ Misses 33 32 -1
- Partials 11 15 +4
Continue to review full report at Codecov.
|
No description provided.