Skip to content

Commit

Permalink
fix Python bug with required fields
Browse files Browse the repository at this point in the history
https://github.com/protocolbuffers/upb/issues/1220

There were two bugs here: Python was incorrectly mandating that a required
field be set during assignment, and it was also incorrectly assuming a non-NULL
return from an internal function call.

PiperOrigin-RevId: 518561818
  • Loading branch information
ericsalo authored and Copybara-Service committed Mar 22, 2023
1 parent 4e25ba4 commit ea4cb79
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 4 deletions.
22 changes: 18 additions & 4 deletions python/message.c
Expand Up @@ -447,16 +447,18 @@ static bool PyUpb_Message_InitRepeatedAttribute(PyObject* _self, PyObject* name,
return ok;
}

static PyObject* PyUpb_Message_MergePartialFrom(PyObject*, PyObject*);

static bool PyUpb_Message_InitMessageAttribute(PyObject* _self, PyObject* name,
PyObject* value) {
PyObject* submsg = PyUpb_Message_GetAttr(_self, name);
if (!submsg) return -1;
assert(!PyErr_Occurred());
bool ok;
if (PyUpb_Message_TryCheck(value)) {
PyObject* tmp = PyUpb_Message_MergeFrom(submsg, value);
PyObject* tmp = PyUpb_Message_MergePartialFrom(submsg, value);
ok = tmp != NULL;
Py_DECREF(tmp);
Py_XDECREF(tmp);
} else if (PyDict_Check(value)) {
assert(!PyErr_Occurred());
ok = PyUpb_Message_InitAttributes(submsg, NULL, value) >= 0;
Expand Down Expand Up @@ -1184,7 +1186,8 @@ static PyObject* PyUpb_Message_ListFields(PyObject* _self, PyObject* arg) {
return NULL;
}

PyObject* PyUpb_Message_MergeFrom(PyObject* self, PyObject* arg) {
static PyObject* PyUpb_Message_MergeInternal(PyObject* self, PyObject* arg,
bool check_required) {
if (self->ob_type != arg->ob_type) {
PyErr_Format(PyExc_TypeError,
"Parameter to MergeFrom() must be instance of same class: "
Expand All @@ -1194,7 +1197,10 @@ PyObject* PyUpb_Message_MergeFrom(PyObject* self, PyObject* arg) {
}
// OPT: exit if src is empty.
PyObject* subargs = PyTuple_New(0);
PyObject* serialized = PyUpb_Message_SerializeToString(arg, subargs, NULL);
PyObject* serialized =
check_required
? PyUpb_Message_SerializeToString(arg, subargs, NULL)
: PyUpb_Message_SerializePartialToString(arg, subargs, NULL);
Py_DECREF(subargs);
if (!serialized) return NULL;
PyObject* ret = PyUpb_Message_MergeFromString(self, serialized);
Expand All @@ -1203,6 +1209,14 @@ PyObject* PyUpb_Message_MergeFrom(PyObject* self, PyObject* arg) {
Py_RETURN_NONE;
}

PyObject* PyUpb_Message_MergeFrom(PyObject* self, PyObject* arg) {
return PyUpb_Message_MergeInternal(self, arg, true);
}

static PyObject* PyUpb_Message_MergePartialFrom(PyObject* self, PyObject* arg) {
return PyUpb_Message_MergeInternal(self, arg, false);
}

static PyObject* PyUpb_Message_SetInParent(PyObject* _self, PyObject* arg) {
PyUpb_Message* self = (void*)_self;
PyUpb_Message_EnsureReified(self);
Expand Down
2 changes: 2 additions & 0 deletions python/message.h
Expand Up @@ -64,6 +64,8 @@ PyObject* PyUpb_Message_MergeFrom(PyObject* self, PyObject* arg);
PyObject* PyUpb_Message_MergeFromString(PyObject* self, PyObject* arg);
PyObject* PyUpb_Message_SerializeToString(PyObject* self, PyObject* args,
PyObject* kwargs);
PyObject* PyUpb_Message_SerializePartialToString(PyObject* self, PyObject* args,
PyObject* kwargs);

// Sets fields of the message according to the attribuges in `kwargs`.
int PyUpb_Message_InitAttributes(PyObject* _self, PyObject* args,
Expand Down

0 comments on commit ea4cb79

Please sign in to comment.