Psych does not complain about duplicate keys

[rehevkor5]

Psych does not complain or raise an error when it encounters a duplicate key. Instead, it happily overwrites the previous value of the key. This is in contrast to the YAML spec, which maintains that the keys must be unique.

Agree/disagree that this is a problem?

[tenderlove]

Well, Psych maintains the behavior that Syck had for this. Until now, nobody has had a problem with the way it works. I don't mind changing it, but it will not be backwards compatible.

[rehevkor5]

Got it. I don't suppose Psych already has a place for configuration options, does it? I don't see one. Would that be a desirable approach? Then, for example, a Rails app in development mode could be more strict/fragile about the YAML than a production deployment.

[Nitrodist]

It's been about 2 years since this was brought up. Let's do it.

[Nitrodist]

So according to this tweet[0], we should add an option to enforce this behaviour (maybe the option is called 'strict').

In my opinion, with this option enabled we should raise a Psych::SyntaxError if we encounter the duplicate keys. This error is already a known error that can be raised on Psych.parse[1].

Psych has a number of interfaces that we would want this behaviour added to (cribbed from the docs[2]):

• (Object) Pysch.load(yaml, filename = nil)
• (Object) Psych.load_documents(yaml, &block)
• (Object) Psych.load_file(filename)
• (Object) Psych.load_stream(yaml, filename = nil)
• (Object) Psych.parse(yaml, filename = nil)
• (Object) Psych.parse_file(filename)
• (Object) Psych.parse_stream(yaml, filename = nil, &block)

Right now none of these methods take an options hash. We can expand the signature with an optional options hash. I think this the right course of action, does anybody have any other suggestion?

Also, consider that the generation of YAML already has an options hash (in Psych.dump, but not Psych.dump_stream), so this isn't an unexpected behaviour, in my opinion.

[0] - https://twitter.com/tenderlove/status/468781805940641793
[1] - http://rubydoc.info/stdlib/psych/1.9.3/Psych#parse-class_method
[2] - http://rubydoc.info/stdlib/psych/1.9.3/Psych

[Nitrodist]

Basically I'm proposing changing from this:

Psych.parse(yaml, filename = nil)

to this:

Psych.parse(yaml, filename = nil, options = {})

(and on all the other methods listed above)

[tenderlove]

@Nitrodist 👍 I think it's good. For a major release, I think we should change the filename to be an option like Psych.load(yaml, filename: name, strict: true) (but that's for a breaking major release). For now, lets add the options hash.

[Nitrodist]

We're having a hacknight tomorrow in Toronto. I'm hoping to work on a solution with @nwjsmith and have it ready by Thursday. 👐

[perryventas]

@Nitrodist What was the outcome of your hacking night? 😄

[Nitrodist]

@coin3d I was able to get it to 'work' but its efficiency left much to be desired. This is what I had:

diff --git lib/psych/handlers/document_stream.rb lib/psych/handlers/document_stream.rb
index e429993..bf04896 100644
--- lib/psych/handlers/document_stream.rb
+++ lib/psych/handlers/document_stream.rb
@@ -17,6 +17,18 @@ module Psych
         @last.implicit_end = implicit_end
         @block.call pop
       end
+
+      def end_mapping
+        mapping = pop
+        keys = []
+        mapping.children.each_slice(2) do |(key_scalar, _)|
+          next if key_scalar.is_a?(Psych::Nodes::Sequence) or key_scalar.is_a?(Psych::Nodes::Alias) or key_scalar.is_a?(Psych::Nodes::Mapping)
+          key = key_scalar.value
+          raise Psych::Exception, "Same key exists on this level" if keys.include? key
+          keys << key
+        end
+        mapping
+      end
     end
   end
 end
diff --git test/psych/test_hash.rb test/psych/test_hash.rb
index dac7f8d..b3ecbc0 100644
--- test/psych/test_hash.rb
+++ test/psych/test_hash.rb
@@ -21,6 +21,16 @@ module Psych
       assert_equal X, x.class
     end

+    def test_error_on_same_key
+      assert_raises(Psych::Exception) do
+        Psych.load <<-EOF
+        -
+          same_key: 'value'
+          same_key: 'value'
+        EOF
+      end
+    end
+
     def test_self_referential
       @hash['self'] = @hash
       assert_cycle(@hash)

[wteuber]

I would have liked a warning or error message like this... I now used https://github.com/adrienverge/yamllint for checking for duplicate keys.

[schmurfy]

What is the state of this ? I don't really mind or care about the default but there should be a way to raise an error on duplicate keys, the consequences of a duplicate key are rather harsh since the first one will simply be entirely overwritten.

[imoverclocked]

If you use a hash instead of a list then the performance shouldn't suffer nearly as much:

+      def end_mapping
+        mapping = pop
+        keys = {}
+        mapping.children.each_slice(2) do |(key_scalar, _)|
+          next if key_scalar.is_a?(Psych::Nodes::Sequence) or key_scalar.is_a?(Psych::Nodes::Alias) or key_scalar.is_a?(Psych::Nodes::Mapping)
+          key = key_scalar.value
+          raise Psych::Exception, "Same key exists on this level" if keys.key? key
+          keys[key] = nil
+        end
+        mapping
+      end

any takers for testing and a PR?

[alex-harvey-z3q]

Is this likely to be implemented any time soon?

[wteuber]

@tenderlove you mentioned backwards compatibility...
Instead raising an error, just (optionally) printing out a warning wouldn't brake backwards compatibility but help many people manage their yaml files. What do you think?