#11 Merged at 9891449
Repository
rbuckland
Branch
default
Repository
asomov
Branch
default
Author
  1. Ramon Buckland
Reviewers
Description

This adds a new LoadingConfig, which introduces an option to enforce no duplicate keys in maps. 3 tests were added with the addition.

  • defaultConfigurationNoErrorsWithDuplicates() - that a file with duplicates does not error (current behaviour)
  • errorOnDuplicateKeys() - that a file with duplicates and the LoadingConfig.setAllowDuplicateKeys(false) does error
  • defaultConfigUniqueKeysWorks() - that a file with no duplicates also works with the new default config

This PR resolves issue #337

The default behaviour is left as "allow duplicates" for potential backwards compatibility. It is noted that "no" original tests failed when the "switch" is made to reject duplicates by default.

  • LoadingConfig can now be used as future extension to support customising the load behaviour where required.
  • LoadingConfig adds this support for "switching" behaviour of rejecting duplicate map keys.
  • All the original API and default behaviour has been preserved.
  • All the tests function as expected.

  • The new Test for Duplicate Key checks shows how to control the behaviour.

Specifically

  @Test
  public void errorOnDuplicateKeys() {
      thrown.expect(org.yaml.snakeyaml.constructor.ConstructorException.class);
      thrown.expectMessage("duplicate key: someitem");
      String input = Util.getLocalResource("issues/issue337-duplicate-keys.yaml");
      LoadingConfig lc = new LoadingConfig();
      lc.setAllowDuplicateKeys(false);
      Yaml yaml = new Yaml(lc);
      MapProvider<String, FooEntry> testdata = yaml.loadAs(input, MapProvider.class);
  }

This change also ensures that the map that is generated, is mutable; can accept duplicate (overriding) keys, after processing.

Please refer to old PR https://bitbucket.org/asomov/snakeyaml/pull-requests/10 for background information.

  • Commit status
  • Issues #337: Option to disallow duplicate keys resolved

