Simplify endpoint block execution and deprecate return call in it

[ericproulx]

Deprecate Endpoint Return Value

Summary

This PR simplifies endpoint's block execution. It was relying on an unbound method but it always been re-binded to endpoint instance. Instead, I just call instance_exec from the endpoint instance. First, we deprecate and next version we will remove the rescue LocalJumpError.

Motivation

• I don't know why we want to use return in an endpoint. Seems to be an old thing.
• generate_api_method was generating a couple of strings that were retained in memory. I've seen it in grape-on-rack.

Changes

• Marked the return value of Grape::Endpoint#call as deprecated in the documentation and code comments.
• Added deprecation warnings where appropriate.
• Updated relevant specs and documentation.
• Its a lamba instead of Proc to be able to capture the LocalJumpError and return its exit_value for not breaking anything.

Impact

• No breaking changes for typical API usage.
• Users who were relying on the return value of Grape::Endpoint#call should update their code to avoid this pattern.

Checklist

• Deprecation warning added
• Documentation updated
• Specs updated

[dblock]

Feel free to merge with the UPGRADING update @ericproulx.

[jarthod]

Hello, I just noticed this deprecation warning while testing the edge version, and I'm wondering what is the recommended alternative for people who did use return ^^ Here is my endpoint which is using to return a different format (and bypasse the rest of the method) in some specific case:

    delete ':id' do
      if recipient = Notifier.recipients(current_user).find {|r| r.key == params[:id] }
        if recipient.type == :sms
          current_user.phone_numbers -= [recipient.identifier]
        elsif recipient.type == :webhook
          webhook = current_user.webhooks.where(id: recipient.identifier).first
          return {'deleted' => webhook.destroy}
        elsif recipient.type == :slack_compatible
          current_user.slack_compatible -= [recipient.identifier]
        elsif recipient.type == :msteams
          current_user.msteams -= [recipient.identifier]
        end
        save_model current_user
        {'deleted' => current_user.previous_changes.any?}
      else
        error! "Recipient not found: #{params[:id]}", 404, {"Cache-Control" => "public, max-age=60", "Vary" => "X-Api-Key"}
      end
    end

Here the return was quite helpful to me to avoid writing another level of conditional, I'm wondering if there's another good option to return early (similar to error! but for a success) ? Thanks !

[dblock]

Maybe like this?

delete ':id' do
  if recipient = Notifier.recipients(current_user).find { |r| r.key == params[:id] }
    deleted = false
    case recipient.type
    when :sms
      current_user.phone_numbers -= [recipient.identifier]
    when :webhook
      webhook = current_user.webhooks.where(id: recipient.identifier).first
      deleted = webhook.destroy
    when :slack_compatible
      current_user.slack_compatible -= [recipient.identifier]
    when :msteams
      current_user.msteams -= [recipient.identifier]
    end

    save_model current_user
    { 'deleted' => deleted || current_user.previous_changes.any? }
  else
    error! "Recipient not found: #{params[:id]}", 404, {
      "Cache-Control" => "public, max-age=60",
      "Vary" => "X-Api-Key"
    }
  end
end

[jarthod]

Thanks for the suggestion, it's not correct though as deleted can be false and that would execute unwanted code and return the wrong value, also the save_model method should not be executed in that case. I can rewrite the code to workaround this that's not an issue, it's just that it would be more verbose so I'm wondering if there's any feature I'm missing in Grape to continue doing this kind of flow. It's handy to be able to stop the flow with error! and return. But if there's a good reason this is no longer supported by Grape I'll stop doing that ^^

[dblock]

@ericproulx wdyt? Using return inside an endpoint is probably more common than we thought? Would there be a way to reimplement support for it "cleanly"?

[ericproulx]

You could extract the case in a method inside your current_user model. Easily testable without Grape. Here's how to woul d do it :

# in your `current_user` model

class RecipientNotFound < StandardError; end

def delete_recipient(recipient_id)
  recipient = Notifier.recipients(self).find { |r| r.key == recipient_id }
  raise RecipientNotFound, "Recipient not found: #{recipient_id}" unless recipient

  deleted = false
  case recipient.type
  when :sms
    self.phone_numbers -= [recipient.identifier]
  when :webhook
    webhook = webhooks.where(id: recipient.identifier).first
    deleted = webhook.destroy
  when :slack_compatible
    self.slack_compatible -= [recipient.identifier]
  when :msteams
    self.msteams -= [recipient.identifier]
  end
 
  save # assuming save_model is doing that
  deleted || previous_changes.any?
end

# Grape API

rescue_from CurrentUserModel::RecipientNotFound do |e| # 
  error! e.message, 404, { "Cache-Control" => "public, max-age=60", "Vary" => "X-Api-Key" }
end

delete ':id' do
  { 'deleted' => current_user.delete_recipient(params[:id]) }
end

@jarthod wdyt ?

[jarthod]

Thanks for the suggestion, but I already know how to refactor my ruby code ^^ (and this examble has the two same problems I pointed above anyway). I'm just asking about Grape features here, if you decide to go ahead and remove this feature with no alternative that's fine for me I'll rewrite my code accordingly. I was just interested in knowing if there was an alternative I didn't knew about maybe. I'm just one user and I'm only using this return flow once so I'm definitely not pushing for the feature to stay. As I understand there's no replacement planned so that's settled ^^.

If you do decide to keep it though, in terms of implementation I believe this could be cleanly implemented using throw / catch, like sinatra does for early response return in it's — similar to grape — block syntax DSL:
https://avdi.codes/throw-catch-raise-rescue-im-so-confused/
https://patshaughnessy.net/2012/3/7/learning-from-the-masters-sinatra-internals

