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

[Bugfix] Allow the "null" JSON string to be used as a parameter in Apipie spec examples (show_in_doc) #641

Conversation

enrique-guillen
Copy link
Contributor

@enrique-guillen enrique-guillen commented Oct 25, 2018

Ensure that Extractor::Recorder always rewinds rack.input because it is always read

This allows the parameters of a rack request to be the null JSON string, "null", when running apipie-example-updating commands:

dude@machine:~$ APIPIE_RECORD=examples bundle exec rspec spec/requests

There are times when parse_data(...) can return nil (specifically, when evaluating JSON.parse("null"), the response is nil).

The current error

This fixes the following types of specs:

let(:params) { nil }
delete :something, params.to_json

# Equivalent to:
delete :something, "null"

# Both valid JSONs.

When running these specs, Apipie will analyze the env:

def analyse_env(env)
  # ...
  if data = parse_data(env["rack.input"].read) # JSON.parse("null") #=> nil
    # ...
    env["rack.input"].rewind
  elsif form_hash = env["rack.request.form_hash"]
    # ...
  end
  # env["rack.input"] not re-wound.
end

So far nothing bad happens, but then, to actually read the specs, action_dispatch/http/request will try to read the params:

    77: def parse_formatted_parameters(parsers)
    78:   return yield if content_length.zero? || content_mime_type.nil?
    79: 
    80:   strategy = parsers.fetch(content_mime_type.symbol) { return yield }
    81: 
    82:   begin
 => 83:     strategy.call(raw_post)
    84:   rescue # JSON or Ruby code block errors
    85:     my_logger = logger || ActiveSupport::Logger.new($stderr)
    86:     my_logger.debug "Error occurred while parsing request parameters.\nContents:\n\n#{raw_post}"
    87: 
    88:     raise ParamsParser::ParseError
    89:   end
    90: end

def raw_post
  unless has_header? "RAW_POST_DATA"
    raw_post_body = body
    set_header("RAW_POST_DATA", raw_post_body.read(content_length)) # Read will equal `nil` because stream starts at the wrong place, because the env key for rack.input was not rewound.
    raw_post_body.rewind if raw_post_body.respond_to?(:rewind)
  end
  get_header "RAW_POST_DATA"
end

# https://github.com/rails/rails/blob/49f9dff9b6ba1451d8c85927d5f75327bd2322d9/activesupport/lib/active_support/json/decoding.rb#L23

def decode(json) # Should be `"null"`, but the non-rewound value is `nil`.
  data = ::JSON.parse(json, quirks_mode: true) # ::JSON.parse(nil)
  # Exception will be thrown.
end
# On OJ
rb_eTypeError, "Nil is not a valid JSON source."

# On the default JSON lib
TypeError: no implicit conversion of nil into String

The fix

Note that if you run the specs without apipie, you get the correct behavior. This fix restores that:

def analyse_env(env)
  # ...

  rack_input = env["rack.input"]
  if data = parse_data(rack_input.read)
    # ...
  end
  rack_input.rewind
end

def raw_post
  # ...
  set_header("RAW_POST_DATA", raw_post_body.read(content_length)) # `"null"`, as it is when running the specs standalone.
  raw_post_body.rewind if raw_post_body.respond_to?(:rewind)
  # ...
end

def decode(json) # Is now `"null"`, as it should be.
  data = ::JSON.parse(json, quirks_mode: true) # ::JSON.parse(nil)
  # ...
end

Other stuff

"null" is a valid JSON string:

json
    element

element
    ws value ws

value
    object
    array
    string
    number
    "true"
    "false"
    "null"

https://www.json.org/

@enrique-guillen enrique-guillen changed the title [Bugfix] Ensure Extractor::Recorder is always rewound because it is always read [WIP] [Bugfix] Allow the "null" JSON string to be used as a parameter in Apipie spec examples (show_in_doc) Oct 25, 2018
@enrique-guillen
Copy link
Contributor Author

No longer a WIP. This fixed my specs with delete :something, "null" in them. 2.5.0/rails42 is the only CI failure.

@enrique-guillen enrique-guillen force-pushed the bugfix/extractor-recorder-should-always-rewind-rack-input branch from 8fcb218 to aaffaaf Compare October 25, 2018 23:59
@iNecas
Copy link
Member

iNecas commented Nov 16, 2018

I've fixed the intermittent test failure in other PR. Thanks for the contribution @enrique-guillen. Makes sense to me.

@iNecas iNecas merged commit 670d9dc into Apipie:master Nov 16, 2018
@iNecas
Copy link
Member

iNecas commented Nov 16, 2018

apipie-rails-0.5.14 has just been released, including this fix

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