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:

go-vinyldns/vinyldns/zones.go

Lines 172 to 177 in 148a5f6

	// ZoneChangesListAll retrieves the complete list of zone changes with the ListFilter criteria passed.
	// Handles paging through results on the user's behalf.
	func (c *Client) ZoneChangesListAll(zoneID string, filter ListFilter) ([]ZoneChange, error) {
	if filter.MaxItems > 100 {
	return nil, fmt.Errorf("MaxItems must be between 1 and 100")
	}

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.

go-vinyldns/vinyldns/resources.go

Lines 45 to 51 in 148a5f6

	// ListFilter represents the list query parameters that may be passed to
	// VinylDNS API endpoints such as /zones and /zones/${zone_id}/recordsets
	type ListFilter struct {
	NameFilter string
	StartFrom string
	MaxItems int
	}

@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.