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

[JavaScript (with promises)] Bug following $ref in object properties #4973

Closed
advance512 opened this issue Mar 8, 2017 · 25 comments
Closed

Comments

@advance512
Copy link

Description

Generated type file ID contains the following:

  /**
   * Constructs a <code>ID</code> from a plain JavaScript object, optionally creating a new instance.
   * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
   * @param {Object} data The plain JavaScript object bearing properties of interest.
   * @param {module:model/ID} obj Optional instance to populate.
   * @return {module:model/ID} The populated <code>ID</code> instance.
   */
  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();

    }
    return obj;
  }

data is an integer input. obj is null. Hence, the result is exports() instead of simply data.

Swagger-codegen version

Build package: io.swagger.codegen.languages.JavascriptClientCodegen
v2.2.2

Swagger declaration file content or url
definitions:

  # ===============================================================================
  # Fragments
  # ===============================================================================

  ID:
    description: An entity identifer
    type: integer
    format: int64
    readOnly: true

  # ===========================================================================
  # Users
  # ===========================================================================

  User:
    type: object
    required:
      - emailAddress
    properties:
      id:
        $ref: '#/definitions/ID'
      emailAddress:
        type: string
        format: email
        minLength: 6
        maxLength: 254
Command line used for generation

java -jar swagger-codegen-cli.jar generate -i ../source.yaml -l javascript --additional-properties usePromises=true -o ./javascript/

Steps to reproduce

Use the above definitions in any operation. You'll get a result object with an exports() objects instead of an ID integer.

Related issues
Suggest a Fix

It tempts me to say:

  /**
   * Constructs a <code>ID</code> from a plain JavaScript object, optionally creating a new instance.
   * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
   * @param {Object} data The plain JavaScript object bearing properties of interest.
   * @param {module:model/ID} obj Optional instance to populate.
   * @return {module:model/ID} The populated <code>ID</code> instance.
   */
  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = **data** || new exports();

    }
    return obj;
  }

but I actually think the issue is that swagger-codegen doesn't follow the $ref properly. Not certain

@wing328
Copy link
Contributor

wing328 commented Mar 8, 2017

@advance512 instead of defining id using $ref, what about defining id as integer inline instead?

@wing328 wing328 added this to the v2.2.3 milestone Mar 8, 2017
@advance512
Copy link
Author

advance512 commented Mar 8, 2017

I wanted to be able to modify the ID type easily, to set it as either a UUID or an Integer in one place for different use-cases.
Also, I liked having it as a different "type" with alternative naming. A bit like a C typedef.

I side stepped the issue for now, but it is definitely not working as expected: the OpenAPI spec says this is valid syntax.

@wing328
Copy link
Contributor

wing328 commented Mar 8, 2017

Yup, I think it's a bug with the Swagger Parser. Please report the issue to github.com/swagger-api/swagger-parser.

For the time being, please define id as integer as a workaround.

@fehguy
Copy link
Contributor

fehguy commented Mar 10, 2017

Eh, I'm committing a test in swagger-parser, but I think it's behaving correctly. Copying the definition right out of here and putting it in this test:

https://github.com/swagger-api/swagger-parser/blob/361d5e834e9dd45d657835ee89d4ef972eaf852b/modules/swagger-parser/src/test/java/io/swagger/parser/RelativeReferenceTest.java#L68

The response is exactly what I'd expect:

{
  "swagger" : "2.0",
  "info" : {
    "description" : "It works.",
    "version" : "1.0.0",
    "title" : "My API"
  },
  "basePath" : "/",
  "schemes" : [ "https" ],
  "produces" : [ "application/json" ],
  "definitions" : {
    "ID" : {
      "type" : "integer",
      "format" : "int64",
      "description" : "An entity identifer"
    },
    "User" : {
      "type" : "object",
      "required" : [ "emailAddress" ],
      "properties" : {
        "id" : {
          "$ref" : "#/definitions/ID"
        },
        "emailAddress" : {
          "type" : "string",
          "format" : "email",
          "minLength" : 6,
          "maxLength" : 254
        }
      }
    }
  }
}

What am I missing?

@advance512
Copy link
Author

Thanks a lot for the super quick response, @fehguy.

The output is indeed a valid Swagger schema (formatted as JSON). It includes the $ref external definition that was defined in the source YAML, properly.

Since I'm not familiar with how swagger-codegen constructs its output clients nor how it uses swagger-parser, I'm not sure at what point the external definition dereferencing happens, and whether it is an issue with the former or latter.

You are saying, it seems to me, that the issue is not in swagger-parser but rather in swagger-codegen?

@advance512
Copy link
Author

This is still broken, even in the latest v3.0.0.

This definition

  ID:
    description: An entity identifer
    type: integer
    format: int64
    readOnly: true

becomes this code:

/**
 *
 * OpenAPI spec version: 0.8.76
 *
 * NOTE: This class is auto generated by the swagger code generator program.
 * https://github.com/swagger-api/swagger-codegen.git
 *
 * Swagger Codegen version: 3.0.0-SNAPSHOT
 *
 * Do not edit the class manually.
 *
 */

(function(root, factory) {
  if (typeof define === 'function' && define.amd) {
    // AMD. Register as an anonymous module.
    define(['ApiClient'], factory);
  } else if (typeof module === 'object' && module.exports) {
    // CommonJS-like environments that support module.exports, like Node.
    module.exports = factory(require('../ApiClient'));
  } else {
    // Browser globals (root is window)
    if (!root.project) {
      root.project = {};
    }
    root.project.ID = factory(root.project.ApiClient);
  }
}(this, function(ApiClient) {
  'use strict';




  /**
   * The ID model module.
   * @module model/ID
   * @version 0.8.76
   */

  /**
   * Constructs a new <code>ID</code>.
   * An entity identifer
   * @alias module:model/ID
   * @class
   */
  var exports = function() {
    var _this = this;

  };

  /**
   * Constructs a <code>ID</code> from a plain JavaScript object, optionally creating a new instance.
   * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
   * @param {Object} data The plain JavaScript object bearing properties of interest.
   * @param {module:model/ID} obj Optional instance to populate.
   * @return {module:model/ID} The populated <code>ID</code> instance.
   */
  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();

    }
    return obj;
  }

  return exports;
}));

I mean, just look at this function:

exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();

    }
    return obj;
  }

An example run would have data be 1, and obj undefined. Yet it still returns obj... it makes no sense.

This was built with useES6=false, which I need to do as I am not transpiling the code.

@advance512
Copy link
Author

I still see this issue, even with v2.4.0 and v3.0.0.

Here's an example Swagger file, murmur.yml:

swagger: '2.0'
schemes:
  - http
basePath: /murmur
consumes:
  - application/json
produces:
  - application/json

paths:

  /question_sets/{questionSetUuid}/labels:

    parameters:
      - $ref: '#/parameters/questionSetUuid'

    get:
      x-swagger-router-controller: api.question_sets
      operationId: getQuestionSetLabels
      summary: Gets the labels of a QuestionSet
      description: Returns a an array of the labels of a QuestionSet based on its ID.
      responses:
        '200':
          description: A list of QuestionSetLabel objects
          schema:
            $ref: '#/definitions/QuestionSetLabelList'
        '403':
          $ref: '#/responses/Forbidden'
        '404':
          $ref: '#/responses/EntityDoesNotExist'
        '500':
          $ref: '#/responses/InternalServerError'
        default:
          $ref: '#/responses/TotallyUnexpectedResponse'


definitions:

  QuestionSetLabel:
    description: A Label that can mark a QuestionSet
    type: string
    minLength: 3
    maxLength: 100


  QuestionSetLabelList:
    type: object
    required:
      - items
    properties:
      items:
        type: array
        minItems: 0
        maxItems: 1000
        items:
          $ref: '#/definitions/QuestionSetLabel'


responses:

  InternalServerError:
    description: An unexpected error occured.
    schema:
      $ref: '#/definitions/Error'
  BadRequest:
    description: Bad request; could not perform requested operation.
  Forbidden:
    description: The server understood the request but refuses to authorize it.
  EntitySuccessfullyCreated:
    description: Entity successfully created.
  EntitySuccessfullyUpdated:
    description: QuestionSet successfully updated.
  EntitySuccessfullyDeleted:
    description: Entity successfully deleted.
  EntityDoesNotExist:
    description: Entity does not exist.
  TotallyUnexpectedResponse:
    description: A totally unexpected response

parameters:

  questionSetLabel:
    name: questionSetLabel
    in: path
    required: true
    description: A Label that can mark a QuestionSet
    type: string
    minLength: 3
    maxLength: 100

Generating a JavaScript file, for example with 3.0.0:

java -jar swagger-codegen-cli-3.0.0-20180112.231857-20.jar generate -i murmur.yaml -l javascript --additional-properties usePromises=true --additional-properties useES6=false -o ./javascript/

It generates this QuestionSetLabel.js file:

/**
 * Murmur
 * Murmur API, the goal of Murmur is to create, update and manage QuestionSets, as well as to store, update, migrate and query QuestionSet results. 
 *
 * OpenAPI spec version: 0.8.18
 *
 * NOTE: This class is auto generated by the swagger code generator program.
 * https://github.com/swagger-api/swagger-codegen.git
 *
 * Swagger Codegen version: 3.0.0-SNAPSHOT
 *
 * Do not edit the class manually.
 *
 */

(function(root, factory) {
  if (typeof define === 'function' && define.amd) {
    // AMD. Register as an anonymous module.
    define(['ApiClient'], factory);
  } else if (typeof module === 'object' && module.exports) {
    // CommonJS-like environments that support module.exports, like Node.
    module.exports = factory(require('../ApiClient'));
  } else {
    // Browser globals (root is window)
    if (!root.Murmur) {
      root.Murmur = {};
    }
    root.Murmur.QuestionSetLabel = factory(root.Murmur.ApiClient);
  }
}(this, function(ApiClient) {
  'use strict';




  /**
   * The QuestionSetLabel model module.
   * @module model/QuestionSetLabel
   * @version 0.8.18
   */

  /**
   * Constructs a new <code>QuestionSetLabel</code>.
   * A Label that can mark a QuestionSet
   * @alias module:model/QuestionSetLabel
   * @class
   */
  var exports = function() {
    var _this = this;

  };

  /**
   * Constructs a <code>QuestionSetLabel</code> from a plain JavaScript object, optionally creating a new instance.
   * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
   * @param {Object} data The plain JavaScript object bearing properties of interest.
   * @param {module:model/QuestionSetLabel} obj Optional instance to populate.
   * @return {module:model/QuestionSetLabel} The populated <code>QuestionSetLabel</code> instance.
   */
  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();
    }
    return obj;
  }

  return exports;
}));

Have a good look at the constructFromObject() function:

  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();
    }
    return obj;
  }

The data parameter is never included in the return value. And obj is always undefined. Hence - we get an exports() object instead of the actual data.

It has been nearly a year. Am I truly the only one that sees this bug?

@wing328
Copy link
Contributor

wing328 commented Feb 7, 2018

cc @CodeNinjai @frol @cliffano to see if they can help answer your questions.

@advance512
Copy link
Author

Thanks, @wing328 , I'd love to begin a conversation and potentially help solve this issue..

@CodeNinjai
Copy link
Contributor

@advance512 I will have a look into it, so we can discuss why this is happening, ok?

@advance512
Copy link
Author

Thanks, @CodeNinjai. I am available for any assistance required

@advance512
Copy link
Author

Any update, @CodeNinjai ?

@advance512
Copy link
Author

I think that PR #7991 might solve this issue.
Here's hoping.

@CodeNinjai
Copy link
Contributor

CodeNinjai commented Apr 12, 2018 via email

@advance512
Copy link
Author

@CodeNinjai
After talking to the person who implemented #7991, I think that it does indeed solve this issue.
I'd love to test this.

@advance512
Copy link
Author

@CodeNinjai
This was merged to master, we should verify it fixed it.

@advance512
Copy link
Author

