Redis.CheckNameAvailability pattern is not implemented correctly #2981
Comments
|
@hovsepm That actually sounds like what I expected the generated code to be. Is there a specific guideline about this? just spent about 10 minutes looking and I couldn't find it. |
|
@hovsepm We can change the response payload to match the one you expect without incrementing api version, and without breaking anyone as long as a) they are just checking the 200 response code, and b) we never return available=false, but instead continue return a 409 error status code. But, if in next REST api version we change the response payload to the recommended available = false , I believe that would actually result a breaking behavioral change in the generated SDK for applications which were expecting an exception to be thrown, but now see a 'false' returned from the function, which their app will not be testing for. Is it acceptable to have these breaking changes, in what people are probably expecting to be a mature management library by now? Is it e.g. acceptable IF we get it documented? What is the story for documenting such breaking changes? |
|
@hovsepm this seems to be complete, please verify and resolve this issue if so |
|
@salameer How is it completed? We didn't yet change anything for check name availability on redis swagger side AFAICR, due to the above concern about breaking behavioral change. |
|
Sorry @TimLovellSmith :) I though I saw some changes related to the issue in your PR. Please ping me directly when you will address this issue. |
|
@hovsepm But I might not ping you. Currently we don't have a plan to address the issue [i.e. update swagger], because our perception is that even though it could make the API more consistent, it would become a soft-breaking return type change in the client library that we think wouldn't actually be appreciated by people upgrading their client library. |
|
Recap; we still aren't planning to fix this. The reason was we feel it doesn't meet the bar to deliberately make what would be a SDK 'breaking' change (for existing apps upgrading the library) in exception vs error behavior, just to be more consistent with other name validation apis (for other resources). Shall we close this? |
|
Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @yegu-ms. Issue DetailsCurrent latest spec describes return type of an operation In the generated code the method will be as
|
|
@msftbot I still think thats hard for us to change without it being an unexpected breaking change to SDK behavior. |
…s.. (#2981) * adding hybridmetadata proxy resource * udpates * updates * modeling identity same as the default * updates * updates
Current latest spec describes return type of an operation
checkNameAvailabilityas void.https://github.com/Azure/azure-rest-api-specs/blob/master/specification/redis/resource-manager/Microsoft.Cache/stable/2018-03-01/redis.json#L91
In the generated code the method will be as
void checkNameAvailability(parameters)and if it will not throw an exception (C#/Java) then the name is available.The text was updated successfully, but these errors were encountered: