From 70611c6ef063b8adde65f13ecaef7ee90fc2aede Mon Sep 17 00:00:00 2001 From: Ben Symonds Date: Tue, 28 Jan 2020 11:46:39 +0000 Subject: [PATCH 1/4] Remove some unnecessary comments I think the code is obvious enough here. --- lib/pliny/log.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/pliny/log.rb b/lib/pliny/log.rb index de4fb168..6f98be86 100644 --- a/lib/pliny/log.rb +++ b/lib/pliny/log.rb @@ -124,7 +124,6 @@ def log_to_stream(stream, data, &block) end def quote_string(k, v) - # try to find a quote style that fits if !v.include?('"') %{#{k}="#{v}"} elsif !v.include?("'") @@ -140,7 +139,7 @@ def unparse(attrs) def unparse_pair(k, v) v = v.call if v.is_a?(Proc) - # only quote strings if they include whitespace + if v == nil nil elsif v == true From 4476025933265198744cc8c881882dc488135643 Mon Sep 17 00:00:00 2001 From: Ben Symonds Date: Tue, 28 Jan 2020 11:48:24 +0000 Subject: [PATCH 2/4] Remove stubbing of test object The problem with the stubbing is that it allows the implementation to output extra _unexpected_ stuff, in addition to what the test expects, without causing a test failure. By removing it we ensure a test will fail if unexpected stuff is output. --- spec/log_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/log_spec.rb b/spec/log_spec.rb index 13603c69..7368e52d 100644 --- a/spec/log_spec.rb +++ b/spec/log_spec.rb @@ -5,7 +5,6 @@ @io = StringIO.new Pliny.stdout = @io Pliny.stderr = @io - allow(@io).to receive(:print) end after do From e8fb35272fdfe505015b5514bfbfc28f29f84fae Mon Sep 17 00:00:00 2001 From: Ben Symonds Date: Tue, 28 Jan 2020 11:52:24 +0000 Subject: [PATCH 3/4] Add some test for existing "unparsing" behaviour I couldn't see explicit tests for this stuff anywhere (some of it is indirectly tested higher up in the file). --- spec/log_spec.rb | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/spec/log_spec.rb b/spec/log_spec.rb index 7368e52d..43839083 100644 --- a/spec/log_spec.rb +++ b/spec/log_spec.rb @@ -129,4 +129,52 @@ end end end + + describe "unparsing" do + it "removes nils from log" do + expect(@io).to receive(:print).with("\n") + + Pliny.log(foo: nil) + end + + it "leaves bare keys for true values" do + expect(@io).to receive(:print).with("foo\n") + + Pliny.log(foo: true) + end + + it "truncates floats" do + expect(@io).to receive(:print).with("foo=3.142\n") + + Pliny.log(foo: Math::PI) + end + + it "outputs times in ISO8601 format" do + expect(@io).to receive(:print).with("foo=2020-01-01T00:00:00Z\n") + + Pliny.log(foo: Time.utc(2020)) + end + + it "quotes strings that contain spaces" do + expect(@io).to receive(:print).with("foo=\"string with spaces\"\n") + + Pliny.log(foo: "string with spaces") + end + + it "by default interpolates objects into strings" do + expect(@io).to receive(:print).with("foo=message\n") + expect(@io).to receive(:print).with("foo=42\n") + expect(@io).to receive(:print).with("foo=bar\n") + + Pliny.log(foo: StandardError.new("message")) + Pliny.log(foo: 42) + + klass = Class.new do + def to_s + "bar" + end + end + Pliny.log(foo: klass.new) + end + end end From 71ad622fe1bbbbe16634650740ec643702716692 Mon Sep 17 00:00:00 2001 From: Ben Symonds Date: Tue, 28 Jan 2020 11:57:57 +0000 Subject: [PATCH 4/4] Ensure all strings with spaces are quoted in logs The current logic covers the case when the log data is already a `String` instance, but not when the data is some other object that, when interpolated into the log string, results in a string containing spaces. The example that led me here is where an exception is logged and the exception's message contains spaces. The exception gets interpolated into the string and has `to_s` called on it. [`Exception#to_s`](https://ruby-doc.org/core-2.6.5/Exception.html#method-i-to_s) returns the message, so the message (with its spaces) go into the log line, but it's bypassed the logic that adds quotes. I used `"#{v}"` instead of `v.to_s` because it seems there is a [subtle difference in their behaviour](https://github.com/ruby/spec/blob/master/language/string_spec.rb#L147-L158), to cover the rare case where `to_s` doesn't return a string. I think we want to keep the existing behaviour here. --- CHANGELOG.md | 1 + lib/pliny/log.rb | 9 ++++++--- spec/log_spec.rb | 14 ++++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1406172e..73ad2e98 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ## [Unreleased] ### Changed + - Ensure all strings with spaces are quoted in logs. ([#332](https://github.com/interagent/pliny/pull/332)) - Allow ActiveSupport 5 or 6. ([#331](https://github.com/interagent/pliny/pull/331)) ### Fixed diff --git a/lib/pliny/log.rb b/lib/pliny/log.rb index 6f98be86..42898460 100644 --- a/lib/pliny/log.rb +++ b/lib/pliny/log.rb @@ -146,12 +146,15 @@ def unparse_pair(k, v) k elsif v.is_a?(Float) "#{k}=#{format("%.3f", v)}" - elsif v.is_a?(String) && v =~ /\s/ - quote_string(k, v) elsif v.is_a?(Time) "#{k}=#{v.iso8601}" else - "#{k}=#{v}" + v = "#{v}" + if v =~ /\s/ + quote_string(k, v) + else + "#{k}=#{v}" + end end end end diff --git a/spec/log_spec.rb b/spec/log_spec.rb index 43839083..0ad43ef4 100644 --- a/spec/log_spec.rb +++ b/spec/log_spec.rb @@ -176,5 +176,19 @@ def to_s end Pliny.log(foo: klass.new) end + + it "quotes strings that are generated from object interpolation" do + expect(@io).to receive(:print).with("foo=\"message with space\"\n") + expect(@io).to receive(:print).with("foo=\"bar with space\"\n") + + Pliny.log(foo: StandardError.new("message with space")) + + klass = Class.new do + def to_s + "bar with space" + end + end + Pliny.log(foo: klass.new) + end end end