Respond with 422 Unprocessable Entity for non-GET HTML/JS requests with errors #223
Conversation
When responding to a non-GET HTML request that had errors on the resource, responders would not set any status, which means Rails would default to `200 OK`. This has been long working like this but also a long discussion in responders to change that behavior, to be more inline with how other types of requests (like JSON/XML) handle responses by setting the HTTP status to 422 Unprocessable Entity when responding with an error. More recently with the Turbo/Hotwire library, they've added support to render form responses as errors with 4xx statuses [1], so it makes even more sense for responders to make the move here. This change now makes responders use the 422 Unprocessable Entity status in such cases when the resource has errors, and should work more out of the box with Turbo/Hotwire. Please note that this is a possible breaking change if you're relying on the previous status to trigger any behavior for errors in your app. [1] hotwired/turbo#39
Much like HTML responses, JS will respond with a 422 Unprocessable Entity for non-GET requests if the given resource contains errors, so it is consistent with all other formats going forward. Closes #159.
carlosantoniodasilva
force-pushed
the
ca-render-error-422
branch
from
175a72a to
ca2ab74
Compare
carlosantoniodasilva
changed the title
carlosantoniodasilva
changed the title
|
Just tested this out, in a website + app running Turbo (+ native).
I don't use any other Devise features in this app, so that was all I tested. Thank you so much for your work on this. re SessionsController, for handling failed login attempts, I have this failure app (which is still necessary even with this PR): class TurboFailureApp < Devise::FailureApp
def respond
if request_format == :turbo_stream
recall
# turbo wants a 422 https://turbo.hotwire.dev/handbook/drive#redirecting-after-a-form-submission
response[0] = 422
else
super
end
end
endIt's based on this: https://discuss.hotwire.dev/t/forms-without-redirect/1606/14 Would you be open to something like this being upstreamed to Devise? |
|
@ghiculescu thanks for your thorough testing and feedback, really appreciated! What a coincidence, I just signed up to discuss.hotwire.dev and replied to that same thread, I had it opened here for a couple of days. 😄 I definitely want to get things working more out of the box with both responders & devise, yes, just playing catch up on many fronts here. I actually think the second method |
|
I think any 4xx is fine. See hotwired/turbo#39 I am happy to help put together a Devise PR, if you're happy with the general gist of my approach. (If you'd prefer to do it yourself that's fine by me too!)
I actually don't think it is needed in my case anymore. I had it there from when I used to call |
|
@ghiculescu if you have the availability feel free to go ahead :), I'd be happy to help where I can. I'm wrapping up a few other things related to this branch, Devise + OmniAuth, etc., for the time being, before I can tackle anything Devise + Hotwire, so maybe I could only check that by next week. Thanks. |
|
Alright, I'll give it a look. For this PR is there anything else outstanding? It's looking great to me - we are running it in production. |
|
I was just giving it a little time to see if anyone else from the original issue would chime in. I'm glad to know it's working for you (and in production!). I'll give it until the weekend and plan for a new release then. Thanks for your help. |
There was a problem hiding this comment.
I think the documentation also needs to get adjusted to the new behaviour.
On line 52 of lib/action_controller/responder.rb the Rails behaviour without responders gem would be equivalent to:
format.html { render :new, status: :unprocessable_entity } <= with the 422 behaviour now in it :)
carlosantoniodasilva
force-pushed
the
ca-render-error-422
branch
from
46300d3 to
b2dd0a6
Compare
|
Thanks @gobijan, those have been updated. |
|
No thank you so much for your continuous effort on so many great ruby gems :)! |
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
A bit late, but here you go! heartcombo/devise#5340 |
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
|
@ghiculescu do you use responders on its own (i.e. respond_with) in other controllers? or just happen to use it through Devise? I ask because I was having some trouble getting it to fully work on its own as it was triggering API behavior, I'll circle back on it but I think something might be missing here still to get it right. (like a proper |
|
I only use it via devise. I can have a look if you can share some more details about your issue? |
|
I'll take another look and maybe try with a fresh app to create a sample, but basically it was not working to return HTML properly when integrated with hotwire/turbo. I'll report back with more info, thanks. |
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
|
Hi @carlosantoniodasilva, thanks for all this work! I'm currently working on migrating an app to Turbo. I'm still unsure if I should handle Devise forms using a custom-made solution in production (not a fan of that idea) or if it would be better to wait for this change to be released with the new version of the Responders gem. Are there plans to release this anytime soon? |
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
|
This is a very useful fix :) I was wondering if you'd consider cutting a new gem for this soon? :) |
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
Devise `FailureApp` now returns a `422` status when running the `recall` flow. This, combined with heartcombo/responders#223, enables Devise to work correctly with the [new Rails defaults](rails/rails#41026) for form errors, as well as with new libraries like Turbo. By default, the `recall` flow only runs on the [`SessionsController`](https://github.com/heartcombo/devise/blob/v4.7.3/app/controllers/devise/sessions_controller.rb#L48).
This essentially makes the previous change merged via #223 configurable, so that we can keep the current behavior on existing apps, returning `200 OK` even for errors, while changing the generator for new apps to be configured with `422 Unprocessable Entity`, making them work better with Hotwire/Turbo installations out of the box. Please note that these defaults are likely to be swapped in a future major version of Responders.
This essentially makes the previous change merged via #223 configurable, so that we can keep the current behavior on existing apps, returning `200 OK` even for errors, while changing the generator for new apps to be configured with `422 Unprocessable Entity`, making them work better with Hotwire/Turbo installations out of the box. Please note that these defaults are likely to be swapped in a future major version of Responders.
This essentially makes the previous change merged via #223 configurable, so that we can keep the current behavior on existing apps, returning `200 OK` even for errors, while changing the generator for new apps to be configured with `422 Unprocessable Entity`, making them work better with Hotwire/Turbo installations out of the box. Please note that these defaults are likely to be swapped in a future major version of Responders.
|
I forgot to ping back here, but v3.1.0 was released a couple weeks ago including this change. |
When responding to a non-GET HTML/JS request that had errors on the
resource, responders would not set any status, which means Rails would
default to
200 OK. This has been long working like this but also along discussion in responders to change that behavior, to be more inline
with how other types of requests (like JSON/XML) handle responses by
setting the HTTP status to 422 Unprocessable Entity when responding with
an error.
More recently with the Turbo/Hotwire library, they've added support to
render form responses as errors with 4xx statuses [1], so it makes even
more sense for responders to make the move here.
This change now makes responders use the 422 Unprocessable Entity status
in such cases when the resource has errors, and should work more out of
the box with Turbo/Hotwire.
Please note that this is a possible breaking change if you're relying on
the previous status to trigger any behavior for errors in your app.
[1] hotwired/turbo#39
Closes #159.