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

Respect content type set in superclass before filter #1649

Merged
merged 1 commit into from Oct 18, 2020

Conversation

jkowens
Copy link
Member

@jkowens jkowens commented Oct 3, 2020

Fixes #1647.

@jkowens jkowens requested a review from namusyaka October 3, 2020 15:32
@jkowens
Copy link
Member Author

jkowens commented Oct 3, 2020

@namusyaka another regression has been identified in v2.1.0.

@namusyaka
Copy link
Member

@jkowens can you provide me which the patch/issue raised this regression?

@jkowens
Copy link
Member Author

jkowens commented Oct 10, 2020

@namusyaka yes, it was PR #1606.

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.
But I think we should do additional work a bit.

  1. Originally, @pinned_response should be initialized for starting each request.
    The @pinned_response looks to be reusing for every request now.
    Probably this won't be an issue in most cases because the filters are managed globally in the class, but not good for me.

  2. I don't think we need to pin the response for both before/after filters.
    Does the following change work well?

diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb
index 11cd284a..9d0ec49a 100644
--- a/lib/sinatra/base.rb
+++ b/lib/sinatra/base.rb
@@ -994,11 +994,11 @@ module Sinatra

     # Run filters defined on the class and all superclasses.
     # Accepts an optional block to call after each filter is applied.
-    def filter!(type, base = settings)
-      filter! type, base.superclass if base.superclass.respond_to?(:filters)
+    def filter!(type, base = settings, &block)
+      filter! type, base.superclass, &block if base.superclass.respond_to?(:filters)
       base.filters[type].each do |args|
         result = process_route(*args)
-        yield result if block_given?
+        block.call(result) if block_given?
       end
     end

yield result if block_given?
end
base.filters[type].each { |args| process_route(*args) }
@pinned_response = !response['Content-Type'].nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to pin the response for both before/after filters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I don't think we do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent solution, I'll push an update now.

@namusyaka
Copy link
Member

Could you also consider the initialization of @pinned_response when receiving a request? This is the same thing with

@params = IndifferentHash.new

@namusyaka namusyaka added this to the v2.1.1 milestone Oct 11, 2020
@jkowens
Copy link
Member Author

jkowens commented Oct 11, 2020

It looks like @pinned_response is initialized here:

@pinned_response = nil # whether a before! filter pinned the content-type

Is that ok?

@namusyaka
Copy link
Member

No. It's executed at the boot time, not every request.

@jkowens
Copy link
Member Author

jkowens commented Oct 11, 2020

Ah got it. Ok, initialization has been moved to the call! method. Thanks 👍

@namusyaka
Copy link
Member

I'm sorry for the late, I'll review this in a few days.

Copy link
Member

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. I'm sorry for the late action.
LGTM with a few nits. Let's merge this after resolving my comments.

@@ -920,7 +920,6 @@ def initialize(app = nil)
super()
@app = app
@template_cache = Tilt::Cache.new
@pinned_response = nil # whether a before! filter pinned the content-type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You may want to leave this here. If I remember correctly, initializing instance variables at the stage of initialization is in line with ruby's way. This will show up a new warning like warning: instance variable @pinned_response not initialized if you keep this change. Of course, I'm recognizing that there are a lot of warnings similar to that already, though.

test/filter_test.rb Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON served as HTML content_type since v2.1
2 participants