Strong params, curl and Rails 5 API #211

Closed
ralphking opened this Issue Jun 8, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@ralphking

Hi,

I'm falling foul of the assign_params method, here: https://github.com/nviennot/nobrainer/blob/master/lib/no_brainer/document/attributes.rb#L73

Using Rails 5 RC1.

Model:

class Venue
  include NoBrainer::Document
  include NoBrainer::Document::Timestamps

  field :name, :type => String
  field :desc, :type => Text
end

Controller:

  # POST /venues
  def create
    @venue = Venue.new(venue_params)

    if @venue.save
      render json: @venue, status: :created, location: @venue
    else
      render json: @venue.errors, status: :unprocessable_entity
    end
  end

def venue_params
      params.require(:venue).permit(:name, :desc)
end

All very standard. When using curl to test the api, I'm hitting the error inside assign_attributes. I have the attributes in the controller, whitelisted.

curl -v -H "Accept: application/json" -H "Content-type: application/json" -X POST -d '{"venue":{"name":"test","desc":"something"}}' http://localhost:3000/venues

It works fine if I pass them as a hash, which is what the error is checking for:

@venue = Venue.new(name: params[:name], desc: params[:desc])

I assume it's doing this because it thinks I'm trying to submit attributes that are not whitelisted. Is this a bug?

Thanks

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101

jeroenvisser101 Jun 8, 2016

Contributor

@ralphking I had the same thing, and am happy to add my patch back to master.

Contributor

jeroenvisser101 commented Jun 8, 2016

@ralphking I had the same thing, and am happy to add my patch back to master.

@ralphking

This comment has been minimized.

Show comment
Hide comment
@ralphking

ralphking Jun 8, 2016

Hmm I was thinking of doing the same - just saw in the Docs that nobrainer should play nicely with Rails 5 so wasn't sure if it was a bug or not

Hmm I was thinking of doing the same - just saw in the Docs that nobrainer should play nicely with Rails 5 so wasn't sure if it was a bug or not

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101

jeroenvisser101 Jun 8, 2016

Contributor

I think it's unexpected, I'll file a PR now.

Contributor

jeroenvisser101 commented Jun 8, 2016

I think it's unexpected, I'll file a PR now.

@ralphking

This comment has been minimized.

Show comment
Hide comment
@ralphking

ralphking Jun 8, 2016

Ok I'll just borrow your fork for now while I test.

Thanks

Ok I'll just borrow your fork for now while I test.

Thanks

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101

jeroenvisser101 Jun 8, 2016

Contributor

@ralphking there's some other things inside rails5 branch related to code reloading that are definitely not production-ready yet, so there be dragons there.

Contributor

jeroenvisser101 commented Jun 8, 2016

@ralphking there's some other things inside rails5 branch related to code reloading that are definitely not production-ready yet, so there be dragons there.

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101

jeroenvisser101 Jun 8, 2016

Contributor

@ralphking I've created a new branch that you can check. The implementation is a little different (it's now really backwards compatible).

Contributor

jeroenvisser101 commented Jun 8, 2016

@ralphking I've created a new branch that you can check. The implementation is a little different (it's now really backwards compatible).

@nviennot nviennot closed this in 9f9c431 Jun 10, 2016

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot Jun 10, 2016

Owner

@ralphking, I've pushed @jeroenvisser101 fix on master. Does it fixes your issue?

Owner

nviennot commented Jun 10, 2016

@ralphking, I've pushed @jeroenvisser101 fix on master. Does it fixes your issue?

@jeroenvisser101

This comment has been minimized.

Show comment
Hide comment
@jeroenvisser101

jeroenvisser101 Jun 10, 2016

Contributor

@nviennot thanks for merging :)

Contributor

jeroenvisser101 commented Jun 10, 2016

@nviennot thanks for merging :)

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