Azure / autorest

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

Autorest CSharp - Multiple errors in generated code #2286

Closed pratyushoak closed 7 years ago

pratyushoak commented 7 years ago

On generating client code for a Swagger doc that I am working with, and importing it into VS17, I get multiple errors. I am not able to pin down the cause of these errors to some specific config that I set. Tried it in VS15 and see the same issues. Added the Microsoft.Rest.ClientRuntime package. Attached the swagger json in question.

partnercenterfdswagger.zip

fearthecowboy commented 7 years ago

Looking into it.

fearthecowboy commented 7 years ago

Hmm. Looks like the prototype xml support is not liking something.

Lemme quickly put together a fix that can disable that for you.

fearthecowboy commented 7 years ago

@pratyushoak

Oh, and I see the other big issue, is the format you've used for your operationIds ... they are not very useful.

Typically, OpenAPI (swagger) docs use an operationId that would be appropriate as a method name.

The swagger spec states:

Unique string used to identify the operation. The id MUST be unique among all operations described in the API. Tools and libraries MAY use the operationId to uniquely identify an operation, therefore, it is recommended to follow common programming naming conventions

Specifically, AutoRest does use operationId for naming methods and grouping methods into classes.

When it encounters:

  "operationId": "Head /v{version}/domains/{domain}",

It tries to normalize that into something legal, and it's creating a method called Headvversiondomainsdomain ... really not what you want.

fearthecowboy commented 7 years ago

You can have AutoRest exclude the generation of XML serialization by creating a configuration file

FILENAME: readme.md

# Partner Center 

> see https://aka.ms/autorest

``` yaml
input-file: partnercenterfdswagger.json

directive:
  # disable xml serialization
  - from: swagger-document
    where: $..consumes
    transform: > 
      result = [];
      for( const each of $) {
          if( each != "application/xml" && each != "text/xml" ) {
              result.push(each)
          }
      }
      return result

  # disable xml serialization
  - from: swagger-document
    where: $..produces
    transform: > 
      result = [];
      for( const each of $) {
          if( each != "application/xml" && each != "text/xml" ) {
              result.push(each)
          }
      }
      return result

```

Place that file in the same folder as the .json file and run

autorest readme.md --csharp

And it should do a lot better.

After that, it still has one little error where it's using a bool? when it should be a bool -- you can fix that manually, I'm trying to track down how that's getting created.

fearthecowboy commented 7 years ago

@olydis - #2010 tries to fix support for x-nullable on return types, but I'm not sure I understand the logic with IsNullableReturnType when there are multiple return values. Can you look into this when you get back?

pratyushoak commented 7 years ago

@fearthecowboy Ah clearly the usage of operationId seems to be wrong. The config file to exclude the serialization seems to work for now, thank you. Is the naming of the operation ids causing the serialization issue?

fearthecowboy commented 7 years ago

Is the naming of the operation ids causing the serialization issue?

No, it's the use of "application/xml" -- which we're triggering off of to turn on xml serialization support (which is really just barely prototyped anyway)

If you don't need xml serialization support in your generated code (and why would you if you have json support) then I wouldn't bother.

pratyushoak commented 7 years ago

Ok makes sense. Thank-you.

Also, can I use the config file in association with other autorest cmd line params - have the input file, o/p folder, etc specified using the appropriate params( -i, -o ) but have the bit to skip xml serialization picked up from the config file? Could not find info on this at https://github.com/Azure/autorest/blob/master/docs/user/literate-file-formats/configuration.md

Ex. autorest .\readme.md -i .\partnercenterfdswagger.json -o .\CSharp --csharp

fearthecowboy commented 7 years ago

I'd avoid using the -i '-o` now. That's from the old command line, and we're going to get rid of that soon.

use --input-file=partnercenterfdswagger.json --ouptut-folder=.\CSharp instead.

If those don't change, just put those in the config file too:


# Partner Center 

> see https://aka.ms/autorest

``` yaml
input-file: partnercenterfdswagger.json
output-folder: ./CSharp
csharp:
   enabled: true
```

The directive is minimally documented ( https://github.com/Azure/autorest/blob/master/docs/user/literate-file-formats/configuration.md#directives---global-or-per-language ) at this point, it's REALLY flexible, but I haven't gotten around to documenting how it all works yet

Documentation is coming....

pratyushoak commented 7 years ago

Perfect, Thanks. Will keep this open while the issue with nullable is still pending.

olydis commented 7 years ago

@fearthecowboy @pratyushoak Semantics behind IsXNullableReturnType: If one response is nullable, then the entire return type will be nullable (it only makes sense that way around, otherwise, what would happen if one of the nullable responses returns null...). Looking at this issue and the entire Swagger, I'm unsure what the actual remaining issue is ^_^ I see no x-nullable in the Swagger - did you attempt something which didn't seem to work? Did x-nullable behave weird?

olydis commented 7 years ago

Closing due to inactivity and unclear problem. Please reopen if there are any open questions 🙂