From f09ccd8ea9e0eabc01eaeb8a1ba0d469a3fdef3b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 22 Feb 2023 12:14:14 +0100 Subject: [PATCH 1/3] doc/cli-guideline: Add JSON guideline --- doc/manual/src/contributing/cli-guideline.md | 104 +++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/doc/manual/src/contributing/cli-guideline.md b/doc/manual/src/contributing/cli-guideline.md index 01a1b1e73..c82021cc7 100644 --- a/doc/manual/src/contributing/cli-guideline.md +++ b/doc/manual/src/contributing/cli-guideline.md @@ -389,6 +389,110 @@ colors, no emojis and using ASCII instead of Unicode symbols). The same should happen when TTY is not detected on STDERR. We should not display progress / status section, but only print warnings and errors. +## Returning future proof JSON + +The machine-readable JSON output should be extensible. This means that the +structure of the JSON should support the addition of extra information in many +places. + +Two definitions are helpful here, because while JSON only defines one "key-value" +object, we use it to cover two use cases: + + - **dictionary**: a map from names to things that all have the same type. In + C++ this would be a `std::map` with string keys. + - **record**: a fixed set of attributes each with their own type. In C++, this + would be represented by a struct. + +It is best not to mix these use cases, as that leads to incompatibilities and +other bugs. For example, adding a record field to a dictionary breaks consumers +that assume all JSON object fields to have the same meaning and type. + +This leads to the following guidelines: + + - **The top-level value** (or **root** of the returned data structure) **must be a record**. + Without this rule, it would be impossible to add per-invocation metadata in + a manner that doesn't break existing consumers. + + - **The value of a dictionary item must always be a record**. As an example, + suppose a command returns a dictionary where each key is the name of a store + type and each value is itself a dictionary representing settings. + + - **List items should be records**. For example, a list of strings is not an + extensible type, as any additions will break code that expects a list of + strings. + If the list is unordered and it has a unique key that is a string, consider + a dictionary instead of a list. If the order of the items needs to be + preserved, return a list of records. + + - **Streaming JSON should return records**. An example of a streaming JSON + format is "JSON lines", where multiple JSON values are streamed by putting + each on its own line in a text stream. These JSON values can be considered + top-level values or list items, and they must be records. + +Examples: + +```javascript +// bad: all keys must be assumed to be store implementations +{ + "local": { ... }, + "remote": { ... }, + "http": { ... } +} +// good: extensible and a little bit self-documenting. +{ + "storeTypes": { "local": { ... }, ... }, + // While the dictionary of store types seemed like a complete response, + // this little bit of info tells the consumer how to proceed if a store type + // is missing. It's not always easy to predict how something will be used, so + // let's keep it open. + "pluginSupport": true +} +``` + +```javascript +// bad: a store type can only hold configuration items +{ + "storeTypes": { + "Local Daemon Store": { + "max-connections": { + "defaultValue": 1 + "value": 1 + }, + "trusted": { + "defaultValue": false, + "value": true + }, + ... + } + } +} +// good: a store type can be extended with other metadata, such as its URI scheme +{ + "storeTypes": { + "Local Daemon Store": { + "uriScheme": "daemon", + "settings": { + "max-connections": { + "defaultValue": 1 + "value": 1 + }, + ... + }, + ... + }, + ... +} +``` + +```javascript +// bad: not extensible +{ "outputs": [ "out" "bin" ] } +// bad: order matters but is lost, as many JSON parsers don't preserve item order. +{ "outputs": { "bin": {}, "out": {} } } +// good: +{ "outputs": [ { "outputName": "out" }, { "outputName": "bin" } ] } +``` + ## Dialog with the user CLIs don't always make it clear when an action has taken place. For every From 17f70b10bfb109dff9963adeb317ee0a5ff8563e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 27 Feb 2023 16:21:40 +0100 Subject: [PATCH 2/3] doc/cli-guideline: Apply suggestions from code review Thanks Valentin! --- doc/manual/src/contributing/cli-guideline.md | 46 +++++++++----------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/doc/manual/src/contributing/cli-guideline.md b/doc/manual/src/contributing/cli-guideline.md index c82021cc7..3e56d9037 100644 --- a/doc/manual/src/contributing/cli-guideline.md +++ b/doc/manual/src/contributing/cli-guideline.md @@ -391,45 +391,39 @@ status section, but only print warnings and errors. ## Returning future proof JSON -The machine-readable JSON output should be extensible. This means that the -structure of the JSON should support the addition of extra information in many -places. +The schema of JSON output should allow for backwards compatible extension. This section explains how to achieve this. Two definitions are helpful here, because while JSON only defines one "key-value" -object, we use it to cover two use cases: +object type, we use it to cover two use cases: - - **dictionary**: a map from names to things that all have the same type. In + - **dictionary**: a map from names to value that all have the same type. In C++ this would be a `std::map` with string keys. - **record**: a fixed set of attributes each with their own type. In C++, this - would be represented by a struct. + would be represented by a `struct`. -It is best not to mix these use cases, as that leads to incompatibilities and -other bugs. For example, adding a record field to a dictionary breaks consumers -that assume all JSON object fields to have the same meaning and type. +It is best not to mix these use cases, as that may lead to incompatibilities when the schema changes. For example, adding a record field to a dictionary breaks consumers that assume all JSON object fields to have the same meaning and type. This leads to the following guidelines: - - **The top-level value** (or **root** of the returned data structure) **must be a record**. - Without this rule, it would be impossible to add per-invocation metadata in - a manner that doesn't break existing consumers. + - The top-level (root) value must be a record. - - **The value of a dictionary item must always be a record**. As an example, - suppose a command returns a dictionary where each key is the name of a store - type and each value is itself a dictionary representing settings. + Otherwise, one can not change the structure of a command's output. - - **List items should be records**. For example, a list of strings is not an - extensible type, as any additions will break code that expects a list of - strings. - If the list is unordered and it has a unique key that is a string, consider - a dictionary instead of a list. If the order of the items needs to be - preserved, return a list of records. + - The value of a dictionary item must be a record. - - **Streaming JSON should return records**. An example of a streaming JSON - format is "JSON lines", where multiple JSON values are streamed by putting - each on its own line in a text stream. These JSON values can be considered - top-level values or list items, and they must be records. + Otherwise, the item type can not be extended. -Examples: + - List items should be records. + + Otherwise, one can not change the structure of the list items. + + If the order of the items does not matter, and each item has a unique key that is a string, consider representing the list as a dictionary instead. If the order of the items needs to be preserved, return a list of records. + + - Streaming JSON should return records. + + An example of a streaming JSON format is [JSON lines](https://jsonlines.org/), where each line represents a JSON value. These JSON values can be considered top-level values or list items, and they must be records. + +### Examples ```javascript // bad: all keys must be assumed to be store implementations From d0d0b9a748051e1957c5ba332b9eb74112c15570 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 28 Feb 2023 16:28:54 +0100 Subject: [PATCH 3/3] doc/cli-guideline: Improve examples Turns out that the settings themselves have a bad data model anyway, so we cut that. They do still occur in the first example, but not in focus. --- doc/manual/src/contributing/cli-guideline.md | 74 ++++++++------------ 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/doc/manual/src/contributing/cli-guideline.md b/doc/manual/src/contributing/cli-guideline.md index 3e56d9037..e53d2d178 100644 --- a/doc/manual/src/contributing/cli-guideline.md +++ b/doc/manual/src/contributing/cli-guideline.md @@ -425,65 +425,49 @@ This leads to the following guidelines: ### Examples -```javascript -// bad: all keys must be assumed to be store implementations + +This is bad, because all keys must be assumed to be store implementations: + +```json { "local": { ... }, "remote": { ... }, "http": { ... } } -// good: extensible and a little bit self-documenting. +``` + +This is good, because the it is extensible at the root, and is somewhat self-documenting: + +```json { "storeTypes": { "local": { ... }, ... }, - // While the dictionary of store types seemed like a complete response, - // this little bit of info tells the consumer how to proceed if a store type - // is missing. It's not always easy to predict how something will be used, so - // let's keep it open. "pluginSupport": true } ``` -```javascript -// bad: a store type can only hold configuration items -{ - "storeTypes": { - "Local Daemon Store": { - "max-connections": { - "defaultValue": 1 - "value": 1 - }, - "trusted": { - "defaultValue": false, - "value": true - }, - ... - } - } -} -// good: a store type can be extended with other metadata, such as its URI scheme -{ - "storeTypes": { - "Local Daemon Store": { - "uriScheme": "daemon", - "settings": { - "max-connections": { - "defaultValue": 1 - "value": 1 - }, - ... - }, - ... - }, - ... -} +While the dictionary of store types seems like a very complete response at first, a use case may arise that warrants returning additional information. +For example, the presence of plugin support may be crucial information for a client to proceed when their desired store type is missing. + + + +The following representation is bad because it is not extensible: + +```json +{ "outputs": [ "out" "bin" ] } ``` -```javascript -// bad: not extensible -{ "outputs": [ "out" "bin" ] } -// bad: order matters but is lost, as many JSON parsers don't preserve item order. +However, simply converting everything to records is not enough, because the order of outputs must be preserved: + +```json { "outputs": { "bin": {}, "out": {} } } -// good: +``` + +The first item is the default output. Deriving this information from the outputs ordering is not great, but this is how Nix currently happens to work. +While it is possible for a JSON parser to preserve the order of fields, we can not rely on this capability to be present in all JSON libraries. + +This representation is extensible and preserves the ordering: + +```json { "outputs": [ { "outputName": "out" }, { "outputName": "bin" } ] } ```