Thanks for grape ❤️

[ericproulx]

Sorry about that, I miss read the situation. You could use break or next instead of return and it should do the trick. Let me know, I will update the UPGRADING notes

[jarthod]

I didn't thought those could work in a block with no iteration but it seems like it does, thanks for the tip!

• break actually give the same warning about return (I suppose it raises a LocalJumpError due to the lack of iterator outside)
• but next indeed seems to work as expected in replacement of return, it exits the block with no exception and return the passed value, in my example:

          next {'deleted' => webhook.destroy}

It's not as explicit when reading but at least it works and does not require extra code on either side. I tried wrapping it in a "render" method or some alias but as a it's a keyword it's not easy to do (method gives an invalid next exception, alias gives method not found as it's not a method).

I guess the simplest is to recommend people to use next instead of return then (ideally in the deprecation message directly)
Thanks!

[a5-stable]

@ericproulx

Thank you for your works on Grape.

I’m also seeing the deprecation warnings related to using return in endpoints.
There are several cases in our codebase where we intentionally rely on return.

I understand that the logic can be refactored, or next can technically
be used as a simple alternative mentioned in #2621.

It’s true that next does not raise a LocalJumpError, but this behavior depends
on Grape's internal implementation details, specifically the shift from using
UnboundMethod to a block-based execution model. That makes a bit harder to reason about at a glance.
Also, refactoring to avoid return can sometimes make the code a bit less clear,
especially in places where an early-return is naturally the simpler expression.
I would appreciate your thoughts on this. Thank you in advance!

[ericproulx]

Hi @a5-stable, thanks for reaching out about this subject. Quite frankly, having return is blocks or lambdas is not a common idioms in ruby and since almost everything is a block, I would rather stick with them. Not to say that some magic was done behind the scenes so that people could use return and I wasn't a fan. I like simple things. That being said, would you mind sharing your cases in a issue so that we could have a proper discussion about it? Maybe Grape's is missing a feature that is forcing you or your team to use return. I would gladly discuss about it. Thanks.

[a5-stable]

@ericproulx Thank you for your kind response!
I'd like to share some use cases from our codebase where return is useful.

• Example 1: Early returns for different conditions

We frequently use early returns to handle different success scenarios:

get '/resource/:id' do
  return { item: cached } if cached = Rails.cache.fetch("item:#{params[:id]}")
  
  resource = Item.find(params[:id])
  
  # Different return values based on conditions
  return { item: resource } if condition_a?
  return { item: resource.related_items.first } if condition_b?
  return { item: special_transform(resource) } if condition_c?
  
  # Default case
  { item: default_transform(resource) }
end

• Example 2: Early return from nested loops

Sometimes we need to return a response when a condition is met inside nested iterations:

get '/search' do
  categories.each do |category|
    category.items.each do |item|
      if item.matches?(params[:query])
        # Want to return from the endpoint, not just skip to next iteration
        return { item: item, category: category.name }
      end
    end
  end
  
  status 404
end

This is impossible with a simple substitution to next. next only skips to the next iteration of the inner loop, not exiting the endpoint.

Let me note that Sinatra supports return in route blocks, though I'm not sure if it's intentional or not:

get '/' do
  return 'Hello world!'
end

Thank you again for your thoughtful consideration of this issue.

[jarthod]

@a5-stable glad to see I was not the only one ^^

[ericproulx]

@a5-stable, thanks I understand the appeal. I'll remove the deprecation and bring back the return. I'll let you know

[a5-stable]

@ericproulx
Thanks a lot for taking the time to discuss this.
I really appreciate the thoughtful discussion and your openness to reconsider the decision!
Thanks again for the work you do on Grape.

@jarthod
Thanks for kicking off this discussion!

[jarthod]

Thanks @ericproulx !
I made peace with using next in my small case but I appreaciate the consideration and I also believe like @a5-stable that it makes more sense with return.

As a quick reminder, if you still want to remove the unbound method thing, I believe this could also be cleanly implemented using throw / catch, like sinatra does for early response return in it's block syntax DSL:
https://avdi.codes/throw-catch-raise-rescue-im-so-confused/
https://patshaughnessy.net/2012/3/7/learning-from-the-masters-sinatra-internals

In which case it would need to be an explicit method of yours instead of return (like response, render or something like that). But that would be a more acceptable workaround I believe as it's more readable than "next" and would support the same use-cases as return (basically a drop-in replacement). @a5-stable if you have any opinion on this let us know.

[ericproulx]

Thanks @ericproulx ! I made peace with using next in my small case but I appreaciate the consideration and I also believe like @a5-stable that it makes more sense with return.

As a quick reminder, if you still want to remove the unbound method thing, I believe this could also be cleanly implemented using throw / catch, like sinatra does for early response return in it's block syntax DSL: https://avdi.codes/throw-catch-raise-rescue-im-so-confused/ https://patshaughnessy.net/2012/3/7/learning-from-the-masters-sinatra-internals

In which case it would need to be an explicit method of yours instead of return (like response, render or something like that). But that would be a more acceptable workaround I believe as it's more readable than "next" and would support the same use-cases as return (basically a drop-in replacement). @a5-stable if you have any opinion on this let us know.

Thanks for sharing. I like the define_method and remove_method. Before the changes, there was a generate_api_method_name that was totally unnecessary. Sinatra is doing the same thing

[a5-stable]

Thanks for sharing for your thoughts.
I also agree that using define_method and remove_method is simple and effective!
Having an explicit method like halt (similar to Sinatra) could be nice too (even in that case, return might be ok).
Though I think the internal implementation and the things about public API (introduce new method) can be considered separately somewhat.