Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(api *Client) GetUsersContext() doesn't return error #557

Closed
henkejosh opened this issue Jul 16, 2019 · 1 comment · Fixed by #565
Closed

(api *Client) GetUsersContext() doesn't return error #557

henkejosh opened this issue Jul 16, 2019 · 1 comment · Fixed by #565

Comments

@henkejosh
Copy link

Actual Behavior:
The (api *Client) GetUsersContext() method stops paginating if it encounters any errors during pagination, and returns a nil error.

Expected Behavior:
The method would return the actual error encountered so that we know that the returned data is incomplete.

Happy to provide more info if necessary, but it seems to be a simple issue w/ the for loop in GetUsersContext:

for p = api.GetUsersPaginated(options...); !p.Done(err); p, err = p.Next(ctx) {
	results = append(results, p.Users...)
}

p.Next(ctx) receives an error from t.c.userRequest() on line 328 and returns an error (that isn't the errPaginationComplete) without setting the t.previousResp. Since we're not done (!p.Done(err)), we continue paginating and hit p.Next(ctx) again. This time, line 314 evaluates to true (since we have a non-nil t.previousResp but an empty t.previousResp.Cursor b/c it was never set on line 334 when we returned early from the previous iteration. We then fire off an errPaginationComplete error, effectively ending pagination but not letting the user know that their data is incomplete.

A quick fix might be as simple as changing the for loop in GetUsersContext to:

for p = api.GetUsersPaginated(options...); !p.Done(err); p, err = p.Next(ctx) {
	if err != nil {
		break
	}
	results = append(results, p.Users...)
}
@james-lawrence
Copy link
Contributor

closing this in favor of armsnyder's issue.

armsnyder added a commit to armsnyder/slack that referenced this issue Aug 9, 2019
Fix slack-go#563 (Auto-Paginated API Methods Can Include Duplicates If Errors Occur)
Fix slack-go#557 (GetUsersContext() doesn't return error)
james-lawrence pushed a commit that referenced this issue Aug 31, 2019
Fix #563 (Auto-Paginated API Methods Can Include Duplicates If Errors Occur)
Fix #557 (GetUsersContext() doesn't return error)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants