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.

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

Fix parsing empty Struct Values from Json #5211

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
5 participants
@leon-barrett
Contributor

leon-barrett commented Oct 1, 2018

This fixes a bug. When parsing a struct from JSON like

struct = json_format.Parse('{"k": {}}', Struct())

then the struct's "k" value would end up not initialized, and accessing
the value would raise an error.

In[1]: struct['k']
ValueError: Value not set

That seems to be because the Struct field of the Value was not set.

In[2]: struct
Out[2]:
fields {
  key: "k"
  value {
  }
}

This commit makes sure that the Value's Struct field is initialized even
if the Struct has no values itself.

This commit also extends a test to cover this case.

Fix parsing empty Struct Values from Json
This fixes a bug. When parsing a struct from JSON like
    struct = json_format.Parse('{"k": {}}', Struct())
then the struct's "k" value would end up not initialized, and accessing
the value would raise an error.
    In[1]: struct['k']
    ValueError: Value not set
That seems to be because the Struct field of the Value was not set.
    In[2]: struct
    Out[2]:
    fields {
      key: "k"
      value {
      }
    }

This commit makes sure that the Value's Struct field is initialized even
if the Struct has no values itself.

This commit also extends a test to cover this case.
@@ -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()

This comment has been minimized.

@anandolee

anandolee Oct 4, 2018

Contributor

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.

@anandolee

anandolee Oct 4, 2018

Contributor

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.

This comment has been minimized.

@leon-barrett

leon-barrett Oct 5, 2018

Contributor

done

@leon-barrett

leon-barrett Oct 5, 2018

Contributor

done

@anandolee anandolee merged commit 9e69594 into protocolbuffers:master Oct 5, 2018

27 of 28 checks passed

Linux Ruby Kokoro build failed
Details
Bazel Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux JavaScript Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug (Allowed Failure) Kokoro build successful
Details
MacOS Obj-C iOS Release (Allowed Failure) Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ (Allowed Failure) Kokoro build successful
Details
MacOS Ruby 2.1 Kokoro build successful
Details
MacOS Ruby 2.2 (Allowed Failure) Kokoro build successful
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment