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

Rails 7.1's new BroadcastLogger causes TypeError in Vonage::Config #288

Closed
ohbarye opened this issue Oct 5, 2023 · 9 comments
Closed

Rails 7.1's new BroadcastLogger causes TypeError in Vonage::Config #288

ohbarye opened this issue Oct 5, 2023 · 9 comments
Assignees

Comments

@ohbarye
Copy link
Contributor

ohbarye commented Oct 5, 2023

Problem

Rails 7.1 introduced ActiveSupport::BroadcastLogger and it violates vonage's type definition.

ref rails/rails#48615

Reproduce procedure

Call Vonage::Client.new with rails 7.1.0 and vonage 7.16.0.

Expected Behavior

No error happens.

Actual Behavior

It raises TypeError.

     TypeError:
       Parameter 'logger': Expected type T.nilable(T.any(Logger, Vonage::Logger)), got type ActiveSupport::BroadcastLogger with hash -2750353119102352789
       Caller: /usr/local/bundle/ruby/3.2.0/gems/vonage-7.16.0/lib/vonage/config.rb:15
       Definition: /usr/local/bundle/ruby/3.2.0/gems/vonage-7.16.0/lib/vonage/config.rb:134
     # /usr/local/bundle/ruby/3.2.0/gems/sorbet-runtime-0.5.11039/lib/types/configuration.rb:296:in `call_validation_error_handler_default'

Cause

That's because vonage's type definition does not allow a class that does not inherits Logger unlike ActiveSupport::Logger .

ref

sig { params(logger: T.nilable(T.any(::Logger, Vonage::Logger))).returns(T.nilable(Vonage::Logger)) }
def logger=(logger)
@logger = T.let(Logger.new(logger), T.nilable(Vonage::Logger))

Other information

Here is the difference of Rails.logger between 7.0.8 and 7.1.0.

# 7.0.8
$ bin/rails c
> ::Rails.logger.class
=> ActiveSupport::Logger

> ActiveSupport::Logger.ancestors
=> 
[ActiveSupport::Logger,
 ActiveSupport::LoggerThreadSafeLevel,
 ActiveSupport::LoggerSilence,
 Logger,
 AwesomePrint::Logger,
 Logger::Severity,
 ActiveSupport::Dependencies::RequireDependency,
 ActiveSupport::ToJsonWithActiveSupportEncoder,
 Object,
 PP::ObjectMixin,
 ActiveSupport::Tryable,
 JSON::Ext::Generator::GeneratorMethods::Object,
 DEBUGGER__::TrapInterceptor,
 Kernel,
 BasicObject]
# 7.1.0
$ bin/rails c
> Rails.logger.class
=> ActiveSupport::BroadcastLogger

> ActiveSupport::BroadcastLogger.ancestors
=> 
[ActiveSupport::BroadcastLogger,                                                   
 ActiveSupport::LoggerThreadSafeLevel,                                             
 ActiveSupport::LoggerSilence,                                                     
 ActiveSupport::Dependencies::RequireDependency,                                   
 ActiveSupport::ToJsonWithActiveSupportEncoder,                                    
 Object,                                                                           
 PP::ObjectMixin,                                                                  
 ActiveSupport::Tryable,                                                           
 JSON::Ext::Generator::GeneratorMethods::Object,                                   
 DEBUGGER__::TrapInterceptor,                                                      
 Kernel,                                                                           
 BasicObject]
@ohbarye
Copy link
Contributor Author

ohbarye commented Oct 5, 2023

Although I'm unfamiliar with Sorbet, I can send an easy patch to change the type definition like the one below. I don't know if it's valid and appropriate.

 sig { params(logger: T.nilable(
    defined?(ActiveSupport::BroadcastLogger) ? 
      T.any(::Logger, Vonage::Logger, ActiveSupport::BroadcastLogger) # rails 7.1.0 introduced 
    : T.any(::Logger, Vonage::Logger)
  )).returns(T.nilable(Vonage::Logger)) } 
 def logger=(logger) 
 end

@ohbarye
Copy link
Contributor Author

ohbarye commented Oct 6, 2023

Just to let you know, I found a runtime check behavior configuration of Sorbet.

https://sorbet.org/docs/tconfiguration#errors-from-invalid-method-calls

After executing the following code, my code stopped raising a TypeError. I feel weird about writing a config for Sorbet even though I don't use Sorbet as a user. 😓

T::Configuration.call_validation_error_handler = lambda do |signature, opts|
  puts opts[:message]
end

Or, SORBET_RUNTIME_DEFAULT_CHECKED_LEVEL=tests environment variable seems to disable raising an error.

https://sorbet.org/docs/tconfiguration#sorbet_runtime_default_checked_level

@superchilled superchilled self-assigned this Oct 6, 2023
@superchilled
Copy link
Contributor

Thanks @ohbarye. I'll look into getting a fix out for this.

@matedemorphy
Copy link

same issue here, is there any workarround?

@superchilled
Copy link
Contributor

@matedemorphy I'm working on a PR which basically implements @ohbarye's solution. I just need to do a bit more testing, but it should hopefully be released later today.

@superchilled
Copy link
Contributor

@ohbarye @matedemorphy the PR is now merged and released as v7.16.1 and available on RubyGems

@superchilled
Copy link
Contributor

Fixed in #290

@ohbarye
Copy link
Contributor Author

ohbarye commented Oct 11, 2023

@superchilled Thank you for your quick fix!
I confirmed that v7.16.1 has resolved the type error on our codebase.

@superchilled
Copy link
Contributor

Thanks @ohbarye 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants