Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't encode proto2 optional fields #973

Open
Demon000 opened this issue Dec 6, 2023 · 7 comments · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2
Open

Don't encode proto2 optional fields #973

Demon000 opened this issue Dec 6, 2023 · 7 comments · May be fixed by Graysonbarton/synthetics-sdk-nodejs#2

Comments

@Demon000
Copy link
Contributor

Demon000 commented Dec 6, 2023

Is this supported and I just can't find the correct flags to use, or is this unsupported?

@stephenh
Copy link
Owner

stephenh commented Dec 6, 2023

Hi @Demon000 , proto2 should be supported; it was not an initial goal of ts-proto, but I think it basically works at the moment.

The biggest flag for proto2 support is usePrototypeForDefaults because it disables the proto3 behavior of "always fill in missing keys with their default value". proto2 code would sometimes rely on the presence of "is this key set or what?", i.e. hazzer functions, and so usePrototypeForDefaults enables hazzer checks.

Otherwise, yeah, it should work. Let me know if it doesn't, and I can try and point you in the right direction of a fix/PR. Thanks!

@stephenh stephenh closed this as completed Dec 6, 2023
@Demon000
Copy link
Contributor Author

Demon000 commented Dec 6, 2023

Hi @Demon000 , proto2 should be supported; it was not an initial goal of ts-proto, but I think it basically works at the moment.

The biggest flag for proto2 support is usePrototypeForDefaults because it disables the proto3 behavior of "always fill in missing keys with their default value". proto2 code would sometimes rely on the presence of "is this key set or what?", i.e. hazzer functions, and so usePrototypeForDefaults enables hazzer checks.

Otherwise, yeah, it should work. Let me know if it doesn't, and I can try and point you in the right direction of a fix/PR. Thanks!

Thanks for the quick answer.

The first issue I encountered is that fields marked as optional would not translate to a field: Type | undefined.

The second issue I have is that fields with set default values will not be sent over the wire.

I'm not sure how usePrototypeForDefaults helps with these cases, it doesn't seem to.

@stephenh
Copy link
Owner

The second issue I have is that fields with set default values will not be sent over the wire.

Ah, okay, that makes sense. I think the only place I've personally used proto2 optional fields is reading them, and not putting them on the wire.

I think we'd need some sort of encodeDefaultValues flag, which hopefully shouldn't be that hard to support, if you're interested in submitting a PR, that'd be great!

@stephenh stephenh reopened this Dec 10, 2023
@stephenh
Copy link
Owner

The first issue I encountered is that fields marked as optional
would not translate to a field: Type | undefined.

I guess this makes sense...when ts-proto was first written, it was when proto3 didn't have optional fields. Then after a few years, proto3 added support for optional fields, as like oneofs, and so we support that ... but probably not the "quick & simple" approach of proto2 of just "do or do not put the value on the wire".

Fwiw personally I wish proto3 would have kept proto2's optional field semantics, instead of asserting for ~4-5 years that "no one needs optional fields", and then just backtracking on it later. 🤷

Apologies for the late replies, I can't always immediately triage ts-proto issues, but try to check-in about once per week or two.

I think both of these issues you're seeing are fixable if you're interested in poking around, like the main.ts generateEncodeMethod iirc.

@Demon000
Copy link
Contributor Author

Sorry, I switched to protobuf-es which seems to handle my usecase correctly. I first tried to fix this project but the code is too all over the place for me.

@stephenh stephenh changed the title Support for proto2 optional fields? Don't encode proto2 optional fields Dec 10, 2023
@stephenh
Copy link
Owner

Ah yeah; I won't deny that--ts-proto's "strength" is that its collected a large array of knobs/flags over the years that, in theory, let users adapt the output to whatever their random/esoteric protobuf internal stacks, but yeah the complexity has gone up to support that. 🤷

Not really a problem I'm paid to solve :-) , so happy you found protobuf-es does what you want.

stephenh pushed a commit that referenced this issue Mar 12, 2024
### Description

This PR aims to implement the features necessary to enable proto2
optional fields and support proto2 default values, requested by #973

The following changes are present in this PR:

- Code changes to `main.ts` and `types.ts` that implement **optional
fields**, as well as [**default
values**](https://protobuf.dev/programming-guides/proto2/).
- Creates two new options: `disableProto2Optionals` and
`disableProto2DefaultValues`. By default these are set to false.
- New integration tests for `proto3` and `proto2` syntax types, intended
to catch any regressions
- Modifications to existing integration tests - this mainly involves:
   - Updating message interfaces to use optional variables
- Updating the createBaseMessage() function to use default values
provided by the proto2 syntax.
   - The following two changes to the `encode` function of a message
1. When checking boolean default values, following the other default
checks by checking if not equals instead of equals:
     ```ts
     // BEFORE
     message.truth === true
     // AFTER
     message.truth !== false
     ```
2. When default checking Long types using the `forceLong=true` option,
always check via a `.equals()` instead of `.isZero()`. This could be
reverted, however, I find it to be cleaner; regardless of whether there
is a default value provided or not this check will always work.
     ```ts
     // BEFORE
     !message.key.isZero()
     // AFTER
     !message.key.equals(Long.ZERO)
     ```

### Outstanding work

In a **subsequent future PRs**, we need to handle the following:
- Updating to `ts-proto-descriptors` to correctly render fields as
optional. This involves running changing the parameters for
`protos/build.sh` to using the build code from `ts-protos`. I tried
doing this as part of this PR, but the updated proto descriptors did not
work well with ts-proto. Going to re-attempt this at a later date.
- Fix the last of the default values: currently all but the Bytes type
is handled correctly. I have added a `todo(proto2)` label around the
places that need updating.


## Past Discussion

Please ignore everything below, as it is out of date. I am keeping it
within the PR description so below comments make sense to future
readers.

##### Broken TypeScript in `src`

> @lukealvoeiro: The code in `src/` is currently broken as a result of
the `ts-proto-descriptors` update, which made a lot of the fields
typically available in interfaces such as `ProtoFileDescriptor`
optional.
> 
> I'm planning on updating all the source code to use the optional
syntax (not actually that hard, there are like 60 TS errors which I can
probably knock out quickly), and then adding a new option that disables
proto2 optional fields (e.g. `disableProto2Optionals=true`). This gives
us better maintainability in the long run, brings us in line with proto2
conventions, and improves our parsing of proto files. It also allows
customers who are used to ts-proto's existing output for proto2 files to
continue to codegen the same TS output via the `disableProto2Optionals`
flag.

As I mentioned above, I tried do this but didn't get very far down this
path. I think in general it makes sense to update `ts-proto-descriptors`
to use the optional fields (and default values), however, it was harder
than expected to get the updated descriptors working with `ts-proto`

##### File-specific context

> @lukealvoeiro: One of the not-so-elegant aspects of this PR is visible
in `main.ts`, where I am passing a parameter `isProto3File: boolean`
around a lot. Wanted to open up a discussion as to whether it would be
helpful to insert file-specific context such as `isProto3File` into the
Context object. After we process each file, we could then overwrite this
portion of the context with the next file's metadata.

I implemented this change, folks can continue adding to the file context
whenever appropriate.

### Testing performed

- [x] Ran unit tests, changes to files are expected and compile / pass
tests
- [x] Ran on my own codebase that has proto2 and proto3 files and
noticed no issues there either.
stephenh pushed a commit that referenced this issue Mar 12, 2024
# [1.169.0](v1.168.0...v1.169.0) (2024-03-12)

### Features

* support proto2 optional and default value fields ([#1007](#1007)) ([1fa1e61](1fa1e61)), closes [#973](#973)
@stephenh
Copy link
Owner

Fwiw @lukealvoeiro I was looking for any issues tagged with "proto2" to close out, after you landed #1007 ... seems like this one is still valid, i.e. an ask to recognize field values as "matching the optional value", and not encode them.

Which is really similar to what proto3 does with its default values, but probably still something we don't do, which is fine imo/not high priority, just noting my current understanding.

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 a pull request may close this issue.

2 participants