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

Add RegExp pattern for VehicleIdentification.VIN property. #786

Merged
merged 1 commit into from
Nov 7, 2024

Conversation

Kostadin-Ivanov
Copy link
Contributor

@Kostadin-Ivanov Kostadin-Ivanov commented Oct 28, 2024

Add Regular Expression pattern for the VehicleIdentification.VIN property.

This PR is dependent on COVESA VSS-Tools PR: 426

The idea is to provide a Regular Expression validation for the which is compliant with the mentioned in the property description - ISO 3779.

Provided RegExp - pattern is tested on: https://regex101.com/, with a test string of: 1CPH423GAP71E2745 and validates VIN strings as per quoted ISO 3779 on below rules:

1. VIN length of 17 chars where:
1.1 First 3 characters are for the VMI: 1CP
1.2 Next 6 characters are for the VDS: H423GA
1.3 Last 8 characters are for the VIS: P71E2745
2. The last 4 characters of the VIS part should always be numbers
3. Letters: I, O and Q shall not be used

NOTE: This update should be delivered AFTER the above mentioned COVESA VSS-Tools PR: 426 gets approved and released. Otherwise it will raise the following ERROR / WARNING:

WARNING: Unknown extra attribute: 'Vehicle.VehicleIdentification.VIN':'pattern'

@Kostadin-Ivanov
Copy link
Contributor Author

With regards to VSS-Tools PR: 426,

The failed check is exactly for VSS-Tools not "accepting" the pattern field:

CRITICAL Forbidden extra attributes detected:             main.py:252
                    [['Vehicle.VehicleIdentification.VIN',                      
                    'pattern']]     

I'd say that once the VSS-Tools PR: 426 gets approved and released, the failing check will pass.

@@ -55,6 +55,7 @@ VehicleIdentification.VIN:
datatype: string
type: attribute
description: 17-character Vehicle Identification Number (VIN) as defined by ISO 3779.
pattern: ^([0-9A-HJ-NPR-Z]{3})([0-9A-HJ-NPR-Z]{6})([0-9A-HJ-NPR-Z]{4}[0-9]{4})$
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea. Lets discuss the general idea of supporting/allowing a patternkeyword at the VSS meeting today.

If we agree that it is a good idea for cases like this then we need to look into details:

  • When should it be allowed, I assume only for string and string[] signals and struct properties.
  • Which syntax is supported - I think it would be good if we could specify something specific like that the pattern should comply with POSIX Basic Regular Expressions . At least for the standard catalog there is a value in selecting a standard/subset which is widely supported in different libraries so that it easily can be verified in VISS (Go), Kuksa (Rust/Python) and vss-tools (Python)

With those questions answered it would be easy to document this. One could also when discussing this think about #771. For VIN it is unlikely that we will have more than one signal with VIN-syntax, but for other use cases one could think of that we want to reuse the pattern, and if we introduce/refactor format-handling then one could in addition consider allowing pattern also for formats, for instance to give regexp that complies with iso8601 timestamps.

Copy link
Contributor Author

@Kostadin-Ivanov Kostadin-Ivanov Oct 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Erik.
I will add recommended validations in the related VSS-Tools PR, where @sschleemilch also advised to provide similar validation of the field in the VSS model.

With regards to iso8601- timestamps Regular Expressions, I already have some checks in the samm exporter so I can add it or try to provide a general RegExp in another PR, once the related VSS-Tools PR and this one get approved and delivered.

@erikbosch
Copy link
Collaborator

MoM:

  • Ulf: Creating a dependency might give problems if standard changes. A bit doubtful on putting it in the tree, could be put in documentation
  • Stefan: Validation proposal is good,
  • Daniel: Better to provide function that do the check, can be standardized for some types.
  • Erik: With format we can reuse
  • Erik: Can iso8601 be described using a regexp, or too many variants
  • Erik: Please review

@Kostadin-Ivanov
Copy link
Contributor Author

MoM:

* Ulf: Creating a dependency might give problems if standard changes. A bit doubtful on putting it in the tree, could be put in documentation

* Stefan: Validation proposal is good,

* Daniel: Better to provide function that do the check, can be standardized for some types.

* Erik: With format we can reuse

* Erik: Can iso8601 be described using a regexp, or too many variants

* Erik: Please review

Hello @erikbosch , I tried to check online about using Regular Expressions for datetime strings and found below two discussions on StackOverflow:

  1. Regex and ISO8601 formatted DateTime [duplicate]
  2. Regex validate correct ISO8601 date string with time

The first one has some good examples and the accepted answer in the second one provides very nice breakdown diagram of the corresponding RegExp.

With regards to the first comment, from Ulf, I'd say that we could just provide some general, in the case of iso8601 (date standard), regular expression and if user needs to update it or have some more strict one, they can always use the VSS Overlays to customize it.

Same can be said about the VIN number, although I doubt that there would be too many variants of VIN strings, but this is just my guess here.

@erikbosch
Copy link
Collaborator

MoM:

  • Ok to merge when vss-tools updated and test passes

Copy link
Collaborator

@erikbosch erikbosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@erikbosch erikbosch merged commit deb14f3 into COVESA:master Nov 7, 2024
5 checks passed
@Kostadin-Ivanov Kostadin-Ivanov deleted the add_vin_reg_exp_spec branch November 21, 2024 09:29
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.

2 participants