Bug in ValidationSequence of if-then-else

[amosonn]

Describe the bug

IfKeyword, ThenKeyword and ElseKeyword all have the same ValidationSequence value. This means, that then and else might be validated before if (if they appear earlier in the document), will silently valid regardless of if, and then the whole schema is always valid.

To Reproduce

using Manatee.Json;
using Manatee.Json.Schema;
using Manatee.Json.Serialization;
using System;

using ManateeJsonSerializer = Manatee.Json.Serialization.JsonSerializer;

namespace test
{
    class Program
    {
        static void Main(string[] args)
        {
            var val = JsonValue.Parse(@"
{
   ""else"": {
     ""required"": [""b""]
   },
   ""then"": {
     ""required"": [""c""]
   },
   ""if"": {
     ""required"": [""a""]
   }
}");
            var ser = new ManateeJsonSerializer();
            var schema = ser.Deserialize<JsonSchema>(val);
            var js = new[]
            {
                @"{""a"": 1, ""c"": 2}",
                @"{""b"": 1, ""c"": 2}",
                @"{""b"": 1}",
                @"{""a"": 1, ""b"": 2, ""c"": 3}",
                @"{""a"": 1, ""b"": 2}",
                @"{""a"": 1}",
            };
            foreach (var j in js)
            {
                SchemaValidationResults validationResult = schema.Validate(JsonValue.Parse(j));
                Console.WriteLine("{0}: {1}", j, validationResult.IsValid);
            }
        }
    }
}

Should print False on the last two jsons, prints True for all of them.

Possible Fix
Bump ThenKeyword and ElseKeyword to have ValidationSequence value of 2. Could this interfere with any other "2's"? I don't think so.

I can draft a PR if wanted; I noticed there are no tests for if-then-else, this might be a good start :).

[gregsdennis]

I think that's due to how I implemented it. if actually validates everything. else and then don't actually do anything during validation.

So it works, just not how you might think. (Unless I "fixed" it at some point. I've since published a new schema implementation in the json-everything repo, so I'm not sure without looking at this code.)

[amosonn]

First of all, thanks for pointing me to json-everything, I'll definitely take a look and perhaps switch! And thanks for all your work on these packages.

Regarding the bug, it's definitely there. As I mentioned above, the two last jsons in the example shouldn't validate (as they fulfil the if but not the then), but they do.

The implementation is divided among if, then and else: if validates and stores the result (as bool) in the context, and then then and else look for that result. If it matches their condition, then they validate; but if it's missing (because the if wasn't validated yet), they just skip. Looking at the code in json-everything, it seems quite similar, so I would expect this bug to appear there as well.

[gregsdennis]

I've added a test (through much debate) to the test suite.

json-schema-org/JSON-Schema-Test-Suite#437

[gregsdennis]

I have created a v13.0.3 branch. The fix should be merged into that, and then I can pull into master an publish.