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

Add required request params to metadata #114

Merged
merged 2 commits into from
May 25, 2023

Conversation

Alexey2257
Copy link

This PR adds ability to specify required request params in metadarta

@@ -11,7 +11,7 @@
request, response = extract_request_response(context)
return if request.nil?

path, summary, tags, raw_path_params, description, security = extract_request_attributes(request, example)
path, summary, tags, required_request_params, raw_path_params, description, security = extract_request_attributes(request, example)

Check notice

Code scanning / Rubocop

Checks that line length does not exceed the configured limit.

Layout/LineLength: Line is too long. [135/120]
@exoego
Copy link
Owner

exoego commented May 13, 2023

Thanks for your contribution!
With this change, required: false will be added to existing query parameters unless users specify required_request_params.
That new behavior might be a breaking change.
But I assume that new behavior is preferred since query parameters are optional in most cases, IMHO.

I requested a small change.

@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@1cc2ecc). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #114   +/-   ##
=========================================
  Coverage          ?   64.56%           
=========================================
  Files             ?       31           
  Lines             ?      649           
  Branches          ?        0           
=========================================
  Hits              ?      419           
  Misses            ?      230           
  Partials          ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Alexey2257
Copy link
Author

Thanks for your contribution! With this change, required: false will be added to existing query parameters unless users specify required_request_params. That new behavior might be a breaking change. But I assume that new behavior is preferred since query parameters are optional in most cases, IMHO.

I requested a small change.

Thanks for your review! Adding "required: false" everywhere shouldn't be breaking change because OpenApi specification suggests that this parameter is default to "false" when omitted. But if you wish to preserve previous behavior we can change code to something like this so required key wouldn't be present when it is not true

parameters << {
  name: build_parameter_name(key, value),
  in: 'query',
  schema: build_property(try_cast(value)),
  example: (try_cast(value) if example_enabled?),
}.tap do |hash|
  hash[:required] = true if record.required_request_params.include?(key)
end.compact

@exoego exoego added the enhancement New feature or request label May 25, 2023
Copy link
Owner

@exoego exoego left a comment

Choose a reason for hiding this comment

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

Sorry for late response.
LGTM 👍

@exoego exoego merged commit bf8ac1e into exoego:master May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants