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

TypeError when parsing OAS with parameters shared between methods in path #48

Closed
klausmeyer opened this issue Jun 6, 2023 · 11 comments
Closed

Comments

@klausmeyer
Copy link
Contributor

Hi @mkon,

we're getting the following error after updating to 0.7.0:

TypeError:
  no implicit conversion of String into Integer
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/schema.rb:29:in `[]'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/schema.rb:29:in `[]'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/method.rb:5:in `initialize'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/path.rb:6:in `new'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/path.rb:6:in `block in initialize'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/schema.rb:29:in `to_h'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/schema.rb:29:in `to_h'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc/path.rb:5:in `initialize'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:20:in `new'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:20:in `block in initialize'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:19:in `to_h'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:19:in `initialize'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:12:in `new'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/openapi_contracts-0.7.0/lib/openapi_contracts/doc.rb:12:in `parse'
# ./spec/support/helpers/openapi_helper.rb:3:in `oas'
# ./spec/controllers/api/v1/configuration/languages_controller_spec.rb:36:in `block (3 levels) in <top (required)>'
# /Users/kmeyer/.rvm/gems/ruby-3.1.3@service-dbs-configuration/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

I did a bit of debugging and figured out being related to the case when having a parameters: block shared between the defined methods of a certain path.

Here a way to reproduce it via change in the fixtures:

diff --git spec/fixtures/openapi/paths/message.yaml spec/fixtures/openapi/paths/message.yaml
index 6961280..e122d26 100644
--- spec/fixtures/openapi/paths/message.yaml
+++ spec/fixtures/openapi/paths/message.yaml
@@ -1,16 +1,16 @@
+parameters:
+- name: id
+  in: path
+  description: Id of the message.
+  required: true
+  schema:
+    type: string
 get:
   tags:
     - Message
   summary: Get Message
   description: Get Message
   operationId: get_message
-  parameters:
-  - name: id
-    in: path
-    description: Id of the message.
-    required: true
-    schema:
-      type: string
   responses:

The issue seems to be mainly the parameters key not being filtered out here (or in the logic before):

@methods = @schema.to_h do |method, _|

Let me know if I should try to provide a fix or if you want to do it on your own.

Thanks, Klaus

@mkon
Copy link
Owner

mkon commented Jun 6, 2023

Hey @klausmeyer, how are you doing? Thanks for discovering this bug, I forgot that there is path-wide parameter definitions so not everything is a http method. Please feel free to submit a PR, ideally with a example schema that is breaking. I will merge & push out a new version ASAP.

@klausmeyer
Copy link
Contributor Author

Hi Konstantin,

thanks for the fast reply. I'm doing good so far and you?

I'll try to find some time next week to work on it.

When playing around with the code yesterday I also found another possible problem but there I'm not sure yet if it was not introduced by my tinkering.

Perhaps we can then also check quickly before shipping a new version just for the one problem.

@klausmeyer
Copy link
Contributor Author

I've now created a PR with a potential fix.

When trying it inside one of my affected applications I run into another error but I assume it might be separate one:

Failure/Error: expect(response).to match_openapi_doc(oas, path: '/languages').with_http_status(:ok)
  
  KeyError:
    key not found: "configuration-v1.openapi"
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:649:in `fetch'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:649:in `block in pointer_uri'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/hana-1.3.7/lib/hana.rb:19:in `each'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/hana-1.3.7/lib/hana.rb:19:in `each'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:643:in `reduce'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:643:in `pointer_uri'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:312:in `validate_ref'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:122:in `validate_instance'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:502:in `block in validate_array'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:495:in `each'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:495:in `each_with_index'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:495:in `validate_array'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:298:in `validate_type'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:211:in `validate_instance'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:314:in `validate_ref'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/json_schemer-0.2.25/lib/json_schemer/schema/base.rb:122:in `validate_instance'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/body.rb:19:in `each'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/body.rb:19:in `validate_schema'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/body.rb:11:in `validate'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/base.rb:15:in `call'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/base.rb:20:in `call'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/validators/base.rb:20:in `call'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/match.rb:14:in `valid?'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/bundler/gems/openapi_contracts-eb7fa58bdb51/lib/openapi_contracts/rspec.rb:10:in `block (2 levels) in <top (required)>'
  # ./spec/controllers/api/v1/configuration/languages_controller_spec.rb:36:in `block (3 levels) in <top (required)>'
  # /Users/kmeyer/.rvm/gems/ruby-3.1.4@service-dbs-configuration/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in `block (2 levels) in <top (required)>'

Note that we're using that feature of having different name for the OAS file:

def oas
  @oas ||= OpenapiContracts::Doc.parse(Rails.root.join('spec/fixtures/oas'), 'configuration-v1.openapi.yaml')
end

@dveljacic
Copy link
Contributor

Hi @klausmeyer,

yes, I believe as well it is the separate one and I may have found a cause.

In this line https://github.com/mkon/openapi_contracts/blob/main/lib/openapi_contracts/doc/file_parser.rb#L60 file name is added here #/<filename>/components/schemas/SomeSchema, which is a problem if schemas are not in a different files (that's why it didn't fail in gem's test suite), and that's why key not found error popped up.

I have attempted a simple fix by modifying the code to {key => "#/#{pointer}"}, which seems to resolve the issue on my application. However, this fix also breaks some tests here, indicating that it may not be a sufficient solution. I'll try to find a proper resolution and submit a different PR.

@mkon
Copy link
Owner

mkon commented Jun 14, 2023

Hey, thanks for reporting this. Would it be possible for you to submit a demo file that causes the issue? It seems to be a bug in how this generates the compound document before passing it to json_schemer. I recently changed something there that allows pointing into other yaml files with a combination of relative file path and json pointer.

@dveljacic
Copy link
Contributor

dveljacic commented Jun 14, 2023

Sure thing.

The OAS file below contains all existing endpoints compounded into one file (including the ones Klaus added, but they are commented out because they cause already described error).

Now, this small test already detects the problem:

#/spec/openapi_contracts/match_spec.rb

context 'when schema pointers are within file' do
  let(:doc) { OpenapiContracts::Doc.parse(FIXTURES_PATH.join('openapi'), 'example.openapi.yaml') }

  it { is_expected.to be_valid }
end
#/spec/fixtures/openapi/example.openapi.yaml

openapi: 3.0.0
info:
  version: 1.0.0
  title: Example.com
  termsOfService: 'https://example.com/terms/'
  contact:
    email: contact@example.com
    url: 'http://example.com/contact'
  license:
    name: Apache 2.0
    url: 'http://www.apache.org/licenses/LICENSE-2.0.html'
  x-logo:
    url: 'https://redocly.github.io/openapi-template/logo.png'
  description: >
    This is an **example** API to demonstrate features of OpenAPI specification
tags:
  - name: Auth
    description: Authentication
  - name: Comments
    description: Comments
  - name: Messages
    description: Messages
  - name: User
    description: Operations about user
servers:
  - url: '//api.host.example'
paths:
  # /comments/{id}: 
  #   parameters:
  #   - name: id
  #     in: path
  #     description: ID of the comment.
  #     required: true
  #     schema:
  #       type: string
  #       example: '112547'
  #   get:
  #     tags:
  #       - Comments
  #     summary: Get Comment
  #     description: Get comment details.
  #     operationId: getComment
  #     responses:
  #       '200':
  #         description: OK
  #   patch:
  #     tags:
  #       - Comments
  #     summary: Update Comment
  #     description: Patch Comment
  #     operationId: patchComment
  #     responses:
  #       '204':
  #         description: OK
  /health:
    get:
      operationId: healthCheck
      summary: Health Check
      responses:
        '200':
          description: OK
        '400':
          description: Bad Request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/BadRequest'
        '500':
          description: Server Error
  /messages/{id}:
    get:
      tags:
        - Messages
      summary: Get Message
      description: Get message details
      operationId: getMessage
      parameters:
      - name: id
        in: path
        description: ID of the message.
        required: true
        schema:
          type: string
          example: '193628'
      responses:
        '200':
          description: OK
          headers:
            x-request-id:
              schema:
                type: string
              required: true
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Message'
        '400':
          description: Bad Request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/BadRequest'
  /user:
    get:
      tags:
        - User
      summary: Get all Users
      description: This endpoint returns all users.
      operationId: getUser
      responses:
        '200':
          description: OK
          headers:
            x-request-id:
              schema:
                type: string
              required: true
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/User'
        '400':
          description: Bad Request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/BadRequest'
    post:
      tags:
        - User
      summary: Create User
      description: This endpoint creates a new user.
      operationId: createUser
      requestBody:
        required: true
        content:
          application/json:
            schema: 
              type: object
              properties:
                type:
                  type: string
                attributes:
                  type: object
                  properties:
                    name:
                      type: string
                      nullable: true
                    email:
                      type: string
                      example: someone@host.example
                  additionalProperties: false
                  required:
                    - name
                    - email
              additionalProperties: false
              required:
                - type
                - attributes
      responses:
        '201':
          description: Created
          headers:
            x-request-id:
              schema:
                type: string
              required: true
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/User'
        '400':
          description: Bad Request
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/BadRequest'
components:
  schemas:
    BadRequest:
      type: object
      properties:
        errors:
          type: array
          items:
            type: object
    Message:
      type: object
      properties:
        data:
          type: object
          properties:
            id:
              type: string
              example: acd3751
            type:
              type: string
            attributes:
              type: object
              properties:
                body:
                  type: string
              additionalProperties: false
              required:
                - body
          additionalProperties: false
          required:
            - id
            - type
            - attributes
      required:
        - data
      additionalProperties: false
    User: 
      type: object
      properties:
        data:
          type: object
          properties:
            id:
              type: string
              example: acd3751
              readOnly: true
            type:
              type: string
            attributes:
              type: object
              properties:
                name:
                  type: string
                  nullable: true
                addresses:
                  type: array
                  items:
                    oneOf:
                      - type: object
                        properties:
                          street:
                            type: string
                          city:
                            type: string
                email:
                  type: string
                  example: someone@host.example
              additionalProperties: false
              required:
                - name
                - email
          additionalProperties: false
          required:
            - id
            - type
            - attributes

      required:
        - data
      additionalProperties: false

P.S. I believe it makes sense to have one kind of file like this with multiple different endpoints and acceptable differences so tests can recognize problems in the future and make the gem more robust to different valid OAS files. Maybe the existing auth.openapi.yaml file could just be replaced with the one below.

@mkon
Copy link
Owner

mkon commented Jun 14, 2023

Ok I see now, thanks @dveljacic. I think I know the bug, it is not related to custom root file but I am not handling correctly JSON pointers in the root file. I always handle it like it is a extending file where the file name has to become part of the pointer.

Eg if the root file is named example.yaml it will generate pointers like "$ref": "#/example/components/schemas/BadRequest" just like in the Numbers example https://github.com/mkon/openapi_contracts/blob/main/spec/openapi_contracts/doc/file_parser_spec.rb#L22 where the file name correctly becomes a part of the pointer in the compound file. The same would actually happen with openapi.yaml.

You can see the compound file by adding a debugger in the end of the Doc::Parser#parse

@mkon
Copy link
Owner

mkon commented Jun 14, 2023

I will try to fix it asap.

@mkon
Copy link
Owner

mkon commented Jun 15, 2023

@klausmeyer can you try the branch rootfile-pointers in your app to see if it resolves the problem?

@klausmeyer
Copy link
Contributor Author

Of course. Here the results:

  • main branch: KeyError
  • rootfile-pointers branch: All good 😀

@mkon
Copy link
Owner

mkon commented Jun 16, 2023

Version 0.7.1 contains the fix.

@mkon mkon closed this as completed Jun 16, 2023
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

No branches or pull requests

3 participants