diff --git a/HISTORY.rst b/HISTORY.rst index 755aba2f..7a416cbc 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -35,8 +35,8 @@ History ``os.pread``. * C extension: - * Fixed segmentation faults from invalid use of ``Metadata``, ``Reader`` and - the internal iterator type. + * Fixed segmentation faults from invalid use of ``Reader`` and the internal + iterator type. * Fixed a segmentation fault on a database with a map key that is not a string. Such a database now raises ``InvalidDatabaseError``. * Fixed large ``uint32`` values, which came back negative on platforms @@ -44,7 +44,6 @@ History * Fixed memory leaks and a use-after-free. A second ``__init__`` on a ``Reader`` now closes the old database, and an iterator from before it raises ``ValueError``. A failed ``__init__`` keeps the old database. - Reinitializing a ``Metadata`` changes nothing. * Fixed a ``RuntimeWarning`` or ``RuntimeError`` on free-threaded Python on macOS when a ``Reader`` failed to open or was used without ``__init__``. * Fixed a deadlock on free-threaded Python when a ``Reader`` was closed @@ -57,8 +56,16 @@ History This includes an error in the data of one record, but not a failure to get the read lock. Previously, the next call returned the remaining networks. - * Added the ``node_byte_size`` and ``search_tree_size`` properties to - ``Metadata``, as the pure Python ``Metadata`` has. + * ``Reader.metadata()`` now returns the pure Python + ``maxminddb.reader.Metadata`` dataclass, as the pure Python reader does. + Metadata from the two readers compares equal with ``==``, and it works + with ``repr()`` and ``dataclasses.asdict()``. It also has the + ``node_byte_size`` and ``search_tree_size`` properties. + ``maxminddb.extension.Metadata`` is now the same class. As with the pure + Python class, ``hash()`` raises ``TypeError``, the constructor takes + only keyword arguments and raises ``TypeError`` for a missing or unknown + argument, and assignment to an attribute raises + ``dataclasses.FrozenInstanceError``. GitHub #468. * Metadata: diff --git a/extension/maxminddb.c b/extension/maxminddb.c index de5aa0ca..1abf7213 100644 --- a/extension/maxminddb.c +++ b/extension/maxminddb.c @@ -91,19 +91,6 @@ typedef struct { // Set once next() has raised StopIteration or an error. bool done; } ReaderIter_obj; - -typedef struct { - PyObject_HEAD /* no semicolon */ - PyObject *binary_format_major_version; - PyObject *binary_format_minor_version; - PyObject *build_epoch; - PyObject *database_type; - PyObject *description; - PyObject *ip_version; - PyObject *languages; - PyObject *node_count; - PyObject *record_size; -} Metadata_obj; // clang-format on // ============================================================================= @@ -139,7 +126,6 @@ static bool can_read(const char *path); static int get_record(PyObject *self, PyObject *args, PyObject **record); static PyObject *metadata_value(PyObject *map, const char *key); static void set_error_from_cause(PyObject *type, const char *message); -static PyObject *Metadata_node_byte_size(PyObject *self, void *closure); static PyObject *reader_iter_next(PyObject *self); static PyObject *corrupt_tree_error(ReaderIter_obj *ri, maxminddb_state *state, @@ -745,18 +731,32 @@ static PyObject *Reader_metadata(PyObject *self, PyObject *UNUSED(args)) { "Error decoding metadata."); } } else { - // The order of the values must match kwlist in Metadata_new. - metadata = PyObject_CallFunction(state->Metadata_Type, - "HHKOOHOIH", - binary_format_major_version, - binary_format_minor_version, - (unsigned long long)build_epoch, - database_type, - description, - ip_version, - languages, - (unsigned int)node_count, - record_size); + // Metadata takes keyword arguments only. + PyObject *kwargs = + Py_BuildValue("{s:H,s:H,s:K,s:O,s:O,s:H,s:O,s:I,s:H}", + "binary_format_major_version", + binary_format_major_version, + "binary_format_minor_version", + binary_format_minor_version, + "build_epoch", + (unsigned long long)build_epoch, + "database_type", + database_type, + "description", + description, + "ip_version", + ip_version, + "languages", + languages, + "node_count", + (unsigned int)node_count, + "record_size", + record_size); + if (kwargs != NULL) { + metadata = PyObject_VectorcallDict( + state->Metadata_Type, NULL, 0, kwargs); + Py_DECREF(kwargs); + } } Py_DECREF(map); } @@ -1158,74 +1158,6 @@ static void free_records(struct record *next) { } } -// Metadata is immutable, so tp_new sets every field and there is no tp_init. -// No object can then have a NULL field or be initialized twice. -static PyObject * -Metadata_new(PyTypeObject *type, PyObject *args, PyObject *kwds) { - PyObject *binary_format_major_version, *binary_format_minor_version, - *build_epoch, *database_type, *description, *ip_version, *languages, - *node_count, *record_size; - - // Reader_metadata passes the values in this order. - static char *kwlist[] = {"binary_format_major_version", - "binary_format_minor_version", - "build_epoch", - "database_type", - "description", - "ip_version", - "languages", - "node_count", - "record_size", - NULL}; - - if (!PyArg_ParseTupleAndKeywords(args, - kwds, - "OOOOOOOOO:Metadata", - kwlist, - &binary_format_major_version, - &binary_format_minor_version, - &build_epoch, - &database_type, - &description, - &ip_version, - &languages, - &node_count, - &record_size)) { - return NULL; - } - - Metadata_obj *obj = (Metadata_obj *)type->tp_alloc(type, 0); - if (obj == NULL) { - return NULL; - } - obj->binary_format_major_version = Py_NewRef(binary_format_major_version); - obj->binary_format_minor_version = Py_NewRef(binary_format_minor_version); - obj->build_epoch = Py_NewRef(build_epoch); - obj->database_type = Py_NewRef(database_type); - obj->description = Py_NewRef(description); - obj->ip_version = Py_NewRef(ip_version); - obj->languages = Py_NewRef(languages); - obj->node_count = Py_NewRef(node_count); - obj->record_size = Py_NewRef(record_size); - return (PyObject *)obj; -} - -static void Metadata_dealloc(PyObject *self) { - Metadata_obj *obj = (Metadata_obj *)self; - Py_DECREF(obj->binary_format_major_version); - Py_DECREF(obj->binary_format_minor_version); - Py_DECREF(obj->build_epoch); - Py_DECREF(obj->database_type); - Py_DECREF(obj->description); - Py_DECREF(obj->ip_version); - Py_DECREF(obj->languages); - Py_DECREF(obj->node_count); - Py_DECREF(obj->record_size); - PyTypeObject *type = Py_TYPE(self); - PyObject_Del(self); - Py_DECREF(type); -} - static PyObject * from_entry_data_list(maxminddb_state *state, MMDB_entry_data_list_s **entry_data_list) { @@ -1431,90 +1363,6 @@ static PyMemberDef Reader_members[] = { static PyMethodDef ReaderIter_methods[] = {{NULL, NULL, 0, NULL}}; -static PyMethodDef Metadata_methods[] = {{NULL, NULL, 0, NULL}}; - -static PyMemberDef Metadata_members[] = { - {"binary_format_major_version", - T_OBJECT, - offsetof(Metadata_obj, binary_format_major_version), - READONLY, - NULL}, - {"binary_format_minor_version", - T_OBJECT, - offsetof(Metadata_obj, binary_format_minor_version), - READONLY, - NULL}, - {"build_epoch", - T_OBJECT, - offsetof(Metadata_obj, build_epoch), - READONLY, - NULL}, - {"database_type", - T_OBJECT, - offsetof(Metadata_obj, database_type), - READONLY, - NULL}, - {"description", - T_OBJECT, - offsetof(Metadata_obj, description), - READONLY, - NULL}, - {"ip_version", - T_OBJECT, - offsetof(Metadata_obj, ip_version), - READONLY, - NULL}, - {"languages", T_OBJECT, offsetof(Metadata_obj, languages), READONLY, NULL}, - {"node_count", - T_OBJECT, - offsetof(Metadata_obj, node_count), - READONLY, - NULL}, - {"record_size", - T_OBJECT, - offsetof(Metadata_obj, record_size), - READONLY, - NULL}, - {NULL, 0, 0, 0, NULL}}; - -static PyObject *Metadata_search_tree_size(PyObject *self, - void *UNUSED(closure)) { - Metadata_obj *obj = (Metadata_obj *)self; - PyObject *node_byte_size = Metadata_node_byte_size(self, NULL); - if (node_byte_size == NULL) { - return NULL; - } - PyObject *search_tree_size = - PyNumber_Multiply(obj->node_count, node_byte_size); - Py_DECREF(node_byte_size); - return search_tree_size; -} - -static PyObject *Metadata_node_byte_size(PyObject *self, - void *UNUSED(closure)) { - Metadata_obj *obj = (Metadata_obj *)self; - PyObject *four = PyLong_FromLong(4); - if (four == NULL) { - return NULL; - } - PyObject *node_byte_size = PyNumber_FloorDivide(obj->record_size, four); - Py_DECREF(four); - return node_byte_size; -} - -// These match the properties of the pure Python Metadata class. -static PyGetSetDef Metadata_getset[] = {{"node_byte_size", - Metadata_node_byte_size, - NULL, - "The size of a node in bytes.", - NULL}, - {"search_tree_size", - Metadata_search_tree_size, - NULL, - "The size of the search tree.", - NULL}, - {NULL, NULL, NULL, NULL, NULL}}; - // ============================================================================= // Type specs for heap type conversion (PEP 489) // ============================================================================= @@ -1537,23 +1385,6 @@ static PyType_Spec Reader_Type_spec = { .slots = Reader_Type_slots, }; -static PyType_Slot Metadata_Type_slots[] = { - {Py_tp_doc, "Metadata object"}, - {Py_tp_dealloc, Metadata_dealloc}, - {Py_tp_new, Metadata_new}, - {Py_tp_methods, Metadata_methods}, - {Py_tp_members, Metadata_members}, - {Py_tp_getset, Metadata_getset}, - {0, NULL}, -}; - -static PyType_Spec Metadata_Type_spec = { - .name = "maxminddb.extension.Metadata", - .basicsize = sizeof(Metadata_obj), - .flags = Py_TPFLAGS_DEFAULT, - .slots = Metadata_Type_slots, -}; - static PyType_Slot ReaderIter_Type_slots[] = { {Py_tp_doc, "Iterator for Reader object"}, {Py_tp_dealloc, ReaderIter_dealloc}, @@ -1614,8 +1445,14 @@ static int maxminddb_exec(PyObject *module) { return -1; } - state->Metadata_Type = - PyType_FromModuleAndSpec(module, &Metadata_Type_spec, NULL); + // metadata() returns the pure Python Metadata, so both readers return + // the same type. + PyObject *reader_mod = PyImport_ImportModule("maxminddb.reader"); + if (reader_mod == NULL) { + return -1; + } + state->Metadata_Type = PyObject_GetAttrString(reader_mod, "Metadata"); + Py_DECREF(reader_mod); if (state->Metadata_Type == NULL) { return -1; } diff --git a/maxminddb/extension.pyi b/maxminddb/extension.pyi index 19c8d4fe..9ee6947d 100644 --- a/maxminddb/extension.pyi +++ b/maxminddb/extension.pyi @@ -5,6 +5,7 @@ from ipaddress import IPv4Address, IPv4Network, IPv6Address, IPv6Network from typing_extensions import Self +from maxminddb.reader import Metadata as Metadata from maxminddb.types import Record, StrOrBytesPath class Reader: @@ -58,78 +59,3 @@ class Reader: def __iter__(self) -> Iterator[tuple[IPv4Network | IPv6Network, Record]]: ... def __enter__(self) -> Self: ... def __exit__(self, *args: object) -> None: ... - -class Metadata: - """Metadata for the MaxMind DB reader.""" - - binary_format_major_version: int - """ - The major version number of the binary format used when creating the - database. - """ - - binary_format_minor_version: int - """ - The minor version number of the binary format used when creating the - database. - """ - - build_epoch: int - """ - The Unix epoch for the build time of the database. - """ - - database_type: str - """ - A string identifying the database type, e.g., "GeoIP2-City". - """ - - description: dict[str, str] - """ - A map from locales to text descriptions of the database. - """ - - ip_version: int - """ - The IP version of the data in a database. A value of "4" means the - database only supports IPv4. A database with a value of "6" may support - both IPv4 and IPv6 lookups. - """ - - languages: list[str] - """ - A list of locale codes supported by the database. - """ - - node_count: int - """ - The number of nodes in the database. - """ - - record_size: int - """ - The bit size of a record in the search tree. - """ - - def __init__( - self, - *, - binary_format_major_version: int, - binary_format_minor_version: int, - build_epoch: int, - database_type: str, - description: dict[str, str], - ip_version: int, - languages: list[str], - node_count: int, - record_size: int, - ) -> None: - """Create new Metadata object from the metadata fields in the spec.""" - - @property - def node_byte_size(self) -> int: - """The size of a node in bytes.""" - - @property - def search_tree_size(self) -> int: - """The size of the search tree.""" diff --git a/tests/reader_test.py b/tests/reader_test.py index b8c716eb..6e8d6329 100644 --- a/tests/reader_test.py +++ b/tests/reader_test.py @@ -53,18 +53,6 @@ # Directory holding the shared MaxMind DB test fixtures. _TEST_DATA_DIR = "tests/data/test-data" _DECODER_DB = f"{_TEST_DATA_DIR}/MaxMind-DB-test-decoder.mmdb" -# Valid arguments for the C extension Metadata. -_METADATA_FIELDS: dict[str, Any] = { - "binary_format_major_version": 2, - "binary_format_minor_version": 0, - "build_epoch": 1, - "database_type": "db", - "description": {}, - "ip_version": 4, - "languages": [], - "node_count": 1, - "record_size": 24, -} _PAYLOAD_TOO_LARGE = ( "^The MaxMind DB file's data section exceeds the maximum payload size$" ) @@ -1148,7 +1136,7 @@ def test_metadata_unknown_attribute(self) -> None: metadata = reader.metadata() with self.assertRaisesRegex( AttributeError, - r"'(maxminddb\.extension\.)?Metadata' object has no attribute 'blah'", + "'Metadata' object has no attribute 'blah'", ): metadata.blah # type: ignore[attr-defined] # noqa: B018 reader.close() @@ -1287,6 +1275,37 @@ def _reinitialize(self, reader: Any, path: str, mode: int) -> None: # noqa: ANN with _database_source(path, mode) as database: reader.__init__(database, mode) + def test_metadata_is_a_dataclass(self) -> None: + with open_database(_DECODER_DB, self.mode) as reader: + metadata = reader.metadata() + self.assertEqual(metadata, reader.metadata()) + with maxminddb.reader.Reader(_DECODER_DB) as reader: + expected = reader.metadata() + self.assertIs(type(metadata), maxminddb.reader.Metadata) + self.assertEqual(metadata, expected) + self.assertEqual(repr(metadata), repr(expected)) + self.assertTrue(repr(metadata).startswith("Metadata(")) + self.assertEqual( + dataclasses.asdict(metadata), + { + "binary_format_major_version": 2, + "binary_format_minor_version": 0, + "build_epoch": expected.build_epoch, + "database_type": "MaxMind DB Decoder Test", + "description": { + "en": "MaxMind DB Decoder Test database - " + "contains every MaxMind DB data type", + }, + "ip_version": 6, + "languages": ["en"], + "node_count": expected.node_count, + "record_size": 24, + }, + ) + # The description dict makes Metadata unhashable. + with self.assertRaises(TypeError): + hash(metadata) + def test_closed_metadata(self) -> None: reader = open_database( "tests/data/test-data/MaxMind-DB-test-decoder.mmdb", @@ -1524,18 +1543,8 @@ def test_record_subscript_works_at_runtime(self) -> None: class TestExtensionObjects(unittest.TestCase): """Objects in states that crashed the extension.""" - def test_new_metadata_requires_arguments(self) -> None: - metadata_class = maxminddb.extension.Metadata - with self.assertRaisesRegex(TypeError, "missing required argument"): - metadata_class.__new__(metadata_class) - - def test_metadata_missing_argument(self) -> None: - with self.assertRaisesRegex(TypeError, "missing required argument"): - maxminddb.extension.Metadata(binary_format_major_version=2) # type: ignore[call-arg] - - def test_metadata_too_many_arguments(self) -> None: - with self.assertRaisesRegex(TypeError, "at most 9"): - maxminddb.extension.Metadata(**_METADATA_FIELDS, unknown=1) # type: ignore[call-arg] + def test_metadata_is_the_pure_python_class(self) -> None: + self.assertIs(maxminddb.extension.Metadata, maxminddb.reader.Metadata) def test_iterate_uninitialized_reader(self) -> None: reader_class = maxminddb.extension.Reader @@ -1705,11 +1714,6 @@ def test_initialize_after_close_on_uninitialized_reader(self) -> None: with reader: self.assertIsNotNone(reader.get("::1.1.1.0")) - def test_reinitialize_metadata_changes_nothing(self) -> None: - metadata = maxminddb.extension.Metadata(**_METADATA_FIELDS) - metadata.__init__(**{**_METADATA_FIELDS, "record_size": 28}) # type: ignore[misc] - self.assertEqual(metadata.record_size, 24) - @unittest.skipUnless( hasattr(sys, "getrefcount") and not sysconfig.get_config_var("Py_GIL_DISABLED"), "needs CPython reference counts on a build with the GIL",