New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

How to express an "any" type for a property. #2855

Closed
Stevenic opened this Issue Feb 21, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@Stevenic
Copy link

Stevenic commented Feb 21, 2018

I'm trying to work out how to express a property that's an "any" type using swagger and autorest. What I found through searching the web is that in swagger 2.0 you need to simply remove the "type" for a property to say its an any which if you look at my value property you'll see I've done:

"CardAction": {
      "description": "A clickable action",
      "type": "object",
      "properties": {
        "type": {
          "$ref": "#/definitions/ActionTypes",
          "description": "The type of action implemented by this button"
        },
        "title": {
          "description": "Text description which appears on the button",
          "type": "string"
        },
        "image": {
          "description": "Image URL which will appear on the button, next to text label",
          "type": "string"
        },
        "text": {
          "description": "Text for this action",
          "type": "string"
        },
        "displayText": {
          "description": "(Optional) text to display in the chat feed if the button is clicked",
          "type": "string"
        },
        "value": {
          "description": "Supplementary parameter for action. Content of this property depends on the ActionType"
        }
      }
    }

But when I run this through autorest using the --typescript switch get a mapping to type object in the generated mapper.ts file which causes the generated client to fail the outbound serialization for non-object values:

export const CardAction = {
  required: false,
  serializedName: 'CardAction',
  type: {
    name: 'Composite',
    className: 'CardAction',
    modelProperties: {
      type: {
        required: false,
        serializedName: 'type',
        type: {
          name: 'String'
        }
      },
      title: {
        required: false,
        serializedName: 'title',
        type: {
          name: 'String'
        }
      },
      image: {
        required: false,
        serializedName: 'image',
        type: {
          name: 'String'
        }
      },
      text: {
        required: false,
        serializedName: 'text',
        type: {
          name: 'String'
        }
      },
      displayText: {
        required: false,
        serializedName: 'displayText',
        type: {
          name: 'String'
        }
      },
      value: {
        required: false,
        serializedName: 'value',
        type: {
          name: 'Object'
        }
      }
    }
  }
};

Is this just a bug in the typescript plugin?

@Stevenic

This comment has been minimized.

Copy link
Author

Stevenic commented Feb 21, 2018

@amarzavery I dug into the code for ms-rest-js and it doesn't look like this is currently even possible. You assume there's a mapper.type.name and then have a limited set of types which you'll serialize with any not being one of them. Given that you don't support union types via oneOf/anyOf either, there's currently no way for us to accurately model our API using the current bits. :(

I'd have to implement a custom serializer and then doctor the generated autorest code after each run which also sucks...

@olydis

This comment has been minimized.

Copy link
Contributor

olydis commented Feb 21, 2018

@Stevenic you're on to likely AutoRest's biggest weirdness! #2827

Indeed, we unfortunately had the wrong behavior in the past, and as detailed in said issue, it's not too trivial to fix: As always, if you have weird behavior, people (including us) start depending on it. Not sure what the timeline is to fix up our own Swaggers (to not rely on wrong behavior anymore), but right after that is done, we'll fix up code generation.

Until then, maybe @amarzavery has some way to unblock you (autorest can actually include minor string-smashing-style doctoring into it's codegeneration, maybe we can brew something together)

@Stevenic

This comment has been minimized.

Copy link
Author

Stevenic commented Feb 21, 2018

I had read that issue so I had already assumed this was likely a core issues but was hopeful I could easily work around it without needing to do anything crazy. It doesn't look like it from my 30 minute dig through your code.

The issue we have is we have a value property that is sometimes a string and other times an object and it depends on the value of a type property as to which it is. This API is part of the Microsoft Bot Framework and is therefore implemented by a dozen partners from Skype to Office so it's not like we can change the API.

@olydis

This comment has been minimized.

Copy link
Contributor

olydis commented Feb 21, 2018

Thanks for your digging efforts, I'm unfortunately in the dark about NodeJS code gen but what you say makes sense - I think I remember similar cases, the mapper is quite rigid. I'll check what @amarzavery's plans regarding any are, after all he's gonna have to generate something for it once we push forward the issue 😉

@olydis

This comment has been minimized.

Copy link
Contributor

olydis commented Feb 23, 2018

@Stevenic The legendary @amarzavery has spoken: He'll update ms-rest-js (should be tagged here soon) to allow you to write

      value: {
        required: false,
        serializedName: 'value',
        type: {
          name: 'Any'    // <======= new thing here
        }
      }

Long term, this will be what AutoRest generates for your Swagger, but well, you know about the issue tracking that 😉 Until then, I'll happily assist you to hack something into your generation command that'll replace Object with Any for that property, in case manually changing that is not feasible (e.g. if you expect to regen very frequently, which would always override your fix...)

@amarzavery

This comment has been minimized.

Copy link
Contributor

amarzavery commented Feb 23, 2018

@Stevenic - 0.2.6 version of ms-rest-js with support for [de]serializing "any" type has been published.

@amarzavery amarzavery reopened this Feb 23, 2018

@Stevenic

This comment has been minimized.

Copy link
Author

Stevenic commented Feb 23, 2018

Awesome! Thanks @olydis. We already have a post processing script to do other similar fixups so I should be good. Thanks again for the quick turn around :)

@Stevenic Stevenic closed this Feb 23, 2018

@amarzavery

This comment has been minimized.

Copy link
Contributor

amarzavery commented Feb 23, 2018

@Stevenic - What are the other fixups that you have to do on the generated client? I am curious to know. It would be nice to get you to a position where no post generation scripts would be required.

@Stevenic

This comment has been minimized.

Copy link
Author

Stevenic commented Feb 23, 2018

@amarzavery String enums don't behave the same way in TypeScript as they do in .NET. TypeScript will generate a String enum well enough but then it forces the developer to always use the enum. So lets say you end up generating this:

export enum PetTypes {
     Cat = "cat",
     Dog = "dog",
     Fish = "fish"
}

export interface Pet {
     type: PetTypes;
     name: string;
     age: number;
}

If a developer tries to do this:

const pet: Pet = {
     type: 'cat',
     name: 'fluffy',
     age: 7
};

They will get a compiler error. TypeScript forces you to do:

const pet: Pet = {
     type: PetTypes.Cat,
     name: 'fluffy',
     age: 7
};

While its fine that you can use the enum it sucks as a JavaScript/TypeScript developer to have to use the enum. So we patch every property that takes a string enum to use a union type:

export interface Pet {
     type: PetTypes|string;
     name: string;
     age: number;
}

Apparently C# will accept either without the patch so just another language inconsistency. It's also worth noting that the any patch isn't needed for C# because in C# strings will apparently auto cast to object.

We have some other patches around converting things to take Partial<type> in some places but those are more idiosyncrasies with our protocol. The issue is we have a fairly complicated protocol we use Swagger to model and it's bi-directional so we will actually send the consumer instances of types we've defined in Swagger to a webhook their service exposes. We then want those same types sent back to us only the rules for which properties are valid are different then whats valid when we send you an instance of the type. And then we do a bunch of other stuff to complicate that even more... :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment