Bump Sinatra to 4.1.1 #411

jlledom · 2024-12-11T13:37:17Z

This is to properly implement #410

Jira: https://issues.redhat.com/browse/THREESCALE-11458

Due to a dependency hell, updating sinatra forces to update:

rack/rackup
falcon
puma

I also had to fork rspec_api_documentation because the gem it's abandoned and it's not compatible with Rack 3.

I also updated hiredis-client which I don't think it's necessary, but #410 does it for some reason so I'll do it as well, just in case I'm missing something.

I did some testing locally and seems to work fine, also the test suite in CircleCI passes for all environments, and the benchmark results doesn't show ant impact in performance.

Needed by yabeda-prometheus after upgrading rack to >= 3.0

Is not compatible with Rack 3 and apparently the gem is abandoned

Disables `prune_bundler` It's causing problems on our test suite, and we don't need this really. prune_bundler is intended to read changes in the Gemfile between phased restarts, but it's only enabled on development and test, where we don't need phased restarting. In production, besides, we use openshift which doesn't use phased restarts

A failing test is removed because the same scenario now returns 403, but I don't think it's important

jlledom · 2024-12-11T17:12:00Z

config/puma.rb

@@ -145,7 +145,7 @@
 # is preserved across a phased-restart. (incompatible with preload_app)
 # (off by default)

-prune_bundler
+# prune_bundler


prune_bundler is required to phase restart puma. However, it's only enabled on test and development envs, where we never phase restart.

It's causing us a lot of problems to run our test suite in Puma 6, so I'm disabling it.

So, this config/puma.rb file is only used for dev/test?

What happens if preload_app! is enabled also?

If it can be, we could probably simplify the config, and remove openshift/config/puma.rb ?

jlledom · 2024-12-11T17:14:53Z

lib/3scale/backend/listener.rb

@@ -411,8 +406,7 @@ def do_api_method(method_name)

        raise_provider_key_error(params) if blank?(provider_key)

-        # no need to check params key encoding. Sinatra framework does it for us.


Not anymore... rack/rack#1797

jlledom · 2024-12-11T17:16:19Z

lib/3scale/backend/rack/exception_catcher.rb

+        rescue ArgumentError => e
+          return unhandled_exception(e) unless e.message.include?(INVALID_BYTE_SEQUENCE_ERR_MSG)
+
+          delete_sinatra_error! env
+          respond_with 400, Backend::NotValidData.new.to_xml


Check 25d53c9

Basically, an Invalid UTF8 error can be raised at any place in the code and we centralize the handling here.

jlledom · 2024-12-11T17:19:47Z

lib/3scale/backend/logging/middleware.rb

@@ -35,7 +35,7 @@ def call(env)
            raise e
          end

-          header = ::Rack::Utils::HeaderHash.new(header)
+          header = ::Rack::Headers.new.merge(header)


Check https://github.com/rack/rack/blob/main/CHANGELOG.md#removed-1

jlledom · 2024-12-11T17:20:56Z

lib/3scale/backend/server/puma.rb

@@ -34,7 +34,7 @@ def self.start(global_options, options, args)
          argv_add argv, true, '-C', CONFIG
          ss_dir = socket_state_dir(global_options[:environment], global_options[:directory] || EXPANDED_ROOT_PATH)
          argv_add argv, true, '-S', File.join(ss_dir, STATE)
-          argv_add argv, true, '--control', "unix://#{File.join(ss_dir, CONTROL_SOCKET)}"
+          argv_add argv, true, '--control-url', "unix://#{File.join(ss_dir, CONTROL_SOCKET)}"


Change in puma v5.0.0. https://github.com/puma/puma/releases/tag/v5.0.0

jlledom · 2024-12-11T17:23:59Z

spec/api/listener/cors_spec.rb

+      REQUIRED_ENDPOINT_HEADERS = %w[access-control-allow-origin access-control-expose-headers]
+      REQUIRED_OPTIONS_HEADERS = REQUIRED_ENDPOINT_HEADERS + %w[access-control-allow-methods access-control-allow-headers]


For Rack 3: https://github.com/rack/rack/blob/main/CHANGELOG.md#spec-changes-2

jlledom · 2024-12-11T17:25:46Z

spec/integration/worker_metrics_spec.rb

@@ -130,7 +130,7 @@ def process_jobs(worker, num)

      def shutdown_metrics_server
        # Yabeda does not expose this
-        ::Rack::Handler::WEBrick.shutdown
+        ::Rackup::Handler::WEBrick.shutdown


Rackup was part of Rack, then moved to an independent gem. Rack 3 compatible apps use Rackup.

jlledom · 2024-12-11T17:26:27Z

test/integration/authorize/basic_test.rb

@@ -566,7 +566,7 @@ def setup
      'HTTP_3SCALE_OPTIONS' => Extensions::REJECTION_REASON_HEADER

    assert_equal 409, last_response.status
-    assert_equal 'limits_exceeded', last_response.header['3scale-rejection-reason']
+    assert_equal 'limits_exceeded', last_response.headers['3scale-rejection-reason']


https://github.com/rack/rack/blob/main/CHANGELOG.md#removed-1

jlledom · 2024-12-11T17:27:08Z

test/integration/report_test.rb

-  test 'returns 400 when param key encoding is not valid utf-8' do
-    post '/transactions.xml', "\xf0\x90\x28\xbc" => 1
-
-    assert_equal 400, last_response.status
-    assert_equal ThreeScale::Backend::NotValidData.new.to_xml, last_response.body
-  end


After rack/rack#1797, this doesn't happen anymore.

I don't quite understand how that PR fixes this behavior... but if this is now considered a valid input, maybe let's keep the test, and just change the expected response?

mayorova · 2024-12-13T09:37:01Z

Gemfile

 end

 # Default server by platform
-gem 'puma', git: 'https://github.com/3scale/puma', branch: '3scale-4.3.9'
+gem 'puma'


Not fixing the version?...

mayorova · 2024-12-13T10:51:12Z

lib/3scale/backend/logging/middleware.rb

@@ -35,7 +35,7 @@ def call(env)
            raise e
          end

-          header = ::Rack::Utils::HeaderHash.new(header)
+          header = ::Rack::Headers.new.merge(header)


Suggested change

header = ::Rack::Headers.new.merge(header)

header = ::Rack::Headers[header]

mayorova · 2024-12-13T10:53:56Z

lib/3scale/backend/listener.rb

-          param_value = input_params[p]
-          if !param_value.nil? && !param_value.valid_encoding?
-            halt 400, ThreeScale::Backend::NotValidData.new.to_xml
+      def check_params_encoding!(input_params)


So, you're now checking all parameters, not only the list predefined in REPORT_EXPECTED_PARAMS ?

mayorova · 2024-12-13T10:59:24Z

lib/3scale/backend/rack/exception_catcher.rb

            delete_sinatra_error! env
            resp = respond_with 400, Backend::BadRequest.new.to_xml
-          elsif resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)
+          else resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)


Suggested change

else resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)

elsif resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)

jlledom mentioned this pull request Dec 11, 2024

build(deps): bump sinatra, rack, rack-test, sinatra-contrib, hiredis-client and falcon #410

Closed

jlledom self-assigned this Dec 11, 2024

jlledom added 11 commits December 11, 2024 17:17

Bump sinatra to 4.1.1

860e08b

Install rackup

90bb16f

Needed by yabeda-prometheus after upgrading rack to >= 3.0

Fork rspec_api_documentation

b7da548

Is not compatible with Rack 3 and apparently the gem is abandoned

Update falcon

d293919

Update hiredis-client

dc3c3f5

Fix typo

ea9ce11

Fix breaking change: replace header by headers

15315aa

Breaking: Utils::HeaderHash renamed to Headers

37ec866

Catch all invalid UTF-8 errors

25d53c9

Fix test suite

5c2213e

A failing test is removed because the same scenario now returns 403, but I don't think it's important

jlledom force-pushed the bump-sinatra-4 branch from 33f7d4a to 5c2213e Compare December 11, 2024 16:18

jlledom marked this pull request as ready for review December 11, 2024 16:38

jlledom requested review from eguzki, mayorova, akostadinov and lvillen and removed request for eguzki December 11, 2024 16:48

jlledom commented Dec 11, 2024

View reviewed changes

mayorova reviewed Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump Sinatra to 4.1.1 #411

Bump Sinatra to 4.1.1 #411

jlledom commented Dec 11, 2024 •

edited

Loading

jlledom Dec 11, 2024

mayorova Dec 13, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

jlledom Dec 11, 2024

mayorova Dec 13, 2024

mayorova Dec 13, 2024

mayorova Dec 13, 2024

mayorova Dec 13, 2024

mayorova Dec 13, 2024

		@@ -411,8 +406,7 @@ def do_api_method(method_name)

		raise_provider_key_error(params) if blank?(provider_key)

		# no need to check params key encoding. Sinatra framework does it for us.

		REQUIRED_ENDPOINT_HEADERS = %w[access-control-allow-origin access-control-expose-headers]
		REQUIRED_OPTIONS_HEADERS = REQUIRED_ENDPOINT_HEADERS + %w[access-control-allow-methods access-control-allow-headers]

	header = ::Rack::Headers.new.merge(header)
	header = ::Rack::Headers[header]

	else resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)
	elsif resp_body.start_with?(INVALID_PERCENT_ENCODING_ERR_MSG)

Bump Sinatra to 4.1.1 #411

Are you sure you want to change the base?

Bump Sinatra to 4.1.1 #411

Conversation

jlledom commented Dec 11, 2024 • edited Loading

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

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

jlledom commented Dec 11, 2024 •

edited

Loading