Skip to content
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

Added genericType handling for request/response objects #71

Merged
merged 5 commits into from Mar 11, 2017

Conversation

Dirrk
Copy link
Contributor

@Dirrk Dirrk commented Mar 5, 2017

This PR implements this feature request #27.

It adds the ability to return and take in generic types.

Example:

@Route('user')
export class UserController {

    @Get('{id}')
    public async getUser(@Path() id: string): Promise<IResponse<User>> {
        return {
            success: true,
            result: new User(id, 'Foo Bar')
        };
    }
}

export interface IResponse<T> {
    success: boolean;
    result: T;
}

export class User {
    public id: string;
    public name: string;

    constructor(id, name) {
        this.id = id;
        this.name = name;
    }
}

@lukeautry
Copy link
Owner

Thanks for the pull request.

The generated routes here look good. The generated swagger spec looks a little strange - excerpt here:

"GenericModel<TestModel>": {
			"description": "",
			"properties": {
				"result": {
					"$ref": "#/definitions/TestModel"
				}
			},
			"required": [
				"result"
			],
			"type": "object"
		},
		"GenericModel<TestModel[]>": {
			"description": "",
			"properties": {
				"result": {
					"items": {
						"$ref": "#/definitions/TestModel"
					},
					"type": "array",
					"description": ""
				}
			},
			"required": [
				"result"
			],
			"type": "object"
		},

I would expect that the generated models would be flattened out; the fact that the requests or responses have generic type arguments is purely a back-end concern. Plus, the generic type argument syntax has no meaning within the swagger world, it's a TypeScript thing.

I believe the solution here would be to make use of Swagger's "allOf" composition property. See http://swagger.io/specification/ under "Composition and Inheritance (Polymorphism)".

If you're not interested in the swagger side of things (i.e. you're just using generated routes), I can look into generating the correct swagger spec.

@lukeautry lukeautry self-requested a review March 5, 2017 16:45
@Dirrk
Copy link
Contributor Author

Dirrk commented Mar 5, 2017

@lukeautry I was wondering if you had a planned solution in mind for this? I provide one below but I don't think its better than whats in the PR. When you have time could you go over it and let me know?

Controller

// UserController.ts
@Route('user')
export class UserController {

    @Get('{id}')
    public async getUser(@Path() id: string): Promise<IResponse<User>> {
        return {
            success: true,
            result: new User(id, 'Foo Bar')
        };
    }
}

export interface IResponse<T> {
    success: boolean;
    result: T;
}

export class User {
    public id: string;
    public name: string;

    constructor(id, name) {
        this.id = id;
        this.name = name;
    }
}

Swagger Definition

// swagger.yml

definitions:
  User:
    properties:
      id:
        type: string
        description: 'user id'
      name:
        type: string
        description: 'user name'
    required:
      - id
      - name
    type: object
  IResponse<User>:
    properties:
      success:
        type: boolean
        description: 'status'
      result:
        $ref: '#/definitions/User'
    required:
      - success
      - result
    type: object

Overview

In the below example (from swagger.io) on how to use allOf.

Swagger docs on it (http://swagger.io/specification/#composition-and-inheritance--polymorphism--83)

Swagger allows combining and extending model definitions using the allOf property of JSON Schema, in effect offering model composition. allOf takes in an array of object definitions that are validated independently but together compose a single object.

// Example from
definitions:
  Pet:
    type: object
    discriminator: petType
    properties:
      name:
        type: string
      petType:
        type: string
    required:
    - name
    - petType
  Cat:
    description: A representation of a cat
    allOf:
    - $ref: '#/definitions/Pet'
    - type: object
      properties:
        huntingSkill:
          type: string
          description: The measured skill for hunting
          default: lazy
          enum:
          - clueless
          - lazy
          - adventurous
          - aggressive
      required:
      - huntingSkill
  Dog:
    description: A representation of a dog
    allOf:
    - $ref: '#/definitions/Pet'
    - type: object
      properties:
        packSize:
          type: integer
          format: int32
          description: the size of the pack the dog is from
          default: 0
          minimum: 0
      required:
      - packSize

I think the above shows a great example of doing polymorphism and inheritance but the same wouldn't work well with the Users model provided above and that generics give.

My solution using AllOf was to define the object IResponse into different models to accomplish using inheritance

Below is an example of how to first break this into two objects that have all the properties except the result: User object

definitions:
  User:
    properties:
      id:
        type: string
        description: 'user id'
      name:
        type: string
        description: 'user name'
    required:
      - id
      - name
    type: object
  IResponse:
    properties:
      success:
        type: boolean
        description: 'status'
    required:
      - success
    type: object

A 3rd definition

# ...
  IResponse<User>:
    allOf:
      - $ref: '#/definitions/IResponse'
      - type: object
        properties:
          result:
            $ref: '#/definitions/User'
    required:
     - result

Personally this seems like its more complex and less readable. It becomes even harder when you add in a subgeneric type or an array. But most importantly I think having documentation like this makes api documentation easier to read.

Thanks!

@Dirrk
Copy link
Contributor Author

Dirrk commented Mar 8, 2017

@lukeautry any update on this?

@lukeautry
Copy link
Owner

@Dirrk Need to take some more time to look at it. Will try to look this week; @isman-usoh if you're interested in this may be worth your time to look as well.

@lukeautry
Copy link
Owner

@Dirrk I finally sat down and started working on this.

What if, for the swagger spec, we normalized the response name, rather than using the generic syntax? e.g. IResponse becomes IResponseUser. I'd be okay with that as a solution here - no need to bring in the complexity of allOf.

@Dirrk
Copy link
Contributor Author

Dirrk commented Mar 11, 2017

@lukeautry that sounds good to me, I will remove the brackets and push that up. Thanks for taking the time to review it!

@Dirrk
Copy link
Contributor Author

Dirrk commented Mar 11, 2017

@lukeautry Fixed the generic type string and resolved the merge conflicts. Should be ready when you get a chance. Thanks

@lukeautry
Copy link
Owner

More merge conflicts, I'm taking care of them.

@lukeautry lukeautry merged commit dd14b6f into lukeautry:master Mar 11, 2017
@Dirrk Dirrk deleted the ISSUE-27 branch March 11, 2017 18:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants