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

Update debugJsonValue to accept unknown #535

Merged
merged 4 commits into from
Aug 1, 2023
Merged

Conversation

GauBen
Copy link
Contributor

@GauBen GauBen commented Jul 26, 2023

Hey! We started migrating a large code base to protobuf, we'll probably be uncovering a few bugs on the way.

This merge request addresses an issue regarding debugJsonValue:

TypeError: Cannot read properties of undefined (reading 'toString')
     at Object.debugJsonValue [as debug]
     ...

@CLAassistant
Copy link

CLAassistant commented Jul 26, 2023

CLA assistant check
All committers have signed the CLA.

@timostamm
Copy link
Member

If this happened during parsing of a valid JSON value, it is a bug.

Note that fromJson only parses from a JSON value (anything returned by JSON.parse). If you coerce the type system and pass undefined or an object containing undefined, you run into undefined behavior. For example:

import { Struct } from "@bufbuild/protobuf";
Struct.fromJson(undefined); 

The compiler should error with TS2345: Argument of type 'undefined' is not assignable to parameter of type 'JsonValue' on line 2. If you run this without compiling, you will see the error you mentioned.

Can you provide details on how this error occurred?

@GauBen
Copy link
Contributor Author

GauBen commented Jul 27, 2023

It was indeed not a valid JSON, but called with undefined as first parameter: Message.fromJson(somethingThatCouldReturnUndefined()).

@timostamm
Copy link
Member

It was indeed not a valid JSON, but called with undefined as first parameter: Message.fromJson(somethingThatCouldReturnUndefined()).

That should be caught by the TypeScript compiler:

import { Duration, JsonValue } from "@bufbuild/protobuf";
function somethingThatCouldReturnUndefined(): JsonValue | undefined {
  return undefined;
}
Duration.fromJson(somethingThatCouldReturnUndefined());
// TS2345: Argument of type 'JsonValue | undefined' is not assignable to
// parameter of type 'JsonValue'. Type 'undefined' is not assignable to type
// 'JsonValue'.

We could change the signature:

- fromJson(jsonValue: JsonValue, options?: Partial<JsonReadOptions>)
+ fromJson(jsonValue: JsonValue | undefined, options?: Partial<JsonReadOptions>)

But it would be very unusual for a function to accept undefined as an argument only to reject it by raising an error. Depending on context, maybe it could help to JSON.stringify() the return value of somethingThatCouldReturnUndefined() and use Message.fromJsonString()? This effectively passes the value through a JSON roundtrip, guaranteeing that the value is valid JSON.

@GauBen
Copy link
Contributor Author

GauBen commented Jul 28, 2023

That should be caught by the TypeScript compiler

It wasn't because one of the branch was returning any (like JSON.parse does), making JsonValue noneffective

I'd advocate for fromJson to keep its actual signature and making debugJsonValue more resilient by accepting unknown as argument instead of JsonValue and performing additional runtime checks

@timostamm
Copy link
Member

making debugJsonValue more resilient by accepting unknown as argument

👍 that is reasonable.

Changing the default case to return String(json) instead of json.toString should be a simple solution for everything that falls through. Would be happy to accept this improvement.

@GauBen
Copy link
Contributor Author

GauBen commented Jul 31, 2023

Updated with this improvement!

@timostamm timostamm changed the title fix: updated debugJsonValue to accept undefined Update debugJsonValue to accept unknown Aug 1, 2023
Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thank you 🙂

Pushed up a commit with updated bundle size benchmark (make bench).

@timostamm timostamm merged commit 7add029 into bufbuild:main Aug 1, 2023
2 checks passed
@GauBen
Copy link
Contributor Author

GauBen commented Aug 1, 2023

It made the package 3 bytes smaller, which is an absolute win

@srikrsna-buf srikrsna-buf mentioned this pull request Aug 30, 2023
srikrsna-buf added a commit that referenced this pull request Aug 30, 2023
## What's Changed
* #533 by @srikrsna-buf
* #535 by @GauBen 

## New Contributors
* @GauBen made their first contribution in #535.

**Full Changelog**:
https://github.com/bufbuild/protobuf-es/compare/v1.3.0..v1.3.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants