x-nullable / nullable / x-ms-nullable support with $ref in response schema

[willemda]

AutoRest doesn't seem to support the following documented use-case (https://github.com/Azure/autorest/blob/main/docs/extensions/readme.md#x-nullable)

Example: An operation that returns an object or null.

"responses": {
  "200": {
    "description": "The active Widget or null.",
    "schema": {
      "x-nullable": true,
      "allOf": {
        "$ref": "#/definitions/Widget"
      }
    }
  }
}

We've tried several variations of this (using nullable, x-nullable or x-ms-nullable next to $ref and/or without the allOf) with no luck.

Looking through existing autorest.typescript issues for nullable this looks like it should have been implemented following this issue: https://github.com/Azure/autorest.typescript/issues/680

[sarangan12]

Thanks @willemda for reporting this issue. I am starting my investigation on this issue. I will update this issue shortly with my findings.

[sarangan12]

I have analyzed the issue. The generator is handling the x-nullable when it is present within an object. But, when it is mentioned on the response, it does not have any effect.

Now, @willemda I am wondering how you are expecting this to be handled? Because, the response could be null. Now, even without any further code modifications, it could be null. So, what do you actually expect in this case? Can you give an example?

@deyaaeldeen / @joheredi Do you remember any previous discussions relating to x-nullable on a response schema?

[willemda]

@sarangan12 We'd love for this GET operation to explicitely define it can return a null response so that it reflects in the return type definition (Widget | null)

The use-case is that for this GET operation a 200 or a 404 status code would share the same schema, but 404 would always be handled as null.

This would make it cleaner to handle non-existing objects when retrieving from an API (instead of handling an error we can check for null and it is clearly defined in the method contract)

Now to go a bit into the details the idea is that our API doesn't actually return null on a 404 but we use an interceptor to change the response to null (and the issue is that this doesn't return null back as it wraps the response into a {body:null})

Is this clear or would a sample swagger and expected output be better?

[willemda]

@sarangan12 If I manually update the body mapper to set the nullable property to true, then this works as expected and my null response doesn't get wrapped into a body property (this still isn't perfect but that would already be a good step forward)

export const Widget: coreClient.CompositeMapper = {
+++  nullable: true,
     type: {
     ....
}

It looks like this body mapper property is supported & used by the core-client library to determine if the response should be wrapped into a body property or not:

https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/core/core-client/src/serviceClient.ts#L185 https://github.com/Azure/azure-sdk-for-js/blob/f89f1a97ce9658f2da50e31673fad805f7f42674/sdk/core/core-client/src/utils.ts#L119 https://github.com/Azure/azure-sdk-for-js/blob/f89f1a97ce9658f2da50e31673fad805f7f42674/sdk/core/core-client/src/utils.ts#L89

[sarangan12]

@joheredi @xirzec Do you have any suggestion on the solution?

Option 1 We will have APIs such as:

getTrue(options?: BoolGetTrueOptionalParams): Promise<BoolGetTrueResponse | null> { //<---- Look at the `| null` in the return value
    return this.client.sendOperationRequest({ options }, getTrueOperationSpec);
  }

Option 2 We could just update the mapper for the return value as:

export const BoolGetTrueResponse : coreClient.CompositeMapper = {
     nullable: true,  // <----- Look at the change here
     type: {
     ....
}

[xirzec]

null is weird to me for two reasons:

1. There is a literal JSON value "null" which theoretically an endpoint could return as the JSON body of a response.
2. null is not the implicit value for a value that doesn't exist in JS.

So for an API that returns a 404 to indicate a remote object does not exist and we don't want to throw, I would expect that method to return undefined instead.

[willemda]

I didn't meant the issue to revolve around how to handle 404, this was just to expose our use-case. I think people might want different behavior there and autorest shouldn't force something on the user (The ability to return undefined in specific scenarios could also be interesting though)

The issue is more about being able to return null if we want to and this doesn't work at the moment because of that missing nullable property being set on the mapper when x-nullable is present in the schema response. Without that our null response gets wrapped and end up being { body : null }

[willemda]

@sarangan12 Would you be open for a PR for Option 2 ? If yes, any suggestion?

I'm thinking we need to somehow achieve for this nullable property to be correctly filled if x-nullable if specified: https://github.com/Azure/autorest.typescript/blob/main/src/transforms/mapperTransforms.ts#L74

For now it only takes the following options into account: https://github.com/Azure/autorest.typescript/blob/main/src/transforms/mapperTransforms.ts#L102

[sarangan12]

@willemda I think I could fix it using Option 2 for the next milestone (october). Do you have any specific time constraints on your side?

[willemda]

This is a blocker for us to start using the generated library but given this is an open source project we're not really expecting any ETA or something so October would be awesome 🤩!

If you have directions in mind I could help and make an attempt at a PR for this if you think this could speed things up and offload some work off you.

[joheredi]

@sarangan12 if we go with option 2, don't we also need to implement a change in the return type to tell that it can be null (option 1)?

[sarangan12]

Yes. Both the options need to be implemented to make the solution complete.