bufbuild / protobuf-es

Protocol Buffers for ECMAScript. The only JavaScript Protobuf library that is fully-compliant with Protobuf conformance tests.
Apache License 2.0
1.16k stars 69 forks source link

Solution for the issue of `snake_case` fields being lost after calling the `create` method #1001

Closed locene closed 3 weeks ago

locene commented 3 weeks ago

Description

Given the following data:

const data = {
    "sample_content": "some things"
}

And the corresponding proto definition:

message Data {
    string sample_content = 1;
}

When using this setup in the create method:

create(DataSchema, {
    data: data
})

The "sample_content" field will be ignored and not converted correctly.

Analysis

In the initMessage method of create.ts, the value is accessed using member.localName:

let value = (init as Record<string, unknown>)[member.localName]

At this point, the value of member.localName has already been converted to camelCase, resulting in "sampleContent", which does not match the original snake_case format of "sample_content", causing this field to be ignored.

Solution

To address this issue, modify the code as follows:

let value = (init as Record<string, unknown>)[member.localName] ?? (init as Record<string, unknown>)[member.name] ?? null;

This change will resolve the issue without impacting the existing functionality.

CLAassistant commented 3 weeks ago

CLA assistant check
All committers have signed the CLA.

timostamm commented 3 weeks ago

From the manual:

Property names are always lowerCamelCase, even if the corresponding Protobuf field uses snake_case. Though there's no official style for ECMAScript, most style guides (AirBnB, MDN, Google) as well as Node.js APIs and browser APIs use lowerCamelCase, and so do we.

We want to keep Protobuf-ES simple and focused, and while this change may not impact existing functionality, it does not help with these goals.

The better solution would be to write a function createFrom for your project, which accepts properties with the original Protobuf field name. The reflection API should make this feasible.

locene commented 3 weeks ago

OK, no problem.

Could you please revoke my CLA on cla-assistant.io? I signed a bit hastily, and there are some issues regarding my ability to sign the CLA on behalf of my employer. Had I known you weren't planning to merge it, I wouldn't have done so.

Thank you for your understanding.

timostamm commented 3 weeks ago

I cannot revoke your CLA. But you can, on https://cla-assistant.io/my-cla

locene commented 3 weeks ago

No, I can't. The list is empty when I open it, which is why I'm reaching out to you. My understanding is that it requires a maintainer to delete it.

cla-assistant/cla-assistant#907 cla-assistant/cla-assistant#380 cla-assistant/cla-assistant#406

timostamm commented 3 weeks ago

Have you tried signing in first on https://cla-assistant.io/ ?

When I do that, I see CLAs I've signed on https://cla-assistant.io/my-cla, and I have a button to revoke:

Screenshot 2024-10-30 at 15 17 48
locene commented 3 weeks ago

Yes, I am already logged in. Here is my screenshot:

(click to enlarge)

This is the CLA I signed for this repository previously, and you can see that it is still in a signed state:

(click to enlarge)

(click to enlarge)

timostamm commented 3 weeks ago

We have looked into it, but we don't see a way to revoke it.

For some context, I don't believe that the state of this PR (or any past ones) will change by revoking a CLA, only future ones. If it would be possible to retroactively revoke a CLA for a contribution, OSS could not safely accept any contributions. But you haven't contributed, and as far as I understand, you have revoked your CLA for future contributions. IANAL, but it looks to me like this is void.

locene commented 3 weeks ago

Alright, I agree that revoking the CLA won't affect the status of existing PRs, but based on my understanding, the CLA for future contributions is still active, and I think that could lead to potential legal issues.

Thank you very much for your assistance. I will go ahead and open a ticket in the cla-assistant/cla-assistant project.