Not all valid VSS paths supported by databroker

[SebastianSchildt]

Consider the following VSS tree (you can test this by hacking PR in #673 or by loading a generated VSS.json as --metadata with such paths

[
    {
        "path": "Vehicle._kuksa.databroker.CommitSha",
        "metadata": {
            "data_type": "STRING",
            "entry_type": "ATTRIBUTE",
            "description": "Commit SHA of current version"
        }
    },
    {
        "path": "Vehicle._kuksa.databroker.CargoVersion",
        "metadata": {
            "data_type": "STRING",
            "entry_type": "ATTRIBUTE",
            "description": "Databroker version as reported by GIT"
        }
    },
    {
        "path": "Vehicle._kuksa.databroker.GitVersion",
        "metadata": {
            "data_type": "STRING",
            "entry_type": "ATTRIBUTE",
            "description": "Databroker version as reported by GIT"
        }
    }
]

one will notice that direct access, e.g. getValue Vehicle._kuksa.databroker.GitVersion or getMetadata getValue Vehicle._kuksa.databroker.GitVersion is not possible leading to an error

Test Client> getValue Vehicle._kuksa.databroker.GitVersion
{
    "error": {
        "code": 400,
        "reason": "bad_request",
        "message": "Bad Path Request"
    },
    "errors": [
        {
            "path": "Vehicle._kuksa.databroker.GitVersion",
            "error": {
                "code": 400,
                "reason": "bad_request",
                "message": "Bad Path Request"
            }
        }
    ]
}

While at the same time it is possible to get the data using wildcards

Test Client> getValue Vehicle.**
[
    {
        "path": "Vehicle._kuksa.databroker.CommitSha",
        "value": {
            "value": "3cca2ba197316b31180c9d597bfbbd8d405fe467",
            "timestamp": "2023-09-26T17:16:11.420578+00:00"
        }
    },
    {
        "path": "Vehicle._kuksa.databroker.CargoVersion",
        "value": {
            "value": "0.4.0",
            "timestamp": "2023-09-26T17:16:11.420569+00:00"
        }
    },
    {
        "path": "Vehicle._kuksa.databroker.GitVersion",
        "value": {
            "value": "N/A",
            "timestamp": "2023-09-26T17:16:11.420394+00:00"
        }
    }
]

Test Client> 

This is definitely not consistent, so: Bug.

The root cause seem seems to be a check here https://github.com/eclipse/kuksa.val/blame/master/kuksa_databroker/databroker/src/glob.rs#L75 which restricts VSS paths to [A-Z][a-zA-Z0-9]*

This is in line with with VSS "recommendations", i.e. see here https://github.com/COVESA/vss-tools/blob/51bf94e3e07cf9501a373a3978fcd3fa636b510f/vspec/model/vsstree.py#L163

But it disallows valid VSS names. As databroker is also to be used with "non-standard", non COVESA models, I fell we should not restrict it. It is fine if vss-tools warn, that you are not following a style guide, but as a "servant" for VSS during runtime in databroker we should strive to support all valid VSS models.

Any (other) opinions/input here? @erikbosch @argerus ?

[argerus]

Just to clarify. This bug is specific to the kuksa.val.v1 interface, as it is doing it's own path validation (introduced as part of adding wildcard support).

Currently, all paths are considered valid by the databroker backend, so it is actually possible to access these paths just fine when using the sdv.databroker.v1 interface (or VISSv2).

For a gRPC request where the path is neatly contained in it's own field, it's possible to allow any valid string as path without causing issues. For json, as long as the path is correctly escaped, it also works just fine.

A better approach would (probably) be to do validation at the point of adding new entries to the backend (and not for every request). But it will most likely lead to issues to allow any character in a path when adding new entries (as is done now).

In other contexts, such as other text based protocols where the path is delimited by reserved characters, it will be more problematic. Or more relevant for databroker, when parsing paths as part of the scope string in an access token. If authorization is enabled, access tokens containing paths that are not compliant with the VSS recommendations will not be considered valid scopes, given the current parser implementation.

Escape sequences can be introduced to allow paths that contain "reserved" characters for a particular parser, but that adds complexity (if at all possible given the protocol). So it would probably be a lot easier to just specify what characters a valid path can contain.

But it disallows valid VSS names.

So, what exactly is a valid VSS name then, if not what the VSS recommendation says?

PS
If we want to introduce kuksa specific signals, why not place them under Kuksa instead?

I.e.
Kuksa.Databroker.Version

or

Vendor.Kuksa.Databroker.Version

Especially as they are not "Vehicle" signals.

[lukasmittag]

For the last part: This would collide with VSS itself because it allows only one super node like Vehicle. So it would be Vehicle.Kuksa.

[argerus]

For the last part: This would collide with VSS itself because it allows only one super node like Vehicle. So it would be Vehicle.Kuksa.

I didn't know that.

Where is this specified?

[lukasmittag]

I do not think/find specification but once tried adding some nodes Camera.xxx.xxx with the guys from a hackathon and at least vss tools complained and said there can only be one primary node to a tree. And for a tree this makes sense if you think of the shape :) @erikbosch maybe you can provide more insights?

[argerus]

Maybe it's just a recommendation 😄

[argerus]

Looking at it another way (especially, if we know that everything coming from a valid VSS model supplied to databroker will live under the Vehicle tree), what better way to guarantee that these internal attributes will not clash with anything in that tree, than to place them in a separate tree?

Then we don't need to add any funky underscores to convey that they are special (which might break clients that expects paths to only contain certain characters, and can still clash if we allow underscores generally).

These attributes are not part of any VSS model and they will not be processed by the vss-tools for example. They only depend on the interface used to interact with databroker supporting them.

[lukasmittag]

Yeah looking at tit that way - it makes sense. But from a VSS perspective there should be only one tree. Another way would be not putting it in as datapoints but as separate atributes in the proto file like another thing added to the API instead using datapoints. For example adding

struct DatabrokerInfo {
      version: int32
      .....
}

and a DatabrokerInfoRequest and DatabrokerInfoResponse

[argerus]

Kinda like this 😄

[lukasmittag]

Ah yes, true. @SebastianSchildt would that suit your use case if we extend it or something like this?

[SebastianSchildt]

Let me check this one by one:

VSS itself only requires "point seperated" paths and did not give any other hard guidlines. It accepts even path starting with numbers , or @erikbosch accepts your famous Vehicle.Do you not like smörgåstårta:

The recommendations are then followed by the standard catalogue, they are all deemed "useful" and reasonable, but for example it has rules like "all booleans should start with Is, where you could see somebody might want to follow a different/laxer rule in his own model.

For reference (although really not much is written there) check: https://covesa.github.io/vehicle_signal_specification/rule_set/basics/#addressing-nodes

So I think databroker should be "as relaxed as possible", with the possible exception of not allowing whitespaces (because as @argerus points out, whitespaces are effectively the delimiters in in our tokens).

I agree that any of those checks should be done at a central place (so i.e. when adding paths)

Wrt to the "multi-root" discussion
I did put the "special datapoints" in #673 under Vehicle. because I was in "VSS" mode, where indeed a single model can only have one route, but I think in databroker actually it would not hurt to put them "outside" the Vehicle root. The information could be put in a RPC as well (not sure "worth" it), what I currently did was just "rescuing" some code that was in hte hardcoded metadata anyway, with the sort of vague vision, that such a branch might later also be extended with e.g. information about registered providers or any other kind of interesting runtime information

[argerus]

(because as @argerus points out, whitespaces are effectively the delimiters in in our tokens)

Yes, whitespace ( ) would be problematic. And colons (:), as they separate the <PATH> part of a scope from the other parts.

And asterisks (*) would also be problematic with how wildcard support is defined in kuksa.val.v1. If they are a valid character in the actual path, it's hard to tell that apart from a wildcard pattern.

And VISSv2 has support for (?) in their wildcard patterns (I think, I'm not sure).

We also had a discussion around supporting negated paths in the access tokens (which would make a leading (!) problematic in a path). But that would be ok for now, as we don't support that.

[argerus]

But as we don't have any limitations in the backend now, deciding to not add any validation would not really change anything. These issues would only present themselves if anyone at some point tried to add some weird path to the model.

So I think it's rather a question of which is best:

• Do not validate, let users find out what breaks when they add uncommon characters in a path name.
• Validate, try to make sure everything supports the paths that are valid, and let users find out what paths they cannot use.

[SebastianSchildt]

@rafaeling The let's exclude during addEntry everything that has `` (whitespace) or : and `*` for now, and then probably not really need filtering on the "GRPC API" level, because if somebody were to request a path with ` ` or `:`via (GRPC) API it is perfectly fine, and would just not be found

[erikbosch]

In VSS documentation as of today there is mentioned here and there that the VSS standard catalog is a tree, but it is not described that you only can have a single tree within a *.vspec file. In VSS-tools converting *.vspec-files to JSON you will get an error if you have multiple roots, but that could theoretically be seen as a limitation as there are no strict rules defined. I will add an issue to VSS project to clarify the "single root policy". But even if VSS-tools only support a single tree at a time, there would not be a problem for KUKSA to handle multiple trees in parallel, possibly be reading multiple *.json files.

Concerning node names VSS is very liberal at the moment just saying that "The VSS specification must adhere to YAML syntax." which could give problems. A vspec/yaml file like below is AFAIK legal Yaml, but would give head ache in downstream implementations.

a:
  type: branch
  description: Branch A.


'a.Do you not like smörgåstårta':
  datatype: float
  type: sensor
  unit: km
  description: A sensor.
  comment: January is a great month!

I have already created an PR in COVESA/vehicle_signal_specification#651 proposing to elevate recommendations to rules, so that downstream tools does not need to support blanks or other odd characters in names

[rafaeling]

PR that fixes this issue: #678
Tested with new vss json file covering new corner cases

[SebastianSchildt]

Fixed in #678