Skip to content

Commit cf2fb59

Browse files
buzapbuzap
authoredAug 18, 2023
fix: always use Map for int64 keys (#708) (#905)
* fix: always use Map for int64 keys (#708) * build: change prettier trailingComma to all
·
v2.7.0v1.156.7
1 parent 26fad31 commit cf2fb59

11 files changed

+146
-73
lines changed
 

‎.prettierrc

+3-2
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
{
2-
"printWidth": 120
3-
}
2+
"printWidth": 120,
3+
"trailingComma": "all"
4+
}

‎integration/simple-long/simple-test.ts

+31-10
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import * as Long from "long";
12
import { SimpleWithMap } from "./simple";
23

34
describe("simple", () => {
@@ -6,7 +7,7 @@ describe("simple", () => {
67
expect(s1).toMatchInlineSnapshot(`
78
{
89
"intLookup": {},
9-
"longLookup": {},
10+
"longLookup": Map {},
1011
"nameLookup": {},
1112
}
1213
`);
@@ -22,7 +23,7 @@ describe("simple", () => {
2223
"1": 2,
2324
"2": 1,
2425
},
25-
"longLookup": {},
26+
"longLookup": Map {},
2627
"nameLookup": {},
2728
}
2829
`);
@@ -31,21 +32,31 @@ describe("simple", () => {
3132
it("can fromPartial maps", () => {
3233
const s1 = SimpleWithMap.fromPartial({
3334
intLookup: { 1: 2, 2: 1 },
34-
longLookup: { "1": 2, "2": 1 },
35+
longLookup: new Map(),
3536
});
37+
s1.longLookup.set(Long.fromInt(1), Long.fromInt(2));
38+
s1.longLookup.set(Long.fromInt(2), Long.fromInt(1));
3639
expect(s1).toMatchInlineSnapshot(`
3740
{
3841
"intLookup": {
3942
"1": 2,
4043
"2": 1,
4144
},
42-
"longLookup": {
43-
"1": Long {
45+
"longLookup": Map {
46+
Long {
47+
"high": 0,
48+
"low": 1,
49+
"unsigned": false,
50+
} => Long {
4451
"high": 0,
4552
"low": 2,
4653
"unsigned": false,
4754
},
48-
"2": Long {
55+
Long {
56+
"high": 0,
57+
"low": 2,
58+
"unsigned": false,
59+
} => Long {
4960
"high": 0,
5061
"low": 1,
5162
"unsigned": false,
@@ -59,8 +70,10 @@ describe("simple", () => {
5970
it("can toJSON/fromJSON maps", () => {
6071
const s1 = SimpleWithMap.fromPartial({
6172
intLookup: { 1: 2, 2: 1 },
62-
longLookup: { "1": 2, "2": 1 },
73+
longLookup: new Map(),
6374
});
75+
s1.longLookup.set(Long.fromInt(1), Long.fromInt(2));
76+
s1.longLookup.set(Long.fromInt(2), Long.fromInt(1));
6477

6578
const json = SimpleWithMap.toJSON(s1);
6679
expect(json).toMatchInlineSnapshot(`
@@ -83,13 +96,21 @@ describe("simple", () => {
8396
"1": 2,
8497
"2": 1,
8598
},
86-
"longLookup": {
87-
"1": Long {
99+
"longLookup": Map {
100+
Long {
101+
"high": 0,
102+
"low": 1,
103+
"unsigned": false,
104+
} => Long {
88105
"high": 0,
89106
"low": 2,
90107
"unsigned": false,
91108
},
92-
"2": Long {
109+
Long {
110+
"high": 0,
111+
"low": 2,
112+
"unsigned": false,
113+
} => Long {
93114
"high": 0,
94115
"low": 1,
95116
"unsigned": false,

‎integration/simple-long/simple.bin

-110 Bytes
Binary file not shown.

‎integration/simple-long/simple.proto

+1-2
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,7 @@ message SimpleWithWrappers {
1414
message SimpleWithMap {
1515
map<string, string> nameLookup = 2;
1616
map<int32, int32> intLookup = 3;
17-
// Ideally we'd test map<int64, int64> but we present maps as JS objects and `Long` cannot be used as a keys.
18-
map<string, int64> longLookup = 4;
17+
map<int64, int64> longLookup = 4;
1918
}
2019

2120
message Numbers {

‎integration/simple-long/simple.ts

+56-34
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,7 @@ export interface SimpleWithWrappers {
1717
export interface SimpleWithMap {
1818
nameLookup: { [key: string]: string };
1919
intLookup: { [key: number]: number };
20-
/** Ideally we'd test map<int64, int64> but we present maps as JS objects and `Long` cannot be used as a keys. */
21-
longLookup: { [key: string]: Long };
20+
longLookup: Map<Long, Long>;
2221
}
2322

2423
export interface SimpleWithMap_NameLookupEntry {
@@ -32,7 +31,7 @@ export interface SimpleWithMap_IntLookupEntry {
3231
}
3332

3433
export interface SimpleWithMap_LongLookupEntry {
35-
key: string;
34+
key: Long;
3635
value: Long;
3736
}
3837

@@ -190,7 +189,7 @@ export const SimpleWithWrappers = {
190189
};
191190

192191
function createBaseSimpleWithMap(): SimpleWithMap {
193-
return { nameLookup: {}, intLookup: {}, longLookup: {} };
192+
return { nameLookup: {}, intLookup: {}, longLookup: new Map() };
194193
}
195194

196195
export const SimpleWithMap = {
@@ -201,7 +200,7 @@ export const SimpleWithMap = {
201200
Object.entries(message.intLookup).forEach(([key, value]) => {
202201
SimpleWithMap_IntLookupEntry.encode({ key: key as any, value }, writer.uint32(26).fork()).ldelim();
203202
});
204-
Object.entries(message.longLookup).forEach(([key, value]) => {
203+
(message.longLookup).forEach((value, key) => {
205204
SimpleWithMap_LongLookupEntry.encode({ key: key as any, value }, writer.uint32(34).fork()).ldelim();
206205
});
207206
return writer;
@@ -241,7 +240,7 @@ export const SimpleWithMap = {
241240

242241
const entry4 = SimpleWithMap_LongLookupEntry.decode(reader, reader.uint32());
243242
if (entry4.value !== undefined) {
244-
message.longLookup[entry4.key] = entry4.value;
243+
message.longLookup.set(entry4.key, entry4.value);
245244
}
246245
continue;
247246
}
@@ -268,11 +267,11 @@ export const SimpleWithMap = {
268267
}, {})
269268
: {},
270269
longLookup: isObject(object.longLookup)
271-
? Object.entries(object.longLookup).reduce<{ [key: string]: Long }>((acc, [key, value]) => {
272-
acc[key] = Long.fromValue(value as Long | string);
270+
? Object.entries(object.longLookup).reduce<Map<Long, Long>>((acc, [key, value]) => {
271+
acc.set(Long.fromValue(key), Long.fromValue(value as Long | string));
273272
return acc;
274-
}, {})
275-
: {},
273+
}, new Map())
274+
: new Map(),
276275
};
277276
},
278277

@@ -296,14 +295,11 @@ export const SimpleWithMap = {
296295
});
297296
}
298297
}
299-
if (message.longLookup) {
300-
const entries = Object.entries(message.longLookup);
301-
if (entries.length > 0) {
302-
obj.longLookup = {};
303-
entries.forEach(([k, v]) => {
304-
obj.longLookup[k] = v.toString();
305-
});
306-
}
298+
if (message.longLookup?.size) {
299+
obj.longLookup = {};
300+
message.longLookup.forEach((v, k) => {
301+
obj.longLookup[longToNumber(k)] = v.toString();
302+
});
307303
}
308304
return obj;
309305
},
@@ -331,15 +327,15 @@ export const SimpleWithMap = {
331327
},
332328
{},
333329
);
334-
message.longLookup = Object.entries(object.longLookup ?? {}).reduce<{ [key: string]: Long }>(
335-
(acc, [key, value]) => {
330+
message.longLookup = (() => {
331+
const m = new Map();
332+
(object.longLookup as Map<Long, Long> ?? new Map()).forEach((value, key) => {
336333
if (value !== undefined) {
337-
acc[key] = Long.fromValue(value);
334+
m.set(key, Long.fromValue(value));
338335
}
339-
return acc;
340-
},
341-
{},
342-
);
336+
});
337+
return m;
338+
})();
343339
return message;
344340
},
345341
};
@@ -489,13 +485,13 @@ export const SimpleWithMap_IntLookupEntry = {
489485
};
490486

491487
function createBaseSimpleWithMap_LongLookupEntry(): SimpleWithMap_LongLookupEntry {
492-
return { key: "", value: Long.ZERO };
488+
return { key: Long.ZERO, value: Long.ZERO };
493489
}
494490

495491
export const SimpleWithMap_LongLookupEntry = {
496492
encode(message: SimpleWithMap_LongLookupEntry, writer: _m0.Writer = _m0.Writer.create()): _m0.Writer {
497-
if (message.key !== "") {
498-
writer.uint32(10).string(message.key);
493+
if (!message.key.isZero()) {
494+
writer.uint32(8).int64(message.key);
499495
}
500496
if (!message.value.isZero()) {
501497
writer.uint32(16).int64(message.value);
@@ -511,11 +507,11 @@ export const SimpleWithMap_LongLookupEntry = {
511507
const tag = reader.uint32();
512508
switch (tag >>> 3) {
513509
case 1:
514-
if (tag !== 10) {
510+
if (tag !== 8) {
515511
break;
516512
}
517513

518-
message.key = reader.string();
514+
message.key = reader.int64() as Long;
519515
continue;
520516
case 2:
521517
if (tag !== 16) {
@@ -535,15 +531,15 @@ export const SimpleWithMap_LongLookupEntry = {
535531

536532
fromJSON(object: any): SimpleWithMap_LongLookupEntry {
537533
return {
538-
key: isSet(object.key) ? String(object.key) : "",
534+
key: isSet(object.key) ? Long.fromValue(object.key) : Long.ZERO,
539535
value: isSet(object.value) ? Long.fromValue(object.value) : Long.ZERO,
540536
};
541537
},
542538

543539
toJSON(message: SimpleWithMap_LongLookupEntry): unknown {
544540
const obj: any = {};
545-
if (message.key !== "") {
546-
obj.key = message.key;
541+
if (!message.key.isZero()) {
542+
obj.key = (message.key || Long.ZERO).toString();
547543
}
548544
if (!message.value.isZero()) {
549545
obj.value = (message.value || Long.ZERO).toString();
@@ -558,7 +554,7 @@ export const SimpleWithMap_LongLookupEntry = {
558554
object: I,
559555
): SimpleWithMap_LongLookupEntry {
560556
const message = createBaseSimpleWithMap_LongLookupEntry();
561-
message.key = object.key ?? "";
557+
message.key = (object.key !== undefined && object.key !== null) ? Long.fromValue(object.key) : Long.ZERO;
562558
message.value = (object.value !== undefined && object.value !== null) ? Long.fromValue(object.value) : Long.ZERO;
563559
return message;
564560
},
@@ -837,6 +833,25 @@ export const Numbers = {
837833
},
838834
};
839835

836+
declare const self: any | undefined;
837+
declare const window: any | undefined;
838+
declare const global: any | undefined;
839+
const tsProtoGlobalThis: any = (() => {
840+
if (typeof globalThis !== "undefined") {
841+
return globalThis;
842+
}
843+
if (typeof self !== "undefined") {
844+
return self;
845+
}
846+
if (typeof window !== "undefined") {
847+
return window;
848+
}
849+
if (typeof global !== "undefined") {
850+
return global;
851+
}
852+
throw "Unable to locate global object";
853+
})();
854+
840855
type Builtin = Date | Function | Uint8Array | string | number | boolean | undefined;
841856

842857
export type DeepPartial<T> = T extends Builtin ? T
@@ -849,6 +864,13 @@ type KeysOfUnion<T> = T extends T ? keyof T : never;
849864
export type Exact<P, I extends P> = P extends Builtin ? P
850865
: P & { [K in keyof P]: Exact<P[K], I[K]> } & { [K in Exclude<keyof I, KeysOfUnion<P>>]: never };
851866

867+
function longToNumber(long: Long): number {
868+
if (long.gt(Number.MAX_SAFE_INTEGER)) {
869+
throw new tsProtoGlobalThis.Error("Value is larger than Number.MAX_SAFE_INTEGER");
870+
}
871+
return long.toNumber();
872+
}
873+
852874
if (_m0.util.Long !== Long) {
853875
_m0.util.Long = Long as any;
854876
_m0.configure();

‎package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
"mongodb": "^5.7.0",
5555
"nice-grpc": "^2.1.4",
5656
"object-hash": "^3.0.0",
57-
"prettier": "^3.0.0",
57+
"prettier": "^2.8.8",
5858
"protobufjs-cli": "^1.1.1",
5959
"reflect-metadata": "^0.1.13",
6060
"rxjs": "^7.8.1",

‎src/enums.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,8 @@ export function generateEnum(
4141
if (options.unrecognizedEnum)
4242
chunks.push(code`
4343
${UNRECOGNIZED_ENUM_NAME} ${delimiter} ${
44-
options.stringEnums ? `"${UNRECOGNIZED_ENUM_NAME}"` : UNRECOGNIZED_ENUM_VALUE.toString()
45-
},`);
44+
options.stringEnums ? `"${UNRECOGNIZED_ENUM_NAME}"` : UNRECOGNIZED_ENUM_VALUE.toString()
45+
},`);
4646

4747
if (options.enumsAsLiterals) {
4848
chunks.push(code`} as const`);

‎src/generate-services.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -309,8 +309,8 @@ function generateCachingRpcMethod(
309309
const responses = requests.map(async request => {
310310
const data = ${inputType}.encode(request).finish()
311311
const response = await this.rpc.request(ctx, "${maybePrefixPackage(fileDesc, serviceDesc.name)}", "${
312-
methodDesc.name
313-
}", data);
312+
methodDesc.name
313+
}", data);
314314
return ${outputType}.decode(${Reader}.create(response));
315315
});
316316
return Promise.all(responses);

‎src/main.ts

+20-13
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import {
4242
packedType,
4343
toReaderCall,
4444
toTypeName,
45+
shouldGenerateJSMapType,
4546
valueTypeName,
4647
} from "./types";
4748
import SourceInfo, { Fields } from "./sourceInfo";
@@ -960,7 +961,7 @@ function generateBaseInstanceFactory(
960961
const val = isWithinOneOf(field)
961962
? "undefined"
962963
: isMapType(ctx, messageDesc, field)
963-
? ctx.options.useMapType
964+
? shouldGenerateJSMapType(ctx, messageDesc, field)
964965
? "new Map()"
965966
: "{}"
966967
: isRepeated(field)
@@ -1098,17 +1099,22 @@ function generateDecode(ctx: Context, fullName: string, messageDesc: DescriptorP
10981099
if (isRepeated(field)) {
10991100
const maybeNonNullAssertion = ctx.options.useOptionals === "all" ? "!" : "";
11001101

1101-
if (isMapType(ctx, messageDesc, field)) {
1102+
const mapType = detectMapType(ctx, messageDesc, field);
1103+
if (mapType) {
11021104
// We need a unique const within the `cast` statement
11031105
const varName = `entry${field.number}`;
1106+
const generateMapType = shouldGenerateJSMapType(ctx, messageDesc, field);
11041107

1105-
const valueSetterSnippet = ctx.options.useMapType
1106-
? `message.${fieldName}${maybeNonNullAssertion}.set(${varName}.key, ${varName}.value)`
1107-
: `message.${fieldName}${maybeNonNullAssertion}[${varName}.key] = ${varName}.value`;
1108+
let valueSetterSnippet: string;
1109+
if (generateMapType) {
1110+
valueSetterSnippet = `message.${fieldName}${maybeNonNullAssertion}.set(${varName}.key, ${varName}.value)`;
1111+
} else {
1112+
valueSetterSnippet = `message.${fieldName}${maybeNonNullAssertion}[${varName}.key] = ${varName}.value`;
1113+
}
11081114
const initializerSnippet = initializerNecessary
11091115
? `
11101116
if (message.${fieldName} === undefined) {
1111-
message.${fieldName} = ${ctx.options.useMapType ? "new Map()" : "{}"};
1117+
message.${fieldName} = ${generateMapType ? "new Map()" : "{}"};
11121118
}`
11131119
: "";
11141120
chunks.push(code`
@@ -1314,9 +1320,10 @@ function generateEncode(ctx: Context, fullName: string, messageDesc: DescriptorP
13141320
}
13151321
`
13161322
: writeSnippet(`{ ${maybeTypeField} key: key as any, value }`);
1317-
const optionalAlternative = isOptional ? (ctx.options.useMapType ? " || new Map()" : " || {}") : "";
1323+
const useMapType = shouldGenerateJSMapType(ctx, messageDesc, field);
1324+
const optionalAlternative = isOptional ? (useMapType ? " || new Map()" : " || {}") : "";
13181325

1319-
if (ctx.options.useMapType) {
1326+
if (useMapType) {
13201327
chunks.push(code`
13211328
(message.${fieldName}${optionalAlternative}).forEach((value, key) => {
13221329
${entryWriteSnippet}
@@ -1833,7 +1840,7 @@ function generateFromJson(ctx: Context, fullName: string, fullTypeName: string,
18331840
const fieldType = toTypeName(ctx, messageDesc, field);
18341841
const i = convertFromObjectKey(ctx, messageDesc, field, "key");
18351842

1836-
if (ctx.options.useMapType) {
1843+
if (shouldGenerateJSMapType(ctx, messageDesc, field)) {
18371844
const fallback = noDefaultValue ? "undefined" : "new Map()";
18381845

18391846
chunks.push(code`
@@ -2023,7 +2030,7 @@ function generateToJson(
20232030
// Maps might need their values transformed, i.e. bytes --> base64
20242031
const i = convertToObjectKey(ctx, messageDesc, field, "k");
20252032

2026-
if (ctx.options.useMapType) {
2033+
if (shouldGenerateJSMapType(ctx, messageDesc, field)) {
20272034
chunks.push(code`
20282035
if (message.${fieldName}?.size) {
20292036
${jsonProperty} = {};
@@ -2189,7 +2196,7 @@ function generateFromPartial(ctx: Context, fullName: string, messageDesc: Descri
21892196
? `(object.${fieldName} === undefined || object.${fieldName} === null) ? undefined : `
21902197
: "";
21912198

2192-
if (ctx.options.useMapType) {
2199+
if (shouldGenerateJSMapType(ctx, messageDesc, field)) {
21932200
chunks.push(code`
21942201
message.${fieldName} = ${noValueSnippet} (() => {
21952202
const m = new Map();
@@ -2261,7 +2268,7 @@ function convertFromObjectKey(
22612268
const { keyType, keyField } = detectMapType(ctx, messageDesc, field)!;
22622269
if (keyType.toCodeString([]) === "string") {
22632270
return code`${variableName}`;
2264-
} else if (isLong(keyField) && ctx.options.useMapType) {
2271+
} else if (isLong(keyField) && shouldGenerateJSMapType(ctx, messageDesc, field)) {
22652272
if (ctx.options.forceLong === LongOption.LONG) {
22662273
return code`${capitalize(keyType.toCodeString([]))}.fromValue(${variableName})`;
22672274
} else if (ctx.options.forceLong === LongOption.BIGINT) {
@@ -2285,7 +2292,7 @@ function convertToObjectKey(
22852292
const { keyType, keyField } = detectMapType(ctx, messageDesc, field)!;
22862293
if (keyType.toCodeString([]) === "string") {
22872294
return code`${variableName}`;
2288-
} else if (isLong(keyField) && ctx.options.useMapType) {
2295+
} else if (isLong(keyField) && shouldGenerateJSMapType(ctx, messageDesc, field)) {
22892296
if (ctx.options.forceLong === LongOption.LONG) {
22902297
return code`${ctx.utils.longToNumber}(${variableName})`;
22912298
} else if (ctx.options.forceLong === LongOption.BIGINT) {

‎src/types.ts

+24-1
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,7 @@ export function toTypeName(
620620
const mapType = messageDesc ? detectMapType(ctx, messageDesc, field) : false;
621621
if (mapType) {
622622
const { keyType, valueType } = mapType;
623-
if (ctx.options.useMapType) {
623+
if (shouldGenerateJSMapType(ctx, messageDesc!, field)) {
624624
return finalize(code`Map<${keyType}, ${valueType}>`, ensureOptional);
625625
}
626626
return finalize(code`{ [key: ${keyType} ]: ${valueType} }`, ensureOptional);
@@ -660,6 +660,29 @@ export function toTypeName(
660660
);
661661
}
662662

663+
/**
664+
* For a protobuf map field, if the generated code should use the javascript Map type.
665+
*
666+
* If the type of a protobuf map key corresponds to the Long type, we always use the Map type. This avoids generating
667+
* invalid code such as below (using Long as key of a javascript object):
668+
*
669+
* export interface Foo {
670+
* bar: { [key: Long]: Long }
671+
* }
672+
*
673+
* See https://github.com/stephenh/ts-proto/issues/708 for more details.
674+
*/
675+
export function shouldGenerateJSMapType(ctx: Context, message: DescriptorProto, field: FieldDescriptorProto): boolean {
676+
if (ctx.options.useMapType) {
677+
return true;
678+
}
679+
const mapType = detectMapType(ctx, message, field);
680+
if (!mapType) {
681+
return false;
682+
}
683+
return isLong(mapType.keyField) && ctx.options.forceLong === LongOption.LONG;
684+
}
685+
663686
export function detectMapType(
664687
ctx: Context,
665688
messageDesc: DescriptorProto,

‎yarn.lock

+6-6
Original file line numberDiff line numberDiff line change
@@ -6622,12 +6622,12 @@ __metadata:
66226622
languageName: node
66236623
linkType: hard
66246624

6625-
"prettier@npm:^3.0.0":
6626-
version: 3.0.0
6627-
resolution: "prettier@npm:3.0.0"
6625+
"prettier@npm:^2.8.8":
6626+
version: 2.8.8
6627+
resolution: "prettier@npm:2.8.8"
66286628
bin:
6629-
prettier: bin/prettier.cjs
6630-
checksum: 6a832876a1552dc58330d2467874e5a0b46b9ccbfc5d3531eb69d15684743e7f83dc9fbd202db6270446deba9c82b79d24383d09924c462b457136a759425e33
6629+
prettier: bin-prettier.js
6630+
checksum: b49e409431bf129dd89238d64299ba80717b57ff5a6d1c1a8b1a28b590d998a34e083fa13573bc732bb8d2305becb4c9a4407f8486c81fa7d55100eb08263cf8
66316631
languageName: node
66326632
linkType: hard
66336633

@@ -7854,7 +7854,7 @@ __metadata:
78547854
mongodb: ^5.7.0
78557855
nice-grpc: ^2.1.4
78567856
object-hash: ^3.0.0
7857-
prettier: ^3.0.0
7857+
prettier: ^2.8.8
78587858
protobufjs: ^7.2.4
78597859
protobufjs-cli: ^1.1.1
78607860
reflect-metadata: ^0.1.13

0 commit comments

Comments
 (0)
Please sign in to comment.