Fix field type inference when FieldDescriptorProto.type is unset#1369
Fix field type inference when FieldDescriptorProto.type is unset#1369jakewisse wants to merge 2 commits intobufbuild:mainfrom
Conversation
|
|
|
Thanks for the PR, Jake. Can you sign the CLA? |
|
@timostamm sorry for the delay here, I actually did sign it right away but for some reason CLA assistant would never sync to the PR… Every time I click the link to sign it shows as already signed to me. |
Some protobuf tooling (e.g. Confluent Schema Registry) omits the `type` field from FieldDescriptorProto when `type_name` is set. According to [`descriptor.proto`][0], this is valid : "If type_name is set, this need not be set." Because descriptor.proto is proto2, protobuf-es places the default value (TYPE_DOUBLE = 1) on the prototype. When `type` is absent from the wire data it is never set as an own property, and `proto.type` inherits the prototype default, causing message and enum fields to be misclassified as scalar. This resolves the field type from the registry via `type_name` when `type` is not explicitly set. From there, thread the resolved type through `getFieldPresence()`, `isPackedField()`, and `isDelimitedEncoding()` to avoid the same prototype fallback in those functions. -- [0]: https://github.com/protocolbuffers/protobuf/blob/c223b2e1fa96feac05f275853e64a87993d557a2/src/google/protobuf/descriptor.proto#L295-L296
jakewisse
force-pushed
the
bugfix/file-descriptor-proto-type-optional
branch
from
e5d8be6 to
3cc7bec
Compare
|
Ah, it was my fault. I created the commit using an email associated with a different Github account. Looks like it has synced properly now. |
…perly I can't find exactly where this changed in TS 5.0, but for some reason in 5.0 and later using `assert.strictEqual(itemsField.fieldKind, 'list')` narrows the value to a `descFieldList`, whereas in 4.9 it doesn't.
|
Thanks for the patience, Jake! I just got a chance to take a look. The fix looks good, but I believe we need to take a different approach 🤔 As you can see, the change affects bundle size. It's an insignificant change, but I suspect that there are similar cases in descriptor.proto we'd want to normalize in the future, and then it becomes a problem. I'll comment on #1368. |
|
See #1368 (comment) for broader thoughts. To detangle your PR from those design decisions, would you be open to refactoring this into a free function that operates on a We can land it as an internal helper with tests until we settle on the broader design. Happy to help if the rework turns out to be more than a small lift. |
3 participants
Some protobuf tooling (e.g. Confluent Schema Registry) omits the
typefield from FieldDescriptorProto whentype_nameis set. According todescriptor.proto, this is valid : "If type_name is set, this need not be set."Because descriptor.proto is proto2, protobuf-es places the default value (TYPE_DOUBLE = 1) on the prototype. When
typeis absent from the wire data it is never set as an own property, andproto.typeinherits the prototype default, causing message and enum fields to be misclassified as scalar.This resolves the field type from the registry via
type_namewhentypeis not explicitly set. From there, we thread the resolved type throughgetFieldPresence(),isPackedField(), andisDelimitedEncoding()to avoid the same prototype fallback in those functions.Fixes #1368.