filter/firewall-rules fix for double API calls #1016
Conversation
|
changelog detected ✅ |
| @@ -91,20 +93,8 @@ func (api *API) Filter(ctx context.Context, rc *ResourceContainer, filterID stri | |||
| // | |||
| // API reference: https://developers.cloudflare.com/firewall/api/cf-filters/get/#get-all-filters | |||
| func (api *API) Filters(ctx context.Context, rc *ResourceContainer, params FilterListParams) ([]Filter, *ResultInfo, error) { | |||
| uri := buildURI(fmt.Sprintf("/zones/%s/filters", rc.Identifier), params) | |||
There was a problem hiding this comment.
this won't work for the multiple pages condition as it requires the ResultInfo to have the pagination populated to move the pager.
what is the issue you're trying to solve here? right now, I'm not particularly fussed with the additional HTTP call providing it's not duplicating results in the array. I have an idea to remove it however, it requires making all endpoints use pagination by default which will need some digging internally to ensure we don't break things.
There was a problem hiding this comment.
I've opened a bug describing my findings: #1024
There was a problem hiding this comment.
And I also updated my PR and verified that the changes work as expected.
There was a problem hiding this comment.
the List* style methods need to support automatic pagination and if someone passes in their own pagination options. i've pulled this locally and confirmed it's still not fixed for the scenario where we allow manual pagination. below is an example of it not working:
package main
import (
"context"
"log"
"github.com/cloudflare/cloudflare-go"
"github.com/davecgh/go-spew/spew"
)
func main() {
api, err := cloudflare.New(os.Getenv("CLOUDFLARE_API_KEY"), os.Getenv("CLOUDFLARE_API_EMAIL"))
if err != nil {
log.Fatal(err)
}
ctx := context.Background()
filters, res, _ := api.Filters(ctx, cloudflare.ZoneIdentifier("0da42c8d2132a9ddaf714f9e7c920711"), cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{Page: 2}})
spew.Dump(len(filters), res)
// => filters should be > 0 however it is empty and `res` shows an empty ResultInfo
//
// (*cloudflare.ResultInfo)(0xc0000e82d8)({
// Page: (int) 0,
// PerPage: (int) 0,
// TotalPages: (int) 0,
// Count: (int) 0,
// Total: (int) 0,
// Cursor: (string) "",
// Cursors: (cloudflare.ResultInfoCursors) {
// Before: (string) "",
// After: (string) ""
// }
// })There was a problem hiding this comment.
Gotcha, I missed that requirement. I've then introduced an autoPaginate var that I'll set to false in case either Page or PerPage is provided. It looks better know I think.
However, I noticed, that not all PerPage values are supported. For example, if you provide 3, you get
filters.api.unknown_error
Wonder if we could handle this better.
Also, I further updated method signatures as discussed.
There was a problem hiding this comment.
Gotcha, I missed that requirement. I've then introduced an autoPaginate var that I'll set to false in case either Page or PerPage is provided. It looks better know I think.
definite improvement however, i'm really not happy with how we are handling pagination as a whole because this is quite verbose to add in all endpoints that need to support it. i'd really prefer if we had a way in the ListParams to explicitly say we only intended to fetch a single result. something we can work on with the experimental client down the track i guess.
Wonder if we could handle this better.
something to raise with the service team; i suspect there are minimums in the per page counts as < 5 (or even 10) don't really make sense for pagination.
There was a problem hiding this comment.
I agree with you. We can do better and abstract away pagination logic in a more efficient and reusable manner.
FWIW, you can retrieve a single results with the current setup from any page.
For example, if you want one result from page 2:
fwRules, resultInfo, err := api.Filters(context.Background(),
cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"),
cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{PerPage: 1, Page: 2}})
Or 1 result from page 1
fwRules, resultInfo, err := api.Filters(context.Background(),
cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"),
cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{PerPage: 1}})
Or everything from page 2
fwRules, resultInfo, err := api.Filters(context.Background(),
cloudflare.ZoneIdentifier("b1fbb152bbde3bd28919a7f4bdca841f"),
cloudflare.FilterListParams{ResultInfo: cloudflare.ResultInfo{Page: 2}})
tamas-jozsa
force-pushed
the
tamas/fix-double-api-calls
branch
from
0854ac3
to
977978f
Compare
tamas-jozsa
force-pushed
the
tamas/fix-double-api-calls
branch
from
977978f
to
397a485
Compare
tamas-jozsa
force-pushed
the
tamas/fix-double-api-calls
branch
from
397a485
to
a3b00d7
Compare
|
if you add a CHANGELOG entry (per the automation), we'll get this merged. thanks! |
Codecov Report
@@ Coverage Diff @@
## master #1016 +/- ##
==========================================
+ Coverage 49.06% 49.18% +0.12%
==========================================
Files 108 112 +4
Lines 10428 10549 +121
==========================================
+ Hits 5116 5189 +73
- Misses 4200 4232 +32
- Partials 1112 1128 +16
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
This functionality has been released in v0.47.0. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
Description
Recent pagination updates to these APIs are causing endpoints to be called twice. This PR aims to fix them.
Has your change been tested?
Unit tests are passing, and I tried this version in cf-terraforming, where it's working as expected
Types of changes
What sort of change does your code introduce/modify?
Checklist:
Closes #1024