go-azure-sdk icon indicating copy to clipboard operation
go-azure-sdk copied to clipboard

Published 20 hours ago • verified publisher icon hashicorp

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

Open ziyeqf opened this issue 2 years ago • 7 comments

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

ziyeqf avatar May 29 '23 07:05 ziyeqf

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

tombuildsstuff avatar May 30 '23 07:05 tombuildsstuff

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?

ziyeqf avatar May 30 '23 08:05 ziyeqf

@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?

tombuildsstuff avatar May 30 '23 09:05 tombuildsstuff

@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)

ziyeqf avatar May 30 '23 10:05 ziyeqf

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?

tombuildsstuff avatar May 30 '23 11:05 tombuildsstuff

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

ziyeqf avatar Aug 14 '23 06:08 ziyeqf

Yeah agreed

tombuildsstuff avatar Aug 14 '23 07:08 tombuildsstuff