Re: ruby-faraday-middleware and its dependency on ruby-safe-yaml

[Antonio Terceiro]

On Thu, Jan 26, 2023 at 02:03:39AM +0100, Daniel Leidert wrote:
> Hi,
> 
> some packages are scheduled for autoremoval due to ruby-faraday-middleware
> FTBFS. The FTBFS was easy to fix, unfortunately it introduces a dependency on
> ruby-safe-yaml. That is suboptimal because I don't think that we can fix ruby-
> safe-yaml.
> 
> I was playing around with replacing SafeYAML.load() by YAML.safe_load(). That
> leaves only the below test issues.
> 
> What do you think? Any better idea?
> 
> > Failures:
> > 
> >   1) FaradayMiddleware::ParseYaml SafeYAML options passes relevant options 
> > to SafeYAML load
> >      Failure/Error:
> >        expect(::SafeYAML).to receive(:load)
> >          .with(body, nil, options[:parser_options])
> >          .and_return(result)
> >      
> >      NameError:
> >        uninitialized constant SafeYAML
> >      
> >              expect(::SafeYAML).to receive(:load)
> >                     ^^^^^^^^^^
> >      # ./spec/unit/parse_yaml_spec.rb:65:in `block (3 levels) in <top 
> > (required)>'
> >      # 
> > /usr/share/rubygems-integration/all/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in
> >  `block (2 levels) in <top (required)>'

Should work if you change that to

        expect(::YAML).to receive(:safe_load)

or something like that

> >   2) FaradayMiddleware::ParseYaml no type matching returns false for empty 
> > body
> >      Failure/Error: expect(process('').body).to be false
> >      
> >        expected false
> >             got nil
> >      # ./spec/unit/parse_yaml_spec.rb:10:in `block (3 levels) in <top 
> > (required)>'
> >      # 
> > /usr/share/rubygems-integration/all/gems/webmock-3.18.1/lib/webmock/rspec.rb:37:in
> >  `block (2 levels) in <top (required)>'

This is due to the difference in how Psych (YAML) and SafeYAML handle
an empty string:

$ irb -ryaml -rsafe_yaml/load
>> YAML.safe_load('')
=> nil
>> SafeYAML.load('', nil, {})
=> false

Depending on what exactly uses this you might 1) just fix the test or 2)
change the YAML.safe_load call to something like

        YAML.safe_load(body) || false

The following changes make all the tests pass for me here:


diff --git a/lib/faraday_middleware/response/parse_yaml.rb 
b/lib/faraday_middleware/response/parse_yaml.rb
index bf8873b..eb0f751 100644
--- a/lib/faraday_middleware/response/parse_yaml.rb
+++ b/lib/faraday_middleware/response/parse_yaml.rb
@ -27,10 +27,10 @@ module FaradayMiddleware
   #       ...
   #     end
   class ParseYaml < ResponseMiddleware
-    dependency 'safe_yaml/load'
+    dependency 'yaml'
 
     define_parser do |body, parser_options|
-      SafeYAML.load(body, nil, parser_options || {})
+      YAML.safe_load(body, **(parser_options || {})) || false
     end
   end
 end
diff --git a/spec/unit/parse_yaml_spec.rb b/spec/unit/parse_yaml_spec.rb
index 33c0312..5afd303 100644
--- a/spec/unit/parse_yaml_spec.rb
+++ b/spec/unit/parse_yaml_spec.rb
@@ -50,7 +50,7 @@ RSpec.describe FaradayMiddleware::ParseYaml, type: :response 
do
     expect { process('{!') }.to raise_error(Faraday::ParsingError)
   end
 
-  context 'SafeYAML options' do
+  context 'YAML options' do
     let(:body) { 'a: 1' }
     let(:result) { { a: 1 } }
     let(:options) do
@@ -61,9 +61,9 @@ RSpec.describe FaradayMiddleware::ParseYaml, type: :response 
do
       }
     end
 
-    it 'passes relevant options to SafeYAML load' do
-      expect(::SafeYAML).to receive(:load)
-        .with(body, nil, options[:parser_options])
+    it 'passes relevant options to YAML safe_load' do
+      expect(::YAML).to receive(:safe_load)
+        .with(body, options[:parser_options])
         .and_return(result)
 
       response = process(body)

signature.asc
Description: PGP signature