diff --git a/docs/developers-manual/mapping-documents/static-mapping-protobuf.md b/docs/developers-manual/mapping-documents/static-mapping-protobuf.md index 39b16251..7e37e46a 100644 --- a/docs/developers-manual/mapping-documents/static-mapping-protobuf.md +++ b/docs/developers-manual/mapping-documents/static-mapping-protobuf.md @@ -21,6 +21,54 @@ - `Option`s may be used to define additional metadata when protobuf lacks features to properly disambiguate the original IFEX concept, and of course comments can be generated to indicate the original feature meaning. - For other features, refer to the previous mappings, but in reverse. +## Field numbers and compatibility + +A Protobuf field number is the field's wire-format identity. It must remain +stable for the lifetime of a published message; declaration order is not an +identifier and reordering IFEX members or method arguments must not renumber +the generated Protobuf fields. + +The IFEX-to-Protobuf generator therefore requires `protobuf_tag` on each +struct member and on each method input, output, and return argument. The +argument tags are used for the fields in the generated request and response +messages. + +```yaml +structs: + - name: VehicleStatus + members: + - name: speed + datatype: uint32 + protobuf_tag: 1 + - name: state + datatype: string + protobuf_tag: 2 +``` + +Generation fails if a tag is missing, duplicated within a message, outside +the range `1..536870911`, or in Protobuf's implementation-reserved range +`19000..19999`. This prevents a newly inserted field from silently changing +the wire identity of existing fields. Protobuf input parsing preserves normal +and map-field numbers so that generated Protobuf can be round-tripped without +losing them. + +Do not renumber or reuse a tag after the interface has been published. To +deprecate a field while retaining wire compatibility, set +`protobuf_deprecated: true` on the member or method argument; the generated +field retains its tag and includes `[deprecated = true]`. + +After a field has been removed, reserve its former number and name on the +containing struct so neither can be reused: + +```yaml +protobuf_reserved_tags: [2] +protobuf_reserved_names: [legacy_state] +``` + +Generation rejects an active struct member that conflicts with either +reservation. Use deprecation while clients still use the field; use a +reservation only after the field is removed. + ## General A few idiosyncracies need to be dealt with: @@ -41,4 +89,3 @@ A few idiosyncracies need to be dealt with: - Similarly, IFEX does not provide standard error types, but the project community may still, optionally, agree on some standard definitions. If definitions such as `google.rpc.Status` are used in an interface definition then a standard translation of that might be created in an IFEX translation of the same. A more recommended approach might be to avoid using the make a definition that is independent of the protobuf/gRPC specific one when moving the interface to the more generic language (IFEX) as this would make things more natural when translating to other IPC/RPC protocols. - IFEX also has a richer `errors` concept, where multiple error types can be defined simultaneousy or through overlays, which enables for example separation of business-logic errors from transport errors. - diff --git a/ifex/models/ifex/ifex_ast.py b/ifex/models/ifex/ifex_ast.py index 4deaa07c..111eaba7 100644 --- a/ifex/models/ifex/ifex_ast.py +++ b/ifex/models/ifex/ifex_ast.py @@ -61,6 +61,12 @@ class Argument: range: Optional[str] = None + protobuf_tag: Optional[int] = None + """Stable Protobuf field number when this argument is generated into a Protobuf message.""" + + protobuf_deprecated: Optional[bool] = False + """Marks the generated Protobuf field as deprecated without changing its tag.""" + @dataclass class Error: @@ -309,6 +315,12 @@ class Member: This key is only allowed if the datatype element specifies an array (ending with []). """ + protobuf_tag: Optional[int] = None + """Stable Protobuf field number when this member is generated into a Protobuf message.""" + + protobuf_deprecated: Optional[bool] = False + """Marks the generated Protobuf field as deprecated without changing its tag.""" + @dataclass class Option: @@ -416,6 +428,12 @@ class Struct: members: Optional[List[Member]] = field(default_factory=EmptyList) """ Contains a list of members of a given struct. """ + protobuf_reserved_tags: Optional[List[int]] = field(default_factory=EmptyList) + """Previously used Protobuf field numbers that must not be reused.""" + + protobuf_reserved_names: Optional[List[str]] = field(default_factory=EmptyList) + """Previously used Protobuf field names that must not be reused.""" + @dataclass class Typedef: diff --git a/ifex/models/protobuf/protobuf_ast.py b/ifex/models/protobuf/protobuf_ast.py index 618bbff0..cbfd1caf 100644 --- a/ifex/models/protobuf/protobuf_ast.py +++ b/ifex/models/protobuf/protobuf_ast.py @@ -41,6 +41,8 @@ class EnumField: class Field: name: str datatype: str + number: Optional[int] = None + deprecated: Optional[bool] = False repeated: Optional[bool] = False optional: Optional[bool] = False required: Optional[bool] = False diff --git a/ifex/models/protobuf/protobuf_lark.py b/ifex/models/protobuf/protobuf_lark.py index 699c07be..934ac18d 100644 --- a/ifex/models/protobuf/protobuf_lark.py +++ b/ifex/models/protobuf/protobuf_lark.py @@ -68,6 +68,21 @@ def truncate_string(s, maxlen=77): else: return s +def parse_integer_literal(token): + """Convert a protobuf decimal, octal, or hexadecimal integer token.""" + literal = token.value + sign = -1 if literal.startswith('-') else 1 + unsigned_literal = literal[1:] if sign == -1 else literal + + if unsigned_literal.lower().startswith('0x'): + base = 16 + elif len(unsigned_literal) > 1 and unsigned_literal.startswith('0'): + base = 8 + else: + base = 10 + + return sign * int(unsigned_literal, base) + # PATTERN MATCHING # # Here we build a set of functions that will take a pattern token-tree @@ -377,8 +392,8 @@ def process_map_field(f): assert_token(next_node, 'IDENT') fieldname = next_node.value - # --- 5 field number (thrown away, for now) - f.children.pop(0) + # --- 5 field number + fieldnumber = parse_integer_literal(f.children.pop(0)) # --- 5 field options --- options = [] @@ -389,10 +404,9 @@ def process_map_field(f): #options.append(process_field_option(o)) options.append(process_option(o)) - # NOTE: The field number follows next, but is discarded until - # we find a reason to keep it - see comments in design document. return Field(name = fieldname, datatype = "map<" + keytype + "," + valuetype + ">", + number = fieldnumber, options = options) @@ -433,8 +447,8 @@ def process_field(f): assert_token(next_node, 'IDENT') fieldname = next_node.value - # --- 4 field number (thrown away, for now) - f.children.pop(0) + # --- 4 field number + fieldnumber = parse_integer_literal(f.children.pop(0)) # --- 5 field options --- options = [] @@ -446,10 +460,9 @@ def process_field(f): options.append(process_option(o)) - # NOTE: The field number follows next, but is discarded until - # we find a reason to keep it - see comments in design document. return Field(name = fieldname, datatype = fieldtype, + number = fieldnumber, repeated = repeated, optional = optional, required = required, @@ -531,7 +544,7 @@ def process_message(m): # === Create Message object in AST, and add to list === return Message(name = msg_name, - fields = ast_fields, + fields = ast_fields + ast_mfields, messages = ast_messages, enums = ast_enums) diff --git a/ifex/output_filters/protobuf/ifex_to_protobuf.py b/ifex/output_filters/protobuf/ifex_to_protobuf.py index ab7867f9..da12628c 100644 --- a/ifex/output_filters/protobuf/ifex_to_protobuf.py +++ b/ifex/output_filters/protobuf/ifex_to_protobuf.py @@ -88,6 +88,88 @@ def construct_response_name(s): def construct_request_name(s): return target_style(s+'Request') + +PROTOBUF_MAX_FIELD_NUMBER = 536870911 +PROTOBUF_RESERVED_FIELD_NUMBER_START = 19000 +PROTOBUF_RESERVED_FIELD_NUMBER_END = 19999 + + +def validate_message_field_numbers(message): + """Validate that a generated message has stable, valid Protobuf field numbers.""" + used_numbers = set() + used_names = set() + for field in message.fields or []: + number = field.number + if isinstance(number, bool) or not isinstance(number, int): + raise ValueError( + f'Message {message.name} field "{field.name}" has no protobuf_tag. ' + 'Protobuf tags must be explicit and stable; field order must not assign them.' + ) + if number < 1 or number > PROTOBUF_MAX_FIELD_NUMBER: + raise ValueError( + f'Message {message.name} field "{field.name}" has invalid protobuf_tag {number}; ' + f'expected a number between 1 and {PROTOBUF_MAX_FIELD_NUMBER}.' + ) + if PROTOBUF_RESERVED_FIELD_NUMBER_START <= number <= PROTOBUF_RESERVED_FIELD_NUMBER_END: + raise ValueError( + f'Message {message.name} field "{field.name}" uses protobuf_tag {number}, ' + f'which is reserved by Protobuf ({PROTOBUF_RESERVED_FIELD_NUMBER_START}--' + f'{PROTOBUF_RESERVED_FIELD_NUMBER_END}).' + ) + if number in used_numbers: + raise ValueError( + f'Message {message.name} has duplicate protobuf_tag {number} ' + f'(including field "{field.name}").' + ) + used_numbers.add(number) + used_names.add(field.name) + + reserved_numbers = set() + reserved_names = set() + for reservation in message.reservations or []: + for range_ in reservation.ranges or []: + if range_.single_value is None: + raise ValueError(f'Message {message.name} has an unsupported protobuf reserved range.') + number = int(range_.single_value) + if number < 1 or number > PROTOBUF_MAX_FIELD_NUMBER: + raise ValueError( + f'Message {message.name} has invalid reserved protobuf_tag {number}; ' + f'expected a number between 1 and {PROTOBUF_MAX_FIELD_NUMBER}.' + ) + if number in reserved_numbers: + raise ValueError(f'Message {message.name} reserves protobuf_tag {number} more than once.') + reserved_numbers.add(number) + for name in reservation.strfieldnames or []: + if name in reserved_names: + raise ValueError(f'Message {message.name} reserves field name "{name}" more than once.') + reserved_names.add(name) + + conflicts = used_numbers & reserved_numbers + if conflicts: + raise ValueError(f'Message {message.name} uses reserved protobuf_tag {min(conflicts)}.') + name_conflicts = used_names & reserved_names + if name_conflicts: + raise ValueError(f'Message {message.name} uses reserved field name "{sorted(name_conflicts)[0]}".') + + for nested_message in message.messages or []: + validate_message_field_numbers(nested_message) + + +def validate_proto_field_numbers(proto): + for message in proto.messages or []: + validate_message_field_numbers(message) + + +def construct_reservations(struct, _): + tags = struct.protobuf_reserved_tags or [] + names = struct.protobuf_reserved_names or [] + if not tags and not names: + return None + return [protobuf.Reserved( + ranges=[protobuf.Range(single_value=str(tag), range_start=None, range_end=None) for tag in tags], + strfieldnames=names, + )] + def if_to_services(interface): res = m2m.transform(mapping_table, interface) # Protobuf model expects a *list* of services. We expect _one_ interface node to be specified (per namespace) in a @@ -144,6 +226,8 @@ def define_rpcs(methods): Default : [ ('datatype', 'datatype', translate_type_name), ('name', 'name'), + ('protobuf_tag', 'number'), + ('protobuf_deprecated', 'deprecated'), ], (ifex.Namespace, protobuf.Proto): [ @@ -178,6 +262,7 @@ def define_rpcs(methods): (ifex.Struct, protobuf.Message): [ ('members', 'fields'), + (construct_reservations, 'reservations'), # TODO Handle variant types # 'datatypes' --> handle_variant_type() ], @@ -226,6 +311,7 @@ def ifex_to_proto(namespace): [m2m.transform(mapping_table, arg) for arg in m.returns] proto.messages.append(protobuf.Message(name = construct_response_name(m.name), fields = fields)) + validate_proto_field_numbers(proto) return proto # --- Script entry point = Test/Dev Code --- diff --git a/ifex/output_filters/protobuf/templates/Field.j2 b/ifex/output_filters/protobuf/templates/Field.j2 index 42f74039..9215fc02 100644 --- a/ifex/output_filters/protobuf/templates/Field.j2 +++ b/ifex/output_filters/protobuf/templates/Field.j2 @@ -1,9 +1,7 @@ -{% if item.repeated %}repeated {% endif %}{% if item.optional %}optional {% endif %}{{ item.datatype }} {{ item.name }} = 0 -{%- if item.options %} +{% if item.repeated %}repeated {% endif %}{% if item.optional %}optional {% endif %}{{ item.datatype }} {{ item.name }} = {{ item.number }}{% if item.deprecated %} [deprecated = true]{% endif %}{% if item.options %} [ - {% for option in item.options %} + {% for option in item.options or [] %} {{ gen(option) }}{{ "" if loop.last else ", " }} {% endfor %} ] -{% endif %}; - +{% endif %};{{ "\n" }} diff --git a/ifex/output_filters/protobuf/templates/Range.j2 b/ifex/output_filters/protobuf/templates/Range.j2 index a5362490..fb0050b9 100644 --- a/ifex/output_filters/protobuf/templates/Range.j2 +++ b/ifex/output_filters/protobuf/templates/Range.j2 @@ -1,5 +1,5 @@ {% if item.single_value %} -{{ item.single_value }} +{{ item.single_value -}} {% else %} -{{ item.range_start }} to {{ item.range_end }} +{{ item.range_start }} to {{ item.range_end -}} {% endif %} diff --git a/ifex/output_filters/protobuf/templates/Reserved.j2 b/ifex/output_filters/protobuf/templates/Reserved.j2 index 14fc91d7..67fa5b47 100644 --- a/ifex/output_filters/protobuf/templates/Reserved.j2 +++ b/ifex/output_filters/protobuf/templates/Reserved.j2 @@ -1,8 +1,2 @@ -reserved { -{% for range in item.ranges %} - {{ gen(range) }}; -{% endfor %} -{% for name in item.strfieldnames %} - "{{ name }}"; -{% endfor %} -} +{% if item.ranges %}reserved {% for range in item.ranges %}{{ gen(range) }}{{ "" if loop.last else ", " }}{% endfor %};{{ "\n" }}{% endif %} +{% if item.strfieldnames %}reserved {% for name in item.strfieldnames %}"{{ name }}"{{ "" if loop.last else ", " }}{% endfor %};{{ "\n" }}{% endif %} diff --git a/tests/ifex_to_protobuf_field_tags_test.py b/tests/ifex_to_protobuf_field_tags_test.py new file mode 100644 index 00000000..80c495ec --- /dev/null +++ b/tests/ifex_to_protobuf_field_tags_test.py @@ -0,0 +1,114 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 +# SPDX-License-Identifier: MPL-2.0 + +import unittest + +from ifex.models.ifex.ifex_ast import Argument, Member, Method, Namespace, Struct +from ifex.output_filters.protobuf.ifex_to_protobuf import ifex_to_proto +from ifex.output_filters.protobuf.grpc_generator import proto_to_text + + +class IfexToProtobufFieldTagsTest(unittest.TestCase): + def test_struct_member_tags_are_preserved_when_members_are_reordered(self): + namespace = Namespace( + name="example", + structs=[Struct(name="Status", members=[ + Member(name="state", datatype="string", protobuf_tag=2), + Member(name="speed", datatype="uint32", protobuf_tag=1), + ])], + ) + + proto = ifex_to_proto(namespace) + + self.assertEqual( + [(field.name, field.number) for field in proto.messages[0].fields], + [("state", 2), ("speed", 1)], + ) + + def test_generated_request_and_response_messages_require_argument_tags(self): + namespace = Namespace( + name="example", + methods=[Method( + name="get_status", + input=[Argument(name="id", datatype="uint32", protobuf_tag=2)], + returns=[Argument(name="state", datatype="string", protobuf_tag=1)], + )], + ) + + proto = ifex_to_proto(namespace) + + self.assertEqual( + [(message.name, [(field.name, field.number) for field in message.fields]) + for message in proto.messages], + [("GetStatusRequest", [("id", 2)]), + ("GetStatusResponse", [("state", 1)])], + ) + + def test_missing_tag_fails_before_rendering(self): + namespace = Namespace( + name="example", + structs=[Struct(name="Status", members=[Member(name="state", datatype="string")])], + ) + + with self.assertRaisesRegex(ValueError, 'has no protobuf_tag'): + ifex_to_proto(namespace) + + def test_duplicate_and_reserved_tags_fail_before_rendering(self): + duplicate_namespace = Namespace( + name="example", + structs=[Struct(name="Status", members=[ + Member(name="state", datatype="string", protobuf_tag=1), + Member(name="speed", datatype="uint32", protobuf_tag=1), + ])], + ) + reserved_namespace = Namespace( + name="example", + structs=[Struct(name="Status", members=[ + Member(name="state", datatype="string", protobuf_tag=19000), + ])], + ) + + with self.assertRaisesRegex(ValueError, 'duplicate protobuf_tag 1'): + ifex_to_proto(duplicate_namespace) + with self.assertRaisesRegex(ValueError, 'reserved by Protobuf'): + ifex_to_proto(reserved_namespace) + + def test_deprecated_member_and_reservations_are_rendered(self): + namespace = Namespace( + name="example", + structs=[Struct( + name="Status", + protobuf_reserved_tags=[2], + protobuf_reserved_names=["legacy_state"], + members=[Member( + name="old_state", + datatype="string", + protobuf_tag=1, + protobuf_deprecated=True, + )], + )], + ) + + generated = proto_to_text(ifex_to_proto(namespace)) + + self.assertIn('string old_state = 1 [deprecated = true];', generated) + self.assertIn('reserved 2;', generated) + self.assertIn('reserved "legacy_state";', generated) + + def test_active_member_cannot_use_a_reserved_tag_or_name(self): + namespace = Namespace( + name="example", + structs=[Struct( + name="Status", + protobuf_reserved_tags=[1], + protobuf_reserved_names=["state"], + members=[Member(name="state", datatype="string", protobuf_tag=1)], + )], + ) + + with self.assertRaisesRegex(ValueError, 'uses reserved protobuf_tag 1'): + ifex_to_proto(namespace) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/testdefs/protobuf_in_out_roundtrip/inputs/one b/tests/testdefs/protobuf_in_out_roundtrip/inputs/one index d190ef74..c5f0dd3e 100644 --- a/tests/testdefs/protobuf_in_out_roundtrip/inputs/one +++ b/tests/testdefs/protobuf_in_out_roundtrip/inputs/one @@ -11,10 +11,9 @@ message HealthState { enum State { S_UNSPECIFIED = 0; S_FAULT_PRESENT = 5; S_UNSUPPORTED = 6; } - optional int32 remaining_life = 0; - State state = 0; + optional int32 remaining_life = 7; + State state = 42; + map status_by_name = 0x2b; } - - diff --git a/tests/testdefs/protobuf_in_out_roundtrip/results/one.out b/tests/testdefs/protobuf_in_out_roundtrip/results/one.out index aef473af..39801d5d 100644 --- a/tests/testdefs/protobuf_in_out_roundtrip/results/one.out +++ b/tests/testdefs/protobuf_in_out_roundtrip/results/one.out @@ -13,6 +13,7 @@ message HealthState { S_FAULT_PRESENT = 5; S_UNSUPPORTED = 6; } - optional int32 remaining_life = 0; - State state = 0; + optional int32 remaining_life = 7; + State state = 42; + map status_by_name = 43; }