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

Consistent pagination experience across all APIs #86

Merged
merged 11 commits into from
Oct 21, 2022
Merged

Consistent pagination experience across all APIs #86

merged 11 commits into from
Oct 21, 2022

Conversation

nfx
Copy link
Contributor

@nfx nfx commented Sep 14, 2022

Goal: present consistent entity listing for the end-user

TODO:

Databricks REST API has diverse pagination style:
image

Only 30% of APIs have pagination, though there are two different implementations of it. This PR proposes generating wrapper methods to return consistent type for a collection of all results. It also adds frequently used wrappers, like name-to-id mapping, which could later be used to generate client-level upserts.

Decisions to discuss:

Return type: slice vs iterator

  • Current implementation proposes returning slice + error
  • theoretically, with added complexity, we can move to iterator + error implementation, though Go ecosystem didn't settle on the standard iterator yet: discussion: standard iterator interface golang/go#54245
  • other ecosystems, like Python, Java, and JavaScript should lean towards iterators

Naming: ListQueriesAll (suffix) vs ListAllQueries (infix) vs all methods returning slices (might be invasive)

  • current template has "All" name suffix for methods returning all results
  • 18% of APIs return slice from the platform.
  • 36% of APIs return all results, but in a single-field response.

Out of scope of this PR

@nfx nfx requested a review from a team September 14, 2022 19:02
databricks/openapi/code/method.go Outdated Show resolved Hide resolved
service/clusters/api.go Outdated Show resolved Hide resolved
service/clusters/api.go Show resolved Hide resolved
service/dbfs/model.go Outdated Show resolved Hide resolved
@nfx
Copy link
Contributor Author

nfx commented Sep 19, 2022

@shreyas-goenka

@pietern pietern self-requested a review September 19, 2022 11:44
@nfx nfx changed the title Unify list return types [25% done] Unify list return types [cursor pagination remaining] Sep 22, 2022
@nfx nfx marked this pull request as ready for review September 23, 2022 20:45
@nfx nfx changed the title Unify list return types [cursor pagination remaining] Unify list return types Sep 30, 2022
Copy link
Contributor

@pietern pietern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to check the 1-based vs 0-based pagination assumption with the following test:

func TestAccQueriesList(t *testing.T) {
	ctx := context.Background()
	wsc := workspaces.New()

	var qs1, qs2 []dbsql.Query

	{
		result, err := wsc.Queries.ListQueries(ctx, dbsql.ListQueriesRequest{PageSize: 10})
		require.NoError(t, err)
		qs1 = result.Results
	}

	{
		result, err := wsc.Queries.ListQueriesAll(ctx, dbsql.ListQueriesRequest{})
		require.NoError(t, err)
		qs2 = result
	}

	for i := 0; i < len(qs1) && i < len(qs2); i++ {
		assert.Equal(t, qs1[i], qs2[i], "Query at index %d not equal", i)
	}
}

It fails with an unrelated unmarshalling error:

        	Error:      	Received unexpected error:
        	            	json: cannot unmarshal object into Go struct field Parameter.results.options.parameters.value of type string
        	Test:       	TestAccQueriesList

internal/repos_test.go Outdated Show resolved Hide resolved
service/.codegen/api.go.tmpl Show resolved Hide resolved
databricks/openapi/code/package.go Show resolved Hide resolved
service/.codegen/api.go.tmpl Show resolved Hide resolved
databricks/openapi/code/method.go Show resolved Hide resolved
for _, v := range response.Events {
results = append(results, v)
}
request.Offset += int64(len(response.Events)) // TODO: check for duplicates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This todo is very important for all pagination that isn't based on tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

no duplicates :)

service/dbfs/interface.go Show resolved Hide resolved
service/dbsql/api.go Show resolved Hide resolved
@nfx nfx changed the title Unify list return types Unify list return types for paginated methods Oct 21, 2022
@nfx nfx changed the title Unify list return types for paginated methods Consistent pagination experience across all APIs Oct 21, 2022
@nfx nfx merged commit 87bb3be into main Oct 21, 2022
@nfx nfx deleted the unify-returns branch October 21, 2022 14:11
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 this pull request may close these issues.

Add utility function to get all repos
2 participants