From 4eecc559a4305c1c1541407765f1c768c78ebbba Mon Sep 17 00:00:00 2001
From: Holger Just <hello@holgerjust.de>
Date: Fri, 30 Jun 2017 19:27:43 +0200
Subject: [PATCH] Transform list of forbidden_keys in
 Rackstash::Fields::Hash.new to a frozen Set

We then expose this frozen Set on the `forbidden_keys` attribute.
---
 lib/rackstash/fields/hash.rb       | 16 +++++++----
 spec/rackstash/fields/hash_spec.rb | 43 ++++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 16 deletions(-)

diff --git a/lib/rackstash/fields/hash.rb b/lib/rackstash/fields/hash.rb
index d078d22..fd2bdfe 100644
--- a/lib/rackstash/fields/hash.rb
+++ b/lib/rackstash/fields/hash.rb
@@ -8,17 +8,23 @@ require 'rackstash/fields/abstract_collection'
 module Rackstash
   module Fields
     class Hash < AbstractCollection
+      # @return [Set<String>] a frozen list of strings which are not allowed to
+      #   be used as keys in this hash.
+      attr_reader :forbidden_keys
+
       # @param forbidden_keys [Set<String>,::Array<String>] a list of strings
       #   which are not allowed to be used as keys in this hash
       def initialize(forbidden_keys: EMPTY_SET)
         @raw = Concurrent::Hash.new
 
-        if forbidden_keys.is_a?(Set)
-          forbidden_keys = forbidden_keys.dup.freeze unless forbidden_keys.frozen?
-          @forbidden_keys = forbidden_keys
-        else
-          @forbidden_keys = Set[*forbidden_keys].freeze
+        unless forbidden_keys.is_a?(Set) &&
+               forbidden_keys.frozen? &&
+               forbidden_keys.all? { |key| String === key && key.frozen? }
+          forbidden_keys = Set.new(forbidden_keys) { |key| utf8_encode key }
+          forbidden_keys.freeze
         end
+
+        @forbidden_keys = forbidden_keys
       end
 
       # Retrieve a stored value from a given `key`
diff --git a/spec/rackstash/fields/hash_spec.rb b/spec/rackstash/fields/hash_spec.rb
index 9984f28..71f5d35 100644
--- a/spec/rackstash/fields/hash_spec.rb
+++ b/spec/rackstash/fields/hash_spec.rb
@@ -18,12 +18,35 @@ describe Rackstash::Fields::Hash do
 
     it 'accepts forbidden_keys as an Array' do
       hash = Rackstash::Fields::Hash.new(forbidden_keys: ['field'])
-      expect(hash.instance_variable_get('@forbidden_keys')).to be_a Set
+
+      expect(hash.forbidden_keys)
+        .to be_a(Set)
+        .and be_frozen
+        .and all(
+          be_frozen.and be_a String
+        )
     end
 
     it 'accepts forbidden_keys as a Set' do
-      hash = Rackstash::Fields::Hash.new(forbidden_keys: Set['field'])
-      expect(hash.instance_variable_get('@forbidden_keys')).to be_a Set
+      forbidden_keys = Set['field']
+      hash = Rackstash::Fields::Hash.new(forbidden_keys: forbidden_keys)
+
+      expect(hash.forbidden_keys)
+        .to be_a(Set)
+        .and be_frozen
+        .and all(
+          be_frozen.and be_a String
+        )
+
+      # We create a new set without affecting the passed one
+      expect(hash.forbidden_keys).not_to equal forbidden_keys
+    end
+
+    it 'accepts forbidden_keys as a frozen Set' do
+      forbidden_keys = Set['field'.freeze].freeze
+      hash = Rackstash::Fields::Hash.new(forbidden_keys: forbidden_keys)
+
+      expect(hash.forbidden_keys).to equal forbidden_keys
     end
   end
 
@@ -73,14 +96,12 @@ describe Rackstash::Fields::Hash do
       it 'denies setting a forbidden field' do
         expect { hash[:forbidden] = 'value' }.to raise_error ArgumentError
         expect { hash['forbidden'] = 'value' }.to raise_error ArgumentError
-      end
 
-      it 'ignores non string-values in forbidden_keys' do
-        expect { hash[:foo] = 'value' }.not_to raise_error
-        expect { hash['foo'] = 'value' }.not_to raise_error
-        expect { hash[42] = 'value' }.not_to raise_error
-        expect { hash['42'] = 'value' }.not_to raise_error
-        expect { hash[:'42'] = 'value' }.not_to raise_error
+        expect { hash[:foo] = 'value' }.to raise_error ArgumentError
+        expect { hash['foo'] = 'value' }.to raise_error ArgumentError
+        expect { hash[42] = 'value' }.to raise_error ArgumentError
+        expect { hash['42'] = 'value' }.to raise_error ArgumentError
+        expect { hash[:'42'] = 'value' }.to raise_error ArgumentError
       end
 
       it 'returns nil when accessing forbidden fields' do
@@ -330,7 +351,7 @@ describe Rackstash::Fields::Hash do
 
     it 'checks if a key is forbidden' do
       expect(hash.forbidden_key?('forbidden')).to be true
-      expect(hash.forbidden_key?('foo')).to be false
+      expect(hash.forbidden_key?('foo')).to be true
     end
   end