Copilot commented on code in PR #3634: URL: https://github.com/apache/thrift/pull/3634#discussion_r3566537944
########## test/rb/Gemfile: ########## @@ -5,10 +5,19 @@ source "https://rubygems.org"; gem 'thrift', path: '../../lib/rb' gem 'rack', '~> 2.2', '>= 2.2.23' +gem 'puma', '~> 6.6' gem 'test-unit', '~> 3.2', '>= 3.2.7' +# Only required for tests with Rack 2.x on Ruby 4.0+ +gem 'cgi' +gem 'ostruct' + group :thin_http_server do gem 'thin', '~> 1.7', '>= 1.7.2' end +group :falcon_http_server, optional: true do + gem 'falcon', '~> 0.51' +end Review Comment: `group :falcon_http_server, optional: true` is not supported by older Bundler versions, and this repo pins Bundler to 2.2.34 in `test/rb/Gemfile.lock` ("BUNDLED WITH"). With Bundler 2.2 this is likely to fail Gemfile evaluation (or behave unexpectedly), breaking `bundle install` for the integration harness. Consider expressing the optionality in plain Ruby (e.g., gate Falcon on Ruby >= 3.3) so the Gemfile remains compatible with the pinned Bundler while still avoiding installing Falcon on older Rubies. ########## lib/rb/lib/thrift/server/rack_application.rb: ########## @@ -0,0 +1,83 @@ +# frozen_string_literal: true +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +require 'rack' +require 'stringio' +require 'thrift/multiplexed_processor' +require 'thrift/transport/io_stream_transport' +require 'thrift/transport/memory_buffer_transport' +require 'thrift/types' + +module Thrift + class RackApplication + THRIFT_HEADER = "application/x-thrift" + + def initialize(processor, protocol_factory = BinaryProtocolFactory.new) + @processor = processor + @protocol_factory = protocol_factory + end + + def call(env) + request = Rack::Request.new(env) + + if self.class.valid_thrift_request?(request) + self.class.successful_request(request, @processor, @protocol_factory).finish + else + self.class.failed_request.finish + end + end + + def self.mapped(path, processor, protocol_factory = BinaryProtocolFactory.new) + Rack::Builder.new do + use Rack::CommonLogger + use Rack::ShowExceptions + use Rack::Lint + use Rack::ContentLength + map path do + run RackApplication.new(processor, protocol_factory) + end + end + end + + def self.successful_request(rack_request, processor, protocol_factory) + response = Rack::Response.new([], 200, {'content-type' => THRIFT_HEADER}) + input_protocol = request_protocol(rack_request, protocol_factory).first + output_protocol = protocol_factory.get_protocol(IOStreamTransport.new(StringIO.new, response)) + processor.process input_protocol, output_protocol Review Comment: Response headers should use the conventional `Content-Type` key (as used elsewhere in the Ruby HTTP client transport) to avoid any edge-case incompatibilities with downstream middleware or consumers that expect canonical casing. ########## lib/rb/lib/thrift/server/rack_application.rb: ########## @@ -0,0 +1,83 @@ +# frozen_string_literal: true +# +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +# + +require 'rack' +require 'stringio' +require 'thrift/multiplexed_processor' +require 'thrift/transport/io_stream_transport' +require 'thrift/transport/memory_buffer_transport' +require 'thrift/types' + +module Thrift + class RackApplication + THRIFT_HEADER = "application/x-thrift" + + def initialize(processor, protocol_factory = BinaryProtocolFactory.new) + @processor = processor + @protocol_factory = protocol_factory + end + + def call(env) + request = Rack::Request.new(env) + + if self.class.valid_thrift_request?(request) + self.class.successful_request(request, @processor, @protocol_factory).finish + else + self.class.failed_request.finish + end + end + + def self.mapped(path, processor, protocol_factory = BinaryProtocolFactory.new) + Rack::Builder.new do + use Rack::CommonLogger + use Rack::ShowExceptions + use Rack::Lint + use Rack::ContentLength + map path do + run RackApplication.new(processor, protocol_factory) + end + end + end + + def self.successful_request(rack_request, processor, protocol_factory) + response = Rack::Response.new([], 200, {'content-type' => THRIFT_HEADER}) + input_protocol = request_protocol(rack_request, protocol_factory).first + output_protocol = protocol_factory.get_protocol(IOStreamTransport.new(StringIO.new, response)) + processor.process input_protocol, output_protocol + + response + end + + def self.failed_request + Rack::Response.new(['Not Found'], 404, {'content-type' => THRIFT_HEADER}) + end Review Comment: Response headers should use the conventional `Content-Type` key (as used elsewhere in the Ruby HTTP client transport) to avoid any edge-case incompatibilities with downstream middleware or consumers that expect canonical casing. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
