swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.89k stars 6.03k forks source link

[Swift] responses: schema: type: object --> RequestBuilder<Inline_response_200> ? #2093

Open hideya opened 8 years ago

hideya commented 8 years ago

When I feed an API with the following responses definition:

      responses:
        '200':
          description: Request was successful
          schema:
            type: object

the codegen generates something like the following client-side code:

public class func usercount(Swaggerwhere Swaggerwhere: String?) -> RequestBuilder<Inline_response_200> {

where I don't understand how to handle the returned object value...? I would expect an plain object is mapped to a dictionary so that access key-value pairs can be read... Would you kindly tell me if I'm mistaking something?

When I tried Obj-C and it also behaves in a similar manner:

-(NSNumber*) userCountWithCompletionBlock :(NSString*) where
    completionHandler: (void (^)(XXInlineResponse200* output, NSError* error))completionBlock;

So, there might be some design decision already discussed and it has been decided to generate Inline_response_200 thing for object response type. But I still don't see how to handle the returned object. Any help would be much appreciated! (@bajtos wanted to keep informed about this too)

wing328 commented 8 years ago

You define the response as "object" without specifying the definition of the "Object" (e.g. properties). Here is an example:

https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.json#L331

(there are a few PRs for Swift merged into master so would also recommend you to pull the latest)

hideya commented 8 years ago

@wing328 thanks for getting back to me this quickly! Yes, for those which has $ref: set, the generated code works like a charm :-) I was wondering how to deal with such a case where the server API returns a plain JS object. Anyhow, I'll pull the latest. Thanks again for your quick action!

wing328 commented 8 years ago

Can you elaborate a bit on the plain JS object by providing an example of the HTTP body? Is it like {} or just empty?

hideya commented 8 years ago

Sure, it is like {count: 123}. By "plain" I meant a JS object that doesn't have associated model definition (does this make sense?)

wing328 commented 8 years ago

Would Map/Dictionary meet your requirement?

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#model-with-mapdictionary-properties

hideya commented 8 years ago

No, I'm afraid not. The value part could be any type of JS value (not limited to "a simple string to string mapping").

wing328 commented 8 years ago

Given that the HTTP response body can be any JSON object, what about modelling your response as string instead so that you can do whatever you want in your program?

hideya commented 8 years ago

The thing is the server side specification cannot be changed that easily since it is not just a server but a framework (https://github.com/strongloop/loopback) which already has users. Some of the methods it provides return an object (used like dictionary to carry some key-value info) and that part cannot be changed easily. So, I'd like to find out a way to handle returned JS object on the client side... Or, in terms of Swagger Spec, is it illegal to specify responses: schema: type: object without $ref:?

bajtos commented 8 years ago

From reading Swagger spec and JSON Schema Validations, I believe it is allowed to specify responses: schema: type: object instead of using responses: schema: $ref.

I think our intent "an object with unspecified properties" would be best described using the following Swagger:

schema:
  type: object
  additionalProperties: true

The value of "additionalProperties" MUST be a boolean or an object. If it is an object, it MUST also be a valid JSON Schema. (...) Successful validation of an object instance against these three keywords depends on the value of "additionalProperties": if its value is boolean true or a schema, validation succeeds; (...)

Now I am not familiar with swagger-codegen code base at all, but perhaps Inline_response_200 should provide methods for accessing properties of the object returned by the server? (E.g. inherit from a JSONObject-like class.)

bajtos commented 8 years ago

Given that the HTTP response body can be any JSON object, what about modelling your response as string instead so that you can do whatever you want in your program?

The thing is the server side specification cannot be changed that easily since it is not just a server but a framework (https://github.com/strongloop/loopback) which already has users. Some of the methods it provides return an object (used like dictionary to carry some key-value info) and that part cannot be changed easily.

I would eventually like to improve our swagger spec generator to correctly describe the response format (instead of using a generic "object" type), see https://github.com/strongloop/loopback-swagger/issues/27. That would solve this particular issue for us. However, I thought it would be good to report the issue to this project and get it fixed here too, as other users may run into the same problem.

wing328 commented 8 years ago

@hideya for your question below

Or, in terms of Swagger Spec, is it illegal to specify responses: schema: type: object without $ref:?

I think the answer is yes.

Given that no $ref, swagger codegen will automatically create a model (e.g. Inline_response_200)

hideya commented 8 years ago

@wing328 thanks for getting bak to me! Would you have any comments on what @bajtos was describing above?

I also checked out Swagger Spec. My take is: Since the spec for Schema Object shows the following Simple Model example, what we should have done was to include "properties" field. Although I couldn't figure out weather this "properties" filed is required or not, but if we don't specify any, then codegen maps the object to some generic model (Inline_response_200 as you mentioned). I'm not sure if this is a spec'ed behaviour, but at least the current implementation is working in such a way. Am I making sense?

type: object
required:
- name
properties:
  name:
    type: string
  address:
    $ref: '#/definitions/Address'
  age:
    type: integer
    format: int32
    minimum: 0
wing328 commented 8 years ago

Yes, ideally one should define the properties for a model as the properties should be known to the spec owner.

hideya commented 8 years ago

@wing328 thanks for the confirmation. @bajtos I think this would be one way to deal with Issue https://github.com/strongloop/loopback-swagger/issues/27

bajtos commented 8 years ago

As far as I understand the spec, property definition is optional, as can be seen in Model with Map/Dictionary Properties. That leads me to conclusion that the following definition is a valid swagger:

schema:
  type: object
  additionalProperties: true

To be clear, I am not saying that swagger-codegen has to support such schemas (maps/dictionaries). As far as LoopBack is concerned, we really should fix our swagger generator to describe response properties, since we have that metadata available (strongloop/loopback-swagger#27).

hideya commented 8 years ago

I think I understood the situation, and am closing this as not an issue.

Swagger spec should have been like the following for such a case where server responds an object like { "count": 123 }:

      responses:
        '200':
          description: Request was successful
          schema:
            type: object
            required:
            - count
            properties:
              count:
                type: number

swagger-codegen automatically generates a client-side model (class) to access this particular response object (in Obj-C case, it is named like XXInlineResponse2002). Leveraging this model, we can write code like the following to obtain the count value through the property:

 [coffeeShopApi coffeeShopCountWithWhere:nil completionHandler:^(XXInlineResponse2001 *output, NSError *error) {
     NSLog(@"count: %@", output.count);
 }];

If we don't list any properties: in the spec, swagger-codegen generates a model with no properties, thus, no way to access the count value (although such schema is still fully valid). This was what was happening in our case. Swagger-codegen was working as expected.

wing328 commented 8 years ago

@hideya @bajtos My apologies for late reply on this.

For the following:

As far as I understand the spec, property definition is optional, as can be seen in Model with Map/Dictionary Properties. That leads me to conclusion that the following definition is a valid swagger:

schema:
  type: object
  additionalProperties: true

Yes, property is optional (and we've seen a lot of cases in which there's no property defined for type: object)

For additionalProperties: true, I don't think that's correct since the value of additionalProperties is essentially the type of the value for a map/dictionary. You can find more examples here

wing328 commented 8 years ago

@hideya @bajtos I've been thinking about this more. For "type: object" without any property or $ref, we should map it as generic object in the respective language.

We're working on a PR to avoid creating inline models without properties or $ref.

hideya commented 8 years ago

@wing328 yes, that is something I initially wanted! Sounds really good to me!

wing328 commented 8 years ago

@hideya when you've time, can you please test PR #2297 to see if that's what you've in your mind?

hideya commented 8 years ago

@wing328 I think I can check that out this coming weekend. Is it too late?

wing328 commented 8 years ago

Take you time. We'll of course perform testing in our side as well.

hideya commented 8 years ago

@wing328 I quickly tested the latest master, and found an issue with Swift. If an argument's schema is type: object now it maps to AnyObject and then try to call encodeToJSON() against it, whereas the implementation is not there. The following is an example. Please pay attention to the credentials parameter: YAML:

  /Users/login:
    post:
      tags:
        - User
      summary: Login a user with username/email and password.
      description: !<tag:yaml.org,2002:js/undefined> ''
      operationId: User.login
      parameters:
        - name: credentials
          in: body
          description: !<tag:yaml.org,2002:js/undefined> ''
          required: true
          schema:
            type: object
        - name: include
          in: query
          description: Related objects to include in the response. See the description of return value for more details.
          required: false
          type: string
          format: JSON
      responses:
          ...

The following generated method causes a syntax error Value of type 'AnyObject' has no member 'encodeToJSON' at credentials.encodeToJSON():

    public class func userLoginWithRequestBuilder(credentials credentials: AnyObject, include: String?) -> RequestBuilder<AnyObject> {
        let path = "/Users/login"
        let URLString = CoffeeShopClientAPI.basePath + path

        let parameters = credentials.encodeToJSON() as? [String:AnyObject]

        let requestBuilder: RequestBuilder<AnyObject>.Type = CoffeeShopClientAPI.requestBuilderFactory.getBuilder()

        return requestBuilder.init(method: "POST", URLString: URLString, parameters: parameters, isBody: false)
    }

It used to generate Credentials class like below for the case where schema is type: object and it was used instead of AnyObject (I guess the generator picked the class name from the argument name, which I think could cause a name conflict, but it at least was not breaking the build):

public class Credentials: JSONEncodable {
    public init() {}

    // MARK: JSONEncodable
    func encodeToJSON() -> AnyObject {
        var nillableDictionary = [String:AnyObject?]()
        let dictionary: [String:AnyObject] = APIHelper.rejectNil(nillableDictionary) ?? [:]
        return dictionary
    }
}

What needs to be done I think is that, instead of simply saying AnyObject, some basic model class should be generated and use it across the arguments whose type: object.

Does the above explanation make sense?

wing328 commented 8 years ago

For PHP, I need to set the type of the data to map (associative array in PHP). Please refer to https://github.com/swagger-api/swagger-codegen/pull/2316 for more information.

I think Swift may need to do something similar. I'll test C# API client later to see if similar changes need to be done.

Java and Ruby have been updated to handled type: object.

wing328 commented 8 years ago

@hideya I've submitted #2317 for C#, which returns Newtonsoft.Json.Linq.JObject for type: object.

For Swift, I think it comes down to how Alamofire handles the deserialization if I'm not mistaken.

There's a discussion related to serialization/deserialization of Object in Swift here

Please let me know if you've any questions.

wing328 commented 8 years ago

@hideya do you need any help with the change for Swift?

hideya commented 8 years ago

@wing328 yeah, I decided to change my job and having a bit of difficulty to spare time on this... sorry for not being able to be helpful...

wing328 commented 8 years ago

@hideya no problem. The community will help work on this. All the best with your new role.

tomekc commented 8 years ago

I came across InlineResponse200 when I decided to take a "shortcut" and define response right in the operation definition. I was thinking about giving more proper name for generated object, for example, based on "title" property (if provided). Do you think it may make sense?

wing328 commented 8 years ago

@tomekc I would suggest you to make your suggestion via https://github.com/OAI/OpenAPI-Specification/issues as the OpenAPI core team is working on the next version of OpenAPI/Swagger spec.

For the time being, please define the model properly to set a proper name for the model.

dmytrobr commented 8 years ago

@tomekc Regarding "title". I have opened issue to use "title", same way as Swagger UI using it.

Use "Title" as model name if provided in swagger definition. Please support

Implementation code is ready too, I'll send pull request for approval