Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Added struct default validation #425

Merged

Conversation

sschleemilch
Copy link
Collaborator

@sschleemilch sschleemilch commented Oct 25, 2024

Adding default validation support for structs

  • Generating json schemas dynamically for structs
  • We can use those schemas then to validate the default input
  • Arrays supported

Also added additional validations

  • min/max only on numeric datatypes
  • default being in min/max boundary
  • forbidding allowed to be used with structs

Example:

types.vspec

Types:
  type: branch
  description: Types

Types.B:
  type: struct
  description: B

Types.B.x:
  type: property
  description: bx
  datatype: boolean[]

Types.B.y:
  type: property
  description: by
  datatype: numeric

Types.A:
  type: struct
  description: A

Types.A.x:
  type: property
  datatype: uint8
  description: ax

Types.A.y:
  type: property
  datatype: string[]
  description: ay

Types.A.z:
  type: property
  datatype: B[]
  description: az

model.vspec with violation (x is declared as uint8):

A:
  type: branch
  description: A

A.X:
  type: attribute
  datatype: Types.A[]
  description: X
  default:
    - x: not a uint8
      y:
      - a
      - b
      z:
        x:
        - true
        y: 300.3
vspec export tree -s model.vspec -t types.vspec

Result:
image

Fixes #423

@sschleemilch sschleemilch force-pushed the feature/default-struct-checking branch 4 times, most recently from bb726cc to f368391 Compare October 25, 2024 13:22
@sschleemilch sschleemilch force-pushed the feature/default-struct-checking branch 3 times, most recently from 4224fe8 to d7cdc95 Compare October 25, 2024 15:24
Signed-off-by: Sebastian Schleemilch <[email protected]>
@sschleemilch sschleemilch force-pushed the feature/default-struct-checking branch from d7cdc95 to bd20706 Compare October 25, 2024 16:02
@erikbosch
Copy link
Collaborator

Great! Lets discuss on the meeting on Tuesday if we want to allow default for structs. The original limitation was added just as a limitation as we did not at that time prioritized to focus on how to define defaults, so there were no "political" reason to not allow default values.

@UlfBj - as you initiated this (in some way) - could you provide some examples on how you want to use default values for structs, so that we can check if it fits proposed syntax and validation

@sschleemilch
Copy link
Collaborator Author

Great! Lets discuss on the meeting on Tuesday if we want to allow default for structs. The original limitation was added just as a limitation as we did not at that time prioritized to focus on how to define defaults, so there were no "political" reason to not allow default values.

@UlfBj - as you initiated this (in some way) - could you provide some examples on how you want to use default values for structs, so that we can check if it fits proposed syntax and validation

I think the complete default functionality is discussable whether we want to have it at all. In my opinion it is out of scope for modelling the structure of data. It mixes defining a data structure and actually using it

@erikbosch
Copy link
Collaborator

I think the complete default functionality is discussable whether we want to have it at all. In my opinion it is out of scope for modelling the structure of data. It mixes defining a data structure and actually using it

Historically we have promoted default primarily for attributes that change rarely. The idea has been that implementations (like VISSR, Kuksa) shall use default value unless any other value is given. In some cases like Vehicle.VersionVSS.Major the implementation will likely never give the value by some other mechanism. In many other cases it can be argued if the standard catalog should contain default values or not, some of the existing ones seems quite useless.

But I think there is a value in defining a syntax, so that the same spec with default info can be used both for VISSR and Kuksa. But I am open to if it we should consider it to be part of "core CSS syntax", or just a "VSS profile for default", similar to how we sometimes discuss if there should be "standardized" "VSS DBC profile" or "VSS Implementation Type profile" that can tools can decide to support or not.

@sschleemilch
Copy link
Collaborator Author

sschleemilch commented Oct 29, 2024

https://raw.githack.com/COVESA/vehicle-information-service-specification/main/spec/VISSv3.0_TransportExamples.html#wss-search-read

Yeah, maybe we can distinguish that a bit.
It feels a bit like an init file then that is being passed to services that work with vss.
But It would not need the full blown vss to do that. On the other hand it probably does not hurt to have them supported.
Users could then always define overlays to set default values or more like attributes of the vehicle.

@UlfBj
Copy link
Contributor

UlfBj commented Oct 29, 2024

Example:

DownloadFile:
  type: actuator
  datatype: Types.Resources.FileDescriptor
  default: {"name":"downloadfile.txt", "hash":"20e87e71b6948d6e6dd11d776e9be79c374751bb", "uid":"1d878212")
  description: File to be used by the vehicle. Default contains internal filesystem path.

UploadFile:
  type: sensor
  datatype: Types.Resources.FileDescriptor
  default: {"name":"uploadfile.txt", "hash":"20e87e71b6948d6e6dd11d776e9be79c374751bb", "uid":"1d878212")
  description: File created by the vehicle. Default contains internal filesystem path incl filename.

@erikbosch
Copy link
Collaborator

MoM:

  • Please review
  • Erik: Please comment if you have objections supporting it for struct

@sschleemilch
Copy link
Collaborator Author

Example:

DownloadFile:
  type: actuator
  datatype: Types.Resources.FileDescriptor
  default: {"name":"downloadfile.txt", "hash":"20e87e71b6948d6e6dd11d776e9be79c374751bb", "uid":"1d878212")
  description: File to be used by the vehicle. Default contains internal filesystem path.

UploadFile:
  type: sensor
  datatype: Types.Resources.FileDescriptor
  default: {"name":"uploadfile.txt", "hash":"20e87e71b6948d6e6dd11d776e9be79c374751bb", "uid":"1d878212")
  description: File created by the vehicle. Default contains internal filesystem path incl filename.

It works fine except I do not understand why JSON is being used 😕 :

MODEL:

Vehicle:
  type: branch
  description: Vehicle

Vehicle.DownloadFile:
  type: actuator
  datatype: Types.Resources.FileDescriptor
  default: 
    name: downloadfile.txt
    hash: 20e87e71b6948d6e6dd11d776e9be79c374751bb
    uid: 1d878212
  description: File to be used by the vehicle. Default contains internal filesystem path.

Vehicle.UploadFile:
  type: sensor
  datatype: Types.Resources.FileDescriptor
  default: 
    name: uploadfile.txt
    hash: 20e87e71b6948d6e6dd11d776e9be79c374751bb
    uid: 1d878212
  description: File created by the vehicle. Default contains internal filesystem path incl filename.

TYPES:

Types:
  type: branch
  description: Types

Types.Resources:
  type: branch
  description: Resources

Types.Resources.FileDescriptor:
  type: struct
  description: File Descriptor

Types.Resources.FileDescriptor.name:
  type: property
  datatype: string
  description: Name

Types.Resources.FileDescriptor.hash:
  type: property
  datatype: string
  description: Hash

Types.Resources.FileDescriptor.uid:
  type: property
  datatype: string
  description: UID

Works fine and when e.g. removing name:
image

@erikbosch
Copy link
Collaborator

MoM : ok to merge

@erikbosch erikbosch merged commit 8105691 into COVESA:master Nov 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allowing default in structs
3 participants