Fix including JSON files

[smortex]

When including a well-formed JSON file, the first opening brace is ignored, resulting in a parser error when we find the corresponding closing brace at the end of the document.

Track that the current stack of the parser state machine has seen an opening brace when applicable, so that we do not choke when we find the corresponding closing brace later.

Fixes #274

[smortex]

In #274, @vstakhov suggested more test that could fail with this change. This allowed to discover more inconsistencies in the current code (success to parse malformed configuration or failure to parse well-formed configuration, inconsistent error messages), that are fixed by this change. The following table show how these inconsistencies are fixed (*** denote an inconsistent error message).

Current code in the master branch:

main config	foo	outcome	expected	error reported
a = b;		✔️	✔️
{ a = b; }		✔️	✔️
a = b; }		❌	❌	object closed with } is not opened with { at line 1
{ a = b;		❌	❌	unmatched open brace at 1;
"a": "b"		✔️	✔️
{ "a": "b" }		✔️	✔️
"a": "b" }		❌	❌	object closed with } is not opened with { at line 1
{ "a" = "b"		❌	❌	unmatched open brace at 1;
.include "foo"	a = b;	✔️	✔️
.include "foo"	{ a = b; }	❌	✔️	object closed with } is not opened with { at line 1***
.include "foo"	a = b; }	❌	❌	object closed with } is not opened with { at line 1
.include "foo"	{ a = b;	✔️	❌
.include "foo"	"a": "b"	✔️	✔️
.include "foo"	{ "a": "b" }	❌	✔️	object closed with } is not opened with { at line 1***
.include "foo"	"a": "b" }	❌	❌	object closed with } is not opened with { at line 1
.include "foo"	{ "a" = "b"	✔️	❌

With this change:

main config	foo	outcome	expected	error reported
a = b;		✔️	✔️
{ a = b; }		✔️	✔️
a = b; }		❌	❌	object closed with } is not opened with { at line 1
{ a = b;		❌	❌	unmatched open brace at 1;
"a": "b"		✔️	✔️
{ "a": "b" }		✔️	✔️
"a": "b" }		❌	❌	object closed with } is not opened with { at line 1
{ "a" = "b"		❌	❌	unmatched open brace at 1;
.include "foo"	a = b;	✔️	✔️
.include "foo"	{ a = b; }	✔️	✔️
.include "foo"	a = b; }	❌	❌	object closed with } is not opened with { at line 1
.include "foo"	{ a = b;	❌	❌	unmatched open brace at 1;
.include "foo"	"a": "b"	✔️	✔️
.include "foo"	{ "a": "b" }	✔️	✔️
.include "foo"	"a": "b" }	❌	❌	object closed with } is not opened with { at line 1
.include "foo"	{ "a" = "b"	❌	❌	unmatched open brace at 1;

[vstakhov]

Please let me know when you have all tests added :)

[smortex]

Please let me know when you have all tests added :)

Hey @vstakhov, I adjusted the test so that all syntax supposed to be working are tested. This allowed to discover another issue, which is also fixed in this PR.

I have not added tests to assert that invalid syntax produce the expected error message as the test suite as it exist today does not allow to do this easily. Also, from my experience, testing non-working things has low value because there are so many ways to do things wrong 😄.

If we really want to go that way and conveniently testing exit codes, stdout and stderr for tests, I can suggest looking into cucumber + aruba that allows to do this kind of things (example), but that's a whole lot of testing dependencies.

[smortex]

Oh, we can also use a simple script that just check a few assertions for failures like I did to check if it works and generate the tables you see above. This was a quick hack but if you think we should somewhat include this, it is an option.

#!/usr/bin/env ruby

tests = [
  ["a = b;", "", 0, /a = "b";/],
  ["{ a = b; }", "", 0, /a = "b";/],
  ["a = b; }", "", 1, /object closed with } is not opened with { at line 1/],
  ["{ a = b;", "", 1, /unmatched open brace at 1;/],
  ["\"a\": \"b\"", "", 0, /a = "b";/],
  ["{ \"a\": \"b\" }", "", 0, /a = "b";/],
  ["\"a\": \"b\" }", "", 1, /object closed with } is not opened with { at line 1/],
  ["{ \"a\" = \"b\"", "", 1, /unmatched open brace at 1;/],
  [".include \"foo\"", "a = b;", 0, /a = "b";/],
  [".include \"foo\"", "{ a = b; }", 0, /a = "b";/],
  [".include \"foo\"", "a = b; }", 1, /object closed with } is not opened with { at line 1/],
  [".include \"foo\"", "{ a = b;", 1, /unmatched open brace at 1;/],
  [".include \"foo\"", "\"a\": \"b\"", 0, /a = "b";/],
  [".include \"foo\"", "{ \"a\": \"b\" }", 0, /a = "b";/],
  [".include \"foo\"", "\"a\": \"b\" }", 1, /object closed with } is not opened with { at line 1/],
  [".include \"foo\"", "{ \"a\" = \"b\"", 1, /unmatched open brace at 1;/]
]

def ret_to_md(ret)
  (ret == 0) ? ":heavy_check_mark:" : ":x:"
end

report = File.open("report.md", "w")
report.puts "main config | foo | outcome | expected | error reported"
report.puts "---|---|---|---|---"

tests.each do |config, foo, expected_ret, expected_error|
  File.write("config", config)
  File.write("foo", foo)
  error = `./tests/test_basic config`.strip
  ret = $?.exitstatus

  report.puts [config, foo, ret_to_md(ret), ret_to_md(expected_ret), if ret != 0
                                                                       error.gsub(/.*(config|foo):1 /, "") + (expected_error.match?(error) ? "" : "\\*\\*\\*")
                                                                     else
                                                                       ""
                                                                     end].join(" | ")

  if ret != expected_ret || !expected_error.match?(error)
    puts [config, foo, expected_ret, expected_error, ret, error].join(" | ")
  end
end

report.close

[vstakhov]

I was more looking at doctest, that I'm using in Rspamd so far. libucl has really quite an ancient system of testing but it's an old project that has some bad design choices...

[smortex]

Yes, from my understanding libucl in more in maintenance phase than active development :-D. I was quite surprised by this bug when I hit it. As so, keeping the test system as it is today seems sensible.

I see that you back-ported the change into rspamd, which mean that the issue I am trying to avoid will vanish on the next update. Thank you!