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

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

Conversation

romanblanco
Copy link
Member

Secure Coding Practices Checklist GitHub Link

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

@romanblanco romanblanco force-pushed the RHINENG-13402-sort-by-entry-missing branch from cb09a04 to c8cd0ef Compare October 3, 2024 12:20
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.83%. Comparing base (8a9fdda) to head (c08616c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2232   +/-   ##
=======================================
  Coverage   98.83%   98.83%           
=======================================
  Files         232      232           
  Lines        5082     5082           
=======================================
  Hits         5023     5023           
  Misses         59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@romanblanco romanblanco force-pushed the RHINENG-13402-sort-by-entry-missing branch 5 times, most recently from c30ebcb to 852b0df Compare October 4, 2024 17:20
@@ -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.

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

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.

@romanblanco romanblanco force-pushed the RHINENG-13402-sort-by-entry-missing branch from 852b0df to 3160480 Compare October 7, 2024 08:22
@romanblanco romanblanco marked this pull request as ready for review October 7, 2024 08:27
@romanblanco romanblanco requested a review from a team as a code owner October 7, 2024 08:27
@romanblanco
Copy link
Member Author

/retest

@romanblanco romanblanco force-pushed the RHINENG-13402-sort-by-entry-missing branch from 3160480 to dc7f3e5 Compare October 7, 2024 08:42
@romanblanco romanblanco closed this Oct 7, 2024
@romanblanco romanblanco force-pushed the RHINENG-13402-sort-by-entry-missing branch from dc7f3e5 to 8a9fdda Compare October 7, 2024 08:55
@romanblanco romanblanco reopened this Oct 7, 2024
@romanblanco romanblanco marked this pull request as draft October 8, 2024 08:02
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