When will I be able to download a binary (JAR file) that includes this fix? (Based on latest master)

@advance512
Copy link
Author

@CodeNinjai
any update?

@advance512
Copy link
Author

@wing328 @CodeNinjai

Any update on this? This has been open for nearly 1.5 years.
Can I sponsor this being fixed?

Thanks.

@advance512
Copy link
Author

This seems fixed in swagger-codegen-cli-2.4.0-20181113.142213-347.jar

@advance512
Copy link
Author

Seems that a new issue has been introduced in swagger-codegen-cli-2.4.0-20181113.142213-347.jar or before.

Code of this format:

definitions:
  LoginResponse:
    type: object
    properties:
      userId:
        type: integer
        format: int64
      userUuid:
        type: string
        format: uuid
      userIdByRef:
        $ref: '#/definitions/ID'
      userUuidByRef:
        $ref: '#/definitions/UUID'

  ID:
    description: An entity identifier
    type: integer
    format: int64
    readOnly: true

  UUID:
    description: The unique universal identifier of this entity
    type: string
    format: uuid
    readOnly: true

generates JavaScript client code like this:

/**
 * OpenAPI spec version: 0.8.222
 *
 * NOTE: This class is auto generated by the swagger code generator program.
 * https://github.com/swagger-api/swagger-codegen.git
 *
 * Swagger Codegen version: 2.4.0-SNAPSHOT
 *
 * Do not edit the class manually.
 *
 */

(function(root, factory) {

   ...
   ...
   ...

  /**
   * The LoginResponse model module.
   * @module model/LoginResponse
   * @version 0.8.222
   */

  /**
   * Constructs a new <code>LoginResponse</code>.
   * @alias module:model/LoginResponse
   * @class
   */
  var exports = function() {
    var _this = this;
  };

  /**
   * Constructs a <code>LoginResponse</code> from a plain JavaScript object, optionally creating a new instance.
   * Copies all relevant properties from <code>data</code> to <code>obj</code> if supplied or a new instance if not.
   * @param {Object} data The plain JavaScript object bearing properties of interest.
   * @param {module:model/LoginResponse} obj Optional instance to populate.
   * @return {module:model/LoginResponse} The populated <code>LoginResponse</code> instance.
   */
  exports.constructFromObject = function(data, obj) {
    if (data) {
      obj = obj || new exports();

      if (data.hasOwnProperty('userId')) {
        obj['userId'] = ApiClient.convertToType(data['userId'], 'Number');
      }
      if (data.hasOwnProperty('userUuid')) {
        obj['userUuid'] = ApiClient.convertToType(data['userUuid'], 'String');
      }
      if (data.hasOwnProperty('userIdByRef')) {
        obj['userIdByRef'] = ID.constructFromObject(data['userIdByRef']);
      }
      if (data.hasOwnProperty('userUuidByRef')) {
        obj['userUuidByRef'] = 'String'.constructFromObject(data['userUuidByRef']);
      }
    }
    return obj;
  }

  /**
   * comment comment
   * @member {Number} userId
   */
  exports.prototype['userId'] = undefined;
  /**
   * @member {String} userUuid
   */
  exports.prototype['userUuid'] = undefined;

   ...
   ...
   ...

  return exports;
}));

You will notice this line specifically is obviously broken:

   obj['userUuidByRef'] = 'String'.constructFromObject(data['userUuidByRef']);

it should be:

   obj['userUuidByRef'] = UUID.constructFromObject(data['userUuidByRef']);

Or at least:

   obj['userUuidByRef'] = ApiClient.convertToType(data['userUuidByRef'], 'String');

This seems like a regression.

@HugoMario
Copy link
Contributor

hey @advance512
i just merged some changes to get the expected output. Can you please check that and share feedback?

@HugoMario
Copy link
Contributor

closing issue, but please let me know if issue is still present and i'll work on asap.

@advance512
Copy link
Author

Hi @HugoMario,

We actually moved to cpenAPI-generator, as the swagger-codegen seemed unmaintained.

If I get some time the coming weeks, I'll try to see what swagger-codegen outputs.

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

No branches or pull requests

5 participants