Duplicate mapping key and parsing error when trying to add a map entry

[chriskelly]

I'm trying to add a key to a map and set its value, but running into issues.

# Input Yaml
- name: foo
  id: "A"
- name: bar

# Desired output Yaml
- name: foo
  id: "A"
- name: bar
  id: "B"

Example code for Duplicate mapping key error

import 'package:yaml_edit/yaml_edit.dart';

void main() {
  const yaml = '''
- name: foo
  id: "A"
- name: bar
  ''';
  final yamlEditor = YamlEditor(yaml);
  yamlEditor.update(
    [1, 'id' ],
    "B",
  );
  print(yamlEditor);
}

For this, I get the following error:

Unhandled exception:
Error on line 3, column 3: Duplicate mapping key.
  ╷
3 │   id: B
  │   ^^
  ╵
#0      Loader._loadMapping (package:yaml/src/loader.dart:171:9)
#1      Loader._loadNode (package:yaml/src/loader.dart:92:16)
#2      Loader._loadSequence (package:yaml/src/loader.dart:146:20)
#3      Loader._loadNode (package:yaml/src/loader.dart:90:16)
#4      Loader._loadDocument (package:yaml/src/loader.dart:68:20)
#5      Loader.load (package:yaml/src/loader.dart:60:20)
#6      loadYamlDocument (package:yaml/yaml.dart:72:25)
#7      loadYamlNode (package:yaml/yaml.dart:57:5)
#8      YamlEditor._performEdit (package:yaml_edit/src/editor.dart:576:24)
#9      YamlEditor.update (package:yaml_edit/src/editor.dart:271:14)
#10     main (package:yaml_playground/example.dart:11:14)
#11     _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#12     _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

If I swap the order around and try to update the first list item instead, I get a different error

import 'package:yaml_edit/yaml_edit.dart';

void main() {
  const yaml = '''
- name: foo
- name: bar
  id: "B"
  ''';
  final yamlEditor = YamlEditor(yaml);
  yamlEditor.update(
    [0, 'id' ],
    "A",
  );
  print(yamlEditor);
}

Unhandled exception:
Error on line 2, column 1: Only expected one document.
  ╷
2 │ ┌ - name: foo
3 │ │ - name: bar
4 │ │   id: "B"
5 │ └   
  ╵
#0      loadYamlDocument (package:yaml/yaml.dart:80:5)
#1      loadYamlNode (package:yaml/yaml.dart:57:5)
#2      YamlEditor._performEdit (package:yaml_edit/src/editor.dart:576:24)
#3      YamlEditor.update (package:yaml_edit/src/editor.dart:271:14)
#4      main (package:yaml_playground/example.dart:11:14)
#5      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#6      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

In a larger example that I can't duplicate in this smaller example, the error I get in this case is: Expected a key while parsing a block mapping.

Using version yaml_edit: ^2.2.0

[chriskelly]

And here's an even weirder error if neither map has the 'id' key

import 'package:yaml_edit/yaml_edit.dart';

void main() {
  const yaml = '''
- name: foo
- name: bar
  ''';
  final yamlEditor = YamlEditor(yaml);
  yamlEditor.update(
    [
      1,
      'id',
    ],
    "B",
  );
  print(yamlEditor);
}

Unhandled exception:
Assertion failed: (package:yaml_edit) Modification did not result in expected result.

# YAML before edit:
> - name: foo
> - name: bar
>   

# YAML after edit:
> - name: foo
>   id: B
> - name: bar
>   

Please file an issue at:
https://github.com/dart-lang/yaml_edit/issues/new?labels=bug

#0      YamlEditor._performEdit (package:yaml_edit/src/editor.dart:578:7)
#1      YamlEditor.update (package:yaml_edit/src/editor.dart:271:14)
#2      main (package:yaml_playground/example.dart:12:14)
#3      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:297:19)
#4      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:184:12)

[jonasfj]

Does this also happen if the top-level is not a list?

To be clear, I don't think I have time to dive into all issues that this package might have. But if you ping me on a PR, or drop me an email jonasfj@google.com, I'm happy to help review a PR with appropriate test cases.

[kekavc24]

@jonasfj I managed to debug this and find the culprit. When mutating the map, YamlEditor tries to respect the key ordering.

https://github.com/dart-lang/yaml_edit/blob/31919348bd2a1bbb805b4eb88a6b7f50d4ab247e/lib/src/utils.dart#L119-L139

There is a dangling check for the index based on the ordering after the loop that presents an edge case for a map with only one key. That may or may not default to zero depending on the results of the compareTo which taints the SourceEdit to be used. This bubbles up and presents itself in all the situations stated by @chriskelly

If you were to comment out the line, YamlEditor works as expected. Possible ways to tackle this:

1. Allow a nullable param (that defaults to false) in YamlEditor to configure it to respect ordering or not.
2. Ignore option 1 above and just check for ordering if more than one key exists.

Also, shouldn't the ordering compare every key with the previous key and not return after just one match?

[jonasfj]

@kekavc24 fantastic investigation!

I'd suggest option (2).

I don't see how (1) would solve the issue, it would just cause the issue to only happen when the option to respect key ordering is enabled.

I think it's great that YamlEditor respects the ordering, if the keys are already alphabetically ordered. I think it's a reasonable heuristic. But if there is only 1 key (or less) we should just append to the end of the map. That's seems very reasonable.

I mean that code only respects ordering, if they keys are already ordered. So it seems reasonable that if there is only 1 (or zero) keys, then we declare that there is no ordering and we just append the key.

I suppose in an ideal world, we would scan the rest of the YAML document to see if other keys are ordered, and if so, then we'd make sure to order in case there is only 1 key. But that also seems very very complicated :rofl:

---

@kekavc24 I'd be happy to review a PR that:

• Returns map.length if map.length <= 1.
• Add new test files in test/testdata/
• Add new test cases in test/update_test.dart.
• And optionally adds new test cases in new files or where it makes sense. I don't think this package is likely to suffer from having too many tests anytime soon :see_no_evil: