Skip to content

Commit

Permalink
fix: Simplify safe key handling. (#950)
Browse files Browse the repository at this point in the history
  • Loading branch information
@stephenh
stephenh committed Oct 13, 2023
1 parent ed9ceb9 commit 5e0e6ca
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 54 deletions.
74 changes: 37 additions & 37 deletions src/main.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,13 @@ import {
assertInstanceOf,
FormattedMethodDescriptor,
getPropertyAccessor,
propertyNameComposition,
impFile,
impProto,
maybeAddComment,
maybePrefixPackage,
getFieldJsonName,
getFieldName,
safeAccessor,
} from "./utils";
import { camelToSnake, capitalize, maybeSnakeToCamel } from "./case";
import {
Expand Down Expand Up @@ -877,10 +878,10 @@ function generateInterfaceDeclaration(

const info = sourceInfo.lookup(Fields.message.field, index);
maybeAddComment(info, chunks, fieldDesc.options?.deprecated);
const { validatedPropertyName } = propertyNameComposition(fieldDesc, options);
const fieldKey = safeAccessor(getFieldName(fieldDesc, options));
const isOptional = isOptionalProperty(fieldDesc, messageDesc.options, options);
const type = toTypeName(ctx, messageDesc, fieldDesc, isOptional);
chunks.push(code`${maybeReadonly(options)}${validatedPropertyName}${isOptional ? "?" : ""}: ${type}, `);
chunks.push(code`${maybeReadonly(options)}${fieldKey}${isOptional ? "?" : ""}: ${type}, `);
});

if (ctx.options.unknownFields) {
Expand Down Expand Up @@ -954,9 +955,9 @@ function generateBaseInstanceFactory(
processedOneofs.add(oneofIndex);

const name = options.useJsonName
? propertyNameComposition(field, options).validatedPropertyName
? getFieldName(field, options)
: maybeSnakeToCamel(messageDesc.oneofDecl[oneofIndex].name, ctx.options);
fields.push(code`${name}: undefined`);
fields.push(code`${safeAccessor(name)}: undefined`);
}
continue;
}
Expand All @@ -965,7 +966,7 @@ function generateBaseInstanceFactory(
continue;
}

const { validatedPropertyName } = propertyNameComposition(field, options);
const fieldKey = safeAccessor(getFieldName(field, options));
const val = isWithinOneOf(field)
? "undefined"
: isMapType(ctx, messageDesc, field)
Expand All @@ -976,7 +977,7 @@ function generateBaseInstanceFactory(
? "[]"
: defaultValue(ctx, field);

fields.push(code`${validatedPropertyName}: ${val}`);
fields.push(code`${fieldKey}: ${val}`);
}

if (addTypeToMessages(options)) {
Expand Down Expand Up @@ -1087,8 +1088,8 @@ function generateDecode(ctx: Context, fullName: string, messageDesc: DescriptorP

// add a case for each incoming field
messageDesc.field.forEach((field) => {
const { propertyName } = propertyNameComposition(field, options);
const messageProperty = getPropertyAccessor("message", propertyName);
const fieldName = getFieldName(field, options);
const messageProperty = getPropertyAccessor("message", fieldName);
chunks.push(code`case ${field.number}:`);

const tag = ((field.number << 3) | basicWireType(field.type)) >>> 0;
Expand Down Expand Up @@ -1178,7 +1179,7 @@ function generateDecode(ctx: Context, fullName: string, messageDesc: DescriptorP
: getPropertyAccessor("message", maybeSnakeToCamel(messageDesc.oneofDecl[field.oneofIndex].name, options));
chunks.push(code`
${tagCheck}
${oneofNameWithMessage} = { $case: '${propertyName}', ${propertyName}: ${readSnippet} };
${oneofNameWithMessage} = { $case: '${fieldName}', ${fieldName}: ${readSnippet} };
`);
} else {
chunks.push(code`
Expand Down Expand Up @@ -1331,8 +1332,8 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP

// then add a case for each field
messageDesc.field.forEach((field) => {
const { propertyName, validatedPropertyName } = propertyNameComposition(field, options);
const messageProperty = getPropertyAccessor("message", propertyName);
const fieldName = getFieldName(field, options);
const messageProperty = getPropertyAccessor("message", fieldName);

// get a generic writer.doSomething based on the basic type
const writeSnippet = getEncodeWriteSnippet(ctx, field);
Expand Down Expand Up @@ -1425,7 +1426,7 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP
writer.uint32(${tag}).fork();
for (const v of ${messageProperty}) {
if (BigInt.asIntN(64, v) !== v) {
throw new Error('a value provided in array field ${propertyName} of type ${fieldType} is too large');
throw new Error('a value provided in array field ${fieldName} of type ${fieldType} is too large');
}
writer.${toReaderCall(field)}(${rhs("v")});
}
Expand All @@ -1438,7 +1439,7 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP
writer.uint32(${tag}).fork();
for (const v of ${messageProperty}) {
if (BigInt.asUintN(64, v) !== v) {
throw new Error('a value provided in array field ${propertyName} of type ${fieldType} is too large');
throw new Error('a value provided in array field ${fieldName} of type ${fieldType} is too large');
}
writer.${toReaderCall(field)}(${rhs("v")});
}
Expand Down Expand Up @@ -1794,7 +1795,8 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,

// add a check for each incoming field
messageDesc.field.forEach((field) => {
const { validatedPropertyName: fieldName } = propertyNameComposition(field, options);
const fieldName = getFieldName(field, options);
const fieldKey = safeAccessor(fieldName);
const jsonName = getFieldJsonName(field, options);
const jsonProperty = getPropertyAccessor("object", jsonName);
const jsonPropertyOptional = getPropertyAccessor("object", jsonName, true);
Expand Down Expand Up @@ -1910,7 +1912,7 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,
const fallback = noDefaultValue ? "undefined" : "new Map()";

chunks.push(code`
${fieldName}: ${ctx.utils.isObject}(${jsonProperty})
${fieldKey}: ${ctx.utils.isObject}(${jsonProperty})
? Object.entries(${jsonProperty}).reduce<${fieldType}>((acc, [key, value]) => {
acc.set(${i}, ${readSnippet("value")});
return acc;
Expand All @@ -1921,7 +1923,7 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,
const fallback = noDefaultValue ? "undefined" : "{}";

chunks.push(code`
${fieldName}: ${ctx.utils.isObject}(${jsonProperty})
${fieldKey}: ${ctx.utils.isObject}(${jsonProperty})
? Object.entries(${jsonProperty}).reduce<${fieldType}>((acc, [key, value]) => {
acc[${i}] = ${readSnippet("value")};
return acc;
Expand All @@ -1935,12 +1937,12 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,
const readValueSnippet = readSnippet("e");
if (readValueSnippet.toString() === code`e`.toString()) {
chunks.push(
code`${fieldName}: ${ctx.utils.globalThis}.Array.isArray(${jsonPropertyOptional}) ? [...${jsonProperty}] : [],`,
code`${fieldKey}: ${ctx.utils.globalThis}.Array.isArray(${jsonPropertyOptional}) ? [...${jsonProperty}] : [],`,
);
} else {
// Explicit `any` type required to make TS with noImplicitAny happy. `object` is also `any` here.
chunks.push(code`
${fieldName}: ${ctx.utils.globalThis}.Array.isArray(${jsonPropertyOptional}) ? ${jsonProperty}.map((e: any) => ${readValueSnippet}): ${fallback},
${fieldKey}: ${ctx.utils.globalThis}.Array.isArray(${jsonPropertyOptional}) ? ${jsonProperty}.map((e: any) => ${readValueSnippet}): ${fallback},
`);
}
}
Expand All @@ -1955,33 +1957,33 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,
}

const ternaryIf = code`${ctx.utils.isSet}(${jsonProperty})`;
const ternaryThen = code`{ $case: '${fieldName}', ${fieldName}: ${readSnippet(`${jsonProperty}`)}`;
const ternaryThen = code`{ $case: '${fieldName}', ${fieldKey}: ${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}(${jsonPropertyOptional})
chunks.push(code`${fieldKey}: ${ctx.utils.isSet}(${jsonPropertyOptional})
? ${readSnippet(`${jsonProperty}`)}
: undefined,
`);
} else if (isStructType(field)) {
chunks.push(
code`${fieldName}: ${ctx.utils.isObject}(${jsonProperty})
code`${fieldKey}: ${ctx.utils.isObject}(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: undefined,`,
);
} else if (isListValueType(field)) {
chunks.push(code`
${fieldName}: ${ctx.utils.globalThis}.Array.isArray(${jsonProperty})
${fieldKey}: ${ctx.utils.globalThis}.Array.isArray(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: undefined,
`);
} else {
const fallback = isWithinOneOf(field) || noDefaultValue ? "undefined" : defaultValue(ctx, field);
chunks.push(code`
${fieldName}: ${ctx.utils.isSet}(${jsonProperty})
${fieldKey}: ${ctx.utils.isSet}(${jsonProperty})
? ${readSnippet(`${jsonProperty}`)}
: ${fallback},
`);
Expand Down Expand Up @@ -2027,10 +2029,10 @@ function generateToJson(

// then add a case for each field
messageDesc.field.forEach((field) => {
const { validatedPropertyName: fieldName, propertyName } = propertyNameComposition(field, options);
const fieldName = getFieldName(field, options);
const jsonName = getFieldJsonName(field, options);
const jsonProperty = getPropertyAccessor("obj", jsonName);
const messageProperty = getPropertyAccessor("message", propertyName);
const messageProperty = getPropertyAccessor("message", fieldName);

const readSnippet = (from: string): Code => {
if (isEnum(field)) {
Expand Down Expand Up @@ -2200,9 +2202,9 @@ function generateFromPartial(ctx: Context, fullName: string, messageDesc: Descri

// add a check for each incoming field
messageDesc.field.forEach((field) => {
const { propertyName } = propertyNameComposition(field, options);
const messageProperty = getPropertyAccessor("message", propertyName);
const objectProperty = getPropertyAccessor("object", propertyName);
const fieldName = getFieldName(field, options);
const messageProperty = getPropertyAccessor("message", fieldName);
const objectProperty = getPropertyAccessor("object", fieldName);

const readSnippet = (from: string): Code => {
if ((isLong(field) || isLongValueType(field)) && options.forceLong === LongOption.LONG) {
Expand Down Expand Up @@ -2300,19 +2302,17 @@ function generateFromPartial(ctx: Context, fullName: string, messageDesc: Descri
`);
}
} else if (isWithinOneOfThatShouldBeUnion(options, field)) {
const oneofName = options.useJsonName
? propertyName
: maybeSnakeToCamel(messageDesc.oneofDecl[field.oneofIndex].name, options);
const oneofName = maybeSnakeToCamel(messageDesc.oneofDecl[field.oneofIndex].name, options);
const oneofNameWithMessage = getPropertyAccessor("message", oneofName);
const oneofNameWithObject = getPropertyAccessor("object", oneofName);
const v = readSnippet(`${oneofNameWithObject}.${propertyName}`);
const v = readSnippet(`${oneofNameWithObject}.${fieldName}`);
chunks.push(code`
if (
${oneofNameWithObject}?.$case === '${propertyName}'
&& ${oneofNameWithObject}?.${propertyName} !== undefined
&& ${oneofNameWithObject}?.${propertyName} !== null
${oneofNameWithObject}?.$case === '${fieldName}'
&& ${oneofNameWithObject}?.${fieldName} !== undefined
&& ${oneofNameWithObject}?.${fieldName} !== null
) {
${oneofNameWithMessage} = { $case: '${propertyName}', ${propertyName}: ${v} };
${oneofNameWithMessage} = { $case: '${fieldName}', ${fieldName}: ${v} };
}
`);
} else if (readSnippet(`x`).toCodeString([]) == "x") {
Expand Down
27 changes: 10 additions & 17 deletions src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,33 +238,26 @@ export function getFieldJsonName(
}
}

export function propertyNameComposition(
export function getFieldName(
field: Pick<FieldDescriptorProto, "name" | "jsonName">,
options: Pick<Options, "snakeToCamel" | "useJsonName">,
): { propertyName: string; validatedPropertyName: string } {
): string {
if (options.useJsonName) {
const jsonName = field.jsonName;
return {
propertyName: jsonName,
validatedPropertyName: validateObjectProperty(jsonName),
};
return field.jsonName;
}
const propertyName = maybeSnakeToCamel(field.name, options);
return {
propertyName,
validatedPropertyName: propertyName,
};
return maybeSnakeToCamel(field.name, options);
}

/**
* https://github.com/eslint-community/eslint-plugin-security/blob/main/docs/the-dangers-of-square-bracket-notation.md
*/
function isValidateObjectProperty(propertyName: string): boolean {
function isValidIdentifier(propertyName: string): boolean {
return /^[a-zA-Z_$][\w$]*$/.test(propertyName);
}

function validateObjectProperty(propertyName: string): string {
return isValidateObjectProperty(propertyName) ? propertyName : JSON.stringify(propertyName);
/** Returns `bar` or `"bar"` if `propertyName` isn't a safe property name. */
export function safeAccessor(propertyName: string): string {
return isValidIdentifier(propertyName) ? propertyName : JSON.stringify(propertyName);
}

/**
Expand All @@ -275,9 +268,9 @@ function validateObjectProperty(propertyName: string): string {
* @param optional
*/
export function getPropertyAccessor(objectName: string, propertyName: string, optional: boolean = false): string {
return isValidateObjectProperty(propertyName)
return isValidIdentifier(propertyName)
? `${objectName}${optional ? "?" : ""}.${propertyName}`
: `${objectName}${optional ? "?." : ""}[${validateObjectProperty(propertyName)}]`;
: `${objectName}${optional ? "?." : ""}[${safeAccessor(propertyName)}]`;
}

export function impFile(options: Options, spec: string) {
Expand Down

0 comments on commit 5e0e6ca

Please sign in to comment.