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

External definition files with arbitrary format not supported #303

Closed
DerkSchooltink opened this issue Feb 23, 2021 · 9 comments · Fixed by #307
Closed

External definition files with arbitrary format not supported #303

DerkSchooltink opened this issue Feb 23, 2021 · 9 comments · Fixed by #307

Comments

@DerkSchooltink
Copy link

Loading an OpenAPI definition file will only attempt to load any external references in schemas that are defined according to the standard OpenAPI definition layout. More specifically, your references must be wrapped in an components object so this tool can inline them into the definition.

This is an assumption on layout, which is not strictly enforced by OpenAPI. For example, I am using this tool with a structure like this:

image

Where the request.json consists of the following:

{
  "type": "object",
  "required": [
    "definition_reference"
  ],
  "properties": {
    "definition_reference": {
      "$ref": "definitions.json#/definitions/External"
    }
  }
}

And definitions.json of the following:

{
  "definitions": {
    "External": {
      "type": "string"
    }
  }
}

Loading the openapi.json will fail with the following message:

failed to resolve "#/definitions/External" in fragment in URI: "External": struct field "External" not found

I think this tool should support arbitrary schema references and not rely on users to always implement it by keeping the references in OpenAPI component objects.

A small side-note, I have attempted to take a shot at fixing this, but I am struggling with overseeing the impact of changes I make in other flows because I am not familiar with the entire codebase. 😅 I would be able to submit a PR if someone could give me some guidance as of where this should be changed (probably somewhere around swagger_loader.go and how to perhaps set this up in a way that it doesn't impact the old implementation of loading references.

I created a branch on my fork which contains a unit test that describes the issue: commit

@fenollp
Copy link
Collaborator

fenollp commented Feb 23, 2021

Hi!
Indeed you're looking in the right place. In drillIntoSwaggerField, try:

	case reflect.Struct:
		numField := val.NumField()
		for i := 0; i < numField; i++ {
			field := val.Type().Field(i)
			tagValue := field.Tag.Get("yaml")
			yamlKey := strings.Split(tagValue, ",")[0]
			if yamlKey == fieldName {
				return val.Field(i).Interface(), nil
			}
		}
		if numField > 0 {
			if ff := val.Type().Field(0); ff.PkgPath == "" && ff.Name == "ExtensionProps" {
				extensions := val.Field(0).Interface().(ExtensionProps).Extensions
				if enc, ok := extensions[fieldName]; ok {
					var dec interface{}
					if err := json.Unmarshal(enc.(json.RawMessage), &dec); err != nil {
						return nil, err
					}
					return dec, nil
				}
			}
		}

this should help a bunch :)

@Integralist
Copy link

I might have stumbled across this myself. Only just started using this library today to do some testing so I'm not wedded to it, but sounds like supporting arbitrary formats would be useful 🙂

@fenollp
Copy link
Collaborator

fenollp commented Feb 25, 2021

@DerkSchooltink @Integralist please take a look at the linked PR. It passes this PR's test code & I'd love to see if it works for you.

@Integralist
Copy link

@fenollp sadly this didn't fix my issue 😞

error resolving reference "version.yaml#/components/schemas/model_version": failed to resolve "timestamp" in fragment in URI: "#/timestamp": struct field "timestamp" not found

I'm quite unfamiliar with OpenAPI so maybe if I explain the set-up I have (as succinctly as I can) then maybe someone here could help point me to what might be wrong (either in this library or in the schemas I have)?

If I look at the schema files I have I can see 'timestamp' is definitely defined.

Here's a break-down of the schema files I've been given to work with (I'm only pasting in the relevant sections of the files as the files themselves are quite long otherwise)...

service.yaml

NOTE: this file is what I'm providing to openapi3.LoadSwaggerFromFile(./path/to/service.yaml).

paths:
  /service:
    get:
      tags:
        - services/service
      summary: List services
      description: List services.
      operationId: list-services
      responses:
        "200":
          description: OK
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: "#/components/schemas/model_service"
              examples:
                body:
                  value:
                    - $ref: "#/components/examples/model_service_example/value"
      parameters:
        - $ref: "common/query.yaml#/page"
        - $ref: "common/query.yaml#/per_page"
        - $ref: "common/query.yaml#/sort_v3"
        - $ref: "common/query.yaml#/direction"

I'm working through this in reverse order, i.e. I'm moving backwards from the actual error being reported and trying to find where it's sourced. So in this case service.yaml is the actual starting point, and in the file is a $ref pointing to /components/schemas/model_service. Later in this same service.yaml file I can find it is definitely defined under 'components'...

components:
  schemas:
    model_service:
      allOf:
        - $ref: "common/properties.yaml#/timestamps"
        - properties:
            comment:
              $ref: "common/properties.yaml#/comment"
            customer_id:
              $ref: "customer.yaml#/components/schemas/id_customer"
            name:
              $ref: "#/components/schemas/name_service"
            paused:
              type: boolean
              description: Whether the service is paused. Services are paused due to a lack of traffic for an extended period of time. Services are resumed either when a draft version is activated or a locked version is cloned and reactivated.
            type:
              type: string
              enum:
                - vcl
                - wasm
              description: The type of this service.
            publish_key:
              type: string
              description: Unused at this time.
            id:
              $ref: "#/components/schemas/id_service"
            version:
              type: integer
              description: Current [version](/reference/api/services/version/) of the service.
            versions:
              type: array
              items:
                $ref: "version.yaml#/components/schemas/model_version"
              description: A list of [versions](/reference/api/services/version/) associated with the service.

Interestingly, the first item under allOf: is $ref: "common/properties.yaml#/timestamps" and the error doesn't suggest a problem finding the timestamp there, but I would expect that to be fine as it is defined in the file that reference points to (see the 'common/properties.yaml' contents further below).

But the error message itself is suggesting the version.yaml file is loaded and then having trouble finding the 'timestamps' reference. I can see it's referenced in the service.yaml snippet above ($ref: "version.yaml#/components/schemas/model_version") so let's look at that file...

version.yaml

components:
  schemas:
    model_version:
      allOf:
        - $ref: "common/properties.yaml#/timestamps"
        - properties:
            service_id:
              $ref: "service.yaml#/components/schemas/id_service"
            active:
              type: boolean
              description: Whether this is the active version or not.
              default: false
            comment:
              $ref: "common/properties.yaml#/comment"
            deployed:
              type: boolean
              description: Unused at this time.
            locked:
              type: boolean
              description: Whether this version is locked or not. Objects can not be added or edited on locked versions.
              default: false
            number:
              type: integer
              readOnly: true
              description: The number of this version.
              example: 1
            staging:
              type: boolean
              description: Unused at this time.
              default: false
            testing:
              type: boolean
              description: Unused at this time.
              default: false

So in the snippet shown above we can see there is a reference to 'timestamps' as we would hope there would be ($ref: "common/properties.yaml#/timestamps") and it's from that point I presume the code attempts to parse the 'common/properties.yaml' file...

common/properties.yaml

Again, I've cut out everything except what I believe to be relevant to show...

timestamp:
  type: string
  # pattern: ^(?:[1-9]\d{3}-(?:(?:0[1-9]|1[0-2])-(?:0[1-9]|1\d|2[0-8])|(?:0[13-9]|1[0-2])-(?:29|30)|(?:0[13578]|1[02])-31)|(?:[1-9]\d(?:0[48]|[2468][048]|[13579][26])|(?:[2468][048]|[13579][26])00)-02-29)T(?:[01]\d|2[0-3]):[0-5]\d:[0-5]\d(?:Z|[+-][01]\d:[0-5]\d)$
  description: Date and time in ISO 8601 format.
  example: "2020-04-09T18:14:30Z"
  readOnly: true
  nullable: true

timestamps:
  type: object
  properties:
    created_at:
      $ref: "#/timestamp"
    deleted_at:
      $ref: "#/timestamp"
    updated_at:
      $ref: "#/timestamp"

We can see timestamps is defined and they reference timestamp which is also defined.

So at this point it looks like the schemas are correct/valid in the sense that what is being described as 'not found' by the library code is in fact incorrect as timestamp is definitely defined.

@DerkSchooltink
Copy link
Author

@DerkSchooltink @Integralist please take a look at the linked PR. It passes this PR's test code & I'd love to see if it works for you.

It fixed my issue, thank you for the effort!

@Integralist
Copy link

@fenollp re: my comments above (#303 (comment)) would it be better for me to open a separate issue?

@fenollp
Copy link
Collaborator

fenollp commented Mar 2, 2021

@Integralist sure feel free to open a dedicated issue.
I have a more minimal bug reproducing draft PR but no bug fix yet.

fenollp added a commit to fenollp/kin-openapi that referenced this issue Mar 2, 2021
Signed-off-by: Pierre Fenoll <[email protected]>
fenollp added a commit to fenollp/kin-openapi that referenced this issue Mar 19, 2021
Signed-off-by: Pierre Fenoll <[email protected]>
fenollp added a commit to fenollp/kin-openapi that referenced this issue Apr 23, 2021
Signed-off-by: Pierre Fenoll <[email protected]>
@fenollp
Copy link
Collaborator

fenollp commented Apr 23, 2021

@Integralist all good now. Your test passes #314 :)

@Integralist
Copy link

Amazing. Thank you for the effort and hard work getting this done.

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 a pull request may close this issue.

3 participants