Skip to content
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

Do we need to clarify that "fixity": null is not allowed, or should we allow it? #558

Closed
zimeon opened this issue Jul 8, 2021 · 18 comments · Fixed by #580
Closed

Do we need to clarify that "fixity": null is not allowed, or should we allow it? #558

zimeon opened this issue Jul 8, 2021 · 18 comments · Fixed by #580

Comments

@zimeon
Copy link
Contributor

zimeon commented Jul 8, 2021

From zimeon/ocfl-py#79:

@zimeon: I think it is clear in https://ocfl.io/1.0/spec/#fixity "The fixity block MUST contain keys corresponding to the controlled vocabulary given in the digest algorithms listed in the Digests section, or in a table given in an Extension." -- this implies a JSON object but if we want to add further clarification then that would be a spec issue
@pwinckles: Right, but I would interpret that to mean that if there is a fixity block then it must have those properties. The fact that fixity is null, to me, means that it does not have a fixity block. Serializing missing fields to null is typical, and often configurable, for some JSON serializers.

@julianmorley
Copy link
Contributor

I'm a proponent of only listing the fixity block if it has digests in it. Having a random "fixity": {} or similar seems potentially messy.

@rosy1280 rosy1280 added this to the 1.1 milestone Feb 3, 2022
@rosy1280
Copy link
Contributor

rosy1280 commented Feb 3, 2022

we all agree that if something isn't in the fixity block then the fixity key should not be present.

@pwinckles
Copy link

For what it's worth, ocfl-java serializes empty fixity blocks as "fixity": {}. This is a product of how serialization works in Java and best practices around null values. I can update the code to not serialize empty fixity blocks, though I'd prefer not to. This feels like an odd thing to legislate. Clients should be able to handle fixity blocks in json that do not exist, exist but are null, and exist but have no value. This is just basic json parsing.

@pwinckles
Copy link

Furthermore, if you were to convince me that it is not a good idea to serialize the empty maps, I still don't see why the spec needs to be involved here. This PR is making it an error to produce what some json libraries produce naturally and every json library should support reading.

@zimeon
Copy link
Contributor Author

zimeon commented Feb 4, 2022

@pwinckles - I do NOT feel we should allow "fixity": null which you brought up earlier as a question, and I thought you were arguing for greater consistency. I can go either way on "fixity": {}

zimeon added a commit to zimeon/ocfl-py that referenced this issue Feb 4, 2022
@pwinckles
Copy link

To me it doesn't seem necessary to specify because I would expect every client to be able to deal with all 3 empty possibilities because that's the nature of working with json. After deserializing, I cannot tell the difference between a field that was present but with a null value and a field that was not present. I also don't see what the harm is of serializing an empty map.

As noted in the PR, if you do disallow it, contentDirectory, message, and user would also need addressed.

@pwinckles
Copy link

In the commit to ocfl-py, the example inventories have empty state and manifest maps. It seems inconsistent to me to forbid that for fixity but accept it other places.

@zimeon
Copy link
Contributor Author

zimeon commented Feb 4, 2022

Yes, that is why I feel it would be OK to allow {}.

I still think null is quite different, even if that Java library treats it the same as {}

@pwinckles
Copy link

I still think null is quite different, even if that Java library treats it the same as {}

It would be the case with any non-map based representation of json. It is simply not possible to know if a field was explicitly set to null or was not present. It's not a problem if you want to disallow it, it bothers me less than disallowing empty maps, but, to me, it seems like an odd thing to call out.

@pwinckles
Copy link

@zimeon Coming around to your point of view, I can see a case for removing any question of ambiguity for null values in case it did matter in some languages. So, maybe just allow the empty map then?

@pwinckles
Copy link

One final thought, and then I'll shut up about it.

