-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add support for handling of RegularExpression definitions for string based properties #426
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,6 +178,10 @@ class VSSDataDatatype(VSSData): | |
arraysize: int | None = None | ||
min: int | float | None = None | ||
max: int | float | None = None | ||
# Field, used to allow definition of Regular Expression constraints | ||
# for string based property nodes. | ||
# Example: VSS - VehicleIdentification.VIN property | ||
pattern: str | None = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would also need a model validator function for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you Sebastian, I will update the model. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
unit: str | None = None | ||
allowed: list[str | int | float | bool] | None = None | ||
default: list[str | int | float | bool] | str | int | float | bool | None = None | ||
|
@@ -277,6 +281,42 @@ def check_datatype_matching_allowed_unit_datatypes(self) -> Self: | |
), f"'{self.datatype}' is not allowed for unit '{self.unit}'" | ||
return self | ||
|
||
@model_validator(mode="after") | ||
def check_datatype_pattern(self) -> Self: | ||
""" | ||
Checks that regular expression datatype 'pattern' field is: | ||
|
||
1. defined only for string typed nodes i.e., STRING and STRING_ARRAY | ||
2. if default value(s) is provided, each default matching the specified pattern | ||
3. if allowed value(s) is provided, each allowed matching the specified pattern | ||
""" | ||
if self.pattern: | ||
# Datatypes.TUPLE[0] is the string name of the type. | ||
allowed_for = f"Allowed types: {[Datatypes.STRING[0], Datatypes.STRING_ARRAY[0]]}" | ||
assert Datatypes.get_type(self.datatype) in [ | ||
Datatypes.STRING, | ||
Datatypes.STRING_ARRAY, | ||
], f"Field 'pattern' is not allowed for type: '{self.datatype}'. {allowed_for}" | ||
|
||
def check_value_match(value_to_check: Any, value_type: str, reg_exp: str) -> None: | ||
check_values = [value_to_check] | ||
|
||
if type(value_to_check) is list: | ||
check_values = value_to_check | ||
|
||
for def_val in check_values: | ||
assert re.match( | ||
reg_exp, def_val | ||
), f"Specified '{value_type}' value: '{def_val}' must match defined pattern: '{self.pattern}'" | ||
|
||
if self.default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if default is a list we need to iterate and do It can be something like: check_values = [self.default]
if is_array(self.default):
check_values = self.default
for value in check_values:
assert re.match(self.pattern, value), f"...." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for this catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Since we are going to use patterns for string arrays, probably would never happen, but just in case I also added validation of allowed data. |
||
check_value_match(self.default, "default", self.pattern) | ||
|
||
if self.allowed: | ||
check_value_match(self.allowed, "allowed", self.pattern) | ||
|
||
return self | ||
|
||
|
||
class VSSDataProperty(VSSDataDatatype): | ||
pass | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
# Copyright (c) 2024 Contributors to COVESA | ||
# | ||
# This program and the accompanying materials are made available under the | ||
# terms of the Mozilla Public License 2.0 which is available at | ||
# https://www.mozilla.org/en-US/MPL/2.0/ | ||
# | ||
# SPDX-License-Identifier: MPL-2.0 | ||
|
||
import subprocess | ||
from pathlib import Path | ||
|
||
HERE = Path(__file__).resolve().parent | ||
TEST_UNITS = HERE / ".." / "test_units.yaml" | ||
TEST_QUANT = HERE / ".." / "test_quantities.yaml" | ||
|
||
|
||
# TODO-Kosta: | ||
# Test that: | ||
# | ||
# FAILS when other than string and string[] datatypes have defined pattern | ||
# | ||
# FAILS when pattern is defined and default is defined and default value does not match pattern | ||
# - there should be test for default and allowed values, | ||
# where allowed is mainly for string[] datatypes | ||
# | ||
# Add above three fail tests and 1 success test | ||
|
||
|
||
def test_datatype_pattern_wrong_type(tmp_path): | ||
spec = HERE / "test_pattern_wrong_datatype.vspec" | ||
output = tmp_path / "out.json" | ||
log = tmp_path / "log.txt" | ||
cmd = ( | ||
f"vspec --log-file {log} export json --pretty -u {TEST_UNITS} -q {TEST_QUANT} --vspec {spec} --output {output}" | ||
) | ||
process = subprocess.run(cmd.split()) | ||
assert process.returncode != 0 | ||
|
||
print(process.stdout) | ||
assert "Field 'pattern' is not allowed for type: 'uint16'. Allowed types: ['string', 'string[]']" in log.read_text() | ||
|
||
|
||
def test_datatype_pattern_no_match(tmp_path): | ||
def get_log_msg(value_type: str, value: str): | ||
return f"Specified '{value_type}' value: '{value}' must match defined pattern: '^[a-z]+$'" | ||
|
||
no_pattern_match_tests_to_run = { | ||
# Test #1: test no match of defined DEFAULT value | ||
"test_pattern_no_default_match.vspec": get_log_msg("default", "WrongLabel1"), | ||
# Test #2: test no match of defined DEFAULT list of values | ||
"test_pattern_no_default_array_match.vspec": get_log_msg("default", "Red"), | ||
# Test #3: test no match of defined ALLOWED value | ||
"test_pattern_no_allowed_match.vspec": get_log_msg("allowed", "Red"), | ||
# Test #4: test no match of defined ALLOWED list of values | ||
"test_pattern_no_allowed_array_match.vspec": get_log_msg("allowed", "Red"), | ||
} | ||
|
||
def run_no_match_pattern_test(test_name: str, log_message: str): | ||
spec = HERE / test_name | ||
output = tmp_path / "out.json" | ||
log = tmp_path / "log.txt" | ||
cmd = f"vspec --log-file {log} export json --pretty -u {TEST_UNITS} -q {TEST_QUANT}" | ||
cmd += f" --vspec {spec} --output {output} --strict" | ||
process = subprocess.run(cmd.split()) | ||
|
||
# Tested command should fail | ||
assert process.returncode != 0 | ||
# Check logged error message | ||
assert log_message in log.read_text() | ||
|
||
# Run no_pattern_match_tests_to_run | ||
for tst_name, tst_msg in no_pattern_match_tests_to_run.items(): | ||
run_no_match_pattern_test(tst_name, tst_msg) | ||
|
||
|
||
def test_datatype_pattern_ok(tmp_path): | ||
# Test #1: test no match of defined DEFAULT value | ||
spec = HERE / "test_pattern_ok.vspec" | ||
output = tmp_path / "out.json" | ||
log = tmp_path / "log.txt" | ||
cmd = f"vspec --log-file {log} export json --pretty -u {TEST_UNITS} -q {TEST_QUANT}" | ||
cmd += f" --vspec {spec} --output {output} --strict" | ||
process = subprocess.run(cmd.split()) | ||
assert process.returncode == 0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test no match of 'allowed' values with specified 'pattern' field. | ||
|
||
A.Colors: | ||
datatype: string | ||
type: attribute | ||
description: Simple Label for colors, which should be a simple collection of all lower case strings. | ||
Should fail on the 'allowed' 'Red' color label. | ||
pattern: ^[a-z]+$ | ||
allowed: [white, green, Red] | ||
default: white |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test no match of 'allowed' value with specified 'pattern' field. | ||
|
||
A.ErrorColors: | ||
datatype: string[] | ||
type: attribute | ||
description: Simple Label for ErrorColors, which should be a simple all lower case string. | ||
Should fail on the 'allowed' (only) 'Red' color label. | ||
pattern: ^[a-z]+$ | ||
allowed: [Red] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test no match of 'default' values with specified 'pattern' field. | ||
|
||
A.Colors: | ||
datatype: string[] | ||
type: attribute | ||
description: Simple Label for colors, which should be a simple collection of all lower case strings. | ||
Should fail on the 'Red' color label. | ||
pattern: ^[a-z]+$ | ||
default: [white, green, Red] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test no match of 'default' value with specified 'pattern' field. | ||
|
||
A.Label: | ||
datatype: string | ||
type: attribute | ||
description: Simple Label, which should be a simple all lower case string. | ||
pattern: ^[a-z]+$ | ||
default: WrongLabel1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test correct attribute (property) with defined 'pattern' field. | ||
|
||
A.Label: | ||
datatype: string | ||
type: attribute | ||
description: Simple Label, which should be a simple all lower case string with a number. | ||
pattern: ^[a-z0-9]+$ | ||
default: label1 | ||
|
||
A.Colors: | ||
datatype: string[] | ||
type: attribute | ||
description: Simple collection with colors, where each color should be a simple lower case string. | ||
pattern: ^[a-z]+$ | ||
allowed: [white, green, red, black, yellow, blue] | ||
default: [white, green, red] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# | ||
A: | ||
type: branch | ||
description: Branch A - used to test wrong usage of 'pattern' property for other than a string datatype. | ||
|
||
A.Year: | ||
datatype: uint16 | ||
type: attribute | ||
description: Unsigned Integer (number) for an Year attribute. | ||
Should fail for defined 'pattern' field because 'pattern' is allowed only for string types. | ||
pattern: ^[0-9]+$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the goal seems to be to check against attributes of the
VSSDataDatatype
node type it can probably be simplified to:Is
VSSDataProperty
not being covered on purpose?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
I will simplify this as advised.
On the question about: VSSDataProperty - I must have missed it there.
Initially I was looking for some VSSData... class for string properties where I can add the pattern field but I just found the main one: VSSDataDatatype.
If needed, I will double check and as asked will provide a validation of the pattern to be defined only for string types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to check them against
None
. So my implementation is not completely correct I suppose. At least themax
andmin
;-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will try to do my best here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @sschleemilch , on the question about VSSDataProperty. The code above was a bit obsolete, since I had below check:
which I guess adds for the VSSDataProperty check. However, since I think that all: sensor, actuator and attribute are children of the VSSDataDatatype, I updated the check as you advised and now it looks like:
I tested this on the whole VSS spect and it seems to be fine for every node which has min/max and pattern constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done