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

invalid update json response with generated controller scaffolding #1742

Closed
codebrew opened this Issue Jun 17, 2011 · 17 comments

Comments

Projects
None yet
10 participants
@codebrew
Copy link

commented Jun 17, 2011

The generated controller update method responds to successful json requests with head :ok. This renders a response with an empty body, which jquery fails to parse. All successful update request made by jquery will end up executing an error callback. Rendering of an empty json object {} fixes this error.

@dmathieu

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2011

Could you provide a failing test case, or a basic app reproducing the problem ?

@codebrew

This comment has been minimized.

Copy link
Author

commented Jun 17, 2011

I created a basic app showing the problem at https://github.com/codebrew/json-issue

The app is using the latests rails from head. I did a scaffolding gen of rails g scaffolding Post title:string content:string then I added some javascript in app/assets/javascripts/posts.js.coffee to make an update 'PUT' request to the server.

start up the server go to localhost:3000/posts, create a new post model, and in the table there will be a link called 'Ajax Update' it will make a successful ajax put request but the response body will be empty which causes jquery fails to parse and will execute the error callback.

@codebrew codebrew closed this Jun 17, 2011

@codebrew codebrew reopened this Jun 17, 2011

@dhh

This comment has been minimized.

Copy link
Member

commented Jul 11, 2011

This seems to be something that jQuery should take care of. Returning a 200 with an empty body should be a completely reasonable response if there's nothing to return. If we need to patch that temporarily in the jquery-rails adapter, then so be it. But it seems silly for us to return an empty json object.

@dhh dhh closed this Jul 11, 2011

@JangoSteve

This comment has been minimized.

Copy link
Member

commented Jul 12, 2011

If we need to patch this in the jquery-rails adapter, be sure to open a ticket in the jquery-ujs issues and I can take a look.

@spastorino spastorino reopened this Jul 14, 2011

@spastorino

This comment has been minimized.

Copy link
Member

commented Jul 14, 2011

I'm going to patch this on Rails

@samuelkadolph

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2011

I think it would be far better to finally update the controller scaffold to use respond_with which already responds with {} for updates.

@codebrew

This comment has been minimized.

Copy link
Author

commented Jul 14, 2011

If you're making a patch to the controller scaffold also update the destroy method, which has the same issue as update.

@samuelkadolph

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2011

All of the controller methods should be updated. I know there was hesitation in the past because using scaffold isn't the preferred way to develop your application.

class GizmosController < ApplicationController
  respond_to :html, :json

  # GET /gizmos
  # GET /gizmos.json
  def index
    @gizmos = Gizmo.all

    respond_with(@gizmos)
  end

  # GET /gizmos/1
  # GET /gizmos/1.json
  def show
    @gizmo = Gizmo.find(params[:id])

    respond_with(@gizmo)
  end

  # GET /gizmos/new
  # GET /gizmos/new.json
  def new
    @gizmo = Gizmo.new

    respond_with(@gizmo)
  end

  # GET /gizmos/1/edit
  def edit
    @gizmo = Gizmo.find(params[:id])

    respond_with(@gizmo)
  end

  # POST /gizmos
  # POST /gizmos.json
  def create
    @gizmo = Gizmo.create(params[:gizmo])

    respond_with(@gizmo, notice: "Gizmo was successfully created.")
  end

  # PUT /gizmos/1
  # PUT /gizmos/1.json
  def update
    @gizmo = Gizmo.find(params[:id])
    @gizmo.update_attributes(params[:gizmo])

    respond_with(@gizmo, notice: "Gizmo was successfully updated.")
  end

  # DELETE /gizmos/1
  # DELETE /gizmos/1.json
  def destroy
    @gizmo = Gizmo.find(params[:id])
    @gizmo.destroy

    respond_with(@gizmo, notice: "Gizmo was successfully deleted.")
  end
end
@mitijain123

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2011

Fixed this issue, please check the pull request
#2197

/cc @spastorino

@ohaibbq

This comment has been minimized.

Copy link

commented Jul 28, 2011

@dhh @codebrew

the problem is that you're calling head :ok which renders " " as the body, not "". I'm sure why rails returns a 1-character body on any head response, but it might have something to do with HTTP standards.

@ghost ghost assigned spastorino Jul 29, 2011

@matthuhiggins

This comment has been minimized.

Copy link

commented Aug 23, 2011

@ohaibbq The single character body is sent to workaround a bug in older versions of Safari. Safari ignored headers if the body has zero length.

@ohaibbq

This comment has been minimized.

Copy link

commented Aug 23, 2011

Alright, that makes sense. Thanks.

@tenderlove

This comment has been minimized.

Copy link
Member

commented Aug 26, 2011

I'm closing this since we pulled in @mitijain123's pull request.

@tenderlove tenderlove closed this Aug 26, 2011

@spastorino spastorino reopened this Aug 27, 2011

@spastorino

This comment has been minimized.

Copy link
Member

commented Aug 27, 2011

This wasn't fixed yet

@mitijain123

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2011

thanks /cc tenderlove .

/cc spastorino let me know if you have any idea in mind, I like to implement that, if you give me a chance.

@spastorino

This comment has been minimized.

Copy link
Member

commented Aug 27, 2011

@mitijain123 we should do head :success, and make that return 204

@spastorino

This comment has been minimized.

Copy link
Member

commented Oct 26, 2011

We fixed that here aef62c4
If you're hitting the issue change head :ok with head :no_content

@spastorino spastorino closed this Oct 26, 2011

merqlove pushed a commit to cnsa/bigbluebutton_rails that referenced this issue Dec 31, 2013

Modify empty json responses to prevent errors with jquery
When using 'head :ok' to respond with a json, the body is sent as ' ' (an empty
space). This is interpreted by jquery as an error even if the response code is 200.
The way we're doing it now sends a string in the body and is interpreted as a
successful response. See more about it at:
* http://stackoverflow.com/questions/12407328/rails-head-ok-interpreted-as-ajaxerror
* rails/rails#1742
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.