Add POC for a proposed ListAll func for paginated resources #261

sergiught · 2023-08-10T16:24:07Z

🔧 Changes

This PR contains a proposed POC for a ListAll func on all paginatable resources. This PR shouldn't get merged as the purpose of it is just to facilitate discussions over the solution. Tests were omitted as well in case the function signature needs adjustments.

📚 References

🔬 Testing

📝 Checklist

All new/changed/fixed functionality is covered by tests (or N/A)
I have added documentation for all new/changed functionality (or N/A)

sergiught · 2023-08-10T16:25:10Z

management/client.go

+// If limit is 0 it will retrieve all the clients, otherwise it will retrieve all the clients until the limit is reached.
+//
+// See: https://auth0.com/docs/api/management/v2#!/Clients/get_clients
+func (m *ClientManager) ListAll(ctx context.Context, limit int, opts ...RequestOption) (clients []*Client, err error) {


Some further ideas for discussions over here on the topic of limit:

A) Remove the limit entirely and always fetch everything

B) Provide the limit as a designated RequestOption for ListAll funcs.

Without a limit, this feature would not be very useful for the Auth0 CLI, because list commands accept a -n flag to limit the number of elements to output. Of course, we could still fetch it all and then drop the rest, but that's not particularly efficient nor DX-friendly (more requests than necessary take longer).

The ListAll function should do just that, list all. It is contradictory for the public API to state that you're getting all when in reality, it's only retrieving up to a limit. Further, the DX improvement here is that I as a user shouldn't need to worry about what the limit is, it's just asking for more cognitive overhead.

Re: Auth0 CLI, it certainly is a use-case to consider but IMO we shouldn't have that decide wether or not we expose a limit as the API. We can make it more private through other means like as a request option (as Sergiu suggested above).

A request option certainly seems like a good fit for it, instead of a regular argument.

+1 on moving to a RequestOption rather than a dedicated part of the API

I think getting the first X elements without having to handle the pagination is a legitimate enough use case. I wonder if it should be a separate method, though. Or if this method should be renamed to something that does not imply getting 'all', like ListPaginated(), and take the limit as a regular argument.

Have you folks discussed this? @Widcket , @ewanharris , @willvedd ?

I'd be in favour of having 2 extra funcs instead of just one:

ListAll(ctx context.Context, opts ...RequestOption) (clients []*Client, err error) # Or ListPaginated, ListWithLimit, ListFirstN, ListUntilLimit(ctx context.Context, limit int, opts ...RequestOption) (clients []*Client, err error)

@ewanharris and I had considered this. It's certainly a valid solution that would address all the technical concerns but I could also see it being too much being added at once. Frankly, as long as there is some ListAll for all lists, we're good; its not the worst thing if custom pagination exists in the Auth0 CLI codebase.

I'd also be concerned, if we add a way to "get all" without also adding a way to "get N", that we could be encouraging "getting all" and then extracting the first N from the result (because it'll be easier than implementing the pagination from scratch).

Have you folks discussed this? @Widcket , @ewanharris , @willvedd ?

I'm afraid I wasn't part of those discussions.

Fully agree with this @Widcket 👍🏻 , however we can split the 2 funcs in 2 separate PRs, but combine both in 1 release.

sergiught · 2023-08-10T16:25:32Z

management/client.go

+	opts []RequestOption,
+	api func(ctx context.Context, opts ...RequestOption) (result []Resource, hasNext bool, err error),
+) ([]Resource, error) {
+	const defaultPageSize = 100


Should we keep this to 100? Or use the current GO SDK default of 50?

Fewer requests, fewer 429s, better performance.

Should we then also change the default of 50 through out the entire SDK to 100?

codecov-commenter · 2023-08-10T16:26:40Z

Codecov Report

Patch coverage: 70.83% and project coverage change: -0.07% ⚠️

Comparison is base (ac7326c) 94.78% compared to head (429f74a) 94.72%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #261      +/-   ##
==========================================
- Coverage   94.78%   94.72%   -0.07%     
==========================================
  Files          46       46              
  Lines        8733     8757      +24     
==========================================
+ Hits         8278     8295      +17     
- Misses        353      358       +5     
- Partials      102      104       +2

Files Changed	Coverage Δ
management/client.go	`84.61% <62.50%> (-2.14%)`	⬇️
management/management_request.go	`81.97% <75.00%> (-0.72%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sergiught · 2023-08-10T16:30:17Z

management/client.go

+	*Client | *Connection | *Organization
+}
+
+func getWithPagination[Resource paginatedResource](


On the topic of testing, I think we can simply test the ListALl funcs and get this covered intrinsically as well, but open to other ideas.

Yes, I think that's fine.

management/client.go

ewanharris · 2023-08-22T16:13:41Z

management/client_test.go

@@ -87,6 +87,17 @@ func TestClient_List(t *testing.T) {
 	assert.Contains(t, clientList.Clients, &Client{ClientID: expectedClient.ClientID})
 }

+func TestClient_ListAll(t *testing.T) {
+	configureHTTPTestRecordings(t)


This doesn't really test the pagination aspect that well, it might be best to allow overriding the PerPage so that we can set it to a low value here and properly test that pagination aspect

Should we add a test similar to https://github.com/auth0/auth0-cli/blob/7d27438692215cc4904f5116e1a3e82a030d25a9/internal/cli/terraform_fetcher_test.go#L43 ?

ewanharris · 2023-08-22T16:15:57Z

@willvedd @Widcket I've removed the limit argument to keep this as strictly a "paginate all" API and also added a small test to cover it so it should be good for review

Widcket · 2023-08-22T17:19:32Z

Interestingly, on node-auth0 v4 getAll() does not use pagination at all, so it does not, in fact, "get all": https://github.com/auth0/node-auth0/blob/beta/src/management/__generated/managers/clients-manager.ts#L109.

So not entirely sure we should go by that token here, given how we typically look at the node-auth0 v4 implementation to model things.

sergiught · 2023-08-23T07:57:05Z

Good point raised @Widcket, however I think that we shouldn't necessarily follow stricly what node-auth0 is doing, we can stay curious and do the best DX for the given programming language. Naming something getAll and not actually returning all the resources is not intuitive IMO.

Widcket · 2023-08-23T08:12:36Z

I think that we shouldn't necessarily follow stricly what node-auth0 is doing, we can stay curious and do the best DX for the given programming language.

Oh I totally agree, was just pointing out observed behavior.

Given that this PoC was originally created after auth0/auth0-cli#797 was raised, to try and find a way to reduce the code needed to add new resources (as per Slack convo), and that as it stands now it won't be able to fulfill this purpose:

Do we want to keep this new functionality as getAll and add a new method that can be used for the CLI, that allows specifying a limit?
Do we want to rename this method to something else that does not imply "getting all", as the name is a problem?
Do we want to keep this new functionality as getAll and not add anything else that can be used for the CLI?

DavidS-ovm · 2023-11-01T15:18:35Z

management/management_request.go

+	var list []Resource
+	var pageNumber int
+	for {
+		opts = append(opts, PerPage(pageSize), Page(pageNumber))


this will lead to opts growing on each iteration. maybe this would be better?

Suggested change

opts = append(opts, PerPage(pageSize), Page(pageNumber))

realOpts = append(opts, PerPage(pageSize), Page(pageNumber))

and then use realOpts below on the API call.

ewanharris · 2023-12-08T17:00:32Z

I'm going to close this PR as we have decided that for now we do not wish to add a ListAll function into the SDK, we will instead look to improve our documentation for functions and link to the pagination documentation as necessary (see #328)

sergiught commented Aug 10, 2023

View reviewed changes

sergiught requested review from Widcket and ewanharris August 10, 2023 16:26

sergiught commented Aug 10, 2023

View reviewed changes

sergiught commented Aug 11, 2023

View reviewed changes

management/client.go Outdated Show resolved Hide resolved

sergiught and others added 4 commits August 22, 2023 17:01

Add POC for a proposed ListAll func for paginated resources

b8df425

Make limit more private

b355d38

Remove limit request option

24c1d37

Remove limit argument

ce5a22a

ewanharris force-pushed the feature/list-all-poc branch from 9c339fb to ce5a22a Compare August 22, 2023 16:03

Add basic test for Client.ListAll

429f74a

ewanharris reviewed Aug 22, 2023

View reviewed changes

ewanharris marked this pull request as ready for review August 22, 2023 16:14

ewanharris requested a review from a team as a code owner August 22, 2023 16:14

ewanharris requested a review from willvedd August 22, 2023 16:15

sergiught marked this pull request as draft September 8, 2023 12:32

DavidS-ovm reviewed Nov 1, 2023

View reviewed changes

ewanharris closed this Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add POC for a proposed ListAll func for paginated resources #261

Add POC for a proposed ListAll func for paginated resources #261

sergiught commented Aug 10, 2023

sergiught Aug 10, 2023

Widcket Aug 10, 2023

willvedd Aug 10, 2023

Widcket Aug 10, 2023

ewanharris Aug 11, 2023

sergiught Aug 23, 2023

willvedd Aug 23, 2023

Widcket Aug 23, 2023

Widcket Aug 23, 2023

sergiught Aug 23, 2023

sergiught Aug 10, 2023

willvedd Aug 10, 2023

sergiught Aug 23, 2023

codecov-commenter commented Aug 10, 2023 •

edited

Loading

sergiught Aug 10, 2023

Widcket Aug 10, 2023

ewanharris Aug 22, 2023

sergiught Aug 23, 2023

ewanharris commented Aug 22, 2023

Widcket commented Aug 22, 2023

sergiught commented Aug 23, 2023

Widcket commented Aug 23, 2023 •

edited

Loading

DavidS-ovm Nov 1, 2023 •

edited

Loading

ewanharris commented Dec 8, 2023

	opts = append(opts, PerPage(pageSize), Page(pageNumber))
	realOpts = append(opts, PerPage(pageSize), Page(pageNumber))

Add POC for a proposed ListAll func for paginated resources #261

Add POC for a proposed ListAll func for paginated resources #261

Conversation

sergiught commented Aug 10, 2023

🔧 Changes

📚 References

🔬 Testing

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ewanharris commented Aug 22, 2023

Widcket commented Aug 22, 2023

sergiught commented Aug 23, 2023

Widcket commented Aug 23, 2023 • edited Loading

DavidS-ovm Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

ewanharris commented Dec 8, 2023

codecov-commenter commented Aug 10, 2023 •

edited

Loading

Widcket commented Aug 23, 2023 •

edited

Loading

DavidS-ovm Nov 1, 2023 •

edited

Loading