Use nested objects instead of dotted keys in JSON responses

[jfsiii]

Some JSON responses use a pattern like "obj.key": "value" to represent a property key in an object/map/dict obj. However, in JS/JSON this is parsed to a shallow object with a single key "obj.key"

{
  "obj.key": "value"
}

Instead, the response format should be like:

{
  "obj": {
    "key": "value"
  }
}

Taking https://epr.elastic.co/package/endpoint/0.17.0/ as an example

  "conditions": {
    "kibana.version": "^7.11.0"
  },

should be

  "conditions": {
    "kibana": {
      "version": "^7.11.0"
    }
  },

and

  "data_streams": [
    {
     ...
      "elasticsearch": {
        "index_template.mappings": {
          "dynamic": false
        },
        "ingest_pipeline.name": "default"
      },
    },
  ]

should be

  "data_streams": [
    {
     ...
      "elasticsearch": {
        "index_template": {
          "mappings": {
            "dynamic": false
          }
        },
        "ingest_pipeline": {
          "name": "default"
        }
      },
    },
  ]

In the data_streams case, this also matches the package-spec definitions for Elasticsearch assets

[ruflin]

Are you proposing to enforce the nested structure in the package definitions itself or only to expand it in the registry response? Supporting in the package itself is very convenient for users creating packages to not always have to use deep hierarchies to define things.

[jfsiii]

This ticket is about the inaccurate JSON response. As I wrote, the current response results in sibling properties, not child/nested ones.

e.g. current/undesirable:

{
  "a": 1,
  "obj.a": 21,
  "obj.b": 22,
  "c": 3
}

accurate

{
  "a": 1,
  "obj": {
    "a": 21,
    "b": 22,
  },
  "c": 3
}

The spec is for an object, as is the author's intent. YAML supports the dotted keys approach so that's appropriate when in YAML. However, the output must remain accurate when serializing to JS/JSON, which means converting as shown.

I'm not sure where the issue is. I expect the YAML & JSON conversion libs to support this already but perhaps they don't.

[jfsiii]

Having looked into this more deeply, it appears that YAML interprets the dotted key a.b simply as a string, not as a map a with key b

I cannot find any specs or parsers which interprets the dotted keys as an object. We've seen how the golang lib deals with them and the JS one used by Kibana does the same thing http://nodeca.github.io/js-yaml/

Is this a Beats convention? Is it meant to indicate an object?

The package spec requires an object, not a string, but if many integrations are already using the string, I'm not sure if we can deal with this in a backwards compatible way. I'll wait for confirmation on the intent and my observation before talking about how we might deal with it, but there definitely seems to be a mismatch between the spec and what's being authored & served.

[ruflin]

In Elasticsearch documents, a.b: c and a: { b: c } are the same thing. So the . indeed gets interpreted as an object. For a user in yaml I think it is very convention to be able to write foo.bar.baz: test instead of having to write:

foo:
  bar:
    baz: test

So I would like to keep this at least on the YAML side but we can "expand" it for JSON and also internal reading. It sounds like we need some custom code for this? How this works with the spec I let @ycombinator comment.

@jfsiii Haven't tried it out, but I guess this means the following would not be a "valid" KB config (taken from https://github.com/elastic/kibana/blob/master/config/kibana.yml#L31). I did not test it:

elasticsearch:
  hosts: ["http://localhost:9200"]
  username: "kibana_system"
  password: "pass"

[jfsiii]

@ruflin thanks for the reminder about kibana/config.yml. I'll look into how it's parsed.

[ycombinator]

We did run into a similar issue on the spec side. @mtojek enhanced the spec validation code to expand dotted keys to nested objects in elastic/package-spec#56. Specifically, see https://github.com/elastic/package-spec/pull/56/files#diff-3f9b15c2dadf2c828484212bfe4eba665707734dd6e17532223de27eb0d31506R58. Note that this enhancement only takes care of parsing package files for validation purposes. It does not actually alter the package files to change dotted keys to nested keys.

As @ruflin mentioned in #666 (comment), it would be convenient if we could allow package authors to use either dotted keys or nested objects in package files. However, I agree that either A) the package registry APIs should expand the dotted keys into nested objects when returning JSON responses or B) the calling client should perform the expansion upon receiving the JSON response. My personal preference is towards the former (option A) as it would benefit any/all clients and there's no actual use case for preserving dotted keys in the JSON responses anyway. So I'd be +1 to making this change in the package registry code rather than the Kibana code but curious to hear others thoughts as well!

[jfsiii]

I believe I found the function which expands the dotted properties into nested objects for config.yml

https://github.com/elastic/kibana/blob/1363a2aa73c8f4051041c49cf40950bd08dae36e/packages/kbn-config/src/raw/ensure_deep_object.ts#L22-L28

This means Fleet or other Kibana code should be able to use it as well.

I agree with @ycombinator's point that updating the registry response benefits all clients (as well as being accurate per spec), but Kibana will need knowledge of how to do this as well so it can process locally uploaded packages.

Perhaps this could be pushed into a function for the package-spec validators, but we'd still want a JS/TS version for elastic/package-spec#58

[ruflin]

@jfsiii Does this mean the Kibana config I posted above actually works?

Seems we all agree on expanding the json in the registry. @ycombinator Should we create an issue for it? @jfsiii Could this break things on the KB side?

[ycombinator]

Should we create an issue for it?

Could we reuse this issue itself? It's already in the package-registry repo. 🙂

[ruflin]

🤦‍♂️ ++

[mtojek]

I don't know if we can modify the API as it's already GA, but definitely it's something we can still discuss and decide.

[mtojek]

If this is only about JSON API, then we can already close this issue as it's implemented in #733.