Fix error thrown when inserting keys

[kekavc24]

Closes dart-lang/tools#1940

• Checks for ordering only when more than one key is present

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.
• Most changes should add an entry to the changelog and may need to rev the pubspec package version.
• Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

[kekavc24]

@jonasfj Before I can add new test files. It seems many of the old tests were based on this bug e.g.

yaml_edit/test/editor_test.dart

Lines 23 to 35 in 3191934

	test('after multiple changes', () {
	final yamlEditor = YamlEditor('YAML: YAML');
	yamlEditor.update(['YAML'], "YAML Ain't Markup Language");
	yamlEditor.update(['XML'], 'Extensible Markup Language');
	yamlEditor.remove(['YAML']);
	expect(yamlEditor.edits, [
	SourceEdit(5, 5, " YAML Ain't Markup Language"),
	SourceEdit(0, 0, 'XML: Extensible Markup Language\n'),
	SourceEdit(32, 32, '')
	]);
	});

The fix now returns the new correct SourceEdit for second update as the correct offset for the edit 32 not 0 anymore. Should I update them?

[jonasfj]

By all means fix all the old tests too.

But are you sure wise to support reverse alphabetical ordering?
It might be unlikely that someone will do that intentionally.

Especially if there is only 2 properties then reverse alphabetical ordering is really just any order that isn't alphabetical :)

[kekavc24]

Oh, yeah. Sorry for the overkill. I just thought it was a great addition. I'll fix that and the tests. Thanks.

[kekavc24]

@jonasfj I'm still debugging the spliceList method. It seems something broke somewhere (or was manifested by the bug fix).

[jonasfj]

Other than tests still failing, I think this pretty good.

I could be convinced that we should sort new entries in maps that only contain a single key.
Given that we already have tests that rely on this behavior.

One could also argue that we should allow ordering to be configurable in YamlEditor -- but that's probably overkill. I wouldn't mind doing it, but if we want to pursue such things I'd suggest we do it in a separate PR.

[jonasfj]

This is happening because the ordering is changing, because this test starts with a map of size 1 and then just adds more keys. Which previously would be ordered, and now won't be.

hmm, I could see a point to ordering maps that have only 1 key. Since we apparently have a lot of tests doing exactly this.

@kekavc24 What do you think we should do? Should we order keys in a map when there is only 1 key?

I don't mind landing this as is.

[kekavc24]

I think we should keep the changes. The tests now mirror expected behaviour rather than the bug.

[jonasfj]

But then we trigger the issue in random_test.dart, so we either have to add skip: true to random_test.dart so we can land this.

Or walk back the change with not sorting 1-key maps. Because then we don't trigger issues in random_test.dart.

We do want to fix the spliceList issue, but ideally in a different PR.

---

And I don't know what solution is best, building sorted maps when adding multiple keys to a map with a single key, feel equally good to just appending the keys.

---

TL;DR: Let's get the tests passing, so we can land. Either fixing spliceList, walking back the 1-key map change, or skipping tests (and later fixing spliceList).

[kekavc24]

Maybe skip? Once this is done, I'll start looking into that issue

[jonasfj]

Go for it... Add a skip thing and let's merge :)

[jonasfj]

By the way, the test that is failing is somewhere in test/random_test.dart.

I'd suggest trying to add more print statements and extract the YAML + the modification being made into a separate test.

Don't try to debug things in random_test.dart, try to extract the test case it generates instead. We'll want it as a hardcoded test case anyways :D

[jonasfj]

I noticed a few weird hacks with prints in random_test.dart and filed: #81

After that the failing test case should print:

Failed to call YamlModificationMethod.splice on:

environment: true


0.03167456183131434:
  - - true  
    - |-
        false
    - 4177511653
  - null
  - 2910742758
  - true
  - - false 
  - 2081477285
  - 1718060652
  - 0.9662696464087974
  - null
  - "9[hd24=s438Y7a8_!M&}s<3cTr{\"e%<1I7vq&xp4p\"9jH#NJv"
762977104: null

with the following arguments:
[0, 1, [0.7396543051030952, null, {P/wQ'@G`$A%6LA$aXk0Uy&f36Pi[~:905@`DJ~nKUs!2:$45hj[vk7V<Qt~\2M^Uk$, vZEjAW=tOT=Q&dpH,cN'hX$_wHR=:q?UvHzC;+8*bLX_5Rgvp)L{E, {null: null, {3368549847: null, 0.9104247116161583: dcN&=EiSL?{sWLMRg -W]%
[=~Ggy@Z9F<77:3dY!{wE;m\uC9R9S2sIv<LyIUK, JRBV\|1]O<K;Wosal33 f30@`?*{}?}ke>^Eg''oUXw+JXq%C|{Ck._BGk: true}: ヽ༼ຈل͜ຈ༽ﾉ ヽ༼ຈل͜ຈ༽ﾉ}, 1853891542]]
and path:   
[0.03167456183131434, 4]

Error Details:
Assertion failed: (package:yaml_edit) Modification did not result in expected result.

# YAML before edit:
>
> environment: true
>
>
> 0.03167456183131434:
>   - - true
>     - |-  
>         false
>     - 4177511653
>   - null  
>   - 2910742758
>   - true  
>   - - false
>   - 2081477285
>   - 1718060652
>   - 0.9662696464087974
>   - null
>   - "9[hd24=s438Y7a8_!M&}s<3cTr{\"e%<1I7vq&xp4p\"9jH#NJv"
> 762977104: null
>

# YAML after edit:
>
> environment: true
>
>
> 0.03167456183131434:
>   - - true
>     - |-
>         false
>     - 4177511653
>   - null
>   - 2910742758
>   - true
>     - 1853891542
>   - - false
>   - 2081477285
>   - 1718060652
>   - 0.9662696464087974
>   - null
>   - "9[hd24=s438Y7a8_!M&}s<3cTr{\"e%<1I7vq&xp4p\"9jH#NJv"
> 762977104: null
>

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:579:7)
dart-lang/tools#1919      YamlEditor.insertIntoList (package:yaml_edit/src/editor.dart:339:5)
dart-lang/yaml_edit#2      YamlEditor.spliceList (package:yaml_edit/src/editor.dart:376:7)
dart-lang/yaml_edit#3      _Generator.performNextModification (file:///workspace/yaml_edit/test/random_test.dart:209:20)
dart-lang/yaml_edit#4      main.<anonymous closure>.<anonymous closure> (file:///workspace/yaml_edit/test/random_test.dart:47:27)
dart-lang/yaml_edit#5      _ReturnsNormally.typedMatches (package:matcher/src/core_matchers.dart:153:8)
dart-lang/yaml_edit#6      FeatureMatcher.matches (package:matcher/src/feature_matcher.dart:16:42)
dart-lang/yaml_edit#7      _expect (package:matcher/src/expect/expect.dart:138:30)
dart-lang/yaml_edit#8      expect (package:matcher/src/expect/expect.dart:56:3)
dart-lang/yaml_edit#9      main.<anonymous closure> (file:///workspace/yaml_edit/test/random_test.dart:46:9)
dart-lang/yaml_edit#10     Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:215:19)
<asynchronous suspension>
dart-lang/tools#1920     Declarer.test.<anonymous closure> (package:test_api/src/backend/declarer.dart:213:7)
<asynchronous suspension>
dart-lang/tools#1921     Invoker._waitForOutstandingCallbacks.<anonymous closure> (package:test_api/src/backend/invoker.dart:258:9)
<asynchronous suspension>


00:03 +15 -1: testing with randomly generated modifications: test 15 [E]
  Expected: return normally
    Actual: <Closure: () => void>
     Which: threw _YamlAssertionError:<Assertion failed: (package:yaml_edit) Modification did not result in expected result.

            # YAML before edit:
            >
            > environment: true
            >
            >
            > 0.03167456183131434:
            >   - - true
            >     - |-
            >         false
            >     - 4177511653
            >   - null
            >   - 2910742758
            >   - true
            >   - - false
            >   - 2081477285
            >   - 1718060652
            >   - 0.9662696464087974
            >   - null
            >   - "9[hd24=s438Y7a8_!M&}s<3cTr{\"e%<1I7vq&xp4p\"9jH#NJv"
            > 762977104: null
            >

            # YAML after edit:
            >
            > environment: true
            >
            >
            > 0.03167456183131434:
            >   - - true
            >     - |-
            >         false
            >     - 4177511653
            >   - null
            >   - 2910742758
            >   - true
            >     - 1853891542
            >   - - false
            >   - 2081477285
            >   - 1718060652
            >   - 0.9662696464087974
            >   - null
            >   - "9[hd24=s438Y7a8_!M&}s<3cTr{\"e%<1I7vq&xp4p\"9jH#NJv"
            > 762977104: null
            >

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

  package:matcher             expect
  test/random_test.dart 46:9  main.<fn>

And from that we should be able to extract the test case.
Once extracted we can craft a minimal example from the test case.

I think it makes sense to have the failing test cases and minimum example as hardcoded test cases going forward.

random_test.dart does make consistent test cases, but the RNG might change in the future and then it'll test other things.

---

Side note: it's pretty cool that random_test.dart actually found an issue. It was really a shot in the dark when we made it 🤣 I had no idea that it would actually prove useful.

(Okay, we don't know that the issue it found is a useful error, but it hopefully is something worth fixing).

[jonasfj]

@kekavc24 I landed #81 I'd suggest rebasing this branch on main, then it'll be much easier to figure out what's wrong in random_test.dart.

[jonasfj]

Also please do consider adding an entry to CHANGELOG.md.
Once we've landed this, I think we should cut a release, it's a pretty important bugfix!

I filed #82 to start the CHANGELOG.md.

[kekavc24]

By the way, the test that is failing is somewhere in test/random_test.dart.

I'd suggest trying to add more print statements and extract the YAML + the modification being made into a separate test.

Don't try to debug things in random_test.dart, try to extract the test case it generates instead. We'll want it as a hardcoded test case anyways :D

@jonasfj I missed your comment. I hadn't refreshed the page 😅 or checked my email for notifications. I managed to find the issue by using the Zone (learned something new today 😅. I hadn't used it before!).

It seems YamlEditor uses the same strategy to append to map and list, that is, by looking for last \n character.

While this is great for a map and appending to the end of the list, for any other positions it may not be great. All methods to mutate a block list may depend on _insertInBlockList directly or indirectly if not inserting at the end.

yaml_edit/lib/src/list_mutations.dart

Lines 164 to 183 in 8fdc96d

	/// Returns a [SourceEdit] describing the change to be made on [yamlEdit] to
	/// achieve the effect of inserting [item] into [list] at [index], noting that
	/// this is a block list.
	///
	/// [index] should be non-negative and less than or equal to `list.length`.
	SourceEdit _insertInBlockList(
	YamlEditor yamlEdit, YamlList list, int index, YamlNode item) {
	RangeError.checkValueInInterval(index, 0, list.length);
	if (index == list.length) return _appendToBlockList(yamlEdit, list, item);
	final formattedValue = _formatNewBlock(yamlEdit, list, item);
	final currNode = list.nodes[index];
	final currNodeStart = currNode.span.start.offset;
	final yaml = yamlEdit.toString();
	final start = yaml.lastIndexOf('\n', currNodeStart) + 1;
	return SourceEdit(start, 0, formattedValue);
	}

It fails for block lists nested within another block list. Simple output :

0.03167456183131434:
  - true
  - - false

If we try to mutate via spliceList on the nested list with false at index 0 and delete it too,

# Insert test

# Results in 👇🏽 which throws an YamlAssertionError
0.03167456183131434:
  - true
  - test
  - - false

# Instead of 👇🏽
0.03167456183131434:
  - true
  - - test

My initial idea based on the existing _insertInBlockList code above,

1. A block sequence element has a dash. So we always account for it as the offset isn't included. So, +1 if no space before value and +2 if there is a space.
2. Scan for the indices of \n, - and ' ' (space). Why?
   • The greater the index, the closer it is. Thus, we will be able to compensate if we are inserting just before the first element of a nested block list.
   • The space just ensures we insert after if nested.

What do you think? Am I missing something?

[jonasfj]

hmm, I'm not sure I entirely follow.

But I have a few questions:

• Is this only an issue given the bug fix we have in this PR?
  • I'm guessing this bug has been there a while, but that this PR just made it surface, right?
• Are we only discovering the issue because of the bug fix we have in this PR?
  Because we no longer make 1 length maps sorted when inserting the second key.
  And this just happens to cause this bug to surface now.

I think if we can make test pass by sorting 1 length maps again, then we should do so.
AND then file an issue to fix the bug in spliceList. That way we can do it in a separate PR.

---

If on the other hand, the bug fix we made here, cause this bug in spliceList, then we should fix it in this PR.

[jonasfj]

I'm pretty sure this is an existing bug that just surfaced, try the test case.

SLICE LIST IN NESTED BLOCK LIST
---
key:
- true
- - false
---
- [splice, [key, 1], 0, 1, ['test']]

Just:

• Create test/testdata/input/splicelist_in_nested_block_list.test with the contents above.
• Run dart test test/golden_test.dart

It fails both on main and on this PR.

---

Question is why it only surfaces in random_test.dart in this PR.

[jonasfj]

If we switch the implementation to:

int getMapInsertionIndex(YamlMap map, Object newKey) {
  final keys = map.nodes.keys.map((k) => k.toString()).toList();

  // We can't deduce ordering if list is empty, so then we just we just append
  if (keys.isEmpty) {
    return map.length;
  }

  for (var i = 1; i < keys.length; i++) {
    if (keys[i].compareTo(keys[i - 1]) < 0) {
      return map.length;
    }
  }

  final insertionIndex =
      keys.indexWhere((key) => key.compareTo(newKey as String) > 0);

  if (insertionIndex != -1) return insertionIndex;

  return map.length;
}

Then we've still fixed the bug we set out to fix, right? @kekavc24
We'll be inserting in sorted order, even when there is only 1 entry in the map, but that's also how it worked before.

And with this random_test.dart just happens to pass. It's deterministically random, but it certainly doesn't try all combinations.

---

If we do that, then we've done the smallest possible change to fix the bug we're trying to fix. Then we can land this and move on the next bug. It's much easier if we don't let PRs grow too big.

I filed dart-lang/tools#1942 for the issue with spliceList. And it'd be great if we can fix that too -- just easier to do in a separate PR.

[kekavc24]

It's not from this fix.

[kekavc24]

@jonasfj Wow, I'm sorry. I really fumbled this. I committed a file I was meant to delete.

My golden tests seem to be failing with no cause. Can you take a look?

[jonasfj]

Make sure you delete the output files before running golden tests... Then review the newly generated output.

Otherwise, I can look tomorrow maybe.

[jonasfj]

@kekavc24 if you interested feel free to take stab at dart-lang/tools#1942, or we can cut a release with these changes if we just want it out.

[kekavc24]

Currently on it. @jonasfj