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.

Sign up for GitHub

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

Jump to bottom

feat: disabled the default check for the upstream encoder #657

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: disabled the default check for the upstream encoder #657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions README.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,11 @@ Generated code will be placed in the Gradle build directory.

- With `--ts_proto_opt=useNumericEnumForJson=true`, the JSON converter (`toJSON`) will encode enum values as int, rather than a string literal.

- With `--ts_proto_opt=disableDefaultCheck=true`, this will disable the if conditional around the encoder for a field which implicitly enforces the default value. For example if
`if(message.name !== ''){ Writer.uint32(1).string(message.name)}` this will become ` Writer.uint32(1).string(message.name)` so you can potential send an undefined to the server.

- With `--ts_proto_opt=disableDefaultEnumCheck=true`, the same as `--ts_proto_opt=disableDefaultCheck=true` but only applied to enums.

### NestJS Support

We have a great way of working together with [nestjs](https://docs.nestjs.com/microservices/grpc). `ts-proto` generates `interfaces` and `decorators` for you controller, client. For more information see the [nestjs readme](NESTJS.markdown).
Expand Down
26 changes: 22 additions & 4 deletions src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1125,11 +1125,29 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP
}
`);
} else if (isScalar(field) || isEnum(field)) {
chunks.push(code`
if (${notDefaultCheck(ctx, field, messageDesc.options, `message.${fieldName}`)}) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @khi195 ; thanks for the PR! Instead of adding more options, I think we should try and solve this by being smarter at handling "there is no enum with value === 0".

I.e. this line is calling notDefaultCheck, which inside of that method has some code that looks like:

      const zerothValue = enumProto.value.find((v) => v.number === 0) || enumProto.value[0];
      if (options.stringEnums) {
        const enumType = messageToTypeName(ctx, field.typeName);
        return code`${maybeNotUndefinedAnd} ${place} !== ${enumType}.${zerothValue.name}`;
      } else {
        return code`${maybeNotUndefinedAnd} ${place} !== ${zerothValue.number}`;
      }

Note the || enumProto.value[0]. That is the line that is tripping up your code, b/c it means ts-proto is treating the "the enum value 1" as the default, which it just shouldn't do.

Can you try removing that || enumProto.value[0] and instead doing something like:

      const zerothValue = enumProto.value.find((v) => v.number === 0);
      if (!zerothValue) {
        return undefined;
      } else if (options.stringEnums) {
        const enumType = messageToTypeName(ctx, field.typeName);
        return code`${maybeNotUndefinedAnd} ${place} !== ${enumType}.${zerothValue.name}`;
      } else {
        return code`${maybeNotUndefinedAnd} ${place} !== ${zerothValue.number}`;
      }

And then here in the generateEncode method, you could do:

const maybeDefaultCheck = notDefaultCheck(ctx, field, messageDesc.options, `message.${fieldName}`);

And if maybeDefaultCheck is undefined, then leave the if statement out, just like you're currently doing by looking at the new disableDefaultCheck options.

I'm pretty sure that should work.

${writeSnippet(`message.${fieldName}`)};
/**
*
* this is needed for proto2 when a default value may not be set on the server
* so remove the if statement around the snippet
*/

const snippet = writeSnippet(`message.${fieldName}`);

const { disableDefaultCheck, disableDefaultEnumCheck } = options;

if (disableDefaultCheck) {
chunks.push(code`${snippet};`);
} else {
if (isEnum(field) && disableDefaultEnumCheck) {
chunks.push(code`${snippet};`);
} else {
chunks.push(code`
if (${notDefaultCheck(ctx, field, messageDesc.options, `message.${fieldName}`)}) {
${snippet};
}
`);
}
`);
}
} else {
chunks.push(code`${writeSnippet(`message.${fieldName}`)};`);
}
Expand Down
4 changes: 4 additions & 0 deletions src/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ export type Options = {
usePrototypeForDefaults: boolean;
useJsonWireFormat: boolean;
useNumericEnumForJson: boolean;
disableDefaultEnumCheck: boolean;
disableDefaultCheck: boolean;
};

export function defaultOptions(): Options {
Expand Down Expand Up @@ -108,6 +110,8 @@ export function defaultOptions(): Options {
usePrototypeForDefaults: false,
useJsonWireFormat: false,
useNumericEnumForJson: false,
disableDefaultEnumCheck: false,
disableDefaultCheck: false,
};
}

Expand Down
2 changes: 2 additions & 0 deletions tests/options-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ describe('options', () => {
"addNestjsRestParameter": false,
"constEnums": false,
"context": false,
"disableDefaultCheck": false,
"disableDefaultEnumCheck": false,
"emitImportedFiles": true,
"enumsAsLiterals": false,
"env": "both",
Expand Down