-
Notifications
You must be signed in to change notification settings - Fork 61
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
Fix crash when schema containing reference is map()
#113
Conversation
@@ -182,7 +182,11 @@ set_current_schema(#state{id = Id} = State, NewSchema0) -> | |||
%% Instead of just removing all the other fields, we put schema as | |||
%% 1st element, so, only `ref' will be validated, while other fields | |||
%% (say, `definitions') may still be referenced. | |||
[{?REF, Ref} | lists:keydelete(?REF, 1, NewSchema0)] | |||
ListSchema = case is_map(NewSchema0) of | |||
true -> maps:to_list(NewSchema0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why convert map to list? It seems like map is a superior data storage mechanism for performance. It also seems like you don't need to do anything at all here for the map case (map keys are unique, unordered). Maybe I don't understand the issue that is solved here very well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thig is that we would alwasy convert schema to list anyway before applying it:
jesse/src/jesse_schema_validator.erl
Line 89 in 6cfd1c0
, jesse_json_path:unwrap_value(JsonSchema) |
Lines 125 to 134 in 6cfd1c0
%% @doc Unwrap data (remove mochijson2 and jiffy specific constructions, | |
%% and also handle `jsx' empty objects) | |
-spec unwrap_value(kvc_obj()) -> kvc_obj(). | |
?IF_MAPS(unwrap_value(Map) when erlang:is_map(Map) -> maps:to_list(Map);) | |
unwrap_value({struct, L}) -> L; | |
unwrap_value({L}) -> L; | |
unwrap_value({}) -> []; | |
unwrap_value([]) -> []; | |
unwrap_value([{}]) -> []; | |
unwrap_value(L) -> L. |
We could, in theory, try to use maps:iterator()
to iterate over map-schema, but that would require quite a lot of work with no clear benefit (because rules in schema are applied iteratively, one-by-one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the issue this code solves is:
When schema contains a reference AND some other validators, and also some definitions to be referenced, say:
{
"type": "object",
"$ref": "#/path/to/other/schema",
"definitions": {
"a": {...}
}
}
(which is kind of artificial, but it is included in the official JSON schema test suite), the following conditions should be true:
- any other validators should be ignored, when there is a
$ref
in a schema, so,"type": "object"
rule should not be applied - schema from the reference should "replace" the current schema
- but all the keys defined in the same object as the one containing '$ref' are still should be accessible for referencing, so, it still should be possible to reference
definitions.a
from somewhere
Which means we should at the same time ignore all the other validators in current schema AND we can't just remove all the other keys (that's why previous attempt to solve this by just removing all the other keys from object containing $ref
failed after we updated the test suite)
Fixes issue introduced in #109 (comment)