Skip to content

Many=True property checks #63

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
ottonomy opened this issue Apr 5, 2017 · 7 comments
Closed

Many=True property checks #63

ottonomy opened this issue Apr 5, 2017 · 7 comments
Assignees

Comments

@ottonomy
Copy link
Collaborator

ottonomy commented Apr 5, 2017

Extend the validate_primitive_property and validate_id_property to be able to accept task_meta['many'] == True for properties where multiple values are allowed.

  • Acceptable values are: Single entries: "_:b0", multiple entries: ["_:b0", "_:b1"]
  • Proposed: If Optional, acceptable values also include: [], None, [None] (In my experience, this was the quirk found most often with implementations. Acceptance of JSON null values for optional properties was something the Mozilla validator did but BadgeCheck did not in v1.x. I am interested to know how platforms would like to see this.. should we strip these properties from the node in the graph? @aweeks?
  • Each value in a list must pass the property validation rules, whether they are for id-type or primitive-type

notes: For Primitive Values, this might work best in the validate_primitive_property function, but for id-type properties, this might work best as part of the task queuing system.

@ottonomy
Copy link
Collaborator Author

ottonomy commented Apr 6, 2017

@mgylling Please review specifically this:

When there is a property that accepts TEXT datatype (for instance) that allows multiple values, is the following table correct in your view about what we should see for passes and fails?

value if required=True if required=False what is in output
None fail pass property omitted
"one" pass pass "one"
[] fail pass property omitted
[None] fail pass property omitted
[None, "one"] pass pass "one"
["one", "two"] pass pass ["one", "two"]

@mgylling
Copy link

mgylling commented Apr 6, 2017

@ottonomy, yes this looks correct to me. Are we sure that omission of null values in the output won't come back and bite us? Thinking that roundtrip (or rather, passthrough) fidelity might perhaps be a desirable feature in the long run. At this point in time, this is a validator, not a repair/cleanup tool.

@ViktorHaag
Copy link
Contributor

Do we assume that "" (a falsey value) is to be treated with the same respect we would the value "one"? (And when empty string is embedded in lists, equivalently the same value as "one" in that context?)

@ottonomy
Copy link
Collaborator Author

ottonomy commented Apr 7, 2017

@mgylling @ViktorHaag I went back in #74 and readdressed this and added a whole bunch of tests combining many and required options.

And I changed my mind on a couple things. I suggest we keep thinking on this a bit. I'm really not sure on a couple of these cases, and also on the idea of stripping out values.

As an API consumer with our backpack service, I would really rather not see extraneous values that aren't in the expected format. But historically, null values for optional properties was the top reason badgecheck rejected certain badges while the Mozilla Validator let them through. I've tried to ask "Should null or falsey values be acceptable for optional properties?" before and not gotten much useful feedback.

value both True req=True, many=False req=False, many=True both False what is in output
None fail fail pass pass property omitted
"one" pass pass pass pass "one"
[] fail fail pass pass property omitted
[None] fail fail pass pass property omitted
[None, "one"] fail fail fail fail [None, "one"]
["one", "two"] pass fail pass fail ["one", "two"]

How can we break this down to some yes/no questions that the spec text (either OB or JSON-LD) would allow us to answer?

  1. Should null values be acceptable for optional properties?
  2. Should other falsey values be acceptable for optional properties?
  3. Should "" be equivalent to None?
  4. IF the answer to 1 is yes, should including null values in a list with other values be acceptable? Is there a meaningful difference between "one", ["one", None] then?

Accept PR #74 at least and we've got 36 tests in tests/test_validation.py that allow us to make sure the code is doing what we want once we decide.

@ViktorHaag
Copy link
Contributor

ViktorHaag commented Apr 7, 2017

Perhaps we can come at this from a different angle -- what do we suppose are the default, unset values for a property, optional or otherwise? My guess is that in nearly all cases, the naturally falsey value for a property makes sense as the default value indicating the property has not been set, and in those cases we should feel free to remove them from serialization.

Are there cases where a set value could be falsey? I suspect the answer is yes. For example, a "middle name" property of null tells me something different to a "middle name" property of "". The former says "I don't know if this entity has a middle name" or perhaps "middle name is not an applicable property for this entity". Empty string says "middle name is a possible property for this entity, and this entity explicitly does not have a middle name value to set".

Would framing like that help our decision making?

@ottonomy
Copy link
Collaborator Author

ottonomy commented Apr 7, 2017

Interesting. That is helpful.

I think we have to lean more on the conventions of JSON-LD here rather than the conventions of the python language. (For instance in Django, "" is the default empty value for text model fields). So I think you're right for the "middleName" example that "" would be saying "There is no middle name for this entity" where null may be closer to "I am not declaring a middle name for this entity".

In JSON-LD, null is the way to say the property is not declared in the document. The property will be stripped out of the expanded/compacted output: See demonstration http://json-ld.org/playground/#/gist/66909d78ada5cbf34b52172610b23b89

json-ld/json-ld.org#76 (comment) :

RESOLVED: Unless otherwise specified, if null is associated with a key in the body of a JSON-LD document, then the JSON-LD processor MUST act as if the key-value pair was never declared. If @value or @list is set to null in expanded form, then the entire JSON object is ignored.

I suspect a number of these cases from my table just won't exist in real life -- i.e. we can't actually get to these state inputs by compacting JSON-LD input of any form.

That said, I don't want to accept empty string as an acceptable value for an optional property. I want to see optional properties as null so they won't even show up in the compacted node value we store in state. This has most often come up with evidence (pre-2.0 evidence was an optional string/url field). In JSON-LD: "evidence": null has the correct meaning, where "evidence": "" doesn't.

There's particularly the issue where if a JSON-LD document @base is set, for a property of type @id where you declare an empty string value, the value will be set to the base. See demonstration. Then we'll see the base value as the string when we start validating. Or, likely, a different platform that isn't as careful about their JSON-LD compaction settings as we are might have that issue using the validated output we provide.

So, I think next step is to filter this use cases list down to the ones that can actually appear in compacted input before worrying about any other case.

@ViktorHaag
Copy link
Contributor

All this makes a good deal of sense, @ottonomy.

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

No branches or pull requests

3 participants