Comments (11)

  1. Andrey Somov repo owner

    Concerning the public API.

    1. The new class should be called LoaderOptions to keep the same pattern as with DumperOptions.
    2. On the other hand we can use it as an argument for Constructor. It will be used differently then DumperOptions. Then we keep the name and add constructors to all Constructors with default loader config.

    I am not sure which approach is better.

    P.S. If I want to experiment with your repository without merging the code then I get 4 (!) heads. Do you really need them all ?

  2. Ramon Buckland author

    On (1/2) Yes I thought calling it LoaderOptions at first, but did consider the fact that it was "configuration" to alter the behaviour. to be honest, it it possibly better to rename it LoaderOptions, because that is consistent :-)

    Regarding the 4 heads :-) .. yes you expose my complete lack of experience with mercurial

    What I was trying to do was remove commits (in git, a rebase, or a "reset") of code that was no longer required. (let me see if I can correct that)

  3. maslovalex

    If we go this way (which I am not sure yet) we have to state explicitly in JavaDoc and maybe also somewhere in Documentation that "Key uniqueness can only be guaranteed for Maps (default maps), but not for SortedMap,Properties,JavaBeans. And don't forget that overriding createDefaultMap() requires also to re-implement this functionality."

    And still IMHO current implementation is not consistent with the image developer gets doing this:

    LoadingConfig lc = new LoadingConfig();
    lc.setAllowDuplicateKeys(false);
    Yaml yaml = new Yaml(lc);
    

    As an example you can try this patch:

    diff --git a/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java b/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    --- a/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    +++ b/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    @@ -123,6 +123,18 @@
         }
    
         @Test
    +    public void errorOnDuplicateKeysInJavaBeanProperty() {
    +       thrown.expect(org.yaml.snakeyaml.constructor.ConstructorException.class);
    +       thrown.expectMessage("duplicate key: name");
    +       String input = Util.getLocalResource("issues/issue337-duplicate-keys-javabean-property.yaml");
    +       LoadingConfig lc = new LoadingConfig();
    +       lc.setAllowDuplicateKeys(false);
    +       Yaml yaml = new Yaml(lc);
    +       MapProvider<String, FooEntry> testdata = yaml.loadAs(input, MapProvider.class);
    +       assertEquals("no-dup-keys", testdata.getName());
    +    }
    +
    +    @Test
         public void defaultConfigUniqueKeysWorks() {
             String input = Util.getLocalResource("issues/issue337-duplicate-keys-no-dups.yaml");
             Yaml yaml = new Yaml();
    diff --git a/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml b/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml
    new file mode 100644
    --- /dev/null
    +++ b/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml
    @@ -0,0 +1,9 @@
    +name: no-dup-keys
    +map:
    +  someitem: !!org.yaml.snakeyaml.issues.issue337.DuplicateKeyTest$FooEntry
    +    id: aead4b16-4b61-4eff-b241-6eff26eaa778
    +    url: http://test.com/sample/v1/foobar
    +  someitem1: !!org.yaml.snakeyaml.issues.issue337.DuplicateKeyTest$FooEntry
    +    id: daaf8911-36e4-4e92-86ea-eb77ac2c1e91
    +    url: http://test.com/sample/v55/foobar
    +name: has-dup-keys
    \ No newline at end of file
    

    it fails because expected exception is not thrown (but there is a duplicate key in yaml file). And also testdata.getName() is not no-dup-keys.

  4. maslovalex

    OK, guys, how about this one? It should cover much more cases, patch applies to the HEAD of @Ramon Buckland's fork (contains patch from previous comment) Maybe more test would be nice. Actually test for merging is needed (not sure merge kills all duplicates. If so, no need to check for duplication after merging).

    And may be then allowDuplicateKeys need to be moved to SafeConstructor, because extending just BaseConstructor will not provide this functionality.

    # HG changeset patch
    # Parent  4f015955b1444b15e556365f20be1d4816936c52
    
    diff --git a/src/main/java/org/yaml/snakeyaml/constructor/BaseConstructor.java b/src/main/java/org/yaml/snakeyaml/constructor/BaseConstructor.java
    --- a/src/main/java/org/yaml/snakeyaml/constructor/BaseConstructor.java
    +++ b/src/main/java/org/yaml/snakeyaml/constructor/BaseConstructor.java
    @@ -16,7 +16,6 @@
     package org.yaml.snakeyaml.constructor;
    
     import java.lang.reflect.Array;
    -import java.util.AbstractMap;
     import java.util.ArrayList;
     import java.util.Collection;
     import java.util.EnumMap;
    @@ -389,12 +388,7 @@
                             new RecursiveTuple<Map<Object, Object>, RecursiveTuple<Object, Object>>(
                                     mapping, new RecursiveTuple<Object, Object>(key, value)));
                 } else {
    -
    -                if (! isAllowDuplicateKeys() && mapping.containsKey(key)) {
    -                    throw new IllegalStateException("duplicate key: " + key);
    -                } else {
    -                    mapping.put(key, value);
    -                }
    +                mapping.put(key, value);
                 }
             }
         }
    diff --git a/src/main/java/org/yaml/snakeyaml/constructor/SafeConstructor.java b/src/main/java/org/yaml/snakeyaml/constructor/SafeConstructor.java
    --- a/src/main/java/org/yaml/snakeyaml/constructor/SafeConstructor.java
    +++ b/src/main/java/org/yaml/snakeyaml/constructor/SafeConstructor.java
    @@ -22,6 +22,7 @@
     import java.util.Calendar;
     import java.util.Collections;
     import java.util.HashMap;
    +import java.util.HashSet;
     import java.util.Iterator;
     import java.util.LinkedHashMap;
     import java.util.List;
    @@ -73,9 +74,33 @@
                 node.setValue(mergeNode(node, true, new HashMap<Object, Integer>(),
                         new ArrayList<NodeTuple>()));
             }
    +        if(!isAllowDuplicateKeys()) {
    +            checkForDuplicateKeys(node);
    +        }
         }
    
    -    /**
    +    protected void checkForDuplicateKeys(MappingNode node) {
    +        List<NodeTuple> nodeValue = (List<NodeTuple>) node.getValue();
    +        Set<Object> keys = new HashSet<Object>(nodeValue.size());
    +        for (NodeTuple tuple : nodeValue) {
    +            Node keyNode = tuple.getKeyNode();
    +            Object key = constructObject(keyNode);
    +            if (key != null) {
    +                try {
    +                    key.hashCode();// check circular dependencies
    +                } catch (Exception e) {
    +                    throw new ConstructorException("while constructing a mapping",
    +                            node.getStartMark(), "found unacceptable key " + key, tuple
    +                                    .getKeyNode().getStartMark(), e);
    +                }
    +            }
    +            if(!keys.add(key)) {
    +                throw new IllegalStateException("duplicate key: " + key);
    +            }
    +        }
    +        }
    +
    +        /**
          * Does merge for supplied mapping node.
          *
          * @param node
    diff --git a/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java b/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    --- a/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    +++ b/src/test/java/org/yaml/snakeyaml/issues/issue337/DuplicateKeyTest.java
    @@ -123,6 +123,18 @@
         }
    
         @Test
    +    public void errorOnDuplicateKeysInJavaBeanProperty() {
    +       thrown.expect(org.yaml.snakeyaml.constructor.ConstructorException.class);
    +       thrown.expectMessage("duplicate key: name");
    +       String input = Util.getLocalResource("issues/issue337-duplicate-keys-javabean-property.yaml");
    +       LoadingConfig lc = new LoadingConfig();
    +       lc.setAllowDuplicateKeys(false);
    +       Yaml yaml = new Yaml(lc);
    +       MapProvider<String, FooEntry> testdata = yaml.loadAs(input, MapProvider.class);
    +       assertEquals("no-dup-keys", testdata.getName());
    +    }
    +
    +    @Test
         public void defaultConfigUniqueKeysWorks() {
             String input = Util.getLocalResource("issues/issue337-duplicate-keys-no-dups.yaml");
             Yaml yaml = new Yaml();
    diff --git a/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml b/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml
    new file mode 100644
    --- /dev/null
    +++ b/src/test/resources/issues/issue337-duplicate-keys-javabean-property.yaml
    @@ -0,0 +1,9 @@
    +name: no-dup-keys
    +map:
    +  someitem: !!org.yaml.snakeyaml.issues.issue337.DuplicateKeyTest$FooEntry
    +    id: aead4b16-4b61-4eff-b241-6eff26eaa778
    +    url: http://test.com/sample/v1/foobar
    +  someitem1: !!org.yaml.snakeyaml.issues.issue337.DuplicateKeyTest$FooEntry
    +    id: daaf8911-36e4-4e92-86ea-eb77ac2c1e91
    +    url: http://test.com/sample/v55/foobar
    +name: has-dup-keys
    \ No newline at end of file
    
    1. Manfredi Giordano

      Thank you for your answer.

      Do you intend to raise a more specific expection, something like DuplicateKeyException? In my opinion even using the already existing YAMLException would be fine, and arguably more appropriate than java.lang.IllegalStateException