Closed dot-i closed 1 year ago
The proto3 spec is pretty clear that, when deserializing repeated fields, the initial value is always an empty list:
"The default value for repeated fields is empty (generally an empty list in the appropriate language)."
https://developers.google.com/protocol-buffers/docs/proto3
I get that doesn't match what you personally are looking for, but that's why it is that way today.
Right now the useOptionals
flag changes message keys from foo: SomeMessage | undefined
to foo?: SomeMessage
, so we could potentially extend that existing flag to let repeated field keys be optional. Note that this flag doesn't change any deserialization behavior, it just changes the types.
IIRC your use case is "take some JSON off the wire and just use it after JSON.parse
w/o Message.fromJSON
" ... that's fine, but that's not how proto3 JSON serialization is spec'd to work, which is why you're running into these problems. I'm fine with ts-proto supporting this use case, but it probably needs to be thought about wholistically and maybe even have its own dedicated flag.
...like... matchJsonOnTheWire=true
or some better than like that.
A baby step PR to make repeated fields optional with the existing useOptional
flag seems like a good baby step, if you'd like to tackle that.
I liked your last suggestion and went for the baby step PR already: https://github.com/stephenh/ts-proto/pull/227
Should be released as v1.70.0, thanks!
Hey @stephenh, this change breaks current usage and introduces an unexpected behavior that doesn't comply with the proto3 defaults guide
Repeated fields should always default to an empty array, not to undefined
. The introduced change both creates an unexpected behavior and misrepresents the type returned from the fromJson
methods.
The reason is that the fromJson
method initializes message.repeatedFieldName = [];
but the returned type is suddenly optional.
I suggest reverting this change and introducing a new change with the following behavior:
useOptionals
only changes field: Type | undefined
to field?: Type
.forceOptionalRepeated
?).Message Type1 {
repeated Type2 field1 = 1;
Type2 field2 = 2;
}
With useOptionals=false
and forceOptionalRepeated=false
export interface Type1 {
field1: Type2[];
field2: Type2 | undefined;
}
With useOptionals=false
and forceOptionalRepeated=true
export interface Type1 {
field1: Type2[] | undefined;
field2: Type2 | undefined;
}
With useOptionals=true
and forceOptionalRepeated=true
export interface Type1 {
field1?: Type2[];
field2?: Type2;
}
With useOptionals=true
and forceOptionalRepeated=false
export interface Type1 {
field1: Type2[];
field2?: Type2;
}
Hey @mharsat , thanks for the report! I was worried about this, per discussion above, i.e. you're right, this is "not proto3" behavior.
I was hoping that existing users of useOptional
would be fine with this, but I guess you're right that useOptionals
as it was before this PR was only a type-system change and not a decoding behavior change, which is what this PR turned it into...
I'll revert and push out a new version.
I like your description of forceOptionalRepeated
... I kinda wonder if something like dontApplyDefaults
or even matchJsonOffTheWire
would be more higher-level/use-case-driven names given that afaiu that is what @dot-i is looking to achieve.
Okay, I pushed v1.72.0 that reverts the change, and we can work on a redo.
Message received and understood, I'll pick it up... 🙂
I implemented the forceOptionalRepeated=true
flag, added two more tests and a short description to the readme.
The current way of handling repeated doesn't feel right.
Adding a new repeated field to the schema, shouldn't require updates to the application code, as per Protobuf spec repeated fields are optional (if not provided, they resolve to an empty list).
While with current ts-proto
implementation, adding a new repeated field makes a breaking change.
As all applications which depend on the schema and create complying objects will crash until we ensure that array values for introduced properties are added. That shouldn't be the case.
Ideally, there should be at least an option to relax that, so repeated fields are not implemented as required
@medikoo have you tried useOptionals=all
?
@stephenh yes, it relaxes this restriction, but it also relaxes restriction on primitive values which are not marked as optional. So it introduces another issue
@medikoo hm, it sounds like you want "repeated fields per spec are optional" (so adding new repeated fields doesn't require updating app code), but you do want "non-optional primitive fields are required" (so adding new primitive fields does require updating app code), even though "primitive fields per proto3 spec are also optional / have default values".
If I'm following, this seems inconsistent?
Fwiw I think this issue was originally file before we had useOptionals=all
, so I'm going to optimistically assume that is good enough, close it out for now.
Happy to re-open/file a follow up issue if there's a good articulation for a new useOptionals=<...>
flag, but in general optional-vs-required is generally a mess in proto3 and it ends up being very hard to please everyone. :-)
@stephenh I believe the correct understanding of spec is as follows:
message Test {
// required primitive property
string lorem = 1;
// optional primitive property
optional string ipsum = 2;
// optional repeated primitive property (note that spec does not support "optional repeated" notation)
repeated string other = 3
}
And while ts-proto
differenties between required and optional primitives (without any extra flags), it assumes repeated as required which I believe is not correct
@medikoo the default ts-proto output is focused on reading values and not writing them.
From the Default values section:
So if a reader does Test.decode(...)
, they cannot tell "did the writer have test.other = not set or did they have test.other = []". The spec says the reader should, for either of these cases, see other = []
as the default value.
Granted, a wrinkle of ts-proto's "just POJO interfaces" approach is that we use the same types for readers vs. writers, and you're focused on "when writing / creating"; for that we have the fromPartial
methods, which will let you leave out other
on create, and let fromPartial
apply the test.other = []
to match the spec-recommended defaults.
If you're unhappy with using fromPartial
, that is what the useOptionals=all
is for, to make the POJO types look more like "what writers care about" instead of "what readers care about".
@medikoo the default ts-proto output is focused on reading values and not writing them.
Yes, and the current situation is that it (1) ignores if optional primitive is not provided, (2) crashes if required primitive is not provided, and (3) (!) crashes if optional repeated primitive is not provided.
If for above schema, I'll do Test.encode({ lorem: "whatever" })
, it'll accept fact that ipsum
was not provided, yet it'll crash on missing other
. This is the problem I'm referring to
@medikoo do you mean "crashes" as in "throws a runtime error" or "crashes" as in it's a typescript compile error?
If you mean "typescript compile error", yes, that behavior is still expected because:
1) for readers, the wire protocol can different "ipsum
is set" vs. "ipsum
is not set"
2) for readers, the wire protocol cannot differentiate "other
is set" vs. "other
is not et"
I.e. you're confusing "what is the best API for readers" (what the Test
interface, by default, is optimized for) with "what is the best API for creating a new message".
I.e. ts-proto does not leave ipsum
out of Test.encode({ lorem: "whatever" })
because it's "optional on create", it is optional because "the reader can actually differentiate ipsum-is-set vs. ipsum-is-not-set". It's just a happy accident that, for the ipsum
field, the "best read api" and "best write api" happen to line up.
With the default ts-proto output, you should use Test.encode(Test.fromPartial({ lorem: "whatever" }))
. Or set useOptionals=all
.
@medikoo do you mean "crashes" as in "throws a runtime error"
I was referring to JavaScript execution of TS compiled to JS.
I do not see any reasonable explanation for being ok not to pass optional primitive, but not being ok to not pass optional repeated primitive.
What happened to this issue in the end?
Let me show you my case.
// from grpc server
@GrpcMethod('UsersService')
findAll(request: GrpcUser.UserQuery): Observable<GrpcUser.UserPage> {
return from(this.userEntity.find()).pipe(
switchMap(users => {
console.log('GrpcMethod', users) // users are '[]'
return of({
users,
})
}),
)
}
// from grpc client
findAll() {
return this.usersGrpcService.findAll({}).pipe(
switchMap(({ users }) => {
console.log('GraphQL', users) // users are 'undefined' !????
return of(users ?? [])
}),
)
}
// compiled ts file
export interface UserPage {
users: User[]; // The return value cannot be undefined.
}
# ts-proto option
TS_ARGS_SERVER=('lowerCaseServiceMethods=true'
'outputEncodeMethods=false'
'outputJsonMethods=false'
'outputClientImpl=false'
'snakeToCamel=true'
'returnObservable=true'
'useDate=true'
'env=node')
An empty array ('[]') is searched in the db, and an empty array ('[]') is sent, but why does the client receive 'undefined'?
@JeHwanYoo any luck here? im also getting undefined for empty arrays in client, dont get what the point of that is
It appears that a repeated field like this one:
Simply gives this as output:
But, if you use the standard NodeJS
protobufjs
serialization without addingdefaults: true
a deserialized message won't actually contain adocuments
field.So it seems more appropriate to output:
Or even:
Of course, it's always possible to add an option to turn this on or off using a setting similar to
--ts_proto_opt=stringEnums=true
(which BTW does nicely fit the theenums: String
override on theprotobufjs
(de)serializer).I could help on a PR for this, but first wanted to check what the preferred solution could be.