Skip to content

Commit

Permalink
Sort ListFields() results.
Browse files Browse the repository at this point in the history
  • Loading branch information
haberman committed Jan 10, 2022
1 parent cbe314d commit 4993f7a
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 16 deletions.
44 changes: 37 additions & 7 deletions python/message.c
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,14 @@ static PyObject* PyUpb_CMessage_IsInitialized(PyObject* _self, PyObject* args) {
}
}

static PyObject* PyUpb_CMessage_ListFieldsLessThan(PyObject* self,
PyObject* val) {
assert(PyTuple_Check(val));
PyObject* field = PyTuple_GetItem(val, 0);
const upb_fielddef* f = PyUpb_FieldDescriptor_GetDef(field);
return PyLong_FromLong(upb_fielddef_number(f));
}

static PyObject* PyUpb_CMessage_ListFields(PyObject* _self, PyObject* arg) {
PyObject* list = PyList_New(0);
upb_msg* msg = PyUpb_CMessage_GetIfReified(_self);
Expand All @@ -967,7 +975,12 @@ static PyObject* PyUpb_CMessage_ListFields(PyObject* _self, PyObject* arg) {
PyObject* py_val = NULL;
PyObject* tuple = NULL;
upb_msgval val;
uint32_t last_field = 0;
bool in_order = true;
while (upb_msg_next(msg, m, symtab, &f, &val, &iter1)) {
const uint32_t field_number = upb_fielddef_number(f);
if (field_number < last_field) in_order = false;
last_field = field_number;
PyObject* field_desc = PyUpb_FieldDescriptor_Get(f);
PyObject* py_val = PyUpb_CMessage_GetFieldValue(_self, f);
if (!field_desc || !py_val) goto err;
Expand All @@ -980,6 +993,23 @@ static PyObject* PyUpb_CMessage_ListFields(PyObject* _self, PyObject* arg) {
tuple = NULL;
}

if (!in_order) {
PyUpb_ModuleState* state = PyUpb_ModuleState_Get();
PyObject* args = PyList_New(0);
PyObject* kwargs = PyDict_New();
PyDict_SetItemString(kwargs, "key", state->listfields_cmp_lt);
PyObject* m = PyObject_GetAttrString(list, "sort");
assert(m);
assert(args);
assert(kwargs);
PyObject* ret = PyObject_Call(m, args, kwargs);
Py_XDECREF(ret);
Py_XDECREF(m);
Py_XDECREF(args);
Py_XDECREF(kwargs);
if (!ret) return NULL;
}

return list;

err:
Expand Down Expand Up @@ -1194,7 +1224,6 @@ static PyObject* PyUpb_CMessage_FindInitializationErrors(PyObject* _self,
if (upb_util_HasUnsetRequired(msg, msgdef, ext_pool, &fields)) {
char* buf = NULL;
size_t size = 0;
size_t i = 0;
assert(fields->field);
while (fields->field) {
upb_FieldPathEntry* field = fields;
Expand Down Expand Up @@ -1416,11 +1445,9 @@ static PyMethodDef PyUpb_CMessage_Methods[] = {
{"WhichOneof", PyUpb_CMessage_WhichOneof, METH_O,
"Returns the name of the field set inside a oneof, "
"or None if no field is set."},
// TODO(https://github.com/protocolbuffers/upb/issues/459)
//{ "_CheckCalledFromGeneratedFile",
//(PyCFunction)_CheckCalledFromGeneratedFile,
// METH_NOARGS | METH_STATIC,
// "Raises TypeError if the caller is not in a _pb2.py file."},
{"_ListFieldsLessThan", PyUpb_CMessage_ListFieldsLessThan,
METH_O | METH_STATIC,
"Compares ListFields() list entries by field number"},
{NULL, NULL}};

static PyType_Slot PyUpb_CMessage_Slots[] = {
Expand Down Expand Up @@ -1664,6 +1691,8 @@ bool PyUpb_InitMessage(PyObject* m) {

if (!state->cmessage_type || !state->message_meta_type) return false;
if (PyModule_AddObject(m, "MessageMeta", message_meta_type)) return false;
state->listfields_cmp_lt =
PyObject_GetAttrString((PyObject*)state->cmessage_type, "_ListFieldsLessThan");

PyObject* mod = PyImport_ImportModule("google.protobuf.message");
if (mod == NULL) return false;
Expand All @@ -1682,7 +1711,8 @@ bool PyUpb_InitMessage(PyObject* m) {
Py_DECREF(enum_type_wrapper);

if (!state->encode_error_class || !state->decode_error_class ||
!state->message_class || !state->enum_type_wrapper_class) {
!state->message_class || !state->listfields_cmp_lt ||
!state->enum_type_wrapper_class) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion python/minimal_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,6 @@ def testExtensionIter(self):
count += 1
self.assertEqual(count, 3)
self.assertEqual(len(expected), 0)

if __name__ == '__main__':
unittest.main(verbosity=2)
7 changes: 2 additions & 5 deletions python/pb_unit_tests/reflection_test_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,10 @@
from google.protobuf.internal import reflection_test
import unittest

# The tests depend on a specific iteration order for extensions, which is not
# specified or guaranteed.
# These tests depend on a specific iteration order for extensions, which is not
# reasonable to guarantee.
reflection_test.Proto2ReflectionTest.testExtensionIter.__unittest_expecting_failure__ = True

reflection_test.Proto2ReflectionTest.testListFieldsAndExtensions.__unittest_expecting_failure__ = True
reflection_test.Proto2ReflectionTest.testRepeatedListExtensions.__unittest_expecting_failure__ = True
reflection_test.Proto2ReflectionTest.testSingularListExtensions.__unittest_expecting_failure__ = True
reflection_test.Proto2ReflectionTest.testStringUTF8Serialization.__unittest_expecting_failure__ = True
reflection_test.Proto2ReflectionTest.testTopLevelExtensionsForOptionalMessage.__unittest_expecting_failure__ = True
reflection_test.Proto2ReflectionTest.testTopLevelExtensionsForRepeatedMessage.__unittest_expecting_failure__ = True
Expand Down
3 changes: 0 additions & 3 deletions python/pb_unit_tests/text_format_test_wrapper.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,11 @@
text_format_test.OnlyWorksWithProto2RightNowTests.testParseLinesGolden.__unittest_expecting_failure__ = True
text_format_test.OnlyWorksWithProto2RightNowTests.testPrintAllFields.__unittest_expecting_failure__ = True
text_format_test.OnlyWorksWithProto2RightNowTests.testPrintAllFieldsPointy.__unittest_expecting_failure__ = True
text_format_test.OnlyWorksWithProto2RightNowTests.testPrintInIndexOrder.__unittest_expecting_failure__ = True
text_format_test.OnlyWorksWithProto2RightNowTests.testPrintMapUsingCppImplementation.__unittest_expecting_failure__ = True
text_format_test.OnlyWorksWithProto2RightNowTests.testPrintUnknownFields.__unittest_expecting_failure__ = True
text_format_test.Proto2Tests.testParseGoldenExtensions.__unittest_expecting_failure__ = True
text_format_test.Proto2Tests.testPrintAllExtensions.__unittest_expecting_failure__ = True
text_format_test.Proto2Tests.testPrintAllExtensionsPointy.__unittest_expecting_failure__ = True
getattr(text_format_test.TextFormatMessageToStringTests, "testCustomOptions" + sep + "0").__unittest_expecting_failure__ = True
getattr(text_format_test.TextFormatMessageToStringTests, "testCustomOptions" + sep + "1").__unittest_expecting_failure__ = True
getattr(text_format_test.TextFormatMessageToStringTests, "testPrintUnknownFieldsEmbeddedMessageInBytes" + sep + "0").__unittest_expecting_failure__ = True
getattr(text_format_test.TextFormatMessageToStringTests, "testPrintUnknownFieldsEmbeddedMessageInBytes" + sep + "1").__unittest_expecting_failure__ = True

Expand Down
1 change: 1 addition & 0 deletions python/protobuf.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ typedef struct {
PyObject *message_class;
PyTypeObject *cmessage_type;
PyTypeObject *message_meta_type;
PyObject* listfields_cmp_lt;

// From protobuf.c
bool allow_oversize_protos;
Expand Down

0 comments on commit 4993f7a

Please sign in to comment.