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

halt for instantly returning a response, interrupting current handler execution #2374

Open
jcagarcia opened this issue Nov 20, 2023 · 6 comments

Comments

@jcagarcia
Copy link
Contributor

jcagarcia commented Nov 20, 2023

Hey 👋

Do you think a halt method can be interesting for instantly returning a response, interrupting current handler execution? An alternative to error! ?

Something like:

class MyAPI < Grape::API
  get '/wadus' do
    halt 401 unless authenticated?
  end
end

I think it can be useful and it's a standard. Also, it's present in some many other frameworks:

https://github.com/alisnic/nyny#request-scope
https://github.com/jeremyevans/roda/blob/master/lib/roda/plugins/halt.rb
https://github.com/hanami/controller#throwable-http-statuses

Thanks!

@jcagarcia jcagarcia changed the title feature: halt for instantly returning a response, interrupting current handler execution halt for instantly returning a response, interrupting current handler execution Nov 20, 2023
@dblock
Copy link
Member

dblock commented Nov 20, 2023

You correctly point out that we have error! that behaves this way by raising an exception. I don't think an alias to halt! is needed, but is there a scenario you would want this without an exception? Also I am not sure how that would be implementable (inside an API you can do next).

@jcagarcia
Copy link
Contributor Author

I was thinking more about naming.

When using the error! message, I'm assuming that I'm going to return a error response code. It's really useful to call this method in the middle of a execution because you don't need to deal with returns or if/else conditions.

So I was thinking if you want to return a response that is not an error itself, maybe a 204 if some condition happens and you don't want to deal with if/else conditions and including body and status in each of the if blocks.

It's true that I can use error!({}, 201) and return a not error status code when using the error! (maybe this should not be allowed?), so that was the reason of offering a error! alias for the use cases you don't want to explicitly return an error.

Anyway, if you consider that this is not something you want for grape, it's totally fine :) feel free to close the issue.

@dblock
Copy link
Member

dblock commented Nov 20, 2023

First, thank you. I (we) are open to suggestions and we absolutely should discuss it. Adding an API is a big deal so expect push back, but if you believe this is the right thing, keep arguing for it, don't give up so easily!

A question, does this work?

status 201
next

Is success! a better name?

Do you have an example where halt! reads significantly better than doing if/else?

@jcagarcia
Copy link
Contributor Author

Hi @dblock ! Thanks for the suggestion :) really appreciated!

I've just tried using the next and it perfectly works.

As I previously said, my issue/concern was more about naming, and as I've seen that other frameworks are using halt for instantly returning a response, that was my suggestion. However, success! or other alternative is a good naming too!

I was trying to build a example where the halt! reads significantly better than doing if/else. However is true that as you need to pass all the status, body and headers parameters to the helper (whatever name we consider), is the same to do this:

if something
  status 201
  header {}
  body {}
  next
end

and to do something like this:

halt!|success!|other!(201, {}, {}) if something

So don't know if it reads significantly better, because at the end you need to manage if conditions as well.

Totally open to add what you think that provides more value to the project, so at this point you have much more context and vision of the project than me.

@dblock
Copy link
Member

dblock commented Nov 21, 2023

Good points. I think success! could be a good idea semantically, but it would need to throw an exception to abort flow, so it's weird that internally we raise an error to signal success. Something like halt! that accepts a different set of parameters than error! is maybe a good idea after all. Want to try to PR it?

I'd like to hear what others have to say.

@jcagarcia
Copy link
Contributor Author

jcagarcia commented Nov 21, 2023

Sure! Totally open to discuss it and to find the better approach :) I can wait a little bit to provide a PR to see if we hear from others.

On the other hand, do you think that error! should limit the status codes that can be passed?

Thank you so much @dblock !!

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

2 participants