`Migrate` @ `2020-01-01` - support for nonstandard next link

hashicorp / go-azure-sdk

An opinionated Go SDK for Azure Resource Manager

Mozilla Public License 2.0

33 stars 42 forks source link

`Migrate` @ `2020-01-01` - support for nonstandard next link #492

Open ziyeqf opened 1 year ago

ziyeqf commented 1 year ago

Is there an existing issue for this?

[X] I have searched the existing issues

Community Note

Please vote on this issue by adding a :thumbsup: reaction to the original issue to help the community and maintainers prioritize this request
Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
If you are interested in working on this issue or have submitted a pull request, please leave a comment

Service Used

Migrate

API Versions Used

2020-01-01

Description

The response is paged with nextLink not in OData and sdk ignored it.

References

Swagger: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/migrate/resource-manager/Microsoft.OffAzure/stable/2020-01-01/migrate.json#L3685

tombuildsstuff commented 1 year ago

@ziyeqf that's a Swagger bug which would need to be fixed - would you mind sending a PR to fix that?

ziyeqf commented 1 year ago

thanks @tombuildsstuff, but I may not get the point. If I submit a PR to rename nextLink to @odata.nextlink, the service will still return in the original format?

tombuildsstuff commented 1 year ago

@ziyeqf the Swagger should be defining x-ms-pageable to state which field contains the nextLink, see here: https://github.com/Azure/autorest/tree/main/docs/extensions#x-ms-pageable

As such just updating the API definition should be sufficient there, I don't think we need a Pandora change for this, I'm kinda surprised there isn't a linter to catch a field containing nextLink without a x-ms-pageable defined actually?

ziyeqf commented 1 year ago

@tombuildsstuff I think it has been defined with x-ms-pageable( swagger ) but the new base layer only support @odata.nextlink as a next link property? (code)

I'm not familiar with the new base layer... But just found it's calling ExecutePaged which only accepts @odata.nextlink as the next link property. (sdk code)

tombuildsstuff commented 1 year ago

Got it, thanks - in which case yeah this is a bug in the new base layer here where we're relying on the OData value rather than using the value we're expecting:

https://github.com/hashicorp/go-azure-sdk/blob/af92233a172a86b8bddc32d55ed22979979f29df/sdk/client/client.go#LL466C1-L483C1

@manicminer mind taking a look into that one?

ziyeqf commented 1 year ago

Hey @tombuildsstuff, I assume the issue should be resolved when the importer/generator has been updated. Maybe we should keep this issue open, WDYT?

tombuildsstuff commented 1 year ago

Yeah agreed