Skip to content

Commit

Permalink
feat: support proto2 optional and default value fields (#1007)
Browse files Browse the repository at this point in the history
### 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.
  • Loading branch information
lukealvoeiro authored Mar 12, 2024
1 parent 51c3d0c commit 1fa1e61
Show file tree
Hide file tree
Showing 64 changed files with 5,195 additions and 524 deletions.
4 changes: 4 additions & 0 deletions README.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,10 @@ Generated code will be placed in the Gradle build directory.

- With `--ts_proto_opt=initializeFieldsAsUndefined=false`, all optional field initializers will be omited from the generated base instances.

- With `--ts_proto_opt=disableProto2Optionals=true`, all optional fields on proto2 files will not be set to be optional. Please note that this flag is primarily for preserving ts-proto's legacy handling of proto2 files, to avoid breaking changes, and as a result, it is not intended to be used moving forward.

- With `--ts_proto_opt=disableProto2DefaultValues=true`, all fields in proto2 files that specify a default value will not actually use that default value. Please note that this flag is primarily for preserving ts-proto's legacy handling of proto2 files, to avoid breaking changes, and as a result, it is not intended to be used moving forward.

- With `--ts_proto_opt=Mgoogle/protobuf/empty.proto=./google3/protobuf/empty`, ('M' means 'importMapping', similar to [protoc-gen-go](https://developers.google.com/protocol-buffers/docs/reference/go-generated#package)), the generated code import path for `./google/protobuf/empty.ts` will reflect the overridden value:

- `Mfoo/bar.proto=@myorg/some-lib` will map `foo/bar.proto` imports into `import ... from '@myorg/some-lib'`.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -477,7 +477,7 @@ export const BoolValue = {

toJSON(message: BoolValue): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
4 changes: 2 additions & 2 deletions integration/bytes-node/google/protobuf/wrappers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -477,7 +477,7 @@ export const BoolValue = {

toJSON(message: BoolValue): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
8 changes: 4 additions & 4 deletions integration/codegen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { generateFile, makeUtils } from "../src/main";
import { createTypeMap } from "../src/types";
import { generateIndexFiles } from "../src/utils";
import { getTsPoetOpts, optionsFromParameter } from "../src/options";
import { Context } from "../src/context";
import { BaseContext, createFileContext } from "../src/context";
import { generateTypeRegistry } from "../src/generate-type-registry";

/**
Expand Down Expand Up @@ -37,8 +37,8 @@ async function generate(binFile: string, baseDir: string, parameter: string) {
continue;
}
const utils = makeUtils(options);
const ctx: Context = { options, typeMap, utils };
const [path, code] = generateFile(ctx, file);
const ctx: BaseContext = { options, typeMap, utils };
const [path, code] = generateFile({ ...ctx, currentFile: createFileContext(file) }, file);
const filePath = `${baseDir}/${path}`;
const dirPath = parse(filePath).dir;
await promisify(mkdir)(dirPath, { recursive: true }).catch(() => {});
Expand All @@ -47,7 +47,7 @@ async function generate(binFile: string, baseDir: string, parameter: string) {

if (options.outputTypeRegistry) {
const utils = makeUtils(options);
const ctx: Context = { options, typeMap, utils };
const ctx: BaseContext = { options, typeMap, utils };

const path = "typeRegistry.ts";
const code = generateTypeRegistry(ctx);
Expand Down
2 changes: 1 addition & 1 deletion integration/emit-default-values-json/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export const DefaultValuesTest = {
if (message.long !== 0) {
writer.uint32(32).int64(message.long);
}
if (message.truth === true) {
if (message.truth !== false) {
writer.uint32(40).bool(message.truth);
}
if (message.description !== "") {
Expand Down
4 changes: 2 additions & 2 deletions integration/extensions/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ export const long: Extension<Long> = {
packed: false,
encode: (value: Long): Uint8Array[] => {
const encoded: Uint8Array[] = [];
if (value !== undefined && !value.isZero()) {
if (value !== undefined && !value.equals(Long.ZERO)) {
const writer = _m0.Writer.create();
writer.int64(value);
encoded.push(writer.finish());
Expand All @@ -542,7 +542,7 @@ export const fixed: Extension<Long> = {
packed: false,
encode: (value: Long): Uint8Array[] => {
const encoded: Uint8Array[] = [];
if (value !== undefined && !value.isZero()) {
if (value !== undefined && !value.equals(Long.UZERO)) {
const writer = _m0.Writer.create();
writer.fixed64(value);
encoded.push(writer.finish());
Expand Down
4 changes: 2 additions & 2 deletions integration/global-this/global-this.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function createBaseBoolean(): Boolean {

export const Boolean = {
encode(message: Boolean, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -239,7 +239,7 @@ export const Boolean = {

toJSON(message: Boolean): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
4 changes: 2 additions & 2 deletions integration/grpc-js/google/protobuf/wrappers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -477,7 +477,7 @@ export const BoolValue = {

toJSON(message: BoolValue): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
4 changes: 2 additions & 2 deletions integration/grpc-web/google/protobuf/wrappers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -477,7 +477,7 @@ export const BoolValue = {

toJSON(message: BoolValue): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
8 changes: 4 additions & 4 deletions integration/map-long-optional/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,10 @@ function createBaseMapBigInt_MapEntry(): MapBigInt_MapEntry {

export const MapBigInt_MapEntry = {
encode(message: MapBigInt_MapEntry, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (!message.key.isZero()) {
if (!message.key.equals(Long.UZERO)) {
writer.uint32(9).fixed64(message.key);
}
if (!message.value.isZero()) {
if (!message.value.equals(Long.ZERO)) {
writer.uint32(16).int64(message.value);
}
if (message._unknownFields !== undefined) {
Expand Down Expand Up @@ -204,10 +204,10 @@ export const MapBigInt_MapEntry = {

toJSON(message: MapBigInt_MapEntry): unknown {
const obj: any = {};
if (!message.key.isZero()) {
if (!message.key.equals(Long.UZERO)) {
obj.key = (message.key || Long.UZERO).toString();
}
if (!message.value.isZero()) {
if (!message.value.equals(Long.ZERO)) {
obj.value = (message.value || Long.ZERO).toString();
}
return obj;
Expand Down
2 changes: 1 addition & 1 deletion integration/meta-typings/google/protobuf/wrappers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down
4 changes: 2 additions & 2 deletions integration/nice-grpc/google/protobuf/wrappers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ function createBaseBoolValue(): BoolValue {

export const BoolValue = {
encode(message: BoolValue, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.value === true) {
if (message.value !== false) {
writer.uint32(8).bool(message.value);
}
return writer;
Expand Down Expand Up @@ -477,7 +477,7 @@ export const BoolValue = {

toJSON(message: BoolValue): unknown {
const obj: any = {};
if (message.value === true) {
if (message.value !== false) {
obj.value = message.value;
}
return obj;
Expand Down
4 changes: 2 additions & 2 deletions integration/omit-optionals/simple.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ function createBaseTestMessage(): TestMessage {

export const TestMessage = {
encode(message: TestMessage, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.field1 === true) {
if (message.field1 !== false) {
writer.uint32(8).bool(message.field1);
}
if (message.field2 !== undefined) {
Expand Down Expand Up @@ -62,7 +62,7 @@ export const TestMessage = {

toJSON(message: TestMessage): unknown {
const obj: any = {};
if (message.field1 === true) {
if (message.field1 !== false) {
obj.field1 = message.field1;
}
if (message.field2 !== undefined) {
Expand Down
4 changes: 2 additions & 2 deletions integration/optional-long/google/protobuf/timestamp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ function createBaseTimestamp(): Timestamp {

export const Timestamp = {
encode(message: Timestamp, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
if (message.seconds !== undefined && !message.seconds.isZero()) {
if (message.seconds !== undefined && !message.seconds.equals(Long.ZERO)) {
writer.uint32(8).int64(message.seconds);
}
if (message.nanos !== undefined && message.nanos !== 0) {
Expand Down Expand Up @@ -167,7 +167,7 @@ export const Timestamp = {

toJSON(message: Timestamp): unknown {
const obj: any = {};
if (message.seconds !== undefined && !message.seconds.isZero()) {
if (message.seconds !== undefined && !message.seconds.equals(Long.ZERO)) {
obj.seconds = (message.seconds || Long.ZERO).toString();
}
if (message.nanos !== undefined && message.nanos !== 0) {
Expand Down
Loading

0 comments on commit 1fa1e61

Please sign in to comment.