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

feat: Normalize toJSON output by omitting fields set to their default values #878

Merged
merged 4 commits into from
Jul 15, 2023
Merged

feat: Normalize toJSON output by omitting fields set to their default values #878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
543c0f8
Normalize `toJSON` output by omitting fields set to their default values
haines Jul 13, 2023
863bb60
Avoid mapping arrays with the identity function
haines Jul 13, 2023
bb6e0a9
Remove unnecessary undefined checks
haines Jul 13, 2023
6f95fd2
Fix tests
haines Jul 13, 2023
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
13 changes: 1 addition & 12 deletions README.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -728,26 +728,15 @@ Foo.fromJSON({ bar: "" }); // => { bar: '' }
Foo.fromJSON({ bar: "baz" }); // => { bar: 'baz' }
```

When writing JSON, `ts-proto` currently does **not** normalize message when converting to JSON, other than omitting unset fields, but it may do so in the future.
When writing JSON, `ts-proto` normalizes messages by omitting unset fields and fields set to their default values.

```typescript
// Current ts-proto behavior
Foo.toJSON({}); // => { }
Foo.toJSON({ bar: undefined }); // => { }
Foo.toJSON({ bar: "" }); // => { bar: '' } - note: this is the default value, but it's not omitted
Foo.toJSON({ bar: "baz" }); // => { bar: 'baz' }
```

```typescript
// Possible future behavior, where ts-proto would normalize message
Foo.toJSON({}); // => { }
Foo.toJSON({ bar: undefined }); // => { }
Foo.toJSON({ bar: "" }); // => { } - note: omitting the default value, as expected
Foo.toJSON({ bar: "baz" }); // => { bar: 'baz' }
```

- Please open an issue if you need this behavior.

# Well-Known Types

Protobuf comes with several predefined message definitions, called "[Well-Known Types](https://developers.google.com/protocol-buffers/docs/reference/google.protobuf)".
Expand Down
4 changes: 3 additions & 1 deletion integration/angular/simple-message.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ export const SimpleMessage = {

toJSON(message: SimpleMessage): unknown {
const obj: any = {};
message.numberField !== undefined && (obj.numberField = Math.round(message.numberField));
if (message.numberField !== 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

Hi @haines ; just sanity checking, but you think we're good to drop the !== undefined check from before?

I know the numberField is typed as number anyway, so the check was being overly cautious, but I'm thinking we explicitly added it b/c of concerns some prior user/contributor had...

Dunno, personally I'm fine dropping it without a strong opinion / test case asserting why we should keep it, but just wanted to see if you had specific thoughts/strong/soft opinions on it. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, personally I think it makes sense to drop it.

As far as I can tell the only reason this check would be necessary is if there is a bug in the calling code that circumvents the type system. fromJSON correctly populates the default value, and we have fromPartial for manually constructing messages without having to specify every field.

I'm not convinced it is worthwhile to guard against bugs in the calling code. We don't have other runtime type checks (e.g. checking that the values that are supposed to be numbers aren't actually strings), so should this be any different?

There was a test case dating back to 2019 that exercised the checks by doing Simple.toJSON({} as Simple), but I really don't think this is a valid use case (the type cast is incorrect) so I removed it.

useOptionals is there when you want to allow things to be undefined in general, otherwise it is straightforward to call fromPartial before calling toJSON if you somehow have an object without the default values populated.

obj.numberField = Math.round(message.numberField);
}
return obj;
},

Expand Down
4 changes: 3 additions & 1 deletion integration/async-iterable-services-abort-signal/simple.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ export const EchoMsg = {

toJSON(message: EchoMsg): unknown {
const obj: any = {};
message.body !== undefined && (obj.body = message.body);
if (message.body !== "") {
obj.body = message.body;
}
return obj;
},

Expand Down
4 changes: 3 additions & 1 deletion integration/async-iterable-services/simple.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ export const EchoMsg = {

toJSON(message: EchoMsg): unknown {
const obj: any = {};
message.body !== undefined && (obj.body = message.body);
if (message.body !== "") {
obj.body = message.body;
}
return obj;
},

Expand Down
25 changes: 18 additions & 7 deletions integration/avoid-import-conflicts/simple.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,12 @@ export const Simple = {

toJSON(message: Simple): unknown {
const obj: any = {};
message.name !== undefined && (obj.name = message.name);
message.otherSimple !== undefined &&
(obj.otherSimple = message.otherSimple ? Simple3.toJSON(message.otherSimple) : undefined);
if (message.name !== "") {
obj.name = message.name;
}
if (message.otherSimple !== undefined) {
obj.otherSimple = Simple3.toJSON(message.otherSimple);
}
return obj;
},

Expand Down Expand Up @@ -197,8 +200,12 @@ export const SimpleEnums = {

toJSON(message: SimpleEnums): unknown {
const obj: any = {};
message.localEnum !== undefined && (obj.localEnum = simpleEnumToJSON(message.localEnum));
message.importEnum !== undefined && (obj.importEnum = simpleEnumToJSON5(message.importEnum));
if (message.localEnum !== 0) {
obj.localEnum = simpleEnumToJSON(message.localEnum);
}
if (message.importEnum !== 0) {
obj.importEnum = simpleEnumToJSON5(message.importEnum);
}
return obj;
},

Expand Down Expand Up @@ -255,7 +262,9 @@ export const FooServiceCreateRequest = {

toJSON(message: FooServiceCreateRequest): unknown {
const obj: any = {};
message.kind !== undefined && (obj.kind = fooServiceToJSON(message.kind));
if (message.kind !== 0) {
obj.kind = fooServiceToJSON(message.kind);
}
return obj;
},

Expand Down Expand Up @@ -311,7 +320,9 @@ export const FooServiceCreateResponse = {

toJSON(message: FooServiceCreateResponse): unknown {
const obj: any = {};
message.kind !== undefined && (obj.kind = fooServiceToJSON(message.kind));
if (message.kind !== 0) {
obj.kind = fooServiceToJSON(message.kind);
}
return obj;
},

Expand Down
8 changes: 6 additions & 2 deletions integration/avoid-import-conflicts/simple2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,12 @@ export const Simple = {

toJSON(message: Simple): unknown {
const obj: any = {};
message.name !== undefined && (obj.name = message.name);
message.age !== undefined && (obj.age = Math.round(message.age));
if (message.name !== "") {
obj.name = message.name;
}
if (message.age !== 0) {
obj.age = Math.round(message.age);
}
return obj;
},

Expand Down
8 changes: 6 additions & 2 deletions integration/barrel-imports/bar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,12 @@ export const Bar = {

toJSON(message: Bar): unknown {
const obj: any = {};
message.name !== undefined && (obj.name = message.name);
message.age !== undefined && (obj.age = Math.round(message.age));
if (message.name !== "") {
obj.name = message.name;
}
if (message.age !== 0) {
obj.age = Math.round(message.age);
}
return obj;
},

Expand Down
8 changes: 6 additions & 2 deletions integration/barrel-imports/foo.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,12 @@ export const Foo = {

toJSON(message: Foo): unknown {
const obj: any = {};
message.name !== undefined && (obj.name = message.name);
message.bar !== undefined && (obj.bar = message.bar ? Bar.toJSON(message.bar) : undefined);
if (message.name !== "") {
obj.name = message.name;
}
if (message.bar !== undefined) {
obj.bar = Bar.toJSON(message.bar);
}
return obj;
},

Expand Down
57 changes: 34 additions & 23 deletions integration/batching-with-context-esModuleInterop/batching.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,8 @@ export const BatchQueryRequest = {

toJSON(message: BatchQueryRequest): unknown {
const obj: any = {};
if (message.ids) {
obj.ids = message.ids.map((e) => e);
} else {
obj.ids = [];
if (message.ids?.length) {
obj.ids = message.ids;
}
return obj;
},
Expand Down Expand Up @@ -147,10 +145,8 @@ export const BatchQueryResponse = {

toJSON(message: BatchQueryResponse): unknown {
const obj: any = {};
if (message.entities) {
obj.entities = message.entities.map((e) => e ? Entity.toJSON(e) : undefined);
} else {
obj.entities = [];
if (message.entities?.length) {
obj.entities = message.entities.map((e) => Entity.toJSON(e));
}
return obj;
},
Expand Down Expand Up @@ -207,10 +203,8 @@ export const BatchMapQueryRequest = {

toJSON(message: BatchMapQueryRequest): unknown {
const obj: any = {};
if (message.ids) {
obj.ids = message.ids.map((e) => e);
} else {
obj.ids = [];
if (message.ids?.length) {
obj.ids = message.ids;
}
return obj;
},
Expand Down Expand Up @@ -277,11 +271,14 @@ export const BatchMapQueryResponse = {

toJSON(message: BatchMapQueryResponse): unknown {
const obj: any = {};
obj.entities = {};
if (message.entities) {
Object.entries(message.entities).forEach(([k, v]) => {
obj.entities[k] = Entity.toJSON(v);
});
const entries = Object.entries(message.entities);
if (entries.length > 0) {
obj.entities = {};
entries.forEach(([k, v]) => {
obj.entities[k] = Entity.toJSON(v);
});
}
}
return obj;
},
Expand Down Expand Up @@ -356,8 +353,12 @@ export const BatchMapQueryResponse_EntitiesEntry = {

toJSON(message: BatchMapQueryResponse_EntitiesEntry): unknown {
const obj: any = {};
message.key !== undefined && (obj.key = message.key);
message.value !== undefined && (obj.value = message.value ? Entity.toJSON(message.value) : undefined);
if (message.key !== "") {
obj.key = message.key;
}
if (message.value !== undefined) {
obj.value = Entity.toJSON(message.value);
}
return obj;
},

Expand Down Expand Up @@ -420,7 +421,9 @@ export const GetOnlyMethodRequest = {

toJSON(message: GetOnlyMethodRequest): unknown {
const obj: any = {};
message.id !== undefined && (obj.id = message.id);
if (message.id !== "") {
obj.id = message.id;
}
return obj;
},

Expand Down Expand Up @@ -476,7 +479,9 @@ export const GetOnlyMethodResponse = {

toJSON(message: GetOnlyMethodResponse): unknown {
const obj: any = {};
message.entity !== undefined && (obj.entity = message.entity ? Entity.toJSON(message.entity) : undefined);
if (message.entity !== undefined) {
obj.entity = Entity.toJSON(message.entity);
}
return obj;
},

Expand Down Expand Up @@ -534,7 +539,9 @@ export const WriteMethodRequest = {

toJSON(message: WriteMethodRequest): unknown {
const obj: any = {};
message.id !== undefined && (obj.id = message.id);
if (message.id !== "") {
obj.id = message.id;
}
return obj;
},

Expand Down Expand Up @@ -644,8 +651,12 @@ export const Entity = {

toJSON(message: Entity): unknown {
const obj: any = {};
message.id !== undefined && (obj.id = message.id);
message.name !== undefined && (obj.name = message.name);
if (message.id !== "") {
obj.id = message.id;
}
if (message.name !== "") {
obj.name = message.name;
}
return obj;
},

Expand Down
Loading