Setting MaxItems to a low value when calling ZonesListAll may drastically reduce performance.

[com-m]

Expected Behavior: Function quickly returns with an array no greater in length than the maxItems parameter

Actual Behavior: If the server response contains a "nextID" field, and the number of potential responses is high for the given query, the function will continually make requests to the server maxItems at a time until all potential responses have been exhausted. This can result in ZoneListAll taking an extended period of time to return, and eventually returning an array longer than the maxItems parameter.

How to reproduce: Run unit tests for the zone functions in the client, mocking a backend that sends the appropriate test fixtures for the request. If maxItems is set to 1, two requests will be sent and two items will be returned in the array instead of one.

[jranson]

@com-m I started looking at this issue in case i can fix it up for Hacktoberfest2020, but i think there is a misunderstanding about the functionality, and want to make sure this isn't actually working as designed already.

In looking @ the VinylDNS API specification I see this:

maxItems: The number of items to return in the page. Valid values are 1 - 100. Defaults to 100 if not provided.

In the golang client code, i see that the description of the ZonesListAll function is commented as follows:

// ZonesListAll retrieves the complete list of zones with the ListFilter criteria passed.
// Handles paging through results on the user's behalf.

So the maxItems flag appears to only matter for paginating the API to collect the records, while the golang client will always pull all zones. If you use maxItems = 1 in the golang client, you should still expect to get all zones, it will just request one item at a time from the server. It probably makes sense to always use maxItems = 100 with no expectation of limiting the result set.

[remerle]

This seems like it's working as it was designed, but that design leaves something to be desired. I would expect MaxItems to limit the number of items returned, not control the internal (hidden) pagination. It seems to be a pattern repeated with the *All methods:

https://github.com/vinyldns/go-vinyldns/blob/148a5f6b8f14334d458615cbce3f6e76855a820a/vinyldns/zones.go#L172-L177

We either need to not allow the user to affect the page size, or actually limit the number of results returned.

Perhaps we can add a RetrieveAllPages bool flag to the ListFilter? This would cause MaxItems to be ignored and it will be defaulted to 100. https://github.com/vinyldns/go-vinyldns/blob/148a5f6b8f14334d458615cbce3f6e76855a820a/vinyldns/resources.go#L45-L51

@mdb What do you think about this. I don't want to break backward compatibility, so we'd need to default RetreiveAllPages to true. I haven't looked at all the code to see if this makes sense. However, the current implementation is a bit counter-intuitive.

[mdb]

@remerle Apologies for the delayed response, here.

This seems like it's working as it was designed, but that design leaves something to be desired.

Yes; I agree with this assessment. the *All-named methods are typically intended to handle paging on users' behalf.

Perhaps we can add a RetrieveAllPages bool flag to the ListFilter? This would cause MaxItems to be ignored and it will be defaulted to 100.

I think this sounds reasonable, assuming there isn't ever a scenario where a ListFilter is passed to a method that will ignore RetrieveAllPages: true (I don't believe there is, currently), in which case the option would be similarly misleading to users.