Set serialized_on_wire when message contains only lists

[FuegoFro]

This fixes a bug where serialized_on_wire was not set when a message contained only repeated values (eg in a list or map). The fix here is to just set it to true in the parse method as soon as we receive any valid data. This also adds a test to expose the behavior.

[FuegoFro]

Let me know if there's another approach here that you think would be better! Also, I wasn't sure if this is the right place to put these tests (I was also looking at the service tests?) but I'm happy to move them if you'd like!

[boukeversteegh]

Thank you for the bugfix @FuegoFro ! Test location is fine I think, we haven't completely organized our tests yet anyway, although there are also tests for serialized_on_wire in test_features.py, but see for yourself if it would fit there or not:

python-betterproto/betterproto/tests/test_features.py

Lines 6 to 33 in eec24e4

	def test_has_field():
	@dataclass
	class Bar(betterproto.Message):
	baz: int = betterproto.int32_field(1)
	@dataclass
	class Foo(betterproto.Message):
	bar: Bar = betterproto.message_field(1)
	# Unset by default
	foo = Foo()
	assert betterproto.serialized_on_wire(foo.bar) == False
	# Serialized after setting something
	foo.bar.baz = 1
	assert betterproto.serialized_on_wire(foo.bar) == True
	# Still has it after setting the default value
	foo.bar.baz = 0
	assert betterproto.serialized_on_wire(foo.bar) == True
	# Manual override (don't do this)
	foo.bar._serialized_on_wire = False
	assert betterproto.serialized_on_wire(foo.bar) == False
	# Can manually set it but defaults to false
	foo.bar = Bar()
	assert betterproto.serialized_on_wire(foo.bar) == False

I quickly added a test for a message without any fields except repeated, but that seemed to work as expected, actually:

    @dataclass
    class MessageWithRepeatedField(betterproto.Message):
        repeated: List[str] = betterproto.string_field(1)

    repeated = MessageWithRepeatedField()
    assert betterproto.serialized_on_wire(repeated) == False

    repeated.repeated = ["foo", "bar", "baz"]
    assert betterproto.serialized_on_wire(repeated) == True

So is the bug only happening when when RPC-services return data? And if so, how come it only occurs for repeated fields? Because the fix seems not specific to repeated fields, but sets the flag for all decoded messages...

[FuegoFro]

Thanks for the quick reply!

The code in question is here:

python-betterproto/betterproto/__init__.py

Lines 777 to 783 in eec24e4

	if meta.proto_type == TYPE_MAP:
	# Value represents a single key/value pair entry in the map.
	current[value.key] = value.value
	elif isinstance(current, list) and not isinstance(value, list):
	current.append(value)
	else:
	setattr(self, field_name, value)

Since we set _serialized_on_wire to True in __setattr__, anything that assigns to a field will will set this. But in the TYPE_MAP and list branches above, we just append to the existing value (which was the sentinel/default value when we created the object, but is now conceptually no longer that sentinel). It's also why on your above example it sets serialized_on_wire to true, since you're assigning to the field, rather than modifying it as is done in parse.

I considered just adding self._serialized_on_wire = True in those two cases, but it seemed to me that if we do get data then, it should be true anyway, and is a bit more robust. This also more closely matches from_dict, which just sets it up top. In fact, to even more closely match from_dict we could just move this to the top of parse.

I think test_has_field is probably a better place for this test, and I'm realizing I don't need to do the whole service dance at all but can rather just call parse with the bytes of a message. Will make that change now!

[boukeversteegh]

I see! Thank you for your clear explanation, and updates!

In that case, it seems that moving it to the top makes more sense, then anything that was parsed will be considered as arriving 'from the wire'. So also cases with int32 and such will be covered.

If you'd like to include that improvement in this PR (and perhaps including an updated test) I will wait for that. Otherwise I'll create an issue for the remaining cases.

Thanks for your work! 🥳 And clever to use bytes() actually instead of RPC to test this :-)

[FuegoFro]

int32 actually works as is, since that falls through into the else case with the setattr in the code I linked to above 🙂 But I'm happy to move it to the top of the method. That is slightly different, in that it will always be True even if there's no data in any of the fields, but I think that's fine and correct? As you say, if you got the thing over the wire at all, then serialized_on_wire should be True.

I think with the latest changes this should be good to go, but let me know if there's anything else you'd like to see changed 😀

[boukeversteegh]

I see, thanks so much for your quick changes~

About the serialized_on_wire, your explanation sounds correct, and it seems like the way to go, but I'd like a second opion on that @danielgtaylor @nat-n @cetanu

[boukeversteegh]

Merging this in absence of further feedback. Thank you for your contribution! 🥇