If you think about it as a map then I agree that it doesn't make sense to allow null values. But, if you think about it as an object that has a finite set of defined attributes, then I think that it makes perfect sense to allow null values. The only difference between "fixity": null and omitting it entirely is that the former is explicitly assigning a null value to an attribute of the object and the later is implicitly assigning a null.

That said, I personally think the explicit form is ugly and I never serialize nulls when I work with json. However, I don't think it's wrong.

@awoods
Copy link
Member

awoods commented Feb 9, 2022

Reflecting on the original issue (zimeon/ocfl-py#79), I am slightly surprised that the validator reports the following as invalid:

    "fixity": null

My thinking is that the OCFL specification and the JSON associated with it (e.g. inventory.json) should be expected to work within the bounds of standard JSON. Empty JSON Objects include null and { } representations. Is there any reason to tighten the JSON specification in the context of OCFL?

If anything, I would suggest clarifying the OCFL specification fixity language along the lines of:

"The fixity block[, if non-empty,] MUST contain keys corresponding to the controlled vocabulary given in the digest algorithms listed in the Digests section, or in a table given in an Extension."

@zimeon
Copy link
Contributor Author

zimeon commented Feb 9, 2022

Per the JSON specification https://www.rfc-editor.org/rfc/rfc8259#section-3 (and also https://www.json.org/json-en.html and https://www.ecma-international.org/publications-and-standards/standards/ecma-404/), the value null is not the same as an empty object. While certain implementations may serialize null and {} interchangeably (I assume those with some model/schema defining the value as an object) they are not the same from a pure JSON point of view. IMO, if we wanted to allow null the spec should then explicitly say that "the value of the fixity key MUST be either a JSON object or null". I would prefer to keep it as only a JSON object.

@pwinckles
Copy link

@zimeon I assume when you say "certain implementations my serialize null and {} interchangeably" that the {} here is representing a missing property and not an empty object, correct?

If so, I now agree with you. I just did some experiments with the JSON schema inventory file and a couple of different validators. They all agree that null is not a valid value for fixity and allow an empty object, {}.

As you've pointed out, the JSON spec does not define the schematics of null apart from it being a distinct value type, and it never describes it in relationship to the absence of properties.

This was a little surprising to me. I do think it's worth stating that the OCFL spec does not accept null values, covering all fields. As you've pointed out, this is not strictly necessary given the definitions in the JSON spec, but it would eliminate all future confusion. I think empty objects should be allowed though.

@pwinckles
Copy link

Ironically, I just tried rocfl against inventories with null fields (fixity, contentDirectory, etc) and its validator rejects them...

@awoods
Copy link
Member

awoods commented Feb 10, 2022

Although I can understand the stylistic preference for not wanting null JSON values, in the case where a JSON attribute is optional, I am not sure I see the benefit in dictating how an implementation represents that notion (null, { }, or omitting the attribute). Are there cases where null introduces ambiguity?

@zimeon
Copy link
Contributor Author

zimeon commented Feb 10, 2022

I don't think the ambiguity test provides us sufficient guidance in this case: Allowing null (if properly described) would not introduce any ambiguity. However, neither would allowing True to stand for an empty or missing attribute -- but that doesn't make it a good idea.

I think clarity and simplicity provide better guidance:

  • If we allow null then I think we should add that to every place it is allowed in the spec. "value MUST be a JSON object" would become "value MUST be a JSON object or null". We also have say somewhere, I think could be once globally, that null should be treated as if the attribute wasn't there (or as equivalent to {}, would have to check which is more consistent)
  • If we do not allow null then I think all we need to do is clarify somewhere, once, that null does not have any implied semantics in OCFL and stands neither for a missing attribute nor an empty object. This could be done in https://ocfl.io/draft/spec/#inventory or in the implementation notes.

zimeon added a commit to zimeon/ocfl-py that referenced this issue Feb 22, 2022
@awoods
Copy link
Member

awoods commented Mar 17, 2022

editors agree we will allow an empty object but will not require it. and will not allow null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment