Preserve stable Protobuf field tags and support field lifecycle management

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.
-

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:

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

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)
 

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 ---

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" }}

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 %}

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 %}