delete a reversible stackable values class #1953

dnesteryuk · 2020-01-04T09:17:08Z

The reversible stackable values object was initialized for every endpoint, however, it is only needed for keeping rescue handlers. The idea is simple, handlers defined "closer" to an endpoint have higher priority. That test demonstrates how it works.

In our project rescue handlers are defined at the top level, so almost every endpoint keeps the unused object.

The mentioned behavior is easy to achieve with the stackable values object and the reverse_each method. Thus, endpoints keeps less objects and have less code to be maintained.

Besides that, there are a few other simple performance optimizations.

The reversible stackable values object was initialized for every endpoint, however, it is only needed for keeping rescue handlers. The idea is simple, handlers defined "closer" to an endpoint have higher priority. That test https://github.com/ruby-grape/grape/blob/master/spec/grape/api_spec.rb#L3215-L3232 demonstrates how it works. In our project rescue handlers are defined at the top level, so almost every endpoint keeps the unused object. The mentioned behavior is easy to achieve with the stackable values object and the `reverse_each` method. Thus, endpoints keeps less objects and have less code to be maintained. Besides that, there are a few other simple performance optimizations.

dblock

Nice work. See small comment.

dblock · 2020-01-04T14:20:00Z

lib/grape/validations/types/build_coercer.rb

@@ -83,7 +83,11 @@ def self.cache_instance(type, method, strict, &_block)
      end

      def self.cache_key(type, method, strict)
-        [type, method, strict].compact.map(&:to_s).join('_')
+        [type, method, strict].each_with_object(+'') do |val, memo|


Should be _ to avoid fo_obar and foo_bar type situations.

I guess joining '_' is unnecessary since the key is meant for "machines" here. Do you think it is necessary? 🙂

Are we 100% sure that type/method/strict don't have the situation like type=fo + method=obar vs. type=foo + method=bar, it would generate the same key. So I think better safe than sorry.

Good point! I've changed it.

dblock · 2020-01-04T14:20:37Z

lib/grape/dsl/settings.rb

@@ -171,7 +168,7 @@ def within_namespace(&_block)
      # the superclass's :inheritable_setting.
      def build_top_level_setting
        Grape::Util::InheritableSetting.new.tap do |setting|
-          if defined?(superclass) && superclass.respond_to?(:inheritable_setting) && superclass != Grape::API
+          if defined?(superclass) && superclass.respond_to?(:inheritable_setting) && superclass != Grape::API::Instance


Is there a bug fix here?

Oh, I forgot to mention this one. Grape::API doesn't make use of Grape::Util::InheritableSetting, but Grape::API::Instance does.

irb(main):006:0> Grape::API.respond_to?(:inheritable_setting) => false irb(main):007:0> Grape::API::Instance.respond_to?(:inheritable_setting) => true

Thus, there is no reason to expect Grape::API.

The condition was added when Grape::API worked with Grape::Util::InheritableSetting.

I understand that part, but without digging into details it's a code change has a side effect. So either we're missing a spec or the entire if is unnecessary?

It didn't cause big harm, however, a test for catching the regression makes sense 👍 Please, have a look! 🙂

dblock · 2020-01-06T04:22:09Z

Merged, 👍

nbulaj · 2020-01-08T10:20:34Z

I updated Grape from git (from d9cf755 to current with this PR merged) and all my test suites began to fail :( Somewhere with Grape params validation messages, in other places with other errors. If I revert Grape to d9cf755 - everything works

dblock · 2020-01-08T11:46:34Z

@nbulaj Please try writing specs for any regressions?

nbulaj · 2020-01-08T12:34:22Z

Oh, it could be hard, but I'll try to

nbulaj · 2020-01-08T12:45:07Z

Ah, I see. It fails with the test that must check this part:

rescue_from ActiveRecord::RecordNotFound do |e|
  logger.warn("[#{e.class.name}] #{e.message}")
  error!({ message: "Not found" }, 404)
end

Instead of rescuing the error it just throws it and I have:

ActiveRecord::RecordNotFound:
Couldn't find Task with [WHERE "tasks"."deleted_at" IS NULL AND "tasks"."key" = $1]

in the test. This rescue_from is in the base class which mounts a lot of other Grape apis... Seems like it just don't work with new changes. I'll try to extract a test app

UPD 2*: yep, I've analyzed RSpec failed specs and seems like all of them related to rescue_from blocks which not work with the latest changes

nbulaj · 2020-01-08T13:53:28Z

Ok, so this is the script to reproduce an breaking change:

Gemfile

source " https://rubygems.org" 

# With just:
#    gem "grape", "~> 1.2" 
# or before this PR merged everything works

gem "grape", git: "https://github.com/ruby-grape/grape"
gem "activerecord"
gem "puma"

server.ru

require 'bundler/setup'
Bundler.setup

require 'grape'
require 'active_record'

module API
  class Endpoint < Grape::API
    desc "Test"
    get :test do
      # Simulate User.find(10)
      raise ::ActiveRecord::RecordNotFound.new("test", "name", "key", [])
    end
  end

  class Root < Grape::API
    version :v1, using: :path
    format :json

    rescue_from ::ActiveRecord::RecordNotFound do |e|
      error!({ message: "Not found" }, 404)
    end

    rescue_from StandardError do |e|
      raise e
    end

    mount API::Endpoint
  end
end

run API::Root

Run the API (bundle exec rackup server.ru) and curl http://localhost:9292/v1/test.json

Expected result:

See JSON with message: "Not found"

Actual result:

See unhandled exception with full stacktrace :(

Sorry, can't write a RSpec test right now - don't have a time. But would be great if @dnesteryuk could take a look

dblock · 2020-01-08T17:39:52Z

I opened #1957 to track. Please someone take a look.

dnesteryuk · 2020-01-08T18:53:11Z

@dblock @nbulaj I will take a look next weekend. It looks like this case was missing in tests 😞

dm1try · 2020-01-09T01:10:59Z

added the failing spec(#1959) for testing functional behavior
though, I'm out of the context for helping to fix this right now) but one thing I know for sure is that something has to be reversed again)

This reverts commit 9f786ad.

* delete a reversible stackable values class The reversible stackable values object was initialized for every endpoint, however, it is only needed for keeping rescue handlers. The idea is simple, handlers defined "closer" to an endpoint have higher priority. That test https://github.com/ruby-grape/grape/blob/master/spec/grape/api_spec.rb#L3215-L3232 demonstrates how it works. In our project rescue handlers are defined at the top level, so almost every endpoint keeps the unused object. The mentioned behavior is easy to achieve with the stackable values object and the `reverse_each` method. Thus, endpoints keeps less objects and have less code to be maintained. Besides that, there are a few other simple performance optimizations.

This reverts commit 9f786ad.

dblock requested changes Jan 4, 2020

View reviewed changes

post-review improvements

b3dca5f

dblock merged commit 9f786ad into ruby-grape:master Jan 6, 2020

dnesteryuk deleted the chore/micto-optimization-3 branch January 6, 2020 07:40

dblock mentioned this pull request Jan 8, 2020

Regression? #1957

Closed

dblock added a commit that referenced this pull request Jan 9, 2020

Revert "delete a reversible stackable values class (#1953)"

581290a

This reverts commit 9f786ad.

dblock mentioned this pull request Jan 9, 2020

Revert "delete a reversible stackable values class" #1961

Merged

dblock added a commit that referenced this pull request Jan 9, 2020

Revert "delete a reversible stackable values class (#1953)"

38c6bc0

This reverts commit 9f786ad.

basjanssen pushed a commit to basjanssen/grape that referenced this pull request Feb 28, 2020

Revert "delete a reversible stackable values class (ruby-grape#1953)"

c50b651

This reverts commit 9f786ad.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delete a reversible stackable values class #1953

delete a reversible stackable values class #1953

dnesteryuk commented Jan 4, 2020

dblock left a comment

dblock Jan 4, 2020

dnesteryuk Jan 5, 2020

dblock Jan 5, 2020

dnesteryuk Jan 5, 2020

dblock Jan 4, 2020

dnesteryuk Jan 5, 2020

dblock Jan 5, 2020

dnesteryuk Jan 5, 2020

dblock commented Jan 6, 2020

nbulaj commented Jan 8, 2020

dblock commented Jan 8, 2020

nbulaj commented Jan 8, 2020

nbulaj commented Jan 8, 2020 •

edited

Loading

nbulaj commented Jan 8, 2020 •

edited

Loading

dblock commented Jan 8, 2020

dnesteryuk commented Jan 8, 2020

dm1try commented Jan 9, 2020 •

edited

Loading

delete a reversible stackable values class #1953

delete a reversible stackable values class #1953

Conversation

dnesteryuk commented Jan 4, 2020

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jan 6, 2020

nbulaj commented Jan 8, 2020

dblock commented Jan 8, 2020

nbulaj commented Jan 8, 2020

nbulaj commented Jan 8, 2020 • edited Loading

nbulaj commented Jan 8, 2020 • edited Loading

dblock commented Jan 8, 2020

dnesteryuk commented Jan 8, 2020

dm1try commented Jan 9, 2020 • edited Loading

nbulaj commented Jan 8, 2020 •

edited

Loading

nbulaj commented Jan 8, 2020 •

edited

Loading

dm1try commented Jan 9, 2020 •

edited

Loading