SpaceApi / schema

SpaceAPI JSON schema files.
24 stars 14 forks source link

nullable state.open #101

Closed Juerd closed 1 month ago

Juerd commented 1 year ago

The nullability of state.open is documented as deprecated but I couldn't find the rationale. A nullable field is much easier to deal with when generating JSON, because the code for conditionally adding a key is much more involved than a simple ternary. The side that reads the JSON won't get much simpler for a non-nullable-but-optional field, as far as I can tell, especially considering backwards compatibility and considering that good programmers write defensive code that can deal with unexpected values, anyway.

Please reconsider whether state.open should indeed be non-nullable. I think it's better the way it is. Or if you must change this anyway, please provide why this change is actually beneficial.

I think it makes sense to keep space.open nullable, and to document that a null value or the lack of this value are to be considered equal, and that it is an indication of a temporary disruption in the availability of the space open state.

rnestler commented 1 year ago

because the code for conditionally adding a key is much more involved than a simple ternary

Is it? For spaceapi-rs it was a simple annotation: https://github.com/spaceapi-community/spaceapi-rs/blob/c60c4e25fad57e49832c2b9acb2ad5c39135c45e/src/status.rs#L49

and to document that a null value or the lack of this value are to be considered equal

I think this is exactly what we want. Previously the API specs said that the field must be present but may be null. Now the field is just optional and I'd interpret both the field missing and being null as the value not present.

I guess it is also common in JSON libraries to handle null and missing the same.

Or does anyone from @SpaceApi/core see any value in forbidding null / having null as a special sentinel value in certain cases?

But it seems the validator currently doesn't allow null for optional fields as it seems? Can we somehow make the validator accept null for optional fields?

rnestler commented 2 months ago

@dbrgn

If we can't fix the validator to accept null for optional fields I wouldn't include this change in v15 yet.

dbrgn commented 1 month ago

I guess it is also common in JSON libraries to handle null and missing the same.

No, that is not the case. null is a concrete value in JSON, and if we don't include null as a valid value for a key, it is not allowed as a value. null is not the same as a key missing.

In many schema based parsers, keys can be marked as optional (they can be omitted) and nullable (they may be set to null). One does not imply the other.

If we wanted to allow null for all optional values, we'd need to add null as a possible value type for all such fields in the JSON schema.

Please reconsider whether state.open should indeed be non-nullable. I think it's better the way it is. Or if you must change this anyway, please provide why this change is actually beneficial.

The schema specifies two pieces of information:

Allowing null as a value to indicate that a field is optional is mixing key requirements and value types. I'm quite convinced that having two different ways to mark values as missing (omitting the key and explicitly setting the key to null) is a bad thing™, unless there are semantics attached to one or the other.

(Of course a parser may be lenient in parsing, and may treat null and a missing key the same way. But the schema that describes a JSON document should be clear about it.)

rnestler commented 1 month ago

I guess it is also common in JSON libraries to handle null and missing the same.

No, that is not the case. null is a concrete value in JSON, and if we don't include null as a valid value for a key, it is not allowed as a value. null is not the same as a key missing.

I meant things like Serde JSON for example which handles it the same when using Option (playground):

#[derive(Debug, Deserialize)]
struct Foo {
    value: Option<f64>
}

fn main() {
    let x: Foo = serde_json::from_str("{\"value\": 1.0}").unwrap();
    println!("{:?}", x);
    let x: Foo = serde_json::from_str("{}").unwrap();
    println!("{:?}", x);
    let x: Foo = serde_json::from_str("{\"value\": null}").unwrap();
    println!("{:?}", x);
}

This will print Foo { value: None } for both absent and explicit null value.

But what I don't like is that we special cased state.open and I'd like to have it the same as every other optional field. Also I think from an efficiency point of view it's better to not transfer null values but just omit the whole keys.

(Of course a parser may be lenient in parsing, and may treat null and a missing key the same way. But the schema that describes a JSON document should be clear about it.)

This convinces me.

dbrgn commented 1 month ago

Yeah, Serde can be configured to treat null values as optional. But JSON Schema can't, and we'd need to include null as a value for every value that isn't listed in "required" 🙂

rnestler commented 1 month ago

@Juerd To answer your question

Or if you must change this anyway, please provide why this change is actually beneficial.

I think its beneficial for consistency reasons: Every optional thing in the SpaceAPI except for state.open was just marked as an optional key without allowing null. To avoid this special casing we first deprecated it in v14 and will remove it in v15.

IMO consumers of the API data (expect for the validator) should just threat null the same as missing to allow it to still work, but producers of the API data should avoid transmitting meaningless null values for consistency and efficiency reasons.

@SpaceApi/core Can you give a :+1: on this if you agree with the reasoning?

Juerd commented 1 month ago

Raphael Nestler skribis 2024-05-20 14:21 (-0700):

but producers of the API data should avoid transmitting meaningless null values for consistency and efficiency reasons.

Did you consider that for many hackerspaces, the only dynamic value in the JSON document will be state.open, and that the rest of the JSON document might have been produced by hand?

When simply generating a string, having the key with a value of null, true, or false can be done with simple string interpolation or concatenation with a single simple variable. To omit a key, though, you'd need an actual templating solution that can handle conditionals, or you'd need to actually generate the JSON with a JSON library.

For RevSpace, where SpaceAPI was invented, for a very long time, the "producer" of the JSON was indeed a static hand crafted string with a single value for the open key (which was not yet moved to state.open). While this is no longer the case and we generate a fresh JSON document each time, I think it makes sense to keep this strategy viable because it makes adoption of SpaceAPI simpler if you don't have to mess with the structure of the JSON, and only the value of the space state.

Even today, the only actually changing value in our SpaceAPI JSON is the space state.

I agree that perhaps the meaning for null and an absent value shouldn't be the same. In that case I suggest documenting null as an explicit way to indicate that the space state reporting functionality is intended to exists, but currently borken.

dbrgn commented 1 month ago

To omit a key, though, you'd need an actual templating solution that can handle conditionals, or you'd need to actually generate the JSON with a JSON library.

You didn't specify the technology you're using. I assume you're using a dynamic language. In case of JS, why shouldn't a ternary work?

> let open = undefined;
> JSON.stringify({name: "Some space", state: {message: "Open most weekends", ...(open !== undefined ? {open} : {})}})
'{"name":"Some space","state":{"message":"Open most weekends"}}'
> open = false;
> JSON.stringify({name: "Some space", state: {message: "Open most weekends", ...(open !== undefined ? {open} : {})}})
'{"name":"Some space","state":{"message":"Open most weekends","open":false}}'

Or in a less functional and more imperative way (but still far from using a templating system):

let open = true;
let schema = {name: "Some space", state: {message: "Open most weekends"}};
if (open !== undefined) {
    schema.state.open = open;
}

> JSON.stringify(schema)
'{"name":"Some space","state":{"message":"Open most weekends","open":true}}'

Or in Python:

>>> import json
>>> open = None
>>> json.dumps({
...     "name": "Some space",
...     "state": {
...         "message": "Open most weekends",
...         **({"open": open} if open != None else {}),
...     }
... })
'{"name": "Some space", "state": {"message": "Open most weekends"}}'
>>> open = False
>>> json.dumps({
...     "name": "Some space",
...     "state": {
...         "message": "Open most weekends",
...         **({"open": open} if open != None else {}),
...     }
... })
'{"name": "Some space", "state": {"message": "Open most weekends", "open": false}}'

I'm not a fan of ternaries myself. I think it is much cleaner and easier to read if you have a base object with the static data, and then add the dynamic data as needed.

>>> base = {"name": "Some space", "state": {"message": "Open most weekends"}}
>>> if open != None:
...     base["state"]["open"] = open
...
>>> json.dumps(base)
'{"name": "Some space", "state": {"message": "Open most weekends", "open": false}}'

Or even if you have a static JSON template file and prefer to run a shell script for every update:

$ echo '{"name": "Some space", "state": {"message": "Open most weekends"}}' > base.json
$ jq '.state.open = false' base.json
{
  "name": "Some space",
  "state": {
    "message": "Open most weekends",
    "open": false
  }
}
$ jq '.state.open = true' base.json
{
  "name": "Some space",
  "state": {
    "message": "Open most weekends",
    "open": true
  }
}

You can write the output into a schema.json file and serve that directly with a static web server.

In modern languages like Rust, you don't even need to write any code, a simple annotation on the struct is sufficient to control how optionals are serialized.

From a programming standpoint, I really don't see any problems with non-nullable state fields.

Juerd commented 1 month ago

Danilo Bargen skribis 2024-05-21 13:46 (-0700):

To omit a key, though, you'd need an actual templating solution that can handle conditionals, or you'd need to actually generate the JSON with a JSON library. You didn't specify the technology you're using.

Because that shouldn't be relevant. The case is generating a string, which can be done in any language, e.g. sh, C, etc.

Either way I'm unsubscribing from this issue as I've lost interest in discussing it. Feel free to close it if you don't see the point.

dbrgn commented 1 month ago

Because that shouldn't be relevant. The case is generating a string, which can be done in any language, e.g. sh, C, etc.

Yeah, that's why I listed a few different languages and techniques, with concrete examples on how to dynamically generate such a SpaceAPI string...

The premise of your change request was this:

(...) because the code for conditionally adding a key is much more involved than a simple ternary (...)

...and I showed a few examples how a key can be conditionally added using a ternary 🤷🏻‍♂️

I'll close this request since the discussion seems to have stalled. In summary:

In case someone has something substantial to add in favor of nullable fields in the schema, feel free to leave a comment. Otherwise, we'll probably go forward with the current v15 draft.