From 4993f7a73a4c68680f18df21c161cb72c1637b1d Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sun, 9 Jan 2022 18:51:10 -0800 Subject: [PATCH] Sort ListFields() results. --- python/message.c | 44 ++++++++++++++++--- python/minimal_test.py | 2 +- .../pb_unit_tests/reflection_test_wrapper.py | 7 +-- .../pb_unit_tests/text_format_test_wrapper.py | 3 -- python/protobuf.h | 1 + 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/python/message.c b/python/message.c index d7c44ba95d..cf88baefda 100644 --- a/python/message.c +++ b/python/message.c @@ -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); @@ -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; @@ -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: @@ -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; @@ -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[] = { @@ -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; @@ -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; } diff --git a/python/minimal_test.py b/python/minimal_test.py index 287f6c77eb..8e7a65a77e 100644 --- a/python/minimal_test.py +++ b/python/minimal_test.py @@ -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) diff --git a/python/pb_unit_tests/reflection_test_wrapper.py b/python/pb_unit_tests/reflection_test_wrapper.py index 32be14a2f5..0a06f8485e 100644 --- a/python/pb_unit_tests/reflection_test_wrapper.py +++ b/python/pb_unit_tests/reflection_test_wrapper.py @@ -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 diff --git a/python/pb_unit_tests/text_format_test_wrapper.py b/python/pb_unit_tests/text_format_test_wrapper.py index 75a4eb1ade..5fc2ef74d7 100644 --- a/python/pb_unit_tests/text_format_test_wrapper.py +++ b/python/pb_unit_tests/text_format_test_wrapper.py @@ -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 diff --git a/python/protobuf.h b/python/protobuf.h index 85979aa709..98e496834c 100644 --- a/python/protobuf.h +++ b/python/protobuf.h @@ -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;