Set defaults to false for loader definitions

[RichieAHB]

This usage of defaults causes issues when using descriptors with protobufjs. Descriptors that default to oneofIndex: 0 cause issues when they aren't actually part of a oneof. Specifically here:

https://github.com/protobufjs/protobuf.js/blob/95b56817ef6fb9bdcb14d956c159da49d0889bff/ext/descriptor/index.js#L218-L219

where it assumes that if there's a oneofIndex then that is a valid index into the oneofDecl array. I could have changed the logic there but this feels more "upstream" and there doesn't seem to be a need for defaults to be true regardless of this issue with protobufjs.

[linux-foundation-easycla]

• ❌ The commit (8968bf8) is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[murgatroid99]

Those options aren't used at all when loading descriptor objects in as package definition objects, so this issue with oneofindex just doesn't seem to be relevant.

[RichieAHB]

They are used in createMessageDefinition which is used by createDefinition, which is used by createPackageDefinition as a means for of creating the definition object. I’m not sure what you mean?

[murgatroid99]

The Protobuf.js function you pointed to is Type#fromDescriptor. Nothing in the chain of functions you just mentioned calls that function.

[RichieAHB]

Ah right, I see what you mean, nothing calls that in that chain. That chain is purely for the creation of the descriptor objects that exhibit this problem. So, when using a descriptor that has been created using that chain of functions (i.e. any of the exported helpers from this file) the protobufjs fromDescriptor can break. I.e when trying to create a message type for a request / response from one of the service descriptors returned by this file. Does that make more sense?

[murgatroid99]

Are you talking about taking that object output by proto-loader and passing it directly to Protobuf.js yourself? If so, that was never an intended use case of this API.

Those message definition and enum definition objects have a structure specified in this proposal. In particular, the type field has the canonical JSON mapping representation of the descriptor message, which is specified to include default values.

[RichieAHB]

I actually read the opposite to be true (by default) in the linked Google page regarding the canonical JSON mapping:

If a field has the default value in the protocol buffer, it will be omitted in the JSON-encoded data by default to save space.

I understand that it wasn't intended but as per that proposal it might be nice to support it given that the defaults would allow this (from my reading)? I'm assuming this is why protobufjs fails to handle this case, because it does assume defaults will not be present in the JSON mapping: but that's just speculation!

[murgatroid99]

I see. I did have that backwards. In that case I think I can accept this change. You'll still need to sign the CLA.

As far as I know, Protobuf.js APIs are not generally concerned with the canonical JSON mapping. I assume it just doesn't account for representing the object with defaults.

[RichieAHB]

Excellent thanks 👍 I have gone through and signed the CLA, do I need to force push a slight change to this branch for it to retrigger?

[murgatroid99]

Looking at the CLA bot comment again, it looks like the problem is that the commit is using an email address that is not associated with your GitHub account. I think you can fix it by either adding that email address to your GitHub account, or modifying the commit with the email address that you already use with GitHub.