Add only not unique items into result errors

[austinov]

If you try to validate the following json:

[{"foo":"bar"},{"foo":"bar"},{"foo":"baz"}]

with the schema:

{"uniqueItems":true}

then all unique and not unique items will be added to the result errors.
For example:

func TestJsonSchema(t *testing.T) {
   data := `[{"foo":"bar"},{"foo":"bar"},{"foo":"baz"}]`
   schema := `{"uniqueItems":true}`

   schemaLoader := gojsonschema.NewStringLoader(schema)
   documentLoader := gojsonschema.NewStringLoader(data)

   // validate
   result, err := gojsonschema.Validate(schemaLoader, documentLoader)
   if err != nil {
      t.Errorf("Error (%s)\n", err.Error())
   }

   for _, vErr := range result.Errors() {
      fmt.Printf("  Value: %s\n", vErr.Value())
   }
   // Output
   // Value: [map[foo:bar] map[foo:bar] map[foo:baz]]
}

Although, in my opinion is more logical to add only non-unique elements into result errors.

This commit fixes it and the result will be:

// Output
// Value: [map[foo:bar]]

[johandorland]

First off thank you for your contribution.

I can see how at first glance it's not intuitive to see {"foo":"baz"} pop up in an error about non-unique items, when the offending item is {"foo":"bar"}. However the way I look at it, a unique items check is performed over the entire array [{"foo":"bar"},{"foo":"bar"},{"foo":"baz"}] and that check fails. The Value() method returns the JSON object on which a check is performed. The JSON object on which the uniqueItems check works its magic in this case is the entire array.

Value() has an implicit contract that it will always return a part of the instance you gave it to validate (albeit parsed to Go types). If you'd only return {"foo":"bar"} you're somewhat breaching that contract, because you're manually slicing and dicing. {"foo":"bar"} on it's own isn't breaking the unique items check, it's only doing so in combination with the other items in the array.

One thing I do realize now that I'm looking into this is that there isn't really any feedback given on which array items cause the check to fail. It just says, this check on this array fails, good luck with that. It might be nice to be a bit more verbose and also indicate which array items cause the check to fail and somehow include that information, but I'm not convinced that this specific way is the right way to go.

[austinov]

Hi @johandorland! Thanks for your detailed answer!

I think you're right that we should not break the contract of Values(). I missed it in my commit.
I could return the original source code in PR and add the index of the not unique element to ErrorDetails, something like this:

if isStringInSlice(stringifiedItems, *vString) {
	result.addInternalError(
		new(ItemsMustBeUniqueError),
		context,
		value,
		ErrorDetails{"type": TYPE_ARRAY, "item": i},
	)
}

[ghostsquad]

Looking forward to this change! Anything that makes finding the problem easier is a great addition here.

[johandorland]

@austinov That seems like a better solution indeed.

You might even want to go a bit further

• Include the indices of both two array items which are the same and call them i and j instead of item. You might have to rewrite the code that is currently using isStringInSlice a bit to do that, also make sure you don't get duplicate errors (if items 2 and 4 are the same, so will 4 and 2 be)
• Include the two indices in the stringified error message.

[austinov]

@johandorland, i fixed the commit as i understood you )).
As a result, the output for

func TestJsonSchema(t *testing.T) {
	data := `[{"foo":"bar"},{"foo":"bar"},{"foo":"baz"},{"foo":"bar"},{"foo":"baz"},{"foo":"baz"}]`
	schema := `{"uniqueItems":true}`

	schemaLoader := NewStringLoader(schema)
	documentLoader := NewStringLoader(data)

	// validate
	result, err := Validate(schemaLoader, documentLoader)
	if err != nil {
		t.Errorf("Error (%s)\n", err.Error())
	}
	for _, vErr := range result.Errors() {
		fmt.Printf("Details: %#v\n", vErr.Details())
		fmt.Printf("Description: %s\n", vErr.Description())
		fmt.Println()
	}
}

will be

Details: gojsonschema.ErrorDetails{"type":"array", "i":0, "j":1, "field":"(root)", "context":"(root)"}
Description: array items[0,1] must be unique

Details: gojsonschema.ErrorDetails{"type":"array", "i":0, "j":3, "field":"(root)", "context":"(root)"}
Description: array items[0,3] must be unique

Details: gojsonschema.ErrorDetails{"type":"array", "i":2, "j":4, "field":"(root)", "context":"(root)"}
Description: array items[2,4] must be unique

Details: gojsonschema.ErrorDetails{"type":"array", "i":2, "j":5, "field":"(root)", "context":"(root)"}
Description: array items[2,5] must be unique

[johandorland]

Thank you for taking the time to put together this PR, it's greatly appreciated.

[austinov]

@johandorland thank you too!