Skip to content

Fix parameters merging - #39

Merged
k0kubun merged 2 commits into
exoego:masterfrom
blocknotes:fix-parameters-merging
Mar 18, 2022
Merged

Fix parameters merging#39
k0kubun merged 2 commits into
exoego:masterfrom
blocknotes:fix-parameters-merging

Conversation

@blocknotes

@blocknotes blocknotes commented Mar 17, 2022

Copy link
Copy Markdown

Hi :)
I think there's a typo / missing | in the parameters hash merging.

I get an error when linting the yaml output of one of my specs:

Operations must have unique `name` + `in` parameters. Repeats of `in:path` + `name:id`

The incorrect part of the YAML:

  "/api/v1/posts/{id}":
    patch:
      summary: update
      tags:
      - Post
      parameters:
      - name: id
        in: path
        required: true
        schema:
          type: integer
        example: 1
      - name: id
        in: path
        required: true
        schema:
          type: integer
        example: 0

Here it is the relevant part of a failing test:

  describe 'PATCH /api/v1/posts/:id' do
    context 'with valid data' do
      let(:post_params) { { title: 'Another title' } }

      context 'with a test post' do
        let(:test_post) { create(:post, title: 'Some title', description: 'Some description') }

        it 'updates the title' do
          expect {
            patch "/api/v1/posts/#{test_post.id}.json", params: { post: post_params }
          }.to(change { test_post.reload.title }.from('Some title').to('Another title'))
        end
      end

      context 'with a missing post' do
        before { patch "/api/v1/posts/0.json", params: { post: post_params } }

        it 'returns a not found error', :aggregate_failures do
          expect(response).not_to be_successful
          expect(response).to have_http_status(:not_found)
        end
      end
    end
  end

@blocknotes
blocknotes marked this pull request as ready for review March 17, 2022 19:32
@k0kubun

k0kubun commented Mar 18, 2022

Copy link
Copy Markdown
Collaborator

Sorry, I can't remember the full context around this code by just looking at #29 and the code. Because the code is "merging" parameters, using | alone doesn't make me think it is a typo (because | merges arrays and || deson't).

Since you have a failing test in your description, could you just add a test case that fails without your implementation change?

@blocknotes

blocknotes commented Mar 18, 2022

Copy link
Copy Markdown
Author

Since you have a failing test in your description, could you just add a test case that fails without your implementation change?

Ok, anyway merging with only the pipe operator there looks odd to me.
In the example above, in the merging point:

[1] pry(#<Object>)> base[key]
=> [{"name"=>"id", "in"=>"path", "required"=>true, "schema"=>{"type"=>"integer"}, "example"=>0}]
[2] pry(#<Object>)> value
=> [{"name"=>"id", "in"=>"path", "required"=>true, "schema"=>{"type"=>"integer"}, "example"=>1}]

We could merge considering only name and in values based on the linting message produced by @redocly/openapi-cli.

@blocknotes
blocknotes marked this pull request as draft March 18, 2022 09:42
@blocknotes

blocknotes commented Mar 18, 2022

Copy link
Copy Markdown
Author

@k0kubun: I investigated more on this issue and updated my PR.


In master branch, linting the current specs sample OpenAPI file with:
npx @redocly/openapi-cli lint spec/rails/doc/openapi.yaml

I get these errors:

No configurations were defined in extends -- using built in recommended configuration by default.

validating spec/rails/doc/openapi.yaml...
[1] spec/rails/doc/openapi.yaml:10:1 at #/servers

Servers must be a non-empty array.

 8 |   title: rspec-openapi
 9 |   version: 1.0.0
10 | servers: []
11 | paths:
12 |   "/tables":

Error was generated by the no-empty-servers rule.


[2] spec/rails/doc/openapi.yaml:179:9 at #/paths/~1tables~1{id}/get/parameters/1

Operations must have unique `name` + `in` parameters. Repeats of `in:path` + `name:id`.

177 |     type: integer
178 |   example: 1
179 | - name: id
180 |   in: path
  … |   < 3 more lines >
184 |   example: 2
185 | responses:
186 |   '200':

Error was generated by the operation-parameters-unique rule.

The first error is not relevant (it can be solved via RSpec::OpenAPI.server_urls).


While in my branch:

rm spec/rails/doc/openapi.yaml
OPENAPI=1 bundle exec rspec --order defined spec/requests/rails_spec.rb
npx @redocly/openapi-cli lint spec/rails/doc/openapi.yaml

The second error is gone.

What do you think?

@blocknotes
blocknotes marked this pull request as ready for review March 18, 2022 11:06
@blocknotes blocknotes changed the title Fix parameters hash merging Fix parameters merging Mar 18, 2022

@k0kubun k0kubun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was much easier to understand the context for me now. Thank you!

@k0kubun
k0kubun merged commit 2011297 into exoego:master Mar 18, 2022
@blocknotes
blocknotes deleted the fix-parameters-merging branch March 20, 2022 09:33
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.

2 participants