api-guidelines icon indicating copy to clipboard operation
api-guidelines copied to clipboard

Prescribed error messages for versioning errors?

Open annelo-msft opened this issue 3 years ago • 20 comments

For client development, it is preferred that clients don't do validation on requests and instead let the service validate and return any errors. The justification for this is that the client then doesn't prevent forward-compatibility if a service makes changes in a later version that weren't allowed in an earlier version.

One practical issue that this raises is that service error messages don't always tell the client user 1) what went wrong and 2) how to fix it. This is a particular problem for backward-compatible clients, when a service returns an error message that doesn't indicate that an operation/property wasn't supported in a given api-version.

There is an argument that could be made in favor of client-side validation in the case of version errors (i.e. a client could prevent a request to an endpoint that didn't exist in an early client version, which would not likely have forward-compatibility implications). However, this would be unnecessary if the Azure API Guidelines clearly prescribed the text of error messages services returned in the case of version-mismatches.

For example, if api-version=v1 query parameter was sent to an endpoint not introduced until api-version=v2, if the service message said something like "api-version=v1 not supported by this operation; operation was added in api-version=v2", this would indicate to the client user that they couldn't call this operation from a v2 client targeting api-version=v1 without the client needing to throw an exception to prevent it. It is less clear whether this would be desired for model property additions, as it would mean going back and changing api-version=v1 error messages when new properties were added in later api-versions. E.g. an error "invalid JSON" from a v1 service would later become "property Baz not supported on model Foo in api-version=v1; property was added in api-version=v2". This would prevent the need for client-side validation in this case.

Is this something the API Board would consider?

annelo-msft avatar Aug 30 '22 18:08 annelo-msft

Yes, in fact this came up in a meeting I had just earlier today! Do you have an initial list of potential error messages we could standardize?

JeffreyRichter avatar Aug 30 '22 18:08 JeffreyRichter

I like the format suggested above:

  • "Operation/property not supported in api-version=x; operation/property was added in api-version=y."

It feels like this would work for any addition? Including extensible enum values, polymorphic type additions, other?

I can propose this in our next DPG design meeting to get further feedback on it.

annelo-msft avatar Aug 30 '22 18:08 annelo-msft

I think we need multiple errors and while I like referring to a new api-version I think service teams will push back on the complexity of implementing it. So, how about:

  • "api-version=x does not support this operation."
  • "api-version=x doesn't support properties: bar, foo, baz."

JeffreyRichter avatar Aug 30 '22 18:08 JeffreyRichter

If that's the best we can do, it's a lot better than not doing it!

I would ask, though, for the property additions, can we ensure that a service doesn't say "not supported in api-version x" for a property unless it was added in a later api-version? While true for any unsupported property, I think the information is not helpful to the caller unless the property is supported in some later version.

annelo-msft avatar Aug 30 '22 18:08 annelo-msft

I don't think the service team wants to keep track of what version a property is added in. So, by saying that api-version=x doesn't support it might encourage a customer to see what api-version does support it. Afterall, the customer is passing the property for a reason so they must think that the service is supposed to accept it.

JeffreyRichter avatar Aug 30 '22 19:08 JeffreyRichter

I guess I'm saying, if I accidentally add a property to my request body that would never be supported in any of the service, e.g. "XJDKFJWERYYY=3", would the service tell me "api-version=x doesn't support property: XJDKFJWERYYY" ? And is that more helpful than "invalid JSON" ? Or are we assuming that if a caller is using a client, only reasonable properties that correspond to some version would be passed ever?

annelo-msft avatar Aug 30 '22 20:08 annelo-msft

I would be a bit cautious about forcing the service to add too much information to the error message. 400 responses should be fast/easy to compute for the service, and there very much should not be a need to modify existing deployments if a new API Version is deployed (e.g. to change the error message from "not supported" to "not supported in API Version X" or "not supported until version X"). Please note that if the primary use case for the error message is for a client calling an older API version using a property only available in a newer API Version, then we are in an unusual/advanced scenario to begin with.

johanste avatar Aug 30 '22 20:08 johanste

If it's too much to ask services to do, I expect we can do post-facto validation on the client to side-step the perf hit/forward-compat concern of request validation and still produce a good error message to client users. This would rely on services following the guidance to return 400 for an invalid query parameter (unsupported api-version) or when a JSON field name is not understood.

annelo-msft avatar Aug 30 '22 20:08 annelo-msft

The SDK won't know which property/quer parameter was bad unless you parse the response body error message which the SDK should not do because we explicitly state that this is NOT part of the API contract.

JeffreyRichter avatar Aug 30 '22 21:08 JeffreyRichter

If we got a 400 back, we could check the outbound request and see if we had set a value that wasn't supported in the api-version we sent with the request. In generated code, we would know which api-version each property was added in from the Cadl.

We could feasibly set this on the request in the Send method - we would know then if something wasn't versioned correctly - but that takes the perf hit of analysis up front.

annelo-msft avatar Aug 30 '22 21:08 annelo-msft

I would not do this myself, but sure.

JeffreyRichter avatar Aug 30 '22 21:08 JeffreyRichter

It would be best if the service could return an error that is helpful to the end-user.

annelo-msft avatar Aug 30 '22 21:08 annelo-msft

Yes, I think it is incumbent upon our team to drive these kinds of changes into the service so that the improvements benefit non-SDK customers too.

JeffreyRichter avatar Aug 30 '22 21:08 JeffreyRichter

  • Operation/property not supported in api-version=x; operation/property was added in api-version=y.

But isn't that necessary for other discussions like ignoring and not returning models/properties for API versions that don't support them anyway?

Also, a bit of a nit: could we use something a little more normal user friendly? Maybe drop the = such that it's just "Operation/property not supported in API version x; operation/property was added in API version y."? It takes nothing away from the message but reads a little better.

In internal discussions about this for TA, @tg-msft also brought up a good point we may want to consider for exposing this error on the client: we should mention the client/model and method/property name that the dev called. We could do this only in cases when we get a specific 400 error back, as being discussed in #312. It's more actionable.

heaths avatar Aug 31 '22 23:08 heaths

The service code for any given api-version will know what properties it supports and any property it doesn't know about is considered unsupported - it won't know if another api-version supports it. So, it can't add the api-version when it was added.

Plus, imagine V2 adds the Foo property and V3 takes it away again. A customer using V1 passes Foo; now what should the service message return?

I'm happy simplifying the message to what you propose: Operation/property not supported in API version YYYY-MM-DD

JeffreyRichter avatar Sep 01 '22 21:09 JeffreyRichter

@KrzysztofCwalina raised today that the service should validate the entire schema and the warning should identify all unsupported properties in the text, so that customers don't need to make subsequent calls to discover that they've made multiple mistakes.

annelo-msft avatar Sep 01 '22 22:09 annelo-msft

Yes, I completely agree.

JeffreyRichter avatar Sep 02 '22 17:09 JeffreyRichter

Well, a common mistake is that people send the wrong model. Pity the fool that would send a NIC model to a create VM endpoint. You are almost going to need a paged error response.

johanste avatar Sep 02 '22 17:09 johanste

OK then, how about the error message includes the first 10 or 20 then?

JeffreyRichter avatar Sep 02 '22 17:09 JeffreyRichter

Also, you really want to keep the error path for services efficient. The more processing we force the service to do, the more we open things up for what is effectively DOS of the service by sending incorrect data.

johanste avatar Sep 02 '22 17:09 johanste