field: fix missing optional support #95

snqk · 2021-05-09T06:17:22Z

Fixes #94 & #75.

Adds support for optional field presence in the proto3 syntax.

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk · 2021-05-09T06:26:23Z

Reviewable! @dkunitsk @akonradi

\cc @glerchundi

glerchundi · 2021-05-09T06:46:35Z

Good job @sarthak40 LGTM

snqk · 2021-05-10T03:52:52Z

Hm, I attempted integrating this with protoc-gen-validate but it looks like we'll need additional changes to make this work.. more specifically in Message & AST related stuff.

Currently the generated code is just rendering all optional fields as synthetic one-of fields, as defined in the field presence doc. I am not sure what the best way to integrate this is yet so a short circuit would be an acceptable solution for now.

I'll mark this as a draft for now, till I can make the required changes. Since this is a generation framework, I don't have to worry about updating any templates thank fully (yet)..

dkunitsk · 2021-05-10T05:52:05Z

Hey @sarthak40. Yes, thank you for pointing this out. The Implementing Proto3 Presence doc gives great advice for what API changes are needed.

Of particular significance is Google's suggestion (per my reading) that, because Synthetic Oneofs are still oneofs, helpers like IsInOneof() etc should act as though the oneof is real. They propose to add additional helpers, e.g. IsInRealOneof() etc, to differentiate real and synthetic - I assume by reading this option.

This means in PGV we'd have to switch to these new Real methods. Since PGV is on top of regular codegen, treating synthetic oneofs as real is impossible since synthetic oneofs won't generate case statements.

I'm weighing this suggestion with the more obvious and backward-compatible approach of just making the existing IsInOneof() etc methods return true only for real oneofs. I'm currently leaning towards the latter.

snqk · 2021-05-10T06:44:23Z

@dkunitsk are you certain that we need to expose synthetic oneofs?

According to the doc:

Avoid generating any oneof-based accessors for the synthetic oneof. Its only purpose is to make reflection-based algorithms work properly if they are not aware of proto3 presence. The synthetic oneof should not appear anywhere in the generated API.

snqk · 2021-05-10T07:00:49Z

Given the above, I think the ast parser can be updated to account for detecting these synthetic oneofs. My proposal:

Update the grapher to detect based on the following conditions if a oneof is synthetic or not.
1. oneof.Syntax() == Proto3
2. len(oneof.Fields()) == 1
3. oneof.Fields()[0].Proto3Optional() == true
If (1) is true, then we proceed by extracting the proto3 oneof field, graphing it & discarding the synthetic oneof.
Otherwise its considered to be an actual oneof field, and we proceed as planned.

Since field presence in proto3 is now the property of the individual fields, it actually makes this particular change quite transparent to the end users. Nothing existing will break, and they will have to explicitly add support for proto3 oneof by checking for field.Proto3Optional().

WDYT?

dkunitsk · 2021-05-10T07:27:22Z

Re the quote: I believe that's more in the same vein as the default codegen not outputting case statements for synthetic oneofs. What's more relevant to PGS is the Updating Reflection guidance.

I think I've had some understanding here, and it starts with #85 being the wrong thing to do. PGS cannot support Optional, only the end user of PGS (the plugin author) can do that. As such, the right approach is to allow the user to set this on their end once they are ready to differentiate synthetic and real oneofs. This could easily be done with very minor modification to Generator (a new SupportedFields field) and letting it be set by way of InitOption:

protoc-gen-star/generator.go

Line 29 in 8bc36ed

func Init(opts ...InitOption) *Generator {

Now that the onus of Optional support is on the plugin author, the question above IMO is more clear: if the user has to indicate that they support Optional, they also indicate that they know about synthetic oneofs and have updated their codegen appropriately. This means the PGS AST can stay "correct" (i.e. actually reflecting the descriptor). We can/should still provide additional helpers though, as suggested in the doc.

This treatment also inherits the positives of the synthetic oneof approach,

Since oneof fields in proto3 already track presence, existing proto3 reflection-based algorithms should correctly preserve presence for proto3 optional fields with no code changes. For example, the JSON and TextFormat parsers/serializers in C++ and Java did not require any changes to support proto3 presence. This is the major benefit of synthetic oneofs.

snqk · 2021-05-10T08:24:32Z

@dkunitsk thanks for looking into this!

I really like the idea for the users to be able to define (through an InitOption) if their plugin supports proto3 optional or not. That shouldn't be hard to implement.

I'll work on the updated helpers later today as well and we can get this moving.

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk · 2021-05-11T15:30:41Z

\cc @dkunitsk @glerchundi @akonradi

Round 2 👍

snqk · 2021-05-13T06:37:06Z

@dkunitsk polite ping, have you had a chance to review this yet?

snqk · 2021-05-15T23:47:38Z

I now have a version of PGV rendering with these changes locally!

@dkunitsk @akonradi if one of you could get this reviewed we can start on resolving bufbuild/protoc-gen-validate#431 👍

akonradi · 2021-05-17T14:58:44Z

I'm not familiar enough with how PGS abstracts over proto2&3 to have opinions on the API (mostly regarding OneOf vs RealOneOf vs SyntheticOneOf) nor am I a maintainer on this project (just PGV). LGTM and looking forward to getting to use this functionality in PGV though.

dkunitsk · 2021-05-18T05:11:28Z

Thanks for these changes. Will review in the morning.

snqk · 2021-05-20T12:17:56Z

@dkunitsk did you get a chance to review?

dkunitsk

My apologies for being late in reviewing this. This seems to be very nearly there. Please take a look at my feedback and let me know what you think. Many of these comments I'm happy to discuss further.

dkunitsk · 2021-05-24T22:33:00Z

field.go

@@ -56,11 +69,31 @@ func (f *field) SourceCodeInfo() SourceCodeInfo               { return f.info }
 func (f *field) Descriptor() *descriptor.FieldDescriptorProto { return f.desc }
 func (f *field) Message() Message                             { return f.msg }
 func (f *field) InOneOf() bool                                { return f.oneof != nil }
+func (f *field) InRealOneOf() bool                            { return f.InOneOf() && !f.desc.GetProto3Optional() }


super nit: move this method below. This top section has the "really obvious" method implementations, and the methods below are the more interesting ones. This one's more interesting IMO.

field.go

dkunitsk · 2021-05-24T23:02:34Z

init_option.go

@@ -47,3 +48,12 @@ func FileSystem(fs afero.Fs) InitOption { return func(g *Generator) { g.persiste
 func BiDirectional() InitOption {
 	return func(g *Generator) { g.workflow = &onceWorkflow{workflow: &standardWorkflow{BiDi: true}} }
 }
+
+// SupportProto3Optional adds support for proto3 field presence.
+// See: https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md#signaling-that-your-code-generator-supports-proto3-optional


point to a commit or version instead of master, as that can change

dkunitsk · 2021-05-24T23:11:33Z

proto.go

+	// Proto3 syntax permits the use of "optional" field presence. Non optional fields default to the zero
+	// 	value of that particular type if not defined.
+	// Most of the field types in the generated go structs are value types.
+	// See: https://github.com/protocolbuffers/protobuf/blob/master/docs/field_presence.md#presence-in-proto3-apis


Same nit about master -> version url

dkunitsk · 2021-05-24T23:27:55Z

protoc-gen-debug/main.go

-	data, err = proto.Marshal(&plugin_go.CodeGeneratorResponse{})
-	if err != nil {
+	var supportedFeatures = uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)
+	if data, err = proto.Marshal(&plugin_go.CodeGeneratorResponse{


IIUC this again makes PGS by default claim to support CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL. My suggestion was to create a new InitOption type (in init_option.go), which will set the generator's persister's supportedFeatures variable. This gives full control to the user.

see https://github.com/lyft/protoc-gen-star/blob/master/init_option.go#L43

I've changed the behaviour in init_option.go, though i've left it as-is in protoc-gen-debug as the latter is a dev tool.

Makes sense to me - but perhaps leave a comment to that effect.

dkunitsk · 2021-05-24T23:31:56Z

field.go

+	return true
+}
+
+func (f *field) Optional() bool {


To be extra clear and follow the guidelines more precisely, perhaps we should call this HasOptionalKeyword()

ash2k · 2021-06-09T06:21:16Z

Would be awesome to get this merged to help resolve bufbuild/protoc-gen-validate#431

snqk · 2021-06-11T20:25:49Z

@dkunitsk thanks for the review!

I haven't been able to find the time to work on this, but i'll see if I can muster up something this week.

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk · 2021-06-16T14:35:20Z

@dkunitsk ICYMI, this is ready for review 👍

nikoncode · 2021-07-15T08:04:40Z

Guys, may I ask status of this MR?

We are building a new transport stack in our company and want to combine both validation and presence tracking support in our proto definitions. And this seems to block validation support and all our concepts as well.

cc: @xmlking, @glerchundi, @dkunitsk

snqk · 2021-07-17T15:37:17Z

@dkunitsk if you could please take another look at this, i'd appreciate it.

dkunitsk · 2021-07-21T00:15:39Z

.travis.yml

@@ -4,8 +4,7 @@ go_import_path: github.com/lyft/protoc-gen-star

 env:
  matrix:
-    - PROTOC_VER="3.5.1"


could we please keep the other ones and just add on 3.16.0

You may need to use https://mickey.dev/posts/go-build-tags-testing/ (for example) to mark which tests apply to the older protoc versions versus newer. But I think I really don't want to lose test coverage for the old versions, esp since it should be relatively easy to keep them. WDYT.

Hmm, this is turning out to be tricker than I anticipated.

I need to first edit the makefiles to ensure they don't compile the proto3 optional bits, then ensure I add the build tags for the golang tests themselves..

I am testing by isolating the testing harness into testdata/proto3_presence for now, but let me know if you prefer a different name.

dkunitsk · 2021-07-21T01:38:48Z

field_test.go

+	f.addType(&mapT{repT: &repT{scalarT: &scalarT{}}})
+	assert.False(t, f.HasPresence())
+
+	f.msg = dummyMsg()


IIUC this shouldn't be necessary according to

protoc-gen-star/field_test.go

Line 192 in 8bc36ed

m.addField(f)

. I.e. it makes a dummy message and adds the field to it, which has the same effect as setting the message of a dummy field.

field_test.go

dkunitsk · 2021-07-21T02:00:10Z

field_test.go

+	assert.True(t, f.HasPresence())
+}
+
+func TestField_Optional(t *testing.T) {


nit: TestField_HasOptionalKeyword

field_test.go

dkunitsk · 2021-07-21T02:25:27Z

field_type.go

@@ -84,7 +83,7 @@ func (s *scalarT) Element() FieldTypeElem { return nil }
 func (s *scalarT) Key() FieldTypeElem     { return nil }

 func (s *scalarT) IsOptional() bool {
-	return !s.fld.Syntax().SupportsRequiredPrefix() || s.ProtoLabel() == Optional
+	return s.fld.HasOptionalKeyword()


This would make all proto3 fields without presence suddenly become IsOptional() == false which is very backwards incompatible.

Proto3 optional label is not the same as proto2. The proto2 label means "this field is not required" whereas the proto3 label means "this field is still not required, but it has explicit presence semantics".

This IsOptional in particular is referring to the field not being required, which is not affected by the explicit presence semantics. So IIUC this method does not need to change.

dkunitsk · 2021-07-21T02:40:41Z

lang/go/type_name_test.go

@@ -88,19 +88,40 @@ func TestType(t *testing.T) {
 		{"Proto3.string", "string"},
 		{"Proto3.bytes", "[]byte"},
 		{"Proto3.enum", "Proto3_Enum"},
-		{"Proto3.ext_enum", "ptype.Syntax"},
+		{"Proto3.ext_enum", "typepb.Syntax"},


hmm why did this work before...

i'm not actually sure on this one, a bit perplexed myself

oneof.go

dkunitsk · 2021-07-21T02:51:01Z

proto.go

-	// value of that particular type. Most of the field types in the generated go
-	// structs are value types.
-	// See: https://developers.google.com/protocol-buffers/docs/proto3
+	// Proto3 syntax permits the use of "optional" field presence. Non optional fields default to the zero


I think this explanation slightly confuses optionality as the opposite of requiredness versus optionality as having explicit presence. Even with explicit presence it is true that Proto3 syntax only allows for optional fields.

I would recommend rewording to something like
Proto3 syntax only allows for optional fields, and it defaults to the zero value of that particular type (unless experimental support for explicit presence tracking for singular proto3 fields is used, see <link>).

(unless experimental support for explicit presence..

According to protobuf v3.15.0 field presence support in proto3 is no longer experimental.

dkunitsk · 2021-07-21T02:52:05Z

protoc-gen-debug/main.go

-	data, err = proto.Marshal(&plugin_go.CodeGeneratorResponse{})
-	if err != nil {
+	var supportedFeatures = uint64(pluginpb.CodeGeneratorResponse_FEATURE_PROTO3_OPTIONAL)
+	if data, err = proto.Marshal(&plugin_go.CodeGeneratorResponse{


Makes sense to me - but perhaps leave a comment to that effect.

dkunitsk · 2021-07-21T02:58:05Z

.travis.yml

@@ -4,8 +4,7 @@ go_import_path: github.com/lyft/protoc-gen-star

 env:
  matrix:
-    - PROTOC_VER="3.5.1"


You may need to use https://mickey.dev/posts/go-build-tags-testing/ (for example) to mark which tests apply to the older protoc versions versus newer. But I think I really don't want to lose test coverage for the old versions, esp since it should be relatively easy to keep them. WDYT.

dkunitsk · 2021-07-21T03:01:34Z

@sarthak40 huge thank you for the diff. I left some last comments. I am confident that after one more iteration it will be fully there.

I know most of the delay has been on my end, but I can promise that once you address the above, I will re-review and merge right away.

snqk · 2021-07-27T08:14:00Z

@dkunitsk thanks for the review!

Let me take a crack at your comments over the weekend and get the ball rolling!

- revert travis coverage for older protoc versions - revert backwards incompatible change for isOptional - nit/style fixes Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

This change splits the testing harness for proto3 field presence, so we can retain functional test coverage for older protoc versions that do not support proto3 field presence. IE >v3.15.0 Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk · 2021-08-07T20:39:26Z

@dkunitsk addressed most of your comments, ready for round 4 👍

glerchundi · 2021-08-14T07:04:07Z

This is really close, great job @sarthak40!! Ultra friendly ping ICYMI @dkunitsk, and hoping that it's not gonna take too much of your time 🤞

snqk · 2021-08-21T17:58:15Z

@dkunitsk friendly ping please

dkunitsk · 2021-08-23T06:28:00Z

@sarthak40 brilliant work. Thank you for pushing this through. It is available here: https://github.com/lyft/protoc-gen-star/releases/tag/v0.6.0.

One ask: do you think you can make a README change to tell folks how to use this and just to advertise the existence of support for field presence in PGS?

snqk · 2021-08-23T16:08:59Z

Fantastic! Likewise, thanks a lot for the reviews, we got it there in the end 😎.

I will be looking at PGV code over the weekend to get that moving now, but can also submit a README update for PGS at the same time.

snqk added 4 commits May 9, 2021 07:06

protoc-gen-debug: add optional support

32ee6de

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

field: fix missing optional support, update protoc

7424ee8

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

proto: updated notes for proto3 field presence

5232966

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

travis: revert golang version

9d6de96

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

glerchundi previously approved these changes May 9, 2021

View reviewed changes

snqk marked this pull request as draft May 10, 2021 03:53

snqk added 2 commits May 11, 2021 03:57

generator: add proto3 optional init option

69f0811

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

pgs: add support for proto3 field presence

552c18e

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk dismissed glerchundi’s stale review via 552c18e May 11, 2021 15:15

pgs: add support for proto3 field presence

e0bab48

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk marked this pull request as ready for review May 11, 2021 15:27

dkunitsk reviewed May 24, 2021

View reviewed changes

xmlking previously approved these changes Jun 11, 2021

View reviewed changes

pgs (field presence): nit/style fixes

2d3f0a8

Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk dismissed xmlking’s stale review via 2d3f0a8 June 13, 2021 18:50

snqk requested a review from dkunitsk July 3, 2021 00:00

dkunitsk reviewed Jul 21, 2021

View reviewed changes

snqk added 2 commits August 7, 2021 19:19

pgs (field presence): nit/fixes

7af882f

- revert travis coverage for older protoc versions - revert backwards incompatible change for isOptional - nit/style fixes Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

pgs (field presence): split proto3 tests

3eb9739

This change splits the testing harness for proto3 field presence, so we can retain functional test coverage for older protoc versions that do not support proto3 field presence. IE >v3.15.0 Signed-off-by: Sarthak Gupta <signed@sarthak.sh>

snqk requested a review from dkunitsk August 8, 2021 19:24

dkunitsk approved these changes Aug 23, 2021

View reviewed changes

dkunitsk merged commit d186061 into lyft:master Aug 23, 2021

snqk deleted the feature/fix-optional-support branch August 23, 2021 16:11

snqk restored the feature/fix-optional-support branch August 23, 2021 16:11

snqk deleted the feature/fix-optional-support branch May 4, 2023 07:48

field: fix missing optional support #95

field: fix missing optional support #95

Conversation

snqk commented May 9, 2021

snqk commented May 9, 2021

glerchundi commented May 9, 2021

snqk commented May 10, 2021

dkunitsk commented May 10, 2021

snqk commented May 10, 2021

snqk commented May 10, 2021

dkunitsk commented May 10, 2021

snqk commented May 10, 2021

snqk commented May 11, 2021

snqk commented May 13, 2021

snqk commented May 15, 2021

akonradi commented May 17, 2021

dkunitsk commented May 18, 2021

snqk commented May 20, 2021

dkunitsk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash2k commented Jun 9, 2021

snqk commented Jun 11, 2021

snqk commented Jun 16, 2021

nikoncode commented Jul 15, 2021 • edited Loading

snqk commented Jul 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snqk Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkunitsk commented Jul 21, 2021

snqk commented Jul 27, 2021

snqk commented Aug 7, 2021 • edited Loading

glerchundi commented Aug 14, 2021

snqk commented Aug 21, 2021

dkunitsk commented Aug 23, 2021

snqk commented Aug 23, 2021

nikoncode commented Jul 15, 2021 •

edited

Loading

snqk Aug 7, 2021 •

edited

Loading

snqk commented Aug 7, 2021 •

edited

Loading