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

Parse JSON array data in body at top level #1730

Open
Mateus-Resende opened this issue Jan 17, 2018 · 12 comments
Open

Parse JSON array data in body at top level #1730

Mateus-Resende opened this issue Jan 17, 2018 · 12 comments

Comments

@Mateus-Resende
Copy link

Hi there,

I'm trying to setup a route that will be able to receive a post request with an array (valid json) without having to name it. For example if I do

curl -XPOST -d '[{"foo": "baz"}, {"foo": "bar"}]' locahost:3000/foo

I want to be able to access this array of data. Is there any way I can do it ?

@dblock
Copy link
Member

dblock commented Jan 17, 2018

It's an interesting problem.

Without naming the params we would be getting a body, but no automatic parsing. That answers your immediate question, you can JSON.parse(request.body.first) in the API or middleware.

require 'spec_helper'

describe Grape::Endpoint do
  subject { Class.new(Grape::API) }

  def app
    subject
  end

  before do
    subject.format :json
    subject.post do
      JSON.parse(request.body.first)
    end
  end

  let(:data) { ::Grape::Json.dump([{ foo: 'bar' }, { foo: 'baz' }]) }

  it 'responds' do
    post '/', data, 'CONTENT_TYPE' => 'application/json'
    expect(last_response.status).to eq 201
    expect(last_response.body).to eq(data)
  end
end

But we want to be able to declare this. With named foos this is easy.

require 'spec_helper'

describe Grape::Endpoint do
  subject { Class.new(Grape::API) }

  def app
    subject
  end

  before do
    subject.format :json
    subject.params do
      requires :foos, type: Array
    end
    subject.post do
      params
    end
  end

  let(:data) { ::Grape::Json.dump(foos: [{ foo: 'bar' }, { foo: 'baz' }]) }

  it 'responds' do
    post '/', data, 'CONTENT_TYPE' => 'application/json'
    expect(last_response.status).to eq 201
    expect(last_response.body).to eq(data)
  end
end

Now what would a declaration look like for an array here? I suppose params could become an Array type instead of a Hash?

require 'spec_helper'

describe Grape::Endpoint do
  subject { Class.new(Grape::API) }

  def app
    subject
  end

  before do
    subject.format :json
    subject.params type: Array do
      requires :foo, type: String
    end
    subject.post do
      params
    end
  end

  let(:data) { ::Grape::Json.dump([{ foo: 'bar' }, { foo: 'baz' }]) }

  it 'responds' do
    post '/', data, 'CONTENT_TYPE' => 'application/json'
    expect(last_response.status).to eq 201
    expect(last_response.body).to eq(data)
  end
end

This doesn't work yet, but I have a failing spec and some first level changes in https://github.com/dblock/grape/tree/issue-1730, so if someone wants to take a stab at it that'd be great.

There's also a problem with how to declare that an array of at least 1 foo is required here. Maybe params gets a requires or optional alias:

    subject.requires type: Array do
      requires :foo, type: String
    end
    subject.optional type: Array do
      requires :foo, type: String
    end

@jnardone
Copy link

jnardone commented Jan 18, 2018

I was wondering if the top level param could be a special case, e.g. params :nil, type: Array or somesuch (this is actually not currently illegal/caught at parse time, which probably falls into the "bug" category) - this keeps the DSL somewhat consistent, though wondering how hacky that truly feels.

@dblock
Copy link
Member

dblock commented Jan 18, 2018

I think params: nil feels very hacky and would rather not do that. I had tried that and definitely agree that the error message you get is unhelpful and you shouldn't be allowed to do that. Appreciate if you could open a separate issue for that, maybe even with a spec or a fix ;)

We always knew that params doesn't have to be a Hash, as you may see the underlying code supports nested arrays, it just never exposed the DSL for top level ones. Lets not be afraid of extending the DSL!

@dm1try
Copy link
Member

dm1try commented Jan 19, 2018

We always knew that params doesn't have to be a Hash, as you may see the underlying code supports nested arrays, it just never exposed the DSL for top level ones. Lets not be afraid of extending the DSL!

we are not afraid :) but
endpoint #params helper returns POST/GET params + named route params
most of the endpoints have the route params (for example, :id)
so it much easier to treat root params as Hash

otherwise, an additional interface should be provided to separate those params interfaces ^(it is not worth it as for me)
or an exception should be raised when an endpoint does include route params or has a content type other than json

@dblock
Copy link
Member

dblock commented Jan 19, 2018

I spent an hour on this and concluded that there're too many assumptions around params being a hash, including in Rack proper. It's easy to add the body with body key for example and I think we could validate and coerce it.

But is something that wraps body the same way we wrap params crazy?

before do
    subject.format :json
    subject.body type: Array do
      requires :foo, type: String
    end
    subject.post do
      body
    end
  end

@dm1try
Copy link
Member

dm1try commented Jan 19, 2018

It's easy to add the body with body key for example and I think we could validate and coerce it.
But is something that wraps body the same way we wrap params crazy?

while I have no strong feelings about this, body looks more applicable, at least such interface more consistent with the usage of endpoint #params

@dblock
Copy link
Member

dblock commented Jan 19, 2018

Cool thx @dm1try. The only caveat is that when body is a Hash, it gets merged onto params since most people send query strings, forms, json, etc., indistinguishable from each-other. Just something to keep in mind when implementing, maybe that behavior is turned off when body is present?

Also, I am not working on it, maybe @Mateus-Resende wants to take a stab?

@dblock dblock changed the title Unnamed top class Array Param Parse JSON array data in body at top level Jan 19, 2018
@nonnenmacher
Copy link

Stumble into the same issue today trying to implement RFC 6902 (https://tools.ietf.org/html/rfc6902) JavaScript Object Notation (JSON) Patch.

The RFC describe a PATCH payload consisting of a stray Array or operations.

Found no way to express this with Grape (along the fact that Swagger generation, just choke on tries like

  params ...
     requires :operation, type: Hash, documentation: { param_type: 'body' } do
          nested validations of `operation`  <key,values> pairs
    end

It could also be nice to have like in Array[JSON] such construct with a stray array, that could rewrap a single { "operation" : { ...} } into a [{ "operation" : { ...} }] so both single entry and array could be handled the same way, as a collection of validated nested params.

@Mateus-Resende
Copy link
Author

I will start working on that now, should have a working PR soon. I agree with @dblock that there are too many assumptions that params is a Hash. So what if the key to wrap the body could be declared by the user allowing the API to infer what's in the body? Something like:

  params { requires :foos, type: Array, infer: true }
  post do 
    params[:foos] 
  end

And this would allow [{foo: 1}, {foo: 2}] to be parsed as {foos: [{foo: 1}, {foo: 2}]}. At the same time, we're adding flexibility for other cases, such as the PATCH payload as @nonnenmacher described. It would become something like:

  params { requires :operations, type: Array, infer: true }
  patch do 
    params[:operations]
  end

How does that sound?

@nonnenmacher
Copy link

perhaps an addition to the DSL could be to add a wrap attributes, with sole role of 'rewrapping' the body as a Hash or Array[Hash/JSON] (Array[hash] would be nice).

Then from that named 'rewrap' we could count on the fact that all assumptions and validations are based on a Hash like structure would hold again.

This also would help swagger generation as the way to have both an form-encoded or a JSON payload in swagger is fro add this documentation: { param_type: 'body' } attributes with has no name for description (and then could take that designation for its documentation instead of placing the name of the API entry point).

@dblock
Copy link
Member

dblock commented Jan 31, 2018

Lets not go crazy, keep things simple and lets look at some code ;)

@nonnenmacher
Copy link

no crazy, no fun ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants