Skip to content
This repository was archived by the owner on Dec 18, 2024. It is now read-only.

Throw an error while reading unset values. #303

Merged

Conversation

nayakned
Copy link
Contributor

getValue on values not explicitly set should throw an error.

This should implement #297

Signed-off-by: nayakned <naresh.nayak@de.bosch.com>
@nayakned nayakned marked this pull request as draft July 22, 2022 12:03
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
Signed-off-by: Sebastian Schildt <sebastian.schildt@de.bosch.com>
@SebastianSchildt
Copy link
Contributor

Tested with websocket and GRPC. Seems to work fine.
I wonder whether we should send another code than 400, but did not find a fitting one on a first glance (definitely not 404, because we use that to indicate the path is no valid). 400 is fine, but generic, and probably used more often. I am wondering if it is easier if client can easily detect this special condition (nothing published yet) by just looking at the number

@SebastianSchildt
Copy link
Contributor

SebastianSchildt commented Jul 22, 2022

Hmm wrt to 404 it seems VISS suggests it https://www.w3.org/TR/viss2-transport/#status-codes

maybe we should get more standard compliant (yeah I know, our other error msgs are not, good way to strart :) )

so current PR does

{
  "action": "get",
  "error": {
    "message": "Attribute value on Vehicle/OBD/Speed has not been set yet.",
    "number": "400",
    "reason": "Value not set"
  },
  "requestId": "3220b1d0-6e6e-4f31-b6f9-7aa64ff4b0f2",
  "ts": "2022-07-22T13:57:59.1658498279Z"
}

according to spec we should do

{
  "action": "get",
  "error": {
    "message": "Attribute value on Vehicle/OBD/Speed has not been set yet.",
    "number": "404",
    "reason": "unavailable_data"
  },
  "requestId": "3220b1d0-6e6e-4f31-b6f9-7aa64ff4b0f2",
  "ts": "2022-07-22T13:57:59.1658498279Z"
}

The spec demands "message" as "The requested data was not found." , but I suggest not following that, because ours is more expressive. But number and reason

@nayakned
Copy link
Contributor Author

@SebastianSchildt This exception will also be thrown when a entire branch is being read, and even if one of the leaves has not been previously set. E.g. getValue on Vehicle.OBD.* will throw this error as soon as it encounters the first datapoint that hasnt been set.

Would that be ok?

@nayakned nayakned marked this pull request as ready for review July 27, 2022 19:43
@SebastianSchildt
Copy link
Contributor

@SebastianSchildt This exception will also be thrown when a entire branch is being read, and even if one of the leaves has not been previously set. E.g. getValue on Vehicle.OBD.* will throw this error as soon as it encounters the first datapoint that hasnt been set.

Would that be ok?

Guess that is ok. We never promised anything regarding that behaviour (it seems to be one of this "atomic or not" discussions. Most common usecase is querying single values, and if you DO query a branch, most likely it is required/expected to get all subelements to be useful at all (e.g. GPS position), so I think it is toally acceptable to bail out if not all of the requested data can be provided.

@SebastianSchildt
Copy link
Contributor

Checked WS and GRPC, works. Will merge & squash as you suggested

@SebastianSchildt SebastianSchildt merged commit 34844f4 into eclipse-archived:master Jul 28, 2022
@nayakned nayakned deleted the fix/error_no_set_read branch July 29, 2022 14:37
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants