Skip to content

codegen.go: support seeding pager with initial nextLink #1502

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

stevekuznetsov
Copy link

If a user wishes to resume paging from a particular link, they may pass a parameter to the list operation method on their client. This next link will be used for the first paging operation.

@stevekuznetsov
Copy link
Author

Not yet sure what's causing the diffs like...

-func (client *PagerWidgetsClient) newListMethodPager(options *pagerWidgetsClientlistMethodOptions) *runtime.Pager[pagerWidgetsClientlistMethodResponse] {
+func (client *PagerWidgetsClient) newListMethodPager(options *pagerWidgetsClientlistMethodOptions) (*runtime.Pager[pagerWidgetsClientlistMethodResponse]) {

need to work through the generators, I get very inscrutable error messages locally on rush tspcompile:

stdout: TypeSpec compiler v0.65.2

Diagnostics were reported during compilation:

warning @azure-tools/typespec-client-generator-core/example-loading: Skipping example loading from /home/stevekuznetsov/code/Azure/autorest.go/src/github.com/Azure/autorest.go/packages/typespec-go/node_modules/@azure-tools/azure-http-specs/specs/azure/payload/pageable/examples because there was an error reading the directory.

Found 1 warning.

@jhendrixMSFT
Copy link
Member

When making changes to the emitted code, you need to "rebuild the world" so the changes are reflected in the test suite.

  • rush rebuild
  • rush regenerate
  • rush tspcompile

This will rebuild all emitter sources, regenerate the tests for autorest.go, and regenerate the tests for typespec-go.

@stevekuznetsov
Copy link
Author

Thanks @jhendrixMSFT - I was missing the rush regenerate step. Mind if I add a CONTRIBUTING.md to this repo to outline the basics? It was fun and all to reverse engineer the CI on previous PRs to see what to do, but the next person to onboard would probably appreciate a doc if the steps aren't soon to change.

@stevekuznetsov
Copy link
Author

stevekuznetsov commented Feb 25, 2025

@jhendrixMSFT quick question for you - after running rush regenerate && rush tspcompile I get a lot of diffs under packages/autorest.go/test/autorest/ that would normally get removed by go fmt -w - I know that the tspcompile.js script runs the formatter, and I've seen it re-format code correctly before, so I am unclear why I'm not getting formatting. The rush tspcompile runs to completion with no errors.

To fix it, I ran the following, but figured it was some mistake in how I was re-generating everything that it was not done automatically?

for dir in $( find -type f -name go.mod | xargs dirname | xargs realpath ); do pushd $dir; go fmt ./...; popd; done

@@ -430,8 +430,10 @@ func (client *ContainerRegistryClient) NewGetManifestsPager(name string, options
Fetcher: func(ctx context.Context, page *ContainerRegistryClientGetManifestsResponse) (ContainerRegistryClientGetManifestsResponse, error) {
ctx = context.WithValue(ctx, runtime.CtxAPINameKey{}, "ContainerRegistryClient.NewGetManifestsPager")
nextLink := ""
if page != nil {
if page != nil && page.Link != nil {
Copy link
Author

Choose a reason for hiding this comment

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

n.b. if everything is a pointer, it's best to check that we're not about to dereference a nil. Not really related to this PR, but good to have anyway.

If a user wishes to resume paging from a particular link, they may pass
a parameter to the list operation method on their client. This next link
will be used for the first paging operation.

Signed-off-by: Steve Kuznetsov <[email protected]>
Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov
Copy link
Author

./zz_container_client.go:959:40: options.NextLink undefined (type *ContainerClientListBlobFlatSegmentOptions has no field or method NextLink)

Looks like I'm not adding the parameter to all paging options, need to re-grok that code.

For some reason, some of these packages are not passing their options to
the `fooCreateRequest()` as a group, so we're generating incorrect
parameters in `getCreateRequestParametersSig` and propagating that up.

Signed-off-by: Steve Kuznetsov <[email protected]>
@@ -16,7 +16,8 @@ type CustomParameterGroup struct {

// PagingClientAppendAPIVersionOptions contains the optional parameters for the PagingClient.NewAppendAPIVersionPager method.
type PagingClientAppendAPIVersionOptions struct {
// placeholder for future optional parameters
// Resumes the paging operation from the provided link.
NextLink string
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to ensure that this value corresponds to the client.method. And since we're supposed to treat next links as opaque values, I don't think we can simply parse the provided value to ensure that the host/path matches (while host should always match, it's possible that path will not).

We have the same constraints with Poller[T]. The way we solved it there is we have method Poller[T].ResumeToken() which essentially dumps the data structure to JSON. So, when you create a new Poller[T] from the resume token, if it doesn't match an error is returned.

While a "next link token" (like above) would solve this, it does mean that you can't just take an arbitrary next link value. You'd have to create the token first from the Pager[T] (mock-up below). How important do you think it is that we accept a "naked" next link?

pager := client.NewFooPager(nil)
tk := pager.ResumeToken()
// other code...
pager = client.NewFooPager(&NewFooPagerOptions{
    ResumeToken: tk,
})

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this poses a problem though.

pager := client.NewFooPager(nil)
tk := pager.ResumeToken()
pager = client.NewBarPager(&NewBarPagerOptions{
    ResumeToken: tk,
})

Creating a Pager[Bar] from a Pager[Foo] token should return an error. However, our New*Pager methods don't return an error. :(

Copy link
Author

Choose a reason for hiding this comment

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

All of this was why I was asking in the e-mail thread about asking folks to update their public-facing typespec to actually encode which query parameter is being used for pagination record-keeping. We also shouldn't allow the domain to change, or the apiVersion, etc. Not sure we can do anything about this if all we have to work with is an opaque value.

Regarding a .ResumeToken() getter - if we keep with the opaque value, we can't parse it to determine anything, so the best we could do is to ensure that the type (e.g. a KeyVault Secret) is the same by saving some tagged union {"tag":"KeyVault.Secret","nextLink":"..."}. I would imagine that this is the least likely mistake anyone would make when reading such a token from durable storage on process start, when resuming from some prior list.

The big downside I see there is that NextLink is already exported from the page types, so it would not be clear to users of the SDK what the difference is and why to use one over the other, or, if all you could use were the typed/encoded one, why the "raw" NextLink is exported.

@microsoft-github-policy-service microsoft-github-policy-service bot added the no-recent-activity There has been no recent activity on this issue. label May 2, 2025
@stevekuznetsov
Copy link
Author

/reopen

@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-recent-activity There has been no recent activity on this issue. label May 12, 2025
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.

2 participants