Azure / autorest

OpenAPI (f.k.a Swagger) Specification code generator. Supports C#, PowerShell, Go, Java, Node.js, TypeScript, Python
MIT License
4.59k stars 731 forks source link

Should header `retry-after` use type `string` or `integer`? #4988

Open mikeharder opened 2 months ago

mikeharder commented 2 months ago

Schema for both autorest v2 and v3 requires header retry-after in examples to be type string instead of int32:

https://github.com/Azure/autorest/blob/3d50bb1315ea6a45012ac5c0a1dc8492cf896cdb/schema/example-schema.json#L77-L81

https://github.com/Azure/autorest/blob/fa08f8cc5047c03320be8928d7f62d9235238f0a/packages/libs/autorest-schemas/example-schema.json#L81-L85

This seems to align with the HTTP spec:

The Retry-After field value can be either an HTTP-date or a number of seconds to delay after receiving the response.

Retry-After = HTTP-date / delay-seconds

A delay-seconds value is a non-negative decimal integer, representing time in seconds.

delay-seconds  = 1*DIGIT

Two examples of its use are

Retry-After: Fri, 31 Dec 1999 23:59:59 GMT
Retry-After: 120

In the latter example, the delay is 2 minutes.

https://www.rfc-editor.org/rfc/rfc9110#field.retry-after

However, existing examples (and at least some specs) in azure-rest-api-specs use a mix of string and int32:

https://github.com/search?q=repo%3AAzure%2Fazure-rest-api-specs+retry-after+language%3AJSON&type=code&l=JSON

Should the schema be updated to allow both string and int32 (if possible)? Or is it by design that all examples should use string, so we should ensure the example generator does this?

Example spec with issue:

"retry-after": {
  "description": "This header will only be present when the operation status is a non-terminal status. It indicates the minimum amount of time in seconds to wait before polling for operation status again.",
  "type": "integer",
  "format": "int32"
}

https://github.com/Azure/azure-rest-api-specs/blob/ab064e0047ec560a700d6b501097d99471ad817b/specification/communication/data-plane/Email/preview/2023-01-15-preview/CommunicationServicesEmail.json#L136-L140

https://github.com/Azure/azure-rest-api-specs/pull/29699

Teams Discussion: https://teams.microsoft.com/l/message/19:0351f5f9404446e4b4fd4eaf2c27448d@thread.skype/1720141321497?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=3e17dcb0-4257-4a30-b843-77f47f1d4121&parentMessageId=1720141321497&teamName=Azure%20SDK&channelName=API%20Spec%20Review&createdTime=1720141321497

@timotheeguerin?

mikekistler commented 1 month ago

We always want the value of retry-after to be "delay-seconds" and not a date, and RFC 7231 says:

A delay-seconds value is a non-negative decimal integer, representing time in seconds.

So integer is the appropriate type for this header value.

Furthermore, it is common that string and integer values are serialized in headers in the same way, so it might be possible to change the API description from type: string to type: integer in a non-breaking way (i.e. without changing the behavior of the service). As long as the service does not wrap the value in quotes, e.g.

Retry-after: "180"

but most services will not do this so it might be fine.

Bottom line: Our tooling should allow Retry-After to be defined as either type: integer (preferred) or type: string.