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

feat: represent field masks as string[] #525

Merged
merged 5 commits into from
Mar 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 13 additions & 18 deletions README.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -620,24 +620,19 @@ Their interpretation is defined by the Protobuf specification, and libraries are

`ts-proto` currently automatically converts these messages to their corresponding native types.

- Wrapper Types:

* [google.protobuf.DoubleValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#DoubleValue) ⇆ `number | undefined`
* [google.protobuf.FloatValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#FloatValue) ⇆ `number | undefined`
* [google.protobuf.Int64Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#Int64Value) ⇆ `number | undefined`
* [google.protobuf.UInt64Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#UInt64Value) ⇆ `number | undefined`
* [google.protobuf.Int32Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#Int32Value) ⇆ `number | undefined`
* [google.protobuf.UInt32Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#UInt32Value) ⇆ `number | undefined`
* [google.protobuf.BoolValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#BoolValue) ⇆ `boolean | undefined`
* [google.protobuf.StringValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#StringValue) ⇆ `string | undefined`
* [google.protobuf.BytesValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#google.protobuf.BytesValue) ⇆ `Uint8Array | undefined`

- JSON Types (Struct Types):

* [google.protobuf.Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#Value) ⇆ `any | undefined` (i.e. `number | string | boolean | null | array | object`)
* [google.protobuf.ListValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#ListValue) ⇆ `any[]`
* [google.protobuf.Struct](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#Struct) ⇆ `{ [key: string]: any } | undefined`
* [google.protobuf.FieldMask](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#fieldmask) ⇆ `string[]` (only in the JSON, `FieldMask` is still a message)
* [google.protobuf.BoolValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#boolvalue) ⇆ `boolean`
* [google.protobuf.BytesValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#bytesvalue) ⇆ `Uint8Array`
* [google.protobuf.DoubleValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#doublevalue) ⇆ `number`
* [google.protobuf.FieldMask](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#fieldmask) ⇆ `string[]`
* [google.protobuf.FloatValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#floatvalue) ⇆ `number`
* [google.protobuf.Int32Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#int32value) ⇆ `number`
* [google.protobuf.Int64Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#int64value) ⇆ `number`
* [google.protobuf.ListValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#listvalue) ⇆ `any[]`
* [google.protobuf.UInt32Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#uint32value) ⇆ `number`
* [google.protobuf.UInt64Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#uint64value) ⇆ `number`
* [google.protobuf.StringValue](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#stringvalue) ⇆ `string`
* [google.protobuf.Value](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#value) ⇆ `any` (i.e. `number | string | boolean | null | array | object`)
* [google.protobuf.Struct](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf#struct) ⇆ `{ [key: string]: any }`

## Wrapper Types

Expand Down
45 changes: 36 additions & 9 deletions integration/fieldmask/fieldmask-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,50 @@ let data = {
};

describe('fieldmask', () => {
it('can decode JSON', () => {
it('can decode canonical JSON', () => {
const f = FieldMaskMessage.fromJSON(data);
expect(f).toMatchInlineSnapshot(`
Object {
"fieldMask": Object {
"paths": Array [
"a",
"b",
"c.d",
],
},
"fieldMask": Array [
"a",
"b",
"c.d",
],
}
`);
});

it('can decode non-canonical JSON', () => {
const f = FieldMaskMessage.fromJSON({
fieldMask: {
paths: ['a', 'b', 'c.d'],
}
});
expect(f).toMatchInlineSnapshot(`
Object {
"fieldMask": Array [
"a",
"b",
"c.d",
],
}
`);
});

it('can encode JSON', () => {
const f = FieldMaskMessage.toJSON({ fieldMask: { paths: ['a', 'b', 'c.d'] } });
const f = FieldMaskMessage.toJSON({ fieldMask: ['a', 'b', 'c.d'] });
expect(f).toEqual(data);
});

it('skips empty paths', () => {
const f = FieldMaskMessage.fromJSON({fieldMask: 'a,,c.d'});
expect(f).toMatchInlineSnapshot(`
Object {
"fieldMask": Array [
"a",
"c.d",
],
}
`);
});
});
Binary file modified integration/fieldmask/fieldmask.bin
Binary file not shown.
13 changes: 6 additions & 7 deletions integration/fieldmask/fieldmask.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { FieldMask } from './google/protobuf/field_mask';
export const protobufPackage = '';

export interface FieldMaskMessage {
fieldMask: FieldMask | undefined;
fieldMask: string[] | undefined;
}

function createBaseFieldMaskMessage(): FieldMaskMessage {
Expand All @@ -16,7 +16,7 @@ function createBaseFieldMaskMessage(): FieldMaskMessage {
export const FieldMaskMessage = {
encode(message: FieldMaskMessage, writer: Writer = Writer.create()): Writer {
if (message.fieldMask !== undefined) {
FieldMask.encode(message.fieldMask, writer.uint32(10).fork()).ldelim();
FieldMask.encode(FieldMask.wrap(message.fieldMask), writer.uint32(10).fork()).ldelim();
}
return writer;
},
Expand All @@ -29,7 +29,7 @@ export const FieldMaskMessage = {
const tag = reader.uint32();
switch (tag >>> 3) {
case 1:
message.fieldMask = FieldMask.decode(reader, reader.uint32());
message.fieldMask = FieldMask.unwrap(FieldMask.decode(reader, reader.uint32()));
break;
default:
reader.skipType(tag & 7);
Expand All @@ -41,20 +41,19 @@ export const FieldMaskMessage = {

fromJSON(object: any): FieldMaskMessage {
return {
fieldMask: isSet(object.fieldMask) ? { paths: object.fieldMask.split(',') } : undefined,
fieldMask: isSet(object.fieldMask) ? FieldMask.unwrap(FieldMask.fromJSON(object.fieldMask)) : undefined,
};
},

toJSON(message: FieldMaskMessage): unknown {
const obj: any = {};
message.fieldMask !== undefined && (obj.fieldMask = message.fieldMask.paths.join());
message.fieldMask !== undefined && (obj.fieldMask = FieldMask.toJSON(FieldMask.wrap(message.fieldMask)));
return obj;
},

fromPartial<I extends Exact<DeepPartial<FieldMaskMessage>, I>>(object: I): FieldMaskMessage {
const message = createBaseFieldMaskMessage();
message.fieldMask =
object.fieldMask !== undefined && object.fieldMask !== null ? FieldMask.fromPartial(object.fieldMask) : undefined;
message.fieldMask = object.fieldMask ?? undefined;
return message;
},
};
Expand Down
25 changes: 16 additions & 9 deletions integration/fieldmask/google/protobuf/field_mask.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,25 +242,32 @@ export const FieldMask = {

fromJSON(object: any): FieldMask {
return {
paths: Array.isArray(object?.paths) ? object.paths.map((e: any) => String(e)) : [],
paths:
typeof object === 'string'
? object.split(',').filter(Boolean)
: Array.isArray(object?.paths)
? object.paths.map(String)
: [],
};
},

toJSON(message: FieldMask): unknown {
const obj: any = {};
if (message.paths) {
obj.paths = message.paths.map((e) => e);
} else {
obj.paths = [];
}
return obj;
toJSON(message: FieldMask): string {
return message.paths.join(',');
},

fromPartial<I extends Exact<DeepPartial<FieldMask>, I>>(object: I): FieldMask {
const message = createBaseFieldMask();
message.paths = object.paths?.map((e) => e) || [];
return message;
},

wrap(paths: string[]): FieldMask {
return { paths: paths };
},

unwrap(message: FieldMask): string[] {
return message.paths;
},
};

type Builtin = Date | Function | Uint8Array | string | number | boolean | undefined;
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"build": "yarn tsc",
"build:test": "yarn proto2bin && yarn proto2pbjs && yarn bin2ts",
"build:test:local": "yarn proto2bin:local && yarn proto2pbjs:local && yarn bin2ts:local",
"prepare": "yarn build",
"proto2bin": "docker-compose run --rm protoc update-bins.sh",
"proto2bin-node": "docker-compose run --rm node update-bins.sh",
"proto2pbjs": "docker-compose run --rm protoc pbjs.sh",
Expand Down
67 changes: 58 additions & 9 deletions src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
isBytesValueType,
isEnum,
isFieldMaskType,
isFieldMaskTypeName,
isListValueType,
isListValueTypeName,
isLong,
Expand Down Expand Up @@ -164,8 +165,8 @@ export function generateFile(ctx: Context, fileDesc: FileDescriptorProto): [stri
staticMembers.push(generateDecode(ctx, fullName, message));
}
if (options.outputJsonMethods) {
staticMembers.push(generateFromJson(ctx, fullName, message));
staticMembers.push(generateToJson(ctx, fullName, message));
staticMembers.push(generateFromJson(ctx, fullName, fullTypeName, message));
staticMembers.push(generateToJson(ctx, fullName, fullTypeName, message));
}
if (options.outputPartialMethods) {
staticMembers.push(generateFromPartial(ctx, fullName, message));
Expand Down Expand Up @@ -819,7 +820,7 @@ function generateDecode(ctx: Context, fullName: string, messageDesc: DescriptorP
} else if (isValueType(ctx, field)) {
const type = basicTypeName(ctx, field, { keepValueType: true });
const unwrap = (decodedValue: any): Code => {
if (isListValueType(field) || isStructType(field) || isAnyValueType(field)) {
if (isListValueType(field) || isStructType(field) || isAnyValueType(field) || isFieldMaskType(field)) {
return code`${type}.unwrap(${decodedValue})`;
}
return code`${decodedValue}.value`;
Expand Down Expand Up @@ -944,7 +945,7 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP

const type = basicTypeName(ctx, field, { keepValueType: true });
const wrappedValue = (place: string): Code => {
if (isAnyValueType(field) || isListValueType(field) || isStructType(field)) {
if (isAnyValueType(field) || isListValueType(field) || isStructType(field) || isFieldMaskType(field)) {
return code`${type}.wrap(${place})`;
}
return code`{${maybeTypeField} value: ${place}!}`;
Expand Down Expand Up @@ -1094,7 +1095,7 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP
* This is very similar to decode, we loop through looking for properties, with
* a few special cases for https://developers.google.com/protocol-buffers/docs/proto3#json.
* */
function generateFromJson(ctx: Context, fullName: string, messageDesc: DescriptorProto): Code {
function generateFromJson(ctx: Context, fullName: string, fullTypeName: string, messageDesc: DescriptorProto): Code {
const { options, utils, typeMap } = ctx;
const chunks: Code[] = [];

Expand All @@ -1112,6 +1113,16 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
messageDesc.field.filter(isWithinOneOf).filter((field) => field.oneofIndex === oneofIndex)
);

const canonicalFromJson: { [key: string]: { [field: string]: (from: string) => Code } } = {
['google.protobuf.FieldMask']: {
paths: (from: string) => code`typeof(${from}) === 'string'
Copy link
Owner

Choose a reason for hiding this comment

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

I really like the map approach; that's nudging us towards a more configuration-driven vs. the current version if-driven approach.

Just wondering, I wonder we're lucky here that the JSON presentation of FieldMask is a single string value that happens to deserialize into the paths field.

I.e. maybe the key of this map should be "just FieldMask-the type" and we should look in the map, and let it just return the whole fromJson body?

Basically more how like toJSON ended up being handled (although again I definitely like the map-driven approach)...

Wdyt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I did consider doing just that. I went this way to make sure any follow up logic will also applied to fieldmasks. Admittedly, the only logic right now is the fact that we return a single JSON object (instead of constructing one field by field).

It's true that this approach only works exactly in this case, and might not work for other types (like Timestamp?).
If you feel returning the object immediately is more future proof or straightforward, I'm fine to change it.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah cool, I'm good with the current approach!

? ${from}.split(",").filter(Boolean)
: Array.isArray(${from}?.paths)
? ${from}.paths.map(String)
: []`,
},
};

// add a check for each incoming field
messageDesc.field.forEach((field) => {
const fieldName = maybeSnakeToCamel(field.name, options);
Expand Down Expand Up @@ -1151,7 +1162,8 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
} else if (isAnyValueType(field) || isStructType(field)) {
return code`${from}`;
} else if (isFieldMaskType(field)) {
return code`{paths: ${from}.split(",")}`;
const type = basicTypeName(ctx, field, { keepValueType: true });
return code`${type}.unwrap(${type}.fromJSON(${from}))`;
} else if (isListValueType(field)) {
return code`[...${from}]`;
} else if (isValueType(ctx, field)) {
Expand Down Expand Up @@ -1210,7 +1222,9 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
};

// and then use the snippet to handle repeated fields if necessary
if (isRepeated(field)) {
if (canonicalFromJson[fullTypeName]?.[fieldName]) {
chunks.push(code`${fieldName}: ${canonicalFromJson[fullTypeName][fieldName]('object')},`);
} else if (isRepeated(field)) {
if (isMapType(ctx, messageDesc, field)) {
const fieldType = toTypeName(ctx, messageDesc, field);
const i = maybeCastToNumber(ctx, messageDesc, field, 'key');
Expand Down Expand Up @@ -1282,10 +1296,32 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
return joinCode(chunks, { on: '\n' });
}

function generateToJson(ctx: Context, fullName: string, messageDesc: DescriptorProto): Code {
function generateCanonicalToJson(fullName: string, fullProtobufTypeName: string): Code | undefined {
if (isFieldMaskTypeName(fullProtobufTypeName)) {
return code`
toJSON(message: ${fullName}): string {
return message.paths.join(',');
}
`;
}
return undefined;
}

function generateToJson(
ctx: Context,
fullName: string,
fullProtobufTypeName: string,
messageDesc: DescriptorProto
): Code {
const { options, utils, typeMap } = ctx;
const chunks: Code[] = [];

const canonicalToJson = generateCanonicalToJson(fullName, fullProtobufTypeName);
if (canonicalToJson) {
chunks.push(canonicalToJson);
return joinCode(chunks, { on: '\n' });
}

// create the basic function declaration
chunks.push(code`
toJSON(${messageDesc.field.length > 0 ? 'message' : '_'}: ${fullName}): unknown {
Expand Down Expand Up @@ -1343,7 +1379,8 @@ function generateToJson(ctx: Context, fullName: string, messageDesc: DescriptorP
} else if (isAnyValueType(field)) {
return code`${from}`;
} else if (isFieldMaskType(field)) {
return code`${from}.paths.join()`;
const type = basicTypeName(ctx, field, { keepValueType: true });
return code`${type}.toJSON(${type}.wrap(${from}))`;
} else if (isMessage(field) && !isValueType(ctx, field) && !isMapType(ctx, messageDesc, field)) {
const type = basicTypeName(ctx, field, { keepValueType: true });
return code`${from} ? ${type}.toJSON(${from}) : ${defaultValue(ctx, field)}`;
Expand Down Expand Up @@ -1598,6 +1635,12 @@ function generateWrap(ctx: Context, fullProtoTypeName: string): Code[] {
}`);
}

if (isFieldMaskTypeName(fullProtoTypeName)) {
chunks.push(code`wrap(paths: string[]): FieldMask {
return {paths: paths};
}`);
}

return chunks;
}

Expand Down Expand Up @@ -1658,6 +1701,12 @@ function generateUnwrap(ctx: Context, fullProtoTypeName: string): Code[] {
}`);
}

if (isFieldMaskTypeName(fullProtoTypeName)) {
chunks.push(code`unwrap(message: FieldMask): string[] {
return message.paths;
}`);
}

return chunks;
}

Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,8 @@ export function valueTypeName(ctx: Context, typeName: string): Code | undefined
return code`any`;
case '.google.protobuf.Struct':
return code`{[key: string]: any}`;
case '.google.protobuf.FieldMask':
return code`string[]`;
default:
return undefined;
}
Expand Down