Repeated fields can be undefined

[dot-i]

It appears that a repeated field like this one:

   repeated Document documents = 2;

Simply gives this as output:

   documents: Document[];

But, if you use the standard NodeJS protobufjs serialization without adding defaults: true a deserialized message won't actually contain a documents field.

So it seems more appropriate to output:

   documents: Document[] | undefined;

Or even:

   documents?: Document[];

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 the enums: String override on the protobufjs (de)serializer).

I could help on a PR for this, but first wanted to check what the preferred solution could be.

[stephenh]

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.

[dot-i]

I liked your last suggestion and went for the baby step PR already: #227

[stephenh]

Should be released as v1.70.0, thanks!

[mharsat]

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:

1. useOptionals only changes field: Type | undefined to field?: Type.
2. A new non-breaking option is introduced for allowing the custom behavior (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;
}

[stephenh]

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.

[stephenh]

Okay, I pushed v1.72.0 that reverts the change, and we can work on a redo.

[dot-i]

Message received and understood, I'll pick it up... 🙂

[dot-i]

I implemented the forceOptionalRepeated=true flag, added two more tests and a short description to the readme.

#234

[medikoo]

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

[stephenh]

@medikoo have you tried useOptionals=all?

[medikoo]

@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

[stephenh]

@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. :-)

[medikoo]

@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

[stephenh]

@medikoo the default ts-proto output is focused on reading values and not writing them.

From the Default values section:

• For strings, the default value is the empty string.
• ...
• The default value for repeated fields is empty

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]

@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

[stephenh]

@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]

@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.

[JeHwanYoo]

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'?

[biljecki]

@JeHwanYoo any luck here? im also getting undefined for empty arrays in client, dont get what the point of that is