Skip to content

std.parseYaml() - new test case fails... #942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pmorch opened this issue Oct 18, 2021 · 8 comments
Closed

std.parseYaml() - new test case fails... #942

pmorch opened this issue Oct 18, 2021 · 8 comments
Assignees
Labels

Comments

@pmorch
Copy link
Contributor

pmorch commented Oct 18, 2021

std.parseYaml() exists in the cpp version in the master branch since commit da1490f. It was introduced by the merge of #899 and the main issue about its introduction is (I think) #460. A release hasn't been made with it yet.

std.parseYaml() chokes on this (I think) valid YAML,:

f1: |
  a
  b
f2: "a\nb"

Running it through std.parseYaml() gives an error:

Something went wrong during jsonnet_evaluate_snippet, please report this: [json.exception.parse_error.101] parse error at line 2, column 0: syntax error while parsing value - invalid string: control character U+000A (LF) must be escaped to \u000A or \n; last read: '"a<U+000A>'
[1]    91637 abort      ./jsonnet parseYaml.jsonnet

Where it should be fine and equal { f1: 'a\nb', f2: 'a\nb' }.

This is a patch for the test_suite to test for it. (Easy to add as a PR - let me know if you want a PR for this...)

diff --git a/test_suite/stdlib.jsonnet b/test_suite/stdlib.jsonnet
index 669c1f9..257c9d5 100644
--- a/test_suite/stdlib.jsonnet
+++ b/test_suite/stdlib.jsonnet
@@ -1467,6 +1467,16 @@ std.assertEqual(
     |||
   ), [1, 2, 3]
 ) &&
+std.assertEqual(
+  std.parseYaml(
+    |||
+      f1: |
+        a
+        b
+      f2: "a\nb"
+    |||
+  ), { f1: 'a\nb', f2: 'a\nb' }
+) &&
 
 std.assertEqual(std.asciiUpper('!@#$%&*()asdfghFGHJKL09876 '), '!@#$%&*()ASDFGHFGHJKL09876 ') &&
 std.assertEqual(std.asciiLower('!@#$%&*()asdfghFGHJKL09876 '), '!@#$%&*()asdfghfghjkl09876 ') &&
@sbarzowski sbarzowski added the bug label Oct 18, 2021
@sbarzowski
Copy link
Collaborator

Thanks for reporting.

Yeah, if you could raise a PR that would be very convenient.

@sbarzowski
Copy link
Collaborator

sbarzowski commented Oct 18, 2021

FWIW go-jsonnet seems to work fine on this example:

❯ cat junk/test.jsonnet
local yaml = |||
  f1: |
    a
    b
  f2: "a\nb"
|||;

std.parseYaml(yaml)
❯ ./jsonnet junk/test.jsonnet
{
   "f1": "a\nb\n",
   "f2": "a\nb"
}

pmorch added a commit to pmorch/jsonnet that referenced this issue Oct 19, 2021
std.parseYaml() doesn't handle this properly:

    f1: |
      a
      b
@pmorch
Copy link
Contributor Author

pmorch commented Oct 19, 2021

FYI: The test case in the PR is different from the one above, because f1 actually equals a\nb\n.

@sparkprime
Copy link
Collaborator

I assume this is a bug in the underlying yaml parsing library?

@sparkprime
Copy link
Collaborator

I.e., rapidyaml does not support text blocks?

@kshpytsya
Copy link

kshpytsya commented Apr 14, 2022

I am not sure whether this deserves a separate issue. String values parseable as integers, floats, booleans, null are wrongly converted to those types, e.g.:

/tmp/jsonnet-0.18.0 $ ./jsonnet -e 'std.parseYaml("[\" null\", \"null\", \" 1\", \"1\", \" 1.1\", \"1.1\", \" false\", \"false\"]")'
[
   " null",
   null,
   " 1",
   1,
   " 1.1",
   1.1000000000000001,
   " false",
   false
]

This issue appears to be caused by the old version (v0.1.0, released of Nov 03, 2020) of rapidyaml. Version of jsonnet which gets installed via Gentoo Linux package manager (which takes a usual stance of unbundling everything possible) built with recent rapidyaml v0.4.1 correctly parses all of those strings as strings.

@johnbartholomew johnbartholomew self-assigned this Feb 9, 2024
@johnbartholomew
Copy link
Collaborator

I'm upgrading Rapid YAML in #1134 - that should fix this case. Then the contributed test case #943 should pass and be mergeable.

johnbartholomew pushed a commit to pmorch/jsonnet that referenced this issue Feb 27, 2024
std.parseYaml() doesn't handle this properly:

    f1: |
      a
      b
@johnbartholomew
Copy link
Collaborator

The bundled RapidYAML has been upgraded, and your test case is now passing and has been merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants
@johnbartholomew @pmorch @sparkprime @sbarzowski @kshpytsya and others