swagger-parser icon indicating copy to clipboard operation
swagger-parser copied to clipboard

VendorExtensions are not parsed properly

Open raspizdaj opened this issue 6 years ago • 7 comments

in case an object property is defined as a $ref and has vendor extensions, the extensions are ignored during parsing. The same happens for arrays.

swagger: '2.0'
info:
  description: 'Good'
  version: '2.0.0'
  title: 'Test'
paths:
  /foo:
    post:
      responses:
        200:
          description: 'OK'
definitions:
  Interval: 
    type: object
    properties:
      start:
        type: number
        x-custom: true
      finish:
        type: number
      distance_type:
        type: object
        properties:
          name:
            type: string
            x-custom: true
  Distance: 
    type: object
    properties:
      interval:
        x-custom: true //<- HERE
        $ref: "#/definitions/Interval"
  Run: 
    type: object
    properties:
      distance:
        x-custom: true
        $ref: "#/definitions/Distance"
  Competition: 
    type: object
    properties:
      runs:
         x-custom: true
         type: array
         items:
           $ref: "#/definitions/Run"


x-cusom in Run->Distance, and Distance->Interval are lost

The patch is attached.

Issue_#798_fix_parsing_vendor_extensions_in_case_of_an_object_is_defined_as_$ref_and_array.patch.txt

raspizdaj avatar Aug 11 '18 22:08 raspizdaj

Hi @raspizdaj

Can you please mention the version (1.5 or 2.0) you have used?

Thanks, Mohammed

ymohdriz avatar Aug 14 '18 05:08 ymohdriz

Hi, @raspizdaj when we use $ref, the rest of the definitions for that property will be discarded, but please take a look at the array at Competition, it's the x-custom extension also being ignored (this should not be)? Thanks!

take a look at: https://tools.ietf.org/html/draft-pbryan-zyp-json-ref-03

The "$ref" string value contains a URI [RFC3986], which identifies
   the location of the JSON value being referenced.  It is an error
   condition if the string value does not conform to URI syntax rules.
   Any members other than "$ref" in a JSON Reference object SHALL be
   ignored.

gracekarina avatar Aug 14 '18 12:08 gracekarina

Hi @gracekarina, thanks for your response. You are right, my example for the array is just wrong. I will fix it. However, the issue I want to try resolving is still there, will explain in a sec.

Any members other than "$ref" in a JSON Reference object SHALL be ignored

I did not realize the root of the issue is even in spec. I guess, this wording brings unnecessary limitation. Imaging, in my Distance object I want to put a custom processing and I have created a special annotation for that which is called for example @CustomProcessing. I can create a custom x-custom extension for swagger and enjoy. However, with the current behavior it would not be possible, because the vendor extension would be simply ignored. I marked id with // <- HERE in my example.

It looks logical to ignore all members other than $ref because they should be defined where there $ref ids pointed, but I guess it would be beneficial if the wording is changed to

Any members other than "$ref" AND ALL VENDOR EXTENSIONS ONES in a JSON Reference object SHALL be ignored.

It would be logical, because now if I define the same object inline without use oh $ref, custom processing suddenly becomes possible. This behavior does not look consistent.

Same issue is for arrays, if I want to add a custom annotation for a property that is defined as array which items are $ref, it is ignored. @ymohdriz as @gracekarina is pointed out it happens for all versions because specification says to do so

raspizdaj avatar Aug 19 '18 09:08 raspizdaj

the valid use for the vendor extension is in

Competition: 
    x-custom: true
    type: array
    items:
      $ref: "#/definitions/Run"

gracekarina avatar Aug 19 '18 21:08 gracekarina

@gracekarina, I am not sure if Competition: x-custom: true type: array items: $ref: "#/definitions/Run" is a valid use. Maybe it is, but for example if x-custom triggers adding a custom @CustomProcessing annotation, where it should be put in this case? I have changed the example in my first message to reflect the issue I have faced in my code.

Do you think if there is any chance for implementation such a behavior? Cause current implementation introduces certain limitation and behavior does not look consistent for inline and defined types and types referenced by $ref

Please let me know. Thanks a lot.

raspizdaj avatar Aug 20 '18 11:08 raspizdaj

@raspizdaj $ref can be combined with other properties if it's wrapped into allOf. Does this work in your scenario?

      interval:
        x-custom: true
        allOf:
          - $ref: "#/definitions/Interval"

hkosova avatar Oct 03 '18 22:10 hkosova

This "sorta" works in some scenarios, but in OpenAPI Generator this can cause strange behaviors where a new class type is created instead of using the existing "Interval" type. Here's the example where I am having the same issue and the behavior is inconsistent in my opinion.

    SkillRating:
      required:
        - id
      type: object
      x-java-class-annotations:
        - "@javax.persistence.MappedSuperclass"
      properties:
        id:
          description: A GUID that uniquely identifies a skill rathing.
          type: string
          format: uuid
          example: f1ad7649-eb70-4499-9c82-a63fe2c6dc71
          x-java-field-annotations:                                  # <- This is parsed and applied properly in the parsed model
            - "@javax.persistence.Id"
            - '@javax.persistence.GeneratedValue(generator = "UUID")'
            - '@org.hibernate.annotations.GenericGenerator(name = "UUID",strategy = "org.hibernate.id.UUIDGenerator")'
            - '@javax.persistence.Column(name = "id", updatable = false, nullable = false)'
        skill:
          description: A GUID that uniquely identifies a skill.
          x-java-field-annotations:                                  # <- This is ignored and not included in the parsed model
            - "@javax.persistence.ManyToOne"
            - '@javax.persistence.JoinColumn(name = "skill_id", nullable=false)'
          $ref: "#/components/schemas/Skill"
        rating:
          description: >-
            The rating -1 -> 4 of the skill. -1 - 'Not Applicable', 0 - No Experience, 1 - Foundational, 2 - Experienced, 3 - Advanced, 4 - Expert
          type: integer
          minimum: -1
          maximum: 4

Note that the id field works properly because it does not use a $ref, but the skill field fails to include the vendor extension data in the parsed model.

When I change it to use the allOf: option suggested by @hkosova above it will work in some generators, but in others it would create a new class called SkillRatingSkill which is NOT a superclass/compatible with Skill. This breaks object compatibility.

I very much feel like there should be consistency.

InfoSec812 avatar Aug 03 '22 11:08 InfoSec812