Skip to content

Commit

Permalink
feat: Support json names containing non-alphanumeric characters (#520)
Browse files Browse the repository at this point in the history 
* Fix yarn watch source event handler signature

* Fix issue where json_names with hyphens would generate invalid TypeScript
  • Loading branch information
@boukeversteegh
boukeversteegh committed Feb 21, 2022
1 parent bcc33dd commit ce44668
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 24 deletions.
Binary file modified integration/simple-json-name/simple.bin
View file
Binary file not shown.
10 changes: 7 additions & 3 deletions integration/simple-json-name/simple.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ import "google/protobuf/timestamp.proto";
package simple;

message Simple {
string name = 1 [ json_name = "other_name" ];
optional int32 age = 2 [ json_name = "other_age" ];
optional google.protobuf.Timestamp created_at = 9 [ json_name="createdAt" ];
string name = 1 [json_name = "other_name"];
optional int32 age = 2 [json_name = "other_age"];
optional google.protobuf.Timestamp created_at = 9 [json_name = "createdAt"];
string hyphen = 3 [json_name = "hyphened-name"];
string spaces = 4 [json_name = "name with spaces"];
string dollarStart = 5 [json_name = "$dollar"];
string dollarEnd = 6 [json_name = "dollar$"];
}
42 changes: 41 additions & 1 deletion integration/simple-json-name/simple.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@ export interface Simple {
name: string;
age?: number | undefined;
createdAt?: Date | undefined;
hyphen: string;
spaces: string;
dollarStart: string;
dollarEnd: string;
}

function createBaseSimple(): Simple {
return { name: '', age: undefined, createdAt: undefined };
return { name: '', age: undefined, createdAt: undefined, hyphen: '', spaces: '', dollarStart: '', dollarEnd: '' };
}

export const Simple = {
Expand All @@ -26,6 +30,18 @@ export const Simple = {
if (message.createdAt !== undefined) {
Timestamp.encode(toTimestamp(message.createdAt), writer.uint32(74).fork()).ldelim();
}
if (message.hyphen !== '') {
writer.uint32(26).string(message.hyphen);
}
if (message.spaces !== '') {
writer.uint32(34).string(message.spaces);
}
if (message.dollarStart !== '') {
writer.uint32(42).string(message.dollarStart);
}
if (message.dollarEnd !== '') {
writer.uint32(50).string(message.dollarEnd);
}
return writer;
},

Expand All @@ -45,6 +61,18 @@ export const Simple = {
case 9:
message.createdAt = fromTimestamp(Timestamp.decode(reader, reader.uint32()));
break;
case 3:
message.hyphen = reader.string();
break;
case 4:
message.spaces = reader.string();
break;
case 5:
message.dollarStart = reader.string();
break;
case 6:
message.dollarEnd = reader.string();
break;
default:
reader.skipType(tag & 7);
break;
Expand All @@ -58,6 +86,10 @@ export const Simple = {
name: isSet(object.other_name) ? String(object.other_name) : '',
age: isSet(object.other_age) ? Number(object.other_age) : undefined,
createdAt: isSet(object.createdAt) ? fromJsonTimestamp(object.createdAt) : undefined,
hyphen: isSet(object['hyphened-name']) ? String(object['hyphened-name']) : '',
spaces: isSet(object['name with spaces']) ? String(object['name with spaces']) : '',
dollarStart: isSet(object.$dollar) ? String(object.$dollar) : '',
dollarEnd: isSet(object.dollar$) ? String(object.dollar$) : '',
};
},

Expand All @@ -66,6 +98,10 @@ export const Simple = {
message.name !== undefined && (obj.other_name = message.name);
message.age !== undefined && (obj.other_age = Math.round(message.age));
message.createdAt !== undefined && (obj.createdAt = message.createdAt.toISOString());
message.hyphen !== undefined && (obj['hyphened-name'] = message.hyphen);
message.spaces !== undefined && (obj['name with spaces'] = message.spaces);
message.dollarStart !== undefined && (obj.$dollar = message.dollarStart);
message.dollarEnd !== undefined && (obj.dollar$ = message.dollarEnd);
return obj;
},

Expand All @@ -74,6 +110,10 @@ export const Simple = {
message.name = object.name ?? '';
message.age = object.age ?? undefined;
message.createdAt = object.createdAt ?? undefined;
message.hyphen = object.hyphen ?? '';
message.spaces = object.spaces ?? '';
message.dollarStart = object.dollarStart ?? '';
message.dollarEnd = object.dollarEnd ?? '';
return message;
},
};
Expand Down
2 changes: 1 addition & 1 deletion integration/watch.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ function yarnRun(yarn: string, task: string, header: string, taskArgument?: stri
}

function srcHandler(yarn: string, task: string, tests: string[]) {
return async (event: FsEvent, triggerPath: string) => {
return async (triggerPath: string) => {
triggerPath = triggerPath.replace(/\\/g, '/');
if (tests.length === 0) {
const notice = `Plugin modified! Press [enter] to regenerate all integration tests or run 'yarn watch [TEST, ...]'. See 'yarn watch --help'.`;
Expand Down
39 changes: 21 additions & 18 deletions src/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,12 @@ import {
import SourceInfo, { Fields } from './sourceInfo';
import {
assertInstanceOf,
determineFieldJsonName,
getFieldJsonName,
FormattedMethodDescriptor,
impProto,
maybeAddComment,
maybePrefixPackage,
getPropertyAccessor,
} from './utils';
import { camelToSnake, capitalize, maybeSnakeToCamel } from './case';
import {
Expand Down Expand Up @@ -1114,7 +1115,8 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
// add a check for each incoming field
messageDesc.field.forEach((field) => {
const fieldName = maybeSnakeToCamel(field.name, options);
const jsonName = determineFieldJsonName(field, options);
const jsonName = getFieldJsonName(field, options);
const jsonProperty = getPropertyAccessor('object', jsonName);

// get code that extracts value from incoming object
const readSnippet = (from: string): Code => {
Expand Down Expand Up @@ -1212,8 +1214,8 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
const fieldType = toTypeName(ctx, messageDesc, field);
const i = maybeCastToNumber(ctx, messageDesc, field, 'key');
chunks.push(code`
${fieldName}: ${ctx.utils.isObject}(object.${jsonName})
? Object.entries(object.${jsonName}).reduce<${fieldType}>((acc, [key, value]) => {
${fieldName}: ${ctx.utils.isObject}(${jsonProperty})
? Object.entries(${jsonProperty}).reduce<${fieldType}>((acc, [key, value]) => {
acc[${i}] = ${readSnippet('value')};
return acc;
}, {})
Expand All @@ -1222,11 +1224,11 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
} else {
const readValueSnippet = readSnippet('e');
if (readValueSnippet.toString() === code`e`.toString()) {
chunks.push(code`${fieldName}: Array.isArray(object?.${jsonName}) ? [...object.${jsonName}] : [],`);
chunks.push(code`${fieldName}: Array.isArray(object?.${jsonName}) ? [...${jsonProperty}] : [],`);
} else {
// Explicit `any` type required to make TS with noImplicitAny happy. `object` is also `any` here.
chunks.push(code`
${fieldName}: Array.isArray(object?.${jsonName}) ? object.${jsonName}.map((e: any) => ${readValueSnippet}): [],
${fieldName}: Array.isArray(object?.${jsonName}) ? ${jsonProperty}.map((e: any) => ${readValueSnippet}): [],
`);
}
}
Expand All @@ -1240,35 +1242,35 @@ function generateFromJson(ctx: Context, fullName: string, messageDesc: Descripto
chunks.push(code`${fieldName}: `);
}

const ternaryIf = code`${ctx.utils.isSet}(object.${jsonName})`;
const ternaryThen = code`{ $case: '${fieldName}', ${fieldName}: ${readSnippet(`object.${jsonName}`)}`;
const ternaryIf = code`${ctx.utils.isSet}(${jsonProperty})`;
const ternaryThen = code`{ $case: '${fieldName}', ${fieldName}: ${readSnippet(`${jsonProperty}`)}`;
chunks.push(code`${ternaryIf} ? ${ternaryThen}} : `);

if (field === lastCase) {
chunks.push(code`undefined,`);
}
} else if (isAnyValueType(field)) {
chunks.push(code`${fieldName}: ${ctx.utils.isSet}(object?.${jsonName})
? ${readSnippet(`object.${jsonName}`)}
? ${readSnippet(`${jsonProperty}`)}
: undefined,
`);
} else if (isStructType(field)) {
chunks.push(
code`${fieldName}: ${ctx.utils.isObject}(object.${jsonName})
? ${readSnippet(`object.${jsonName}`)}
code`${fieldName}: ${ctx.utils.isObject}(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: undefined,`
);
} else if (isListValueType(field)) {
chunks.push(code`
${fieldName}: Array.isArray(object.${jsonName})
? ${readSnippet(`object.${jsonName}`)}
${fieldName}: Array.isArray(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: undefined,
`);
} else {
const fallback = isWithinOneOf(field) ? 'undefined' : defaultValue(ctx, field);
chunks.push(code`
${fieldName}: ${ctx.utils.isSet}(object.${jsonName})
? ${readSnippet(`object.${jsonName}`)}
${fieldName}: ${ctx.utils.isSet}(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: ${fallback},
`);
}
Expand All @@ -1292,7 +1294,8 @@ function generateToJson(ctx: Context, fullName: string, messageDesc: DescriptorP
// then add a case for each field
messageDesc.field.forEach((field) => {
const fieldName = maybeSnakeToCamel(field.name, options);
const jsonName = determineFieldJsonName(field, options);
const jsonName = getFieldJsonName(field, options);
const jsonProperty = getPropertyAccessor('obj', jsonName);

const readSnippet = (from: string): Code => {
if (isEnum(field)) {
Expand Down Expand Up @@ -1382,10 +1385,10 @@ function generateToJson(ctx: Context, fullName: string, messageDesc: DescriptorP
// oneofs in a union are only output as `oneof name = ...`
const oneofName = maybeSnakeToCamel(messageDesc.oneofDecl[field.oneofIndex].name, options);
const v = readSnippet(`message.${oneofName}?.${fieldName}`);
chunks.push(code`message.${oneofName}?.$case === '${fieldName}' && (obj.${jsonName} = ${v});`);
chunks.push(code`message.${oneofName}?.$case === '${fieldName}' && (${jsonProperty} = ${v});`);
} else {
const v = readSnippet(`message.${fieldName}`);
chunks.push(code`message.${fieldName} !== undefined && (obj.${jsonName} = ${v});`);
chunks.push(code`message.${fieldName} !== undefined && (${jsonProperty} = ${v});`);
}
});
chunks.push(code`return obj;`);
Expand Down
15 changes: 14 additions & 1 deletion src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export class FormattedMethodDescriptor implements MethodDescriptorProto {
}
}

export function determineFieldJsonName(field: FieldDescriptorProto, options: Options): string {
export function getFieldJsonName(field: FieldDescriptorProto, options: Options): string {
// jsonName will be camelCased by the protocol compiler, plus can be overridden by the user,
// so just use that instead of our own maybeSnakeToCamel
if (options.snakeToCamel.includes('json')) {
Expand All @@ -184,6 +184,19 @@ export function determineFieldJsonName(field: FieldDescriptorProto, options: Opt
}
}

/**
* Returns a snippet for reading an object's property, such as `foo.bar`, or `foo['bar']` if the property name contains unusual characters.
* For simplicity, we don't match the ECMA 5/6 rules for valid identifiers exactly, and return array syntax liberally.
* @param objectName
* @param propertyName
*/
export function getPropertyAccessor(objectName: string, propertyName: string): string {
let validIdentifier = /^[a-zA-Z_$][\w$]*$/;
return validIdentifier.test(propertyName)
? `${objectName}.${propertyName}`
: `${objectName}[${JSON.stringify(propertyName)}]`;
}

export function impProto(options: Options, module: string, type: string): Import {
return imp(`${type}@./${module}${options.fileSuffix}`);
}

0 comments on commit ce44668

Please sign in to comment.