Adding Duplicate key check in JacksonJrsTreeCodec #127
Conversation
| @@ -60,6 +62,9 @@ private JrsValue nodeFrom(JsonParser p) throws IOException | |||
| Map<String, JrsValue> values = _map(); | |||
| while (p.nextToken() != JsonToken.END_OBJECT) { | |||
| final String currentName = p.currentName(); | |||
| if(JSON.Feature.FAIL_ON_DUPLICATE_MAP_KEYS.enabledByDefault() && values.containsKey(currentName)){ | |||
There was a problem hiding this comment.
Did you read my notes on #51? This does not work, it only considers default setting for feature but not its actual current state.
So without solving that problem this issue cannot really be solved.
There was a problem hiding this comment.
thanks for pointing that out. As you said, TreeCodec doesn't have JSON as a parameter, so we cannot bring the JSON value of the current state up to this point. and its not a part of this library. (and also it doesn't make sense to change a library API just for a single line of code in another API. How should we tackle this problem...
There was a problem hiding this comment.
Ideally configuration would be passed -- maybe there is something I overlooked earlier, as I did not spend tons of time on it? I am open to suggestions, as always.
|
@cowtowncoder , how about the extension storing the config information? That makes sense, that when we register the extension, we should have the configuration stored as well 🙌 |
jr-stree/src/main/java/com/fasterxml/jackson/jr/stree/JacksonJrsTreeCodec.java
Outdated
Show resolved
Hide resolved
| return JSON.builder() | ||
| // 13-Feb-2020, tatu: There are 2 different ways actually.. | ||
| // .treeCodec(new JacksonJrsTreeCodec()) | ||
| .register(new JrSimpleTreeExtension(config)) |
There was a problem hiding this comment.
This is backwards incompatible change, breaking all existing users. I don't think that is
doable.
But there are other ways to achieve this, suggested below.
|
Ok, no, let's not force passing of Instead, we should probably add a callback in codec that |
|
Oh awesome! this is nice way to handle the state. I didn't know there was an Extension context!! that makes much more sense. Thanks for help 😃 @cowtowncoder |
|
Thank you @Shounaks -- made minor tweaks, merged in for 2.17.0 |
Issue #51 : A check was missing for duplicate keys.