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

Odata: enhance nextLink unmarshal #927

Closed
wants to merge 1 commit into from

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Mar 14, 2024

try to resolves hashicorp/terraform-provider-azurerm#24948

Seems the error is because code-gen does not handle x-ms-pageable.nextLinkName

And the nextLink is used in

func (c *Client) ExecutePaged(ctx context.Context, req *Request) (*Response, error) {
// Perform the request
resp, err := c.Execute(ctx, req)
if err != nil {
return resp, err
}
// Check for json content before handling pagination
contentType := strings.ToLower(resp.Header.Get("Content-Type"))
if !strings.HasPrefix(contentType, "application/json") {
return resp, fmt.Errorf("unsupported content-type %q received, only application/json is supported for paged results", contentType)
}
// Unmarshal the response
firstOdata, err := odata.FromResponse(resp.Response)
if err != nil {
return resp, err
}
if firstOdata == nil {
// No results, return early
return resp, nil
}
// Get results from this page
firstValue, ok := firstOdata.Value.([]interface{})
if !ok || firstValue == nil {
// No more results on this page
return resp, nil
}
// Get a Link for the next results page
var nextLink *odata.Link
if req.Pager == nil {
nextLink = firstOdata.NextLink
} else {
nextLink, err = odata.NextLinkFromCustomPager(resp.Response, req.Pager)
if err != nil {
return resp, err
}
}
if nextLink == nil {

This PR simply hardcode the nextLink to handle the case pageable List API returns next link through nextLink field instead of standard @odata.nextLink field.

A more robust solution is to make use of x-ms-pageable.nextLinkName for pageable List API at the generator-go-sdk , but it might break existing code struct.

@teowa teowa requested a review from a team as a code owner March 14, 2024 06:28
@github-actions github-actions bot added the release-once-merged The SDK should be released once this PR is merged label Mar 14, 2024
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @teowa

Thanks for this PR.

I've taken a look through and left a comment inline, and whilst this approach will work for the default values for ARM, the nextLink value can come from one of a number of fields depending on the API in question - as such I believe it'd be better to fix this in hashicorp/pandora's generator-go-sdk tool, which has access to the name of the field containing the paginated value instead?

Thanks!

@@ -131,5 +131,17 @@ func (o *OData) UnmarshalJSON(data []byte) error {
}
}

for _, k := range []string{"nextLink", "@odata.nextlink"} {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a number of possible values for this field depending on the part of the API in question, so these two fields aren't necessarily sufficient.

Instead the SDK supports defining a custom pager which, given that Pandora has the JSONName for the field containing the pagination details, would allow us to output a CustomPager (like so) which is then configured on the client.RequestObject struct and ultimately used when the SDK method definition is output.

We're currently using this information to build out the AutoRest methods that use polling so once the CustomPoller is output, that should be all that's needed.

As such, rather than doing this here - can we update generator-go-sdk to output a CustomPoller as needed here instead, which'll fix this regardless of the field type?

@teowa
Copy link
Contributor Author

teowa commented Mar 15, 2024

Thanks @tombuildsstuff for reviewing this and providing helpful detailed advice, I have submitted hashicorp/pandora#3958 to solve the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-once-merged The SDK should be released once this PR is merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hashicorp/go-azure-sdk refactoring for azurerm_subscriptions - pagination and modelling issue
2 participants