Fix failing FieldMask.Merge for well-known wrapper field types #9602

vassilvk · 2022-03-08T15:14:03Z

Closes #9283.

google-cla · 2022-03-08T15:14:07Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

acozzette · 2022-03-08T16:22:29Z

@vassilvk Thank you for the fix! We need the CLA before we can review this, but it sounds like you're already working on sorting that out. Let me know if you need any help dealing with the CLA.

vassilvk · 2022-03-08T18:30:14Z

Thank you @acozzette.

We are trying to get through the process of establishing a corporate contributor CLA for my employer (and it is taking some time).
What is the best way to reach you in case we have questions?

acozzette · 2022-03-08T18:32:31Z

You can either ask here or feel free to email me at [email protected].

vassilvk · 2022-04-09T20:45:02Z

@acozzette, FYI, the CLA is signed.

jskeet · 2022-04-12T07:03:28Z

Do we know whether Java has the same issue? It looks like it, but I don't know if getJavaType() behaves differently for wrapper types.

jskeet · 2022-04-12T07:05:22Z

Ah, I suspect that in Java it's okay because the reflection value for a wrapper field is still the wrapper, whereas in .NET it's the boxed primitive.

However, this leads to an issue in this fix - because it means that the for wrapper fields, options.ReplacePrimitiveFields is used instead of options.ReplaceMessageFields - when fundamentally it's still a message field. I'd like to see the options behave consistently.

vassilvk · 2022-04-12T14:35:07Z

Hi @jskeet,

Please see cef9f67.

Please note that the well known wrappers are semantically primitive values in a sense that the Replace* options would have no logical bearing on the outcome of them being merged. In other words, for those fields "merge" is equivalent to "replace".

That said, the above commit moves the check into the message branch of the merge operation as per your suggestion.

jskeet · 2022-04-12T14:50:20Z

I'll have a look at this tomorrow - I'm afraid I won't be able to get to it today.

jskeet · 2022-04-13T07:34:15Z

Okay, I think I understand now (this whole piece of code is new to me). I think the new behavior is reasonable, but I can imagine it possibly behaving differently to the equivalent Java code. I suspect that in the Java code, the wrapper types are treated like any other message types - so a wrapper type with a value of 0 won't actually end up replacing the original value, because it will be merged field-wise, with the absent value field being ignored.

Given the way reflection for wrapper types works in C#, I suspect we don't want to replicate that behavior, but I'd like @acozzette's opinion before merging.

vassilvk · 2022-05-05T21:20:28Z

Hello @acozzette. Any update?
Thanks.

acozzette · 2022-05-05T23:55:12Z

Sorry I missed this! I think Jon brings up an interesting point about how in Java, a zero-valued wrapper type can't override the original value. I glanced at the C++ implementation and I suspect this is true in C++ as well, and probably in just about every language, since as far as I know we didn't take any special care to handle this situation.

So unfortunately I think we probably want to imitate the Java behavior--even though it's less intuitive, I think it's best to aim for consistency across languages. Hopefully this is an easy tweak to make: I think we just want to ignore the source field in the special case where a wrapper field is present in both the source and destination but the contents of the source field are empty.

vassilvk · 2022-05-07T20:04:36Z

Thanks @acozzette.
Suggested changes are complete.

jskeet · 2022-05-09T06:09:42Z

I'll have a look later in the week, thanks.

jskeet

Production code looks good to me, although it's possible that I've missed some subtlety. I think it would be good to separate out a "wrapper field with null value" test (with both ReplaceMessageFields true and false) and "wrapper field with non-null value" (where we probably don't need parameterization).

csharp/src/Google.Protobuf.Test/FieldMaskTreeTest.cs

jskeet · 2022-05-11T11:07:15Z

csharp/src/Google.Protobuf.Test/FieldMaskTreeTest.cs

+                },
+                false);
+
+            // Make sure only the targeted fields were updated.


I would expect this to look at Int32Field and BoolField... given that StringField is targeted, I wouldn't expect it to be asserted in this section. I suspect we effectively want two commented sections:

Make sure only the targeted fields were updated (Int32Field and BoolField haven't changed)

"Prove that ReplaceMessageFields works for wrapper types" - I'd expect the same test for both StringField and Int64Field here (see earlier comment about mixing null and non-null behavior in one test).

Please see comment above and c4e7df1

csharp/src/Google.Protobuf/FieldMaskTree.cs

jskeet · 2022-05-11T11:13:21Z

csharp/src/Google.Protobuf/FieldMaskTree.cs

                                }
                                else
                                {
-                                    field.Accessor.SetValue(destination, field.MessageType.Parser.ParseFrom(sourceByteString));
+                                    var sourceByteString = ((IMessage)sourceField).ToByteString();


Note for anyone else reviewing this: the code here really hasn't changed at all other than indentation - the diff is just confusing. (It's slightly better in split mode than in unified mode, but either way it's not clear that really we're just conditionalizing the existing behavior.)

jskeet

I'll merge this, as I believe the production code is correct - but the test really is quite hard to understand at the moment, from the perspective of someone who has come into this "cold" as it were.

jskeet · 2022-05-16T07:59:17Z

Ah, the CLA check is still failing - I'll need to check that manually.

jskeet · 2022-05-16T08:01:45Z

