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

Allow path param and request body to be passed at the same time #256

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jalyna
Copy link

@jalyna jalyna commented Mar 18, 2020

Right now it is not possible in open api 3 to set a path param and a request body.

Not sure though if this is the right way but it worked for the spec we are using.

Copy link
Member

@ota42y ota42y left a comment

Choose a reason for hiding this comment

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

The problem you pointed out is correct and this needs to be fixed.
However, this change greatly disrupts backward compatibility.

Currently, we are use 4 data (path, query, form and request body).
The problem occurs because we combine all of this into a one variable specified by params_key.
I think you can avoid them by using different keys
So we need add path_params_key, query_params_key, form_params_key, and request_params_key to set values.

@kurko
Copy link

kurko commented May 26, 2020

@ota42y can you be more specific? Why would that break older versions?

I have an interest in this PR and would like to help move it forward.

@ota42y
Copy link
Member

ota42y commented Jun 2, 2020

The committee support convert datetime string to datetime object.

If we don't use committee with this code (Ruby on Rails),
we'll get ArgumentError because params[:registered_at] is string and Time.now is Time class.
But the committee convert params[:registered_at] to DateTime, so this code is valid.

class UsersController < ApplicationController
    skip_before_action :verify_authenticity_token

    def create
        binding.pry
        registered_at = params[:registered_at]
        raise "error" unless registered_at < Time.now
        user = ["debug user1"] # do something

        head :ok
    end
end

When we use this feature on rails, we need this config to set converted value to ActionController::Parameters.

    schema_path = Rails.root.join("config/openapi.yaml").to_s
    config.middleware.use Committee::Middleware::RequestValidation, 
      schema_path: schema_path,
      params_key: 'action_dispatch.request.request_parameters'

But this change break this code because this code pass nested hast to ActionController::Parameters.

  • old version
    • {"registerd_at": DateTime Object}
  • this version
    • {"body": {"registered_at": DateTime Object}}

https://gist.github.com/ota42y/0870100c03444d39d78107a23de9f171

@hedselu
Copy link

hedselu commented Apr 7, 2021

Any update on this issue?

Nick Campbell and others added 3 commits January 10, 2022 10:48
Warning:
  DEPRECATION WARNING: Rails 7.1 will return Content-Type header without
  modification. If you want just the MIME type, please use #media_type
  instead. (called from request_media_type at
  /home/circleci/repo/vendor/bundle/ruby/3.0.0/bundler/gems/committee-2a1e9e507867/lib/committee/schema_validator.rb:7))

Since we don't need to strip any encoding etc from the string, we can
just use this value unchanged.
@makdad
Copy link
Contributor

makdad commented Jan 29, 2023

@ota42y I believe with 5.0 released, this issue is "fixable" in the sense that backward compatibility is less of an issue for the hash keys. Could you confirm? This is a pretty big issue for Open API Spec compatibility IMO.

I'd even be happy to jump in and help on the PR if necessary.

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.

6 participants