feat(protobuf): generate nested enum for string fields with allowed values#502
feat(protobuf): generate nested enum for string fields with allowed values#502aki1770-del wants to merge 5 commits intoCOVESA:masterfrom
Conversation
| nodes: tuple[VSSNode], fd: TextIOWrapper, static_uid: bool, add_optional: bool, include_comments: bool | ||
| ): | ||
| # Pass 1: write nested enum definitions for every string field with allowed values. | ||
| for node in nodes: |
There was a problem hiding this comment.
Why do we need to iterate twice?
| for node in nodes: | ||
| if isinstance(node.data, VSSDataDatatype): | ||
| base = node.data.datatype.strip("[]") | ||
| if base == "string" and node.data.allowed: |
There was a problem hiding this comment.
"string" literal will probably never change. Still use the the one from datatypes.Datatypes
…alues When a VSS signal has `datatype: string` and an `allowed` attribute, the protobuf exporter previously emitted a plain `string` field, allowing any value without enforcement. This silently accepted values outside the declared allowed set at the protobuf layer. Fix: `print_messages()` now uses a two-pass approach: - Pass 1: emit a nested `enum <FieldName>Enum` for each string+allowed field, with allowed values assigned indices starting at 0 (satisfying the proto3 zero-value requirement). - Pass 2: emit fields using the enum type instead of `string`. `repeated string[]` fields with `allowed` become `repeated <EnumType>`. Non-string fields and string fields without `allowed` are unchanged. Closes COVESA#493 Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
ruff-format reformatted the multi-line assert to put the condition arguments on their own line instead of wrapping in parentheses. Keeps pre-commit CI green. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
- Collapse two-pass loop into one: _write_nested_enum is called immediately before the field declaration, removing the redundant pre-iteration. - Replace "string" literal with Datatypes.STRING[0] from datatypes module. - Update expected.proto: enum definitions now appear inline before their respective fields rather than grouped at the top of the message. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
Move `from vss_tools.datatypes import Datatypes` before `from vss_tools.main import get_trees` — alphabetical within the vss_tools.* group as required by ruff/isort. Co-Authored-By: Claude and aki1770-del <aki1770@gmail.com> Signed-off-by: Akihiko Komada <aki1770@gmail.com>
3e69703 to
c151e10
Compare
| def _write_nested_enum(fd: TextIOWrapper, field_name: str, allowed: list, indent: str = " ") -> None: | ||
| """Write a nested proto3 enum for a string field constrained to allowed values. | ||
|
|
||
| Proto3 requires the first enum value to equal 0. We assign values in the |
There was a problem hiding this comment.
VSS supports the use of default for allowed values, see e.g. https://github.com/COVESA/vehicle_signal_specification/blob/b6e55988161ca67b20a3cdc3bdc1953c2faae4df/spec/Powertrain/CombustionEngine.vspec#L50
VSS in itself does not give the order for allowed any meaning. I took a quick look at the VSS catalog and in the few cases where we use both allowed and default the default value is the first item. But theoretically it could be something else and the question is then if the string specified as default better should be placed first, or if that just would make things more complex and error-prone.
Not saying that anythings needs to be changed in this PR, it was just a reflection I made.
|
Thanks @erikbosch — sharp catch on the proto3 zero-default ↔ VSS-default alignment. You're right that ordering To respect the scope of this PR (which you and @sschleemilch have already reviewed), I've filed it as a follow-up: #505 — happy to take it as a separate PR if the direction is confirmed there. AI-assisted — authored with Claude, reviewed by Komada. |
|
MoM:
|
Closes #493
Problem
When a VSS signal has
datatype: stringand anallowedattribute, the protobuf exporter emits a plainstringfield. This silently accepts any value at the protobuf layer — the constraint declared in the VSS spec is invisible to protobuf consumers.Fix
print_messages()now uses a two-pass approach insrc/vss_tools/exporters/protobuf.py:string+allowedfield, emit a nestedenum <FieldName>Enumwith allowed values assigned indices starting at 0 (satisfying the proto3 zero-value requirement).string.repeated string[]fields withallowedbecomerepeated <EnumType>. String fields withoutallowedand all non-string fields are unchanged.Note on proto3 zero-value semantics: proto3 uses the first enum entry as the default for unset fields. This implementation assigns index 0 to the first entry in the VSS
allowedlist. For signals that follow the convention of listingUNKNOWNfirst (e.g.,Vehicle.Exterior.RoadSurfaceCondition), this aligns naturally. For signals without a natural "unset" first value, the first allowed entry becomes the proto default. Happy to discuss if a different index assignment strategy is preferred.Changes
src/vss_tools/exporters/protobuf.py_enum_type_name(),_write_nested_enum(), two-pass logic inprint_messages()tests/vspec/test_protobuf_enum_allowed/tests/vspec/test_protobuf_comments/expected_*.protoStringWithAllowedfixtureTests
All 13 protobuf-related tests pass, including the two existing
test_protobuf_commentsparametrized cases (no-comments / with-comments) which already contained astring+allowedfield in their fixture.AI-assisted — authored with Claude, reviewed by Komada.