Hmm - if you could email me ([email protected]) with details about the CLA signing (e.g. whether it's been done in a corporate way rather than for your GitHub account) that would be appreciated.

vassilvk · 2022-05-16T13:04:37Z

I believe the production code is correct - but the test really is quite hard to understand at the moment, from the perspective of someone who has come into this "cold" as it were.

Hi @jskeet - I understand.
Please see commit 9baab1f with split tests.

I'll take a look at the CLA.

vassilvk · 2022-05-16T13:09:57Z

I see what the problem is with the CLA.
I believe it trips on your @pobox.com email identity:

jskeet · 2022-05-16T13:11:32Z

Ah, right - thanks. I'll manually override that when I've reviewed the new tests.

jskeet · 2022-05-16T13:16:47Z

Okay, the CLA manual override process has changed over time.

I suspect the simplest approach now is for you to:

Soft-reset to 92cdf87 (the parent of the first commit)
Commit all the changes in a single commit (we don't need all the history)
Force push

That should end up with a single commit just by you, which should avoid all the CLA problems. Sorry for the bureaucracy :(

vassilvk · 2022-05-16T13:42:31Z

Hi @jskeet, no problem.
Done.

jskeet · 2022-05-16T14:12:28Z

Thanks for your patience :)

vassilvk · 2022-05-16T14:41:40Z

Thanks for your patience :)

Thank you for yours too :)

olivierl · 2022-06-22T14:14:42Z

Hi, do you know when this fix will be released? Thx

jskeet · 2022-06-22T14:19:02Z

@olivierl: I don't think it made it into 3.21.x, so I'm afraid you'll need to wait for 3.22.x. I don't have a timeline for that.

fowles · 2022-06-22T14:27:13Z

Not committing to anything, but I anticipate 22.x will happen in 2 or so months.

RogerioWagner · 2024-06-20T14:10:19Z

Hi, the issue of FieldMask.Merge with Wrappers has been fixed, or is there any workaround to create path's for wrappers? I just experienced same behavior reported here: #9283 (comment)

jskeet · 2024-06-20T14:25:02Z

@RogerioWagner: I've just tried the exact repro in #9283, and it's fine using the latest NuGet package. So either you're not actually experiencing the same behavior (i.e. your situation is different in a way we can't know) or you're using an old version where this was still broken. I suggest you file a new issue with a minimal repro.

RogerioWagner · 2024-06-20T14:44:42Z

@jskeet, you were right, it's a different behavior after I bumped the version to 3.26.0, now instead of an exception, it failed silently with a warning at MaskFilter ({ "@warning": "Invalid FieldMask", "paths": [ "description", "usage", "templateId" ] }), where description is an optional string, usage is an enum and templateId is an optional google.protobuf.Int32Value.

jskeet · 2024-06-20T14:46:02Z

@RogerioWagner: Rather than half-describing the problem in a merged PR, please file a new issue with a minimal but complete repro.

acozzette added c# release notes: yes labels Mar 8, 2022

acozzette added the kokoro:run label Apr 11, 2022

acozzette requested a review from jskeet April 11, 2022 15:59

protobuf-kokoro removed the kokoro:run label Apr 11, 2022

jskeet reviewed May 11, 2022

View reviewed changes

jskeet approved these changes May 16, 2022

View reviewed changes

jskeet added the cla: yes label May 16, 2022

vassilvk force-pushed the 9283-fieldmask-merge-for-wellknown-wrappers-2 branch from 9baab1f to 391e4ed Compare May 16, 2022 13:37

google-cla bot added cla: no and removed cla: yes labels May 16, 2022

Fix failing FieldMask.Merge for well-known wrapper field types

1aed573

vassilvk force-pushed the 9283-fieldmask-merge-for-wellknown-wrappers-2 branch from 391e4ed to 1aed573 Compare May 16, 2022 13:41

google-cla bot added cla: yes and removed cla: no labels May 16, 2022

jskeet merged commit 9d7a449 into protocolbuffers:main May 16, 2022

vassilvk deleted the 9283-fieldmask-merge-for-wellknown-wrappers-2 branch May 16, 2022 14:44

Fix failing FieldMask.Merge for well-known wrapper field types #9602

Fix failing FieldMask.Merge for well-known wrapper field types #9602

Conversation

vassilvk commented Mar 8, 2022

google-cla bot commented Mar 8, 2022

acozzette commented Mar 8, 2022

vassilvk commented Mar 8, 2022

acozzette commented Mar 8, 2022

vassilvk commented Apr 9, 2022

jskeet commented Apr 12, 2022

jskeet commented Apr 12, 2022

vassilvk commented Apr 12, 2022

jskeet commented Apr 12, 2022

jskeet commented Apr 13, 2022

vassilvk commented May 5, 2022

acozzette commented May 5, 2022

vassilvk commented May 7, 2022

jskeet commented May 9, 2022

jskeet left a comment

Choose a reason for hiding this comment

jskeet May 11, 2022

Choose a reason for hiding this comment

vassilvk May 14, 2022

Choose a reason for hiding this comment

jskeet May 11, 2022

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

jskeet commented May 16, 2022

jskeet commented May 16, 2022

vassilvk commented May 16, 2022 • edited Loading

vassilvk commented May 16, 2022

jskeet commented May 16, 2022

jskeet commented May 16, 2022

vassilvk commented May 16, 2022

jskeet commented May 16, 2022

vassilvk commented May 16, 2022 • edited Loading

olivierl commented Jun 22, 2022

jskeet commented Jun 22, 2022

fowles commented Jun 22, 2022

RogerioWagner commented Jun 20, 2024

jskeet commented Jun 20, 2024

RogerioWagner commented Jun 20, 2024 • edited Loading

jskeet commented Jun 20, 2024

vassilvk commented May 16, 2022 •

edited

Loading

vassilvk commented May 16, 2022 •

edited

Loading

RogerioWagner commented Jun 20, 2024 •

edited

Loading