Skip to content

Preserve unknown singular enum values in unknown fields in Swift - #3708

Open
loganblevins wants to merge 1 commit into
square:masterfrom
loganblevins:loganblevins/swift-preserve-singular-unknown-enums
Open

Preserve unknown singular enum values in unknown fields in Swift#3708
loganblevins wants to merge 1 commit into
square:masterfrom
loganblevins:loganblevins/swift-preserve-singular-unknown-enums

Conversation

@loganblevins

@loganblevins loganblevins commented Sep 2, 2026

Copy link
Copy Markdown
Member

Under ProtoDecoder's .returnNil strategy, an unrecognized value in a singular (or oneof) enum field is read and discarded: it never reaches unknownFields, so reencoding the message silently drops the field. This diverges from:

  • the same strategy's handling of repeated and map enum fields, which already preserve unrecognized values in unknown fields (ProtoReader.decode(into:));
  • generated Kotlin/Java, which catches EnumConstantNotFoundException and calls addUnknownField(...) for singular fields (pinned by UnknownFieldsTest, which asserts the raw varint survives in unknownFields);
  • proto2 semantics / protobuf C++ & ObjC runtimes, which treat unknown enum values like unknown fields;
  • the .returnNil doc comment itself, which already described unknown values as "added to a collection for the same tag in unknown fields" — previously true only for collections.

The practical impact is in fetch → decode → mutate → reencode → full-object update flows: when a server starts sending a new enum value, a stale client that edits any other field of the object silently clears the enum field on the way back up.

Change

Route the unrecognized raw value into the current message frame's unknown fields in the singular enum decode path, mirroring the existing repeated-field path (including its currentTag guard). Runtime-only; no generator or generated-code changes.

Tests

  • Updated the two tests that pinned the drop (testDecodeUnknownEnumNilStrategy now asserts the preserved bytes; testDecodeUnknownEnumInOneOfNilStrategy now expects the value in unknownFields).
  • New RoundTripTests covering a singular/oneof unknown enum (proto2 shape) and a proto3 field (interaction with the zero-value backfill), both asserting the reencoded bytes are identical to the input.

swift test: full suite passing locally; all CI jobs green.

Behavior change note

Messages decoded under .returnNil now carry unknownFields entries where they previously had none, which is observable through Equatable/Hashable and reencoded bytes. Suggested CHANGELOG entry (under ### Swift):

  • Behavior change: ProtoDecoder with .returnNil now preserves unrecognized singular enum values in the message's unknownFields — matching repeated/map fields, generated Kotlin/Java, and proto2 semantics — so they survive reencoding instead of being silently dropped.

Known pre-existing edge (disclosed, not addressed here)

If the same singular enum tag occurs twice on the wire as [unknown, known], generated Swift assigns each decode result unconditionally, so the later unknown occurrence overwrites the previously decoded known value with nil (generated Kotlin's catch skips the assignment; protobuf runtimes keep the recognized value). That generator shape predates this PR; preservation makes it observable across multiple decode/reencode hops. Happy to follow up with a generator change (skip assignment when the decode returns nil) if full parity is wanted there.

The Codable path's JSONDecoder.EnumDecodingStrategy.returnNil is a distinct type and is unchanged.

When decoding with the .returnNil strategy, an unrecognized value in a
singular (or oneof) enum field was read and discarded, so reencoding the
message silently dropped the field. Repeated and map enum fields already
preserve unrecognized values in unknown fields, as does generated
Kotlin/Java code via EnumConstantNotFoundException, and proto2 semantics
call for unknown enum values to be treated like unknown fields.

Route singular unknown enum values into the current message frame's
unknown fields, mirroring the repeated-field path, so they survive a
decode/reencode round trip.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@loganblevins
loganblevins marked this pull request as ready for review September 2, 2026 22:03
@loganblevins

Copy link
Copy Markdown
Member Author

@oldergod @dnkoutso critical bug to fix affecting some paths blocking us on a current migration project from GPB

@dnkoutso

dnkoutso commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

🤖 One behavior change worth calling out explicitly for reviewers, since it isn't covered by the "Known pre-existing edge" section:

After this PR, a stale client that explicitly edits a singular enum field decoded with an unrecognized value will have that edit overridden on the wire.

Example (.returnNil, OneOfs.standalone_enum is tag 4, client only knows A = 1):

Server sends:        20 05        // standalone_enum = 5 (unknown to client)
Client decodes:      standalone_enum = nil, unknownFields[4] = 20 05   // new: previously nothing
Client sets:         standalone_enum = .A
Client reencodes:    20 01 20 05  // known field, then unknown fields
Server parses:       standalone_enum = 5   // last-wins → client's edit is lost

Before this PR the same flow produced 20 01 and the edit won (because the unknown value was dropped). After this PR, edits to other fields no longer wipe the enum (the bug being fixed), but an edit to this field is silently ineffective until the client is updated.

This is exactly what Wire Kotlin/Java already do today. Generated Kotlin catches EnumConstantNotFoundException and calls reader.addUnknownField(...) (KotlinGenerator.kt:2071-2073), and encode writes value.unknownFields after all known fields (KotlinGenerator.kt:1917) — so a Kotlin client doing .copy(standalone_enum = A) emits the same 20 01 20 05. C++ and GPB (proto2) behave the same way. Within Wire Swift it also already applies to map enum fields under .returnNil (unknown entry reemitted after known entries, last key wins) and, additively, to repeated enum fields. So the PR brings singular/oneof fields in line with everything else rather than introducing a new class of behavior.

Recommendation: merge with this added to the behavior-change note and pinned by a test (20 05 → set .A → assert 20 01 20 05), and don't block on a fix — every other runtime has shipped these semantics for years. If the migration actually has stale-client edit flows on enum fields, the right follow-up is a generator change that clears unknownFields[tag] when an enum-typed field is mutated (didSet; doesn't fire during decode/init), because that's the only shape that also fixes the pre-existing map/repeated case. A cheaper runtime-only alternative — ProtoWriter skipping unknown-field entries for tags it explicitly wrote — works for singular fields but would drop preserved values on untouched non-empty collections, so it'd have to stay singular-only. Worth filing as a tracked follow-up rather than guessing at whether the product path exists.

Separately, a small nit in the "Known pre-existing edge" paragraph: the problematic order is [known, unknown], not [unknown, known] — with [unknown, known] the later known occurrence is what gets assigned last, which is the correct result.

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