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.

Sign up for GitHub

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

Jump to bottom

fix(APIv2): RHINENG-13402 enable multidimensional sort in OpenAPI spec #2232

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix(APIv2): RHINENG-13402 enable multidimensional sort in OpenAPI spec #2232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion spec/swagger_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ def sort_params(model = nil)
end

def sort_params_v2(model = nil, except: [])
parameter name: :sort_by, in: :query, required: false,
parameter name: :sort_by, in: :query, required: false, style: :form, explode: true,
Copy link
Member

Choose a reason for hiding this comment

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

Why is the style: :form needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Following rswag documentation: https://github.com/rswag/rswag/wiki/FAQ#for-openapi-3

https://swagger.io/docs/specification/v3_0/serialization/#query-parameters

Copy link
Member

Choose a reason for hiding this comment

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

Okay, still don't understand why is this needed.

Copy link
Member Author

@romanblanco romanblanco Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

form – (default) ampersand-separated values, also known as form-style query expansion. Corresponds to the {?param_name} URI template.

I believe this is the option we want to use.

Copy link
Member

Choose a reason for hiding this comment

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

If it's default, why do we need this now? The format checks out, we only need the explode as far as I understand.

Copy link
Member

Choose a reason for hiding this comment

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

And why just here be explicit? If it's a default value, it should be consistent across all parameters in the whole specification. So either change it everywhere to style: form or please delete this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want this to be applied on those parameters of type: :array. Where else do you see this missing?

Copy link
Member

Choose a reason for hiding this comment

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

If this is default (which is), it means it is applied to everything, not just the type: :array so I am asking again, why is this case so special that you want to have an explicit setting that is already implicitly set on it? Why and how is this one single case different from all the other parameters that may or may not have a type: :array which is not connected to the formatting that you are trying to set now explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

The referenced ticket answers your question.

Copy link
Member

Choose a reason for hiding this comment

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

No it doesn't, you have 2 independent attributes that you are for some reason try to bind into a single context. One of them is default, so its setting is irrelevant and to maintain consistency across the OpenAPI spec, it makes no sense to explicitly specify in a single location.

description: 'Attribute and direction to sort the items by. ' \
'Represented by an array of fields with an optional direction ' \
'(`<key>:asc` or `<key>:desc`).<br><br>' \
Expand Down
Loading
Loading