Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix parsing empty Struct Values from Json #5211

Merged
merged 2 commits into from Oct 5, 2018
Merged

Fix parsing empty Struct Values from Json #5211

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3bcbeac
Fix parsing empty Struct Values from Json
Oct 1, 2018
439b2f5
Additionally test for empty list
Oct 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions python/google/protobuf/internal/json_format_test.py
View file
Open in desktop
Expand Up @@ -495,6 +495,7 @@ def testStructMessage(self):
message.value['email'] = None
message.value.get_or_create_struct('address')['city'] = 'SFO'
message.value['address']['house_number'] = 1024
message.value.get_or_create_struct('empty')
struct_list = message.value.get_or_create_list('list')
struct_list.extend([6, 'seven', True, False, None])
struct_list.add_struct()['subkey2'] = 9
Expand All @@ -509,6 +510,7 @@ def testStructMessage(self):
' "city": "SFO", '
' "house_number": 1024'
' }, '
' "empty": {}, '
' "age": 10, '
' "name": "Jim", '
' "attend": true, '
Expand All @@ -519,6 +521,7 @@ def testStructMessage(self):
'}'))
parsed_message = json_format_proto3_pb2.TestStruct()
self.CheckParseBack(message, parsed_message)
parsed_message.value['empty'] # check for regression; this used to raise

def testValueMessage(self):
message = json_format_proto3_pb2.TestValue()
Expand Down
3 changes: 3 additions & 0 deletions python/google/protobuf/json_format.py
View file
Open in desktop
Expand Up @@ -614,6 +614,9 @@ def _ConvertStructMessage(self, value, message):
if not isinstance(value, dict):
raise ParseError(
'Struct must be in a dict which is {0}.'.format(value))
# Clear will mark the struct as modified so it will be created even if
# there are no values.
message.Clear()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me and thanks for the fix!
Just notice that ListValue also called clear on line 608, but didn't find empty ListValue test. Can you help to add such tests as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey guys! This change breaks behavior. Prior to this, structs were deep-merged by parse. Now they are not merged anymore...

for key in value:
self._ConvertValueMessage(value[key], message.fields[key])
return
Expand Down