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

Build deeply nested parameters #29

Merged
merged 2 commits into from
Jun 17, 2021

Conversation

bf4
Copy link
Contributor

@bf4 bf4 commented Jun 16, 2021

Build deeply nested parameters

e.g. with query params { "filter" => { "name" => "Benjamin" } }

  • now: build_parameter_name("filter", { "name" => "Benjamin" }) #=> filter[name]

  • before: "filter".to_s #=> filter

  • failing test building out multiple parameters. (I'm not yet sure why it didn't rebuild to the yaml or test it)

xref OAI/OpenAPI-Specification#1706 https://github.com/ahx/openapi_first/pull/75/files

and test building out multiple parameters
@bf4 bf4 force-pushed the build_deeply_nested_parameters branch from 51e61da to 37277f7 Compare June 16, 2021 15:39
@k0kubun
Copy link
Collaborator

k0kubun commented Jun 16, 2021

failing test building out multiple parameters. (I'm not yet sure why it didn't rebuild to the yaml or test it)

Yeah, I'm curious about why the test is passing without any changes too.

@bf4
Copy link
Contributor Author

bf4 commented Jun 16, 2021

Since neither rspec nor rake ran all the specs, I ran OPENAPI=1 bundle exec rspec $(git ls-files | grep '_spec\.rb' | xargs) and it also generated a pretty big change to the roda spec and had one failure.

@bf4
Copy link
Contributor Author

bf4 commented Jun 16, 2021

I didn't really dig into how the merging works, but I confirmed on my local that it's still creating only one parameters element even though the parameter name is now correct.

(❤️ for this project. I also liked the https://github.com/r7kamura/autodoc quite a bit. 🙌 )

@bf4
Copy link
Contributor Author

bf4 commented Jun 16, 2021

I can also write a unit test in the meantime if you'd like. I tried to follow the pattern of the repo.

@k0kubun
Copy link
Collaborator

k0kubun commented Jun 16, 2021

(❤️ for this project. I also liked the https://github.com/r7kamura/autodoc quite a bit. 🙌 )

🤝

end

it 'with deep query parameters' do
get '/tables', params: { filter: { "name" => "Example Table" } }, headers: { authorization: 'k0kubun' }
Copy link
Collaborator

@k0kubun k0kubun Jun 16, 2021

Choose a reason for hiding this comment

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

It's just a guess, but maybe the test is passing ignoring this because the above test generates parameters, and running this probably doesn't override it. So copying the same endpoint to somewhere else and calling it from this spec might fix the test issue.

Copy link
Contributor Author

@bf4 bf4 Jun 16, 2021

Choose a reason for hiding this comment

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

True enough, though I'd like to be able to generate a spec like

openapi: 3.0.3
info:
  title: rspec-openapi
  version: 1.0.0
paths:
  "/tables":
    get:
      summary: index
      tags:
      - Table
      parameters:
      - name: page
        in: query
        schema:
          type: integer
        example: 1
      - name: per
        in: query
        schema:
          type: integer
        example: 10
      - name: "filter[name]"
        in: query
        schema:
          type: string
        example: Example Table
      - name: "filter[price]"
        in: query
        schema:
          type: integer
        example: 0
      responses:
        '200':
          description: returns a list of tables
          content:
            application/json:
              schema:
                anyOf:
                  - type: array
                  items:
                    type: object
                    # etc

it's been a while since I studied the oas spec, so perhaps I'm misunderstanding it...

https://swagger.io/docs/specification/describing-responses/

Can I have different responses based on a request parameter? Such as:
GET /something -> {200, schema_1}
GET /something?foo=bar -> {200, schema_2}

In OpenAPI 3.0, you can use `oneOf` to specify alternate schemas for the response and document possible dependencies verbally in the response description. However, there is no way to link specific schemas to certain parameter combinations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(If I remove the 'with flat query parameters' spec and rm spec/rails/doc/openapi.yaml and run OPENAPI=1 bundle exec rspec spec/requests/rails_spec.rb it generates a new spec but still with only one param. )

Copy link
Collaborator

@k0kubun k0kubun Jun 16, 2021

Choose a reason for hiding this comment

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

I agree. That's something we should fix as well. But if you're not trying to address that, maybe we should separate that for now and possibly merge them in a PR that addresses the problem.

@@ -71,6 +71,16 @@ def build_parameters(record)
parameters
end

def build_parameter_name(key, value)
key = key.to_s
if value.is_a?(Hash) && (value_keys = value.keys).size == 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

1️⃣ build { "filter" => { "name" => "Benjamin" } } as filter[name] instead of filter

elsif base_key_value.is_a?(Array) && value.is_a?(Array)
if key == "parameters"
# merge arrays
base[key] |= value
Copy link
Contributor Author

Choose a reason for hiding this comment

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

2️⃣ allow multiple parameter members to be built in the same request document

required: true
schema:
type: integer
example: 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bug when parameters are otherwise the same but have different examples

Copy link
Collaborator

@k0kubun k0kubun left a comment

Choose a reason for hiding this comment

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

Thanks!

@k0kubun k0kubun merged commit 8bf8798 into exoego:master Jun 17, 2021
@bf4 bf4 deleted the build_deeply_nested_parameters branch June 18, 2021 03:26
@k0kubun k0kubun mentioned this pull request Mar 18, 2022
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