From 9d62c76051cf80cb9491aa58b0826bb1a13f679f Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Mon, 11 Nov 2024 00:50:00 +0100 Subject: [PATCH 01/27] ENH: capture all errors logged by gdal when opening a file fails --- pyogrio/_err.pxd | 5 + pyogrio/_err.pyx | 316 +++++++++++++++++++++++++++-- pyogrio/_io.pyx | 42 ++-- pyogrio/_ogr.pxd | 1 + pyogrio/tests/conftest.py | 5 + pyogrio/tests/test_geopandas_io.py | 12 ++ 6 files changed, 354 insertions(+), 27 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index 53d52a13..7996d578 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -2,3 +2,8 @@ cdef object exc_check() cdef int exc_wrap_int(int retval) except -1 cdef int exc_wrap_ogrerr(int retval) except -1 cdef void *exc_wrap_pointer(void *ptr) except NULL + +cdef class StackChecker: + cdef object error_stack + cdef int exc_wrap_int(self, int retval) except -1 + cdef void *exc_wrap_pointer(self, void *ptr) except NULL diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 51f2fcfb..f8113021 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -1,11 +1,25 @@ -# ported from fiona::_err.pyx -from enum import IntEnum +"""Error handling code for GDAL/OGR. + +Ported from fiona::_err.pyx +""" + +import contextlib +import logging import warnings +from contextvars import ContextVar +from enum import IntEnum +from itertools import zip_longest from pyogrio._ogr cimport ( CE_None, CE_Debug, CE_Warning, CE_Failure, CE_Fatal, CPLErrorReset, CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, - CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPushErrorHandler) + CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPopErrorHandler, + CPLPushErrorHandler, CPLPushErrorHandlerEx) + +log = logging.getLogger(__name__) + +_ERROR_STACK = ContextVar("error_stack") +_ERROR_STACK.set([]) # CPL Error types as an enum. @@ -17,9 +31,9 @@ class GDALError(IntEnum): fatal = CE_Fatal - class CPLE_BaseError(Exception): """Base CPL error class. + For internal use within Cython only. """ @@ -103,6 +117,10 @@ class CPLE_AWSSignatureDoesNotMatchError(CPLE_BaseError): pass +class CPLE_AWSError(CPLE_BaseError): + pass + + class NullPointerError(CPLE_BaseError): """ Returned from exc_wrap_pointer when a NULL pointer is passed, but no GDAL @@ -111,6 +129,21 @@ class NullPointerError(CPLE_BaseError): pass +class CPLError(CPLE_BaseError): + """ + Returned from exc_wrap_int when a error code is returned, but no GDAL + error was set. + """ + pass + + +cdef dict _LEVEL_MAP = { + 0: 0, + 1: logging.DEBUG, + 2: logging.WARNING, + 3: logging.ERROR, + 4: logging.CRITICAL +} # Map of GDAL error numbers to the Python exceptions. exception_map = { @@ -132,12 +165,51 @@ exception_map = { 13: CPLE_AWSObjectNotFoundError, 14: CPLE_AWSAccessDeniedError, 15: CPLE_AWSInvalidCredentialsError, - 16: CPLE_AWSSignatureDoesNotMatchError + 16: CPLE_AWSSignatureDoesNotMatchError, + 17: CPLE_AWSError } +cdef dict _CODE_MAP = { + 0: 'CPLE_None', + 1: 'CPLE_AppDefined', + 2: 'CPLE_OutOfMemory', + 3: 'CPLE_FileIO', + 4: 'CPLE_OpenFailed', + 5: 'CPLE_IllegalArg', + 6: 'CPLE_NotSupported', + 7: 'CPLE_AssertionFailed', + 8: 'CPLE_NoWriteAccess', + 9: 'CPLE_UserInterrupt', + 10: 'ObjectNull', + 11: 'CPLE_HttpResponse', + 12: 'CPLE_AWSBucketNotFound', + 13: 'CPLE_AWSObjectNotFound', + 14: 'CPLE_AWSAccessDenied', + 15: 'CPLE_AWSInvalidCredentials', + 16: 'CPLE_AWSSignatureDoesNotMatch', + 17: 'CPLE_AWSError' +} + + +cdef class GDALErrCtxManager: + """A manager for GDAL error handling contexts.""" + + def __enter__(self): + CPLErrorReset() + return self + + def __exit__(self, exc_type=None, exc_val=None, exc_tb=None): + cdef int err_type = CPLGetLastErrorType() + cdef int err_no = CPLGetLastErrorNo() + cdef const char *msg = CPLGetLastErrorMsg() + # TODO: warn for err_type 2? + if err_type >= 2: + raise exception_map[err_no](err_type, err_no, msg) + cdef inline object exc_check(): - """Checks GDAL error stack for fatal or non-fatal errors + """Checks GDAL error stack for fatal or non-fatal errors. + Returns ------- An Exception, SystemExit, or None @@ -173,8 +245,31 @@ cdef inline object exc_check(): return +cdef get_last_error_msg(): + """Checks GDAL error stack for the latest error message. + + Returns + ------- + An error message or empty string + + """ + err_msg = CPLGetLastErrorMsg() + + if err_msg != NULL: + # Reformat messages. + msg_b = err_msg + msg = msg_b.decode('utf-8') + msg = msg.replace("`", "'") + msg = msg.replace("\n", " ") + else: + msg = "" + + return msg + + cdef void *exc_wrap_pointer(void *ptr) except NULL: - """Wrap a GDAL/OGR function that returns GDALDatasetH etc (void *) + """Wrap a GDAL/OGR function that returns GDALDatasetH etc (void *). + Raises an exception if a non-fatal error has be set or if pointer is NULL. """ if ptr == NULL: @@ -188,10 +283,9 @@ cdef void *exc_wrap_pointer(void *ptr) except NULL: cdef int exc_wrap_int(int err) except -1: - """Wrap a GDAL/OGR function that returns CPLErr or OGRErr (int) - Raises an exception if a non-fatal error has be set. + """Wrap a GDAL/OGR function that returns CPLErr or OGRErr (int). - Copied from Fiona (_err.pyx). + Raises an exception if a non-fatal error has be set. """ if err: exc = exc_check() @@ -218,9 +312,11 @@ cdef int exc_wrap_ogrerr(int err) except -1: cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil: """Custom CPL error handler to match the Python behaviour. - Generally we want to suppress error printing to stderr (behaviour of the - default GDAL error handler) because we already raise a Python exception - that includes the error message. + For non-fatal errors (CE_Failure), error printing to stderr (behaviour of + the default GDAL error handler) is suppressed, because we already raise a + Python exception that includes the error message. + + Warnings are converted to Python warnings. """ if err_class == CE_Fatal: # If the error class is CE_Fatal, we want to have a message issued @@ -248,3 +344,197 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil def _register_error_handler(): CPLPushErrorHandler(error_handler) + +cpl_errs = GDALErrCtxManager() + + +cdef class StackChecker: + + def __init__(self, error_stack=None): + self.error_stack = error_stack or {} + + cdef int exc_wrap_int(self, int err) except -1: + """Wrap a GDAL/OGR function that returns CPLErr (int). + + Raises an exception if a non-fatal error has been added to the + exception stack. + """ + if err: + stack = self.error_stack.get() + for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): + if error is not None and cause is not None: + error.__cause__ = cause + + if stack: + last = stack.pop() + if last is not None: + raise last + + return err + + cdef void *exc_wrap_pointer(self, void *ptr) except NULL: + """Wrap a GDAL/OGR function that returns a pointer. + + Raises an exception if a non-fatal error has been added to the + exception stack. + """ + if ptr == NULL: + stack = self.error_stack.get() + for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): + if error is not None and cause is not None: + error.__cause__ = cause + + if stack: + last = stack.pop() + if last is not None: + raise last + + return ptr + + +cdef void log_error( + CPLErr err_class, + int err_no, + const char* msg, +) noexcept with gil: + """Send CPL errors to Python's logger. + + Because this function is called by GDAL with no Python context, we + can't propagate exceptions that we might raise here. They'll be + ignored. + """ + if err_no in _CODE_MAP: + # We've observed that some GDAL functions may emit multiple + # ERROR level messages and yet succeed. We want to see those + # messages in our log file, but not at the ERROR level. We + # turn the level down to INFO. + if err_class == CE_Failure: + log.info( + "GDAL signalled an error: err_no=%r, msg=%r", + err_no, + msg.decode("utf-8") + ) + elif err_no == 0: + log.log(_LEVEL_MAP[err_class], "%s", msg.decode("utf-8")) + else: + log.log(_LEVEL_MAP[err_class], "%s:%s", _CODE_MAP[err_no], msg.decode("utf-8")) + else: + log.info("Unknown error number %r", err_no) + + +IF UNAME_SYSNAME == "Windows": + cdef void __stdcall stacking_error_handler( + CPLErr err_class, + int err_no, + const char* err_msg + ) noexcept with gil: + """Custom CPL error handler that adds non-fatal errors to a stack. + + All non-fatal errors (CE_Failure) are not printed to stderr (behaviour + of the default GDAL error handler), but they are converted to python + exceptions and added to a stack, so they can be dealt with afterwards. + + Warnings are converted to Python warnings. + """ + global _ERROR_STACK + log_error(err_class, err_no, err_msg) + + if err_class == CE_Fatal: + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated + CPLDefaultErrorHandler(err_class, err_no, err_msg) + + return + + elif err_class == CE_Failure: + # For Failures, add them to the error exception stack + stack = _ERROR_STACK.get() + stack.append( + exception_map.get(err_no, CPLE_BaseError)(err_class, err_no, err_msg.decode("utf-8")), + ) + _ERROR_STACK.set(stack) + + return + + elif err_class == CE_Warning: + msg_b = err_msg + msg = msg_b.decode("utf-8") + warnings.warn(msg, RuntimeWarning) + + return + + # Fall back to the default handler for non-failure messages since + # they won't be translated into exceptions. + CPLDefaultErrorHandler(err_class, err_no, err_msg) + +ELSE: + cdef void stacking_error_handler( + CPLErr err_class, + int err_no, + const char* err_msg + ) noexcept with gil: + """Custom CPL error handler that adds non-fatal errors to a stack. + + All non-fatal errors (CE_Failure) are not printed to stderr (behaviour + of the default GDAL error handler), but they are converted to python + exceptions and added to a stack, so they can be dealt with afterwards. + + Warnings are converted to Python warnings. + """ + global _ERROR_STACK + log_error(err_class, err_no, err_msg) + + if err_class == CE_Fatal: + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated + CPLDefaultErrorHandler(err_class, err_no, err_msg) + + return + + elif err_class == CE_Failure: + # For Failures, add them to the error exception stack + stack = _ERROR_STACK.get() + stack.append( + exception_map.get(err_no, CPLE_BaseError)(err_class, err_no, err_msg.decode("utf-8")), + ) + _ERROR_STACK.set(stack) + + return + + elif err_class == CE_Warning: + msg_b = err_msg + msg = msg_b.decode("utf-8") + warnings.warn(msg, RuntimeWarning) + + return + + # Fall back to the default handler for non-failure messages since + # they won't be translated into exceptions. + CPLDefaultErrorHandler(err_class, err_no, err_msg) + + +@contextlib.contextmanager +def stack_errors(): + """A context manager that captures all GDAL non-fatal errors occuring. + + It adds all errors to a single stack, so it assumes that no more than one + GDAL function is called. + + Yields a StackChecker object that can be used to check the error stack. + """ + CPLErrorReset() + global _ERROR_STACK + _ERROR_STACK.set([]) + + # stacking_error_handler records GDAL errors in the order they occur and + # converts them to exceptions. + CPLPushErrorHandlerEx(stacking_error_handler, NULL) + + # Run code in the `with` block. + yield StackChecker(_ERROR_STACK) + + CPLPopErrorHandler() + _ERROR_STACK.set([]) + CPLErrorReset() diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index a9c934e5..83be5362 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -3,7 +3,6 @@ """IO support for OGR vector data sources """ - import contextlib import datetime import locale @@ -26,9 +25,18 @@ from cpython.pycapsule cimport PyCapsule_New, PyCapsule_GetPointer import numpy as np from pyogrio._ogr cimport * -from pyogrio._err cimport * +from pyogrio._err cimport ( + exc_check, exc_wrap_int, exc_wrap_ogrerr, exc_wrap_pointer, StackChecker +) from pyogrio._vsi cimport * -from pyogrio._err import CPLE_BaseError, CPLE_NotSupportedError, NullPointerError +from pyogrio._err import ( + CPLE_AppDefinedError, + CPLE_BaseError, + CPLE_NotSupportedError, + CPLE_OpenFailedError, + NullPointerError, + stack_errors, +) from pyogrio._geometry cimport get_geometry_type, get_geometry_type_code from pyogrio.errors import CRSError, DataSourceError, DataLayerError, GeometryError, FieldError, FeatureError @@ -185,7 +193,8 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: options : char **, optional dataset open options """ - cdef void* ogr_dataset = NULL + cdef void *ogr_dataset = NULL + cdef StackChecker error_stack_checker # Force linear approximations in all cases OGRSetNonLinearGeometriesEnabledFlag(0) @@ -196,28 +205,33 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: else: flags |= GDAL_OF_READONLY - try: # WARNING: GDAL logs warnings about invalid open options to stderr # instead of raising an error - ogr_dataset = exc_wrap_pointer( - GDALOpenEx(path_c, flags, NULL, options, NULL) - ) - - return ogr_dataset + with stack_errors() as error_stack_checker: + ogr_dataset = GDALOpenEx(path_c, flags, NULL, options, NULL) + return error_stack_checker.exc_wrap_pointer(ogr_dataset) except NullPointerError: raise DataSourceError( - "Failed to open dataset (mode={}): {}".format(mode, path_c.decode("utf-8")) + f"Failed to open dataset (mode={mode}): {path_c.decode('utf-8')}" ) from None except CPLE_BaseError as exc: - if str(exc).endswith("a supported file format."): + # If there are inner exceptions, append their errmsg to the errmsg + errmsg = str(exc) + inner = exc.__cause__ + while inner is not None: + errmsg = f"{errmsg}; {inner}" + inner = inner.__cause__ + + if exc.errmsg.endswith("a supported file format."): raise DataSourceError( - f"{str(exc)} It might help to specify the correct driver explicitly by " + f"{errmsg}; It might help to specify the correct driver explicitly by " "prefixing the file path with ':', e.g. 'CSV:path'." ) from None - raise DataSourceError(str(exc)) from None + + raise DataSourceError(errmsg) from None cdef ogr_close(GDALDatasetH ogr_dataset): diff --git a/pyogrio/_ogr.pxd b/pyogrio/_ogr.pxd index 8ce6a578..3a797925 100644 --- a/pyogrio/_ogr.pxd +++ b/pyogrio/_ogr.pxd @@ -33,6 +33,7 @@ cdef extern from "cpl_error.h" nogil: ctypedef void (*CPLErrorHandler)(CPLErr, int, const char*) void CPLDefaultErrorHandler(CPLErr, int, const char *) void CPLPushErrorHandler(CPLErrorHandler handler) + void CPLPushErrorHandlerEx(CPLErrorHandler handler, void *userdata) void CPLPopErrorHandler() diff --git a/pyogrio/tests/conftest.py b/pyogrio/tests/conftest.py index d6bea86b..ebc4e18d 100644 --- a/pyogrio/tests/conftest.py +++ b/pyogrio/tests/conftest.py @@ -124,6 +124,11 @@ def naturalearth_lowres_all_ext(tmp_path, naturalearth_lowres, request): return prepare_testfile(naturalearth_lowres, tmp_path, request.param) +@pytest.fixture(scope="function", params=[".geojson"]) +def naturalearth_lowres_geojson(tmp_path, naturalearth_lowres, request): + return prepare_testfile(naturalearth_lowres, tmp_path, request.param) + + @pytest.fixture(scope="function") def naturalearth_lowres_vsi(tmp_path, naturalearth_lowres): """Wrap naturalearth_lowres as a zip file for VSI tests""" diff --git a/pyogrio/tests/test_geopandas_io.py b/pyogrio/tests/test_geopandas_io.py index 112eef84..c32b8424 100644 --- a/pyogrio/tests/test_geopandas_io.py +++ b/pyogrio/tests/test_geopandas_io.py @@ -12,6 +12,7 @@ list_drivers, list_layers, read_info, + set_gdal_config_options, vsi_listtree, vsi_unlink, ) @@ -227,6 +228,17 @@ def test_read_force_2d(tmp_path, use_arrow): assert not df.iloc[0].geometry.has_z +def test_read_geojson_error(naturalearth_lowres_geojson, use_arrow): + set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": 0.01}) + with pytest.raises( + DataSourceError, + match="Failed to read GeoJSON data; .* GeoJSON object too complex", + ): + read_dataframe(naturalearth_lowres_geojson, use_arrow=use_arrow) + + set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": None}) + + def test_read_layer(tmp_path, use_arrow): filename = tmp_path / "test.gpkg" From d4f917c6d25060f584bcdaad94abb5ba0e5538d8 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Mon, 11 Nov 2024 00:51:42 +0100 Subject: [PATCH 02/27] Update CHANGES.md --- CHANGES.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index ac51e8c7..9a38a75c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,11 @@ # CHANGELOG +## 0.11.0 (????-??-??) + +### Improvements + +- Capture all errors logged by gdal when opening a file fails (#495). + ## 0.10.0 (2024-09-28) ### Improvements From 3d7028031691665f668c1790541caddb81d368f5 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 17:03:25 +0100 Subject: [PATCH 03/27] Apply feedback: use helper function to clean up error message --- pyogrio/_err.pyx | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index f8113021..29a7818b 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -214,32 +214,17 @@ cdef inline object exc_check(): ------- An Exception, SystemExit, or None """ - cdef const char *msg_c = NULL - err_type = CPLGetLastErrorType() err_no = CPLGetLastErrorNo() - err_msg = CPLGetLastErrorMsg() - - if err_msg == NULL: - msg = "No error message." - else: - # Reformat messages. - msg_b = err_msg - - try: - msg = msg_b.decode('utf-8') - msg = msg.replace("`", "'") - msg = msg.replace("\n", " ") - except UnicodeDecodeError as exc: - msg = f"Could not decode error message to UTF-8. Raw error: {msg_b}" + err_msg = get_last_error_msg() - if err_type == 3: + if err_type == CE_Failure: CPLErrorReset() return exception_map.get( - err_no, CPLE_BaseError)(err_type, err_no, msg) + err_no, CPLE_BaseError)(err_type, err_no, err_msg) - if err_type == 4: - return SystemExit("Fatal error: {0}".format((err_type, err_no, msg))) + if err_type == CE_Fatal: + return SystemExit("Fatal error: {0}".format((err_type, err_no, err_msg))) else: return @@ -258,11 +243,15 @@ cdef get_last_error_msg(): if err_msg != NULL: # Reformat messages. msg_b = err_msg - msg = msg_b.decode('utf-8') - msg = msg.replace("`", "'") - msg = msg.replace("\n", " ") + try: + msg = msg_b.decode("utf-8") + msg = msg.replace("`", "'") + msg = msg.replace("\n", " ") + except UnicodeDecodeError as exc: + msg = f"Could not decode error message to UTF-8. Raw error: {msg_b}" + else: - msg = "" + msg = "No error message." return msg From c07997196661289250492e1b77eb50cfa6dece65 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 19:30:32 +0100 Subject: [PATCH 04/27] Extract clean_error_message in seperate function and use where possible --- pyogrio/_err.pyx | 46 +++++++++++++++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 29a7818b..b5554387 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -24,6 +24,10 @@ _ERROR_STACK.set([]) # CPL Error types as an enum. class GDALError(IntEnum): + """GDAL error types/classes. + + GDAL doc: https://gdal.org/en/latest/doxygen/cpl__error_8h.html#a463ba7c7202a505416ff95b1aeefa2de + """ none = CE_None debug = CE_Debug warning = CE_Warning @@ -238,10 +242,24 @@ cdef get_last_error_msg(): An error message or empty string """ - err_msg = CPLGetLastErrorMsg() + return clean_error_message(CPLGetLastErrorMsg()) + + +cdef clean_error_message(const char* err_msg): + """Cleans up error messages from GDAL. + Parameters + ---------- + err_msg : const char* + The error message to clean up. + + Returns + ------- + str + The cleaned up error message or empty string + """ if err_msg != NULL: - # Reformat messages. + # Reformat message. msg_b = err_msg try: msg = msg_b.decode("utf-8") @@ -251,7 +269,7 @@ cdef get_last_error_msg(): msg = f"Could not decode error message to UTF-8. Raw error: {msg_b}" else: - msg = "No error message." + msg = "" return msg @@ -298,7 +316,7 @@ cdef int exc_wrap_ogrerr(int err) except -1: return err -cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil: +cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) noexcept nogil: """Custom CPL error handler to match the Python behaviour. For non-fatal errors (CE_Failure), error printing to stderr (behaviour of @@ -321,9 +339,7 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) nogil elif err_class == CE_Warning: with gil: - msg_b = err_msg - msg = msg_b.decode('utf-8') - warnings.warn(msg, RuntimeWarning) + warnings.warn(clean_error_message(err_msg), RuntimeWarning) return # Fall back to the default handler for non-failure messages since @@ -440,16 +456,16 @@ IF UNAME_SYSNAME == "Windows": # For Failures, add them to the error exception stack stack = _ERROR_STACK.get() stack.append( - exception_map.get(err_no, CPLE_BaseError)(err_class, err_no, err_msg.decode("utf-8")), + exception_map.get(err_no, CPLE_BaseError)( + err_class, err_no, clean_error_message(err_msg) + ), ) _ERROR_STACK.set(stack) return elif err_class == CE_Warning: - msg_b = err_msg - msg = msg_b.decode("utf-8") - warnings.warn(msg, RuntimeWarning) + warnings.warn(clean_error_message(err_msg), RuntimeWarning) return @@ -486,16 +502,16 @@ ELSE: # For Failures, add them to the error exception stack stack = _ERROR_STACK.get() stack.append( - exception_map.get(err_no, CPLE_BaseError)(err_class, err_no, err_msg.decode("utf-8")), + exception_map.get(err_no, CPLE_BaseError)( + err_class, err_no, clean_error_message(err_msg) + ), ) _ERROR_STACK.set(stack) return elif err_class == CE_Warning: - msg_b = err_msg - msg = msg_b.decode("utf-8") - warnings.warn(msg, RuntimeWarning) + warnings.warn(clean_error_message(err_msg), RuntimeWarning) return From c04497b770600b980c7ef5dbeef0e899882d5d93 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 19:43:07 +0100 Subject: [PATCH 05/27] Remove logging GDAL messages via python logging.log --- pyogrio/_err.pyx | 42 ------------------------------------------ 1 file changed, 42 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index b5554387..9c804c2a 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -16,8 +16,6 @@ from pyogrio._ogr cimport ( CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPopErrorHandler, CPLPushErrorHandler, CPLPushErrorHandlerEx) -log = logging.getLogger(__name__) - _ERROR_STACK = ContextVar("error_stack") _ERROR_STACK.set([]) @@ -141,14 +139,6 @@ class CPLError(CPLE_BaseError): pass -cdef dict _LEVEL_MAP = { - 0: 0, - 1: logging.DEBUG, - 2: logging.WARNING, - 3: logging.ERROR, - 4: logging.CRITICAL -} - # Map of GDAL error numbers to the Python exceptions. exception_map = { 1: CPLE_AppDefinedError, @@ -397,36 +387,6 @@ cdef class StackChecker: return ptr -cdef void log_error( - CPLErr err_class, - int err_no, - const char* msg, -) noexcept with gil: - """Send CPL errors to Python's logger. - - Because this function is called by GDAL with no Python context, we - can't propagate exceptions that we might raise here. They'll be - ignored. - """ - if err_no in _CODE_MAP: - # We've observed that some GDAL functions may emit multiple - # ERROR level messages and yet succeed. We want to see those - # messages in our log file, but not at the ERROR level. We - # turn the level down to INFO. - if err_class == CE_Failure: - log.info( - "GDAL signalled an error: err_no=%r, msg=%r", - err_no, - msg.decode("utf-8") - ) - elif err_no == 0: - log.log(_LEVEL_MAP[err_class], "%s", msg.decode("utf-8")) - else: - log.log(_LEVEL_MAP[err_class], "%s:%s", _CODE_MAP[err_no], msg.decode("utf-8")) - else: - log.info("Unknown error number %r", err_no) - - IF UNAME_SYSNAME == "Windows": cdef void __stdcall stacking_error_handler( CPLErr err_class, @@ -442,7 +402,6 @@ IF UNAME_SYSNAME == "Windows": Warnings are converted to Python warnings. """ global _ERROR_STACK - log_error(err_class, err_no, err_msg) if err_class == CE_Fatal: # If the error class is CE_Fatal, we want to have a message issued @@ -488,7 +447,6 @@ ELSE: Warnings are converted to Python warnings. """ global _ERROR_STACK - log_error(err_class, err_no, err_msg) if err_class == CE_Fatal: # If the error class is CE_Fatal, we want to have a message issued From a760ab676290bb3c937fa5a65085c3d16d647cdd Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 20:47:54 +0100 Subject: [PATCH 06/27] Remove __stdcall on windows as it is not necessary --- pyogrio/_err.pyx | 112 ++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 79 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 9c804c2a..035ba905 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -387,95 +387,49 @@ cdef class StackChecker: return ptr -IF UNAME_SYSNAME == "Windows": - cdef void __stdcall stacking_error_handler( - CPLErr err_class, - int err_no, - const char* err_msg - ) noexcept with gil: - """Custom CPL error handler that adds non-fatal errors to a stack. - - All non-fatal errors (CE_Failure) are not printed to stderr (behaviour - of the default GDAL error handler), but they are converted to python - exceptions and added to a stack, so they can be dealt with afterwards. - - Warnings are converted to Python warnings. - """ - global _ERROR_STACK - - if err_class == CE_Fatal: - # If the error class is CE_Fatal, we want to have a message issued - # because the CPL support code does an abort() before any exception - # can be generated - CPLDefaultErrorHandler(err_class, err_no, err_msg) - - return - - elif err_class == CE_Failure: - # For Failures, add them to the error exception stack - stack = _ERROR_STACK.get() - stack.append( - exception_map.get(err_no, CPLE_BaseError)( - err_class, err_no, clean_error_message(err_msg) - ), - ) - _ERROR_STACK.set(stack) +cdef void stacking_error_handler( + CPLErr err_class, + int err_no, + const char* err_msg +) noexcept with gil: + """Custom CPL error handler that adds non-fatal errors to a stack. - return + All non-fatal errors (CE_Failure) are not printed to stderr (behaviour + of the default GDAL error handler), but they are converted to python + exceptions and added to a stack, so they can be dealt with afterwards. - elif err_class == CE_Warning: - warnings.warn(clean_error_message(err_msg), RuntimeWarning) - - return + Warnings are converted to Python warnings. + """ + global _ERROR_STACK - # Fall back to the default handler for non-failure messages since - # they won't be translated into exceptions. + if err_class == CE_Fatal: + # If the error class is CE_Fatal, we want to have a message issued + # because the CPL support code does an abort() before any exception + # can be generated CPLDefaultErrorHandler(err_class, err_no, err_msg) -ELSE: - cdef void stacking_error_handler( - CPLErr err_class, - int err_no, - const char* err_msg - ) noexcept with gil: - """Custom CPL error handler that adds non-fatal errors to a stack. - - All non-fatal errors (CE_Failure) are not printed to stderr (behaviour - of the default GDAL error handler), but they are converted to python - exceptions and added to a stack, so they can be dealt with afterwards. - - Warnings are converted to Python warnings. - """ - global _ERROR_STACK - - if err_class == CE_Fatal: - # If the error class is CE_Fatal, we want to have a message issued - # because the CPL support code does an abort() before any exception - # can be generated - CPLDefaultErrorHandler(err_class, err_no, err_msg) - - return + return - elif err_class == CE_Failure: - # For Failures, add them to the error exception stack - stack = _ERROR_STACK.get() - stack.append( - exception_map.get(err_no, CPLE_BaseError)( - err_class, err_no, clean_error_message(err_msg) - ), - ) - _ERROR_STACK.set(stack) + elif err_class == CE_Failure: + # For Failures, add them to the error exception stack + stack = _ERROR_STACK.get() + stack.append( + exception_map.get(err_no, CPLE_BaseError)( + err_class, err_no, clean_error_message(err_msg) + ), + ) + _ERROR_STACK.set(stack) - return + return - elif err_class == CE_Warning: - warnings.warn(clean_error_message(err_msg), RuntimeWarning) + elif err_class == CE_Warning: + warnings.warn(clean_error_message(err_msg), RuntimeWarning) - return + return - # Fall back to the default handler for non-failure messages since - # they won't be translated into exceptions. - CPLDefaultErrorHandler(err_class, err_no, err_msg) + # Fall back to the default handler for non-failure messages since + # they won't be translated into exceptions. + CPLDefaultErrorHandler(err_class, err_no, err_msg) @contextlib.contextmanager From 1c2ad34f833a675fbde26a6f35b3876002e5d589 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 21:47:09 +0100 Subject: [PATCH 07/27] Rename new error handling class, context manager --- pyogrio/_err.pxd | 2 +- pyogrio/_err.pyx | 14 ++++++++------ pyogrio/_io.pyx | 10 +++++----- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index 7996d578..54943542 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -3,7 +3,7 @@ cdef int exc_wrap_int(int retval) except -1 cdef int exc_wrap_ogrerr(int retval) except -1 cdef void *exc_wrap_pointer(void *ptr) except NULL -cdef class StackChecker: +cdef class ErrorHandler: cdef object error_stack cdef int exc_wrap_int(self, int retval) except -1 cdef void *exc_wrap_pointer(self, void *ptr) except NULL diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 035ba905..a694ce63 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -343,7 +343,7 @@ def _register_error_handler(): cpl_errs = GDALErrCtxManager() -cdef class StackChecker: +cdef class ErrorHandler: def __init__(self, error_stack=None): self.error_stack = error_stack or {} @@ -352,7 +352,8 @@ cdef class StackChecker: """Wrap a GDAL/OGR function that returns CPLErr (int). Raises an exception if a non-fatal error has been added to the - exception stack. + exception stack. Only checks for errors if `err` is set to a nonzero + value. """ if err: stack = self.error_stack.get() @@ -371,7 +372,7 @@ cdef class StackChecker: """Wrap a GDAL/OGR function that returns a pointer. Raises an exception if a non-fatal error has been added to the - exception stack. + exception stack. Only checks for errors if `ptr` is `NULL`. """ if ptr == NULL: stack = self.error_stack.get() @@ -433,13 +434,14 @@ cdef void stacking_error_handler( @contextlib.contextmanager -def stack_errors(): +def capture_errors(): """A context manager that captures all GDAL non-fatal errors occuring. It adds all errors to a single stack, so it assumes that no more than one GDAL function is called. - Yields a StackChecker object that can be used to check the error stack. + Yields an ErrorHandler object that can be used to handle the errors + if any were captured. """ CPLErrorReset() global _ERROR_STACK @@ -450,7 +452,7 @@ def stack_errors(): CPLPushErrorHandlerEx(stacking_error_handler, NULL) # Run code in the `with` block. - yield StackChecker(_ERROR_STACK) + yield ErrorHandler(_ERROR_STACK) CPLPopErrorHandler() _ERROR_STACK.set([]) diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index 83be5362..1f1a6add 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -26,7 +26,7 @@ import numpy as np from pyogrio._ogr cimport * from pyogrio._err cimport ( - exc_check, exc_wrap_int, exc_wrap_ogrerr, exc_wrap_pointer, StackChecker + exc_check, exc_wrap_int, exc_wrap_ogrerr, exc_wrap_pointer, ErrorHandler ) from pyogrio._vsi cimport * from pyogrio._err import ( @@ -35,7 +35,7 @@ from pyogrio._err import ( CPLE_NotSupportedError, CPLE_OpenFailedError, NullPointerError, - stack_errors, + capture_errors, ) from pyogrio._geometry cimport get_geometry_type, get_geometry_type_code from pyogrio.errors import CRSError, DataSourceError, DataLayerError, GeometryError, FieldError, FeatureError @@ -194,7 +194,7 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: dataset open options """ cdef void *ogr_dataset = NULL - cdef StackChecker error_stack_checker + cdef ErrorHandler errors # Force linear approximations in all cases OGRSetNonLinearGeometriesEnabledFlag(0) @@ -208,9 +208,9 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: try: # WARNING: GDAL logs warnings about invalid open options to stderr # instead of raising an error - with stack_errors() as error_stack_checker: + with capture_errors() as errors: ogr_dataset = GDALOpenEx(path_c, flags, NULL, options, NULL) - return error_stack_checker.exc_wrap_pointer(ogr_dataset) + return errors.exc_wrap_pointer(ogr_dataset) except NullPointerError: raise DataSourceError( From 135fa808b7844ff6381344abe74b0f2fe957d1e4 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 22:42:20 +0100 Subject: [PATCH 08/27] Rename exc_wrap_* functions to check_* --- pyogrio/_err.pxd | 12 +++---- pyogrio/_err.pyx | 74 +++++++++++++++++++++++++++---------------- pyogrio/_geometry.pyx | 2 +- pyogrio/_io.pyx | 56 ++++++++++++++++---------------- pyogrio/_ogr.pyx | 6 ++-- 5 files changed, 85 insertions(+), 65 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index 54943542..c3890613 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -1,9 +1,9 @@ -cdef object exc_check() -cdef int exc_wrap_int(int retval) except -1 -cdef int exc_wrap_ogrerr(int retval) except -1 -cdef void *exc_wrap_pointer(void *ptr) except NULL +cdef object check_last_error() +cdef int check_int(int retval) except -1 +cdef int check_ogrerr(int retval) except -1 +cdef void *check_pointer(void *ptr) except NULL cdef class ErrorHandler: cdef object error_stack - cdef int exc_wrap_int(self, int retval) except -1 - cdef void *exc_wrap_pointer(self, void *ptr) except NULL + cdef int check_int(self, int retval) except -1 + cdef void *check_pointer(self, void *ptr) except NULL diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index a694ce63..b003dc8f 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -125,7 +125,7 @@ class CPLE_AWSError(CPLE_BaseError): class NullPointerError(CPLE_BaseError): """ - Returned from exc_wrap_pointer when a NULL pointer is passed, but no GDAL + Returned from check_pointer when a NULL pointer is passed, but no GDAL error was raised. """ pass @@ -133,7 +133,7 @@ class NullPointerError(CPLE_BaseError): class CPLError(CPLE_BaseError): """ - Returned from exc_wrap_int when a error code is returned, but no GDAL + Returned from check_int when a error code is returned, but no GDAL error was set. """ pass @@ -201,8 +201,12 @@ cdef class GDALErrCtxManager: raise exception_map[err_no](err_type, err_no, msg) -cdef inline object exc_check(): - """Checks GDAL error stack for fatal or non-fatal errors. +cdef inline object check_last_error(): + """Checks if the last GDAL error was a fatal or non-fatal error. + + When a non-fatal error is found, an appropriate exception is raised. + + When a fatal error is found, SystemExit is called. Returns ------- @@ -264,13 +268,16 @@ cdef clean_error_message(const char* err_msg): return msg -cdef void *exc_wrap_pointer(void *ptr) except NULL: - """Wrap a GDAL/OGR function that returns GDALDatasetH etc (void *). +cdef void *check_pointer(void *ptr) except NULL: + """Check the pointer returned by a GDAL/OGR function. - Raises an exception if a non-fatal error has be set or if pointer is NULL. + If `ptr` is `NULL`, an exception inheriting from CPLE_BaseError is raised. + When the last error registered by GDAL/OGR was a non-fatal error, the + exception raised will be customized appropriately. Otherwise a + NullPointerError is raised. """ if ptr == NULL: - exc = exc_check() + exc = check_last_error() if exc: raise exc else: @@ -279,29 +286,33 @@ cdef void *exc_wrap_pointer(void *ptr) except NULL: return ptr -cdef int exc_wrap_int(int err) except -1: - """Wrap a GDAL/OGR function that returns CPLErr or OGRErr (int). +cdef int check_int(int err) except -1: + """Check the CPLErr (int) value returned by a GDAL/OGR function. - Raises an exception if a non-fatal error has be set. + If `err` is a nonzero value, an exception inheriting from CPLE_BaseError is + raised. + When the last error registered by GDAL/OGR was a non-fatal error, the + exception raised will be customized appropriately. Otherwise a CPLError is + raised. """ if err: - exc = exc_check() + exc = check_last_error() if exc: raise exc else: # no error message from GDAL - raise CPLE_BaseError(-1, -1, "Unspecified OGR / GDAL error") + raise CPLError(-1, -1, "Unspecified OGR / GDAL error") + return err -cdef int exc_wrap_ogrerr(int err) except -1: - """Wrap a function that returns OGRErr (int) but does not use the - CPL error stack. +cdef int check_ogrerr(int err) except -1: + """Check the OGRErr (int) value returned by a GDAL/OGR function. - Adapted from Fiona (_err.pyx). + If `err` is a nonzero value, a CPLE_BaseError is raised. """ if err != 0: - raise CPLE_BaseError(3, err, f"OGR Error code {err}") + raise CPLE_BaseError(CE_Failure, err, f"OGR Error code {err}") return err @@ -348,12 +359,14 @@ cdef class ErrorHandler: def __init__(self, error_stack=None): self.error_stack = error_stack or {} - cdef int exc_wrap_int(self, int err) except -1: - """Wrap a GDAL/OGR function that returns CPLErr (int). + cdef int check_int(self, int err) except -1: + """Check the CPLErr (int) value returned by a GDAL/OGR function. - Raises an exception if a non-fatal error has been added to the - exception stack. Only checks for errors if `err` is set to a nonzero - value. + If `err` is a nonzero value, an exception inheriting from + CPLE_BaseError is raised. + When a non-fatal GDAL/OGR error was captured in the error stack, the + exception raised will be customized appropriately. Otherwise, a + CPLError is raised. """ if err: stack = self.error_stack.get() @@ -365,14 +378,19 @@ cdef class ErrorHandler: last = stack.pop() if last is not None: raise last + else: + raise CPLError(CE_Failure, err, "Unspecified OGR / GDAL error") return err - cdef void *exc_wrap_pointer(self, void *ptr) except NULL: - """Wrap a GDAL/OGR function that returns a pointer. + cdef void *check_pointer(self, void *ptr) except NULL: + """Check the pointer returned by a GDAL/OGR function. - Raises an exception if a non-fatal error has been added to the - exception stack. Only checks for errors if `ptr` is `NULL`. + If `ptr` is `NULL`, an exception inheriting from CPLE_BaseError is + raised. + When a non-fatal GDAL/OGR error was captured in the error stack, the + exception raised will be customized appropriately. Otherwise, a + NullPointerError is raised. """ if ptr == NULL: stack = self.error_stack.get() @@ -384,6 +402,8 @@ cdef class ErrorHandler: last = stack.pop() if last is not None: raise last + else: + raise NullPointerError(-1, -1, "NULL pointer error") return ptr diff --git a/pyogrio/_geometry.pyx b/pyogrio/_geometry.pyx index b41a6640..95706df4 100644 --- a/pyogrio/_geometry.pyx +++ b/pyogrio/_geometry.pyx @@ -84,7 +84,7 @@ cdef str get_geometry_type(void *ogr_layer): cdef OGRwkbGeometryType ogr_type try: - ogr_featuredef = exc_wrap_pointer(OGR_L_GetLayerDefn(ogr_layer)) + ogr_featuredef = check_pointer(OGR_L_GetLayerDefn(ogr_layer)) except NullPointerError: raise DataLayerError("Could not get layer definition") diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index 1f1a6add..c08eafe6 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -26,7 +26,7 @@ import numpy as np from pyogrio._ogr cimport * from pyogrio._err cimport ( - exc_check, exc_wrap_int, exc_wrap_ogrerr, exc_wrap_pointer, ErrorHandler + check_last_error, check_int, check_ogrerr, check_pointer, ErrorHandler ) from pyogrio._vsi cimport * from pyogrio._err import ( @@ -210,7 +210,7 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: # instead of raising an error with capture_errors() as errors: ogr_dataset = GDALOpenEx(path_c, flags, NULL, options, NULL) - return errors.exc_wrap_pointer(ogr_dataset) + return errors.check_pointer(ogr_dataset) except NullPointerError: raise DataSourceError( @@ -241,7 +241,7 @@ cdef ogr_close(GDALDatasetH ogr_dataset): if ogr_dataset != NULL: IF CTE_GDAL_VERSION >= (3, 7, 0): if GDALClose(ogr_dataset) != CE_None: - return exc_check() + return check_last_error() return @@ -250,7 +250,7 @@ cdef ogr_close(GDALDatasetH ogr_dataset): # GDAL will set an error if there was an error writing the data source # on close - return exc_check() + return check_last_error() cdef OGRLayerH get_ogr_layer(GDALDatasetH ogr_dataset, layer) except NULL: @@ -272,10 +272,10 @@ cdef OGRLayerH get_ogr_layer(GDALDatasetH ogr_dataset, layer) except NULL: if isinstance(layer, str): name_b = layer.encode('utf-8') name_c = name_b - ogr_layer = exc_wrap_pointer(GDALDatasetGetLayerByName(ogr_dataset, name_c)) + ogr_layer = check_pointer(GDALDatasetGetLayerByName(ogr_dataset, name_c)) elif isinstance(layer, int): - ogr_layer = exc_wrap_pointer(GDALDatasetGetLayer(ogr_dataset, layer)) + ogr_layer = check_pointer(GDALDatasetGetLayer(ogr_dataset, layer)) # GDAL does not always raise exception messages in this case except NullPointerError: @@ -318,11 +318,11 @@ cdef OGRLayerH execute_sql(GDALDatasetH ogr_dataset, str sql, str sql_dialect=No sql_b = sql.encode('utf-8') sql_c = sql_b if sql_dialect is None: - return exc_wrap_pointer(GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL)) + return check_pointer(GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, NULL)) sql_dialect_b = sql_dialect.encode('utf-8') sql_dialect_c = sql_dialect_b - return exc_wrap_pointer(GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, sql_dialect_c)) + return check_pointer(GDALDatasetExecuteSQL(ogr_dataset, sql_c, NULL, sql_dialect_c)) # GDAL does not always raise exception messages in this case except NullPointerError: @@ -350,7 +350,7 @@ cdef str get_crs(OGRLayerH ogr_layer): cdef char *ogr_wkt = NULL try: - ogr_crs = exc_wrap_pointer(OGR_L_GetSpatialRef(ogr_layer)) + ogr_crs = check_pointer(OGR_L_GetSpatialRef(ogr_layer)) except NullPointerError: # No coordinate system defined. @@ -397,7 +397,7 @@ cdef get_driver(OGRDataSourceH ogr_dataset): cdef void *ogr_driver try: - ogr_driver = exc_wrap_pointer(GDALGetDatasetDriver(ogr_dataset)) + ogr_driver = check_pointer(GDALGetDatasetDriver(ogr_dataset)) except NullPointerError: raise DataLayerError(f"Could not detect driver of dataset") from None @@ -440,7 +440,7 @@ cdef get_feature_count(OGRLayerH ogr_layer, int force): feature_count = 0 while True: try: - ogr_feature = exc_wrap_pointer(OGR_L_GetNextFeature(ogr_layer)) + ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) feature_count +=1 except NullPointerError: @@ -484,7 +484,7 @@ cdef get_total_bounds(OGRLayerH ogr_layer, int force): cdef OGREnvelope ogr_envelope try: - exc_wrap_ogrerr(OGR_L_GetExtent(ogr_layer, &ogr_envelope, force)) + check_ogrerr(OGR_L_GetExtent(ogr_layer, &ogr_envelope, force)) bounds = ( ogr_envelope.MinX, ogr_envelope.MinY, ogr_envelope.MaxX, ogr_envelope.MaxY ) @@ -635,7 +635,7 @@ cdef get_fields(OGRLayerH ogr_layer, str encoding, use_arrow=False): cdef const char *key_c try: - ogr_featuredef = exc_wrap_pointer(OGR_L_GetLayerDefn(ogr_layer)) + ogr_featuredef = check_pointer(OGR_L_GetLayerDefn(ogr_layer)) except NullPointerError: raise DataLayerError("Could not get layer definition") from None @@ -652,7 +652,7 @@ cdef get_fields(OGRLayerH ogr_layer, str encoding, use_arrow=False): for i in range(field_count): try: - ogr_fielddef = exc_wrap_pointer(OGR_FD_GetFieldDefn(ogr_featuredef, i)) + ogr_fielddef = check_pointer(OGR_FD_GetFieldDefn(ogr_featuredef, i)) except NullPointerError: raise FieldError(f"Could not get field definition for field at index {i}") from None @@ -714,7 +714,7 @@ cdef apply_where_filter(OGRLayerH ogr_layer, str where): # logs to stderr if err != OGRERR_NONE: try: - exc_check() + check_last_error() except CPLE_BaseError as exc: raise ValueError(str(exc)) @@ -987,7 +987,7 @@ cdef get_features( break try: - ogr_feature = exc_wrap_pointer(OGR_L_GetNextFeature(ogr_layer)) + ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) except NullPointerError: # No more rows available, so stop reading @@ -1081,7 +1081,7 @@ cdef get_features_by_fid( fid = fids[i] try: - ogr_feature = exc_wrap_pointer(OGR_L_GetFeature(ogr_layer, fid)) + ogr_feature = check_pointer(OGR_L_GetFeature(ogr_layer, fid)) except NullPointerError: raise FeatureError(f"Could not read feature with fid {fid}") from None @@ -1136,7 +1136,7 @@ cdef get_bounds( break try: - ogr_feature = exc_wrap_pointer(OGR_L_GetNextFeature(ogr_layer)) + ogr_feature = check_pointer(OGR_L_GetNextFeature(ogr_layer)) except NullPointerError: # No more rows available, so stop reading @@ -1938,7 +1938,7 @@ cdef void * ogr_create(const char* path_c, const char* driver_c, char** options) # Get the driver try: - ogr_driver = exc_wrap_pointer(GDALGetDriverByName(driver_c)) + ogr_driver = check_pointer(GDALGetDriverByName(driver_c)) except NullPointerError: raise DataSourceError(f"Could not obtain driver: {driver_c.decode('utf-8')} (check that it was installed correctly into GDAL)") @@ -1958,7 +1958,7 @@ cdef void * ogr_create(const char* path_c, const char* driver_c, char** options) # Create the dataset try: - ogr_dataset = exc_wrap_pointer(GDALCreate(ogr_driver, path_c, 0, 0, 0, GDT_Unknown, options)) + ogr_dataset = check_pointer(GDALCreate(ogr_driver, path_c, 0, 0, 0, GDT_Unknown, options)) except NullPointerError: raise DataSourceError(f"Failed to create dataset with driver: {path_c.decode('utf-8')} {driver_c.decode('utf-8')}") from None @@ -1980,7 +1980,7 @@ cdef void * create_crs(str crs) except NULL: crs_c = crs_b try: - ogr_crs = exc_wrap_pointer(OSRNewSpatialReference(NULL)) + ogr_crs = check_pointer(OSRNewSpatialReference(NULL)) err = OSRSetFromUserInput(ogr_crs, crs_c) if err: raise CRSError("Could not set CRS: {}".format(crs_c.decode('UTF-8'))) from None @@ -2204,12 +2204,12 @@ cdef create_ogr_dataset_layer( layer_b = layer.encode('UTF-8') layer_c = layer_b - ogr_layer = exc_wrap_pointer( + ogr_layer = check_pointer( GDALDatasetCreateLayer(ogr_dataset, layer_c, ogr_crs, geometry_code, layer_options)) else: - ogr_layer = exc_wrap_pointer(get_ogr_layer(ogr_dataset, layer)) + ogr_layer = check_pointer(get_ogr_layer(ogr_dataset, layer)) # Set dataset and layer metadata set_metadata(ogr_dataset, dataset_metadata) @@ -2348,7 +2348,7 @@ def ogr_write( name_b = fields[i].encode(encoding) try: - ogr_fielddef = exc_wrap_pointer(OGR_Fld_Create(name_b, field_type)) + ogr_fielddef = check_pointer(OGR_Fld_Create(name_b, field_type)) # subtypes, see: https://gdal.org/development/rfc/rfc50_ogr_field_subtype.html if field_subtype != OFSTNone: @@ -2359,7 +2359,7 @@ def ogr_write( # TODO: set precision - exc_wrap_int(OGR_L_CreateField(ogr_layer, ogr_fielddef, 1)) + check_int(OGR_L_CreateField(ogr_layer, ogr_fielddef, 1)) except: raise FieldError(f"Error adding field '{fields[i]}' to layer") from None @@ -2503,7 +2503,7 @@ def ogr_write( # Add feature to the layer try: - exc_wrap_int(OGR_L_CreateFeature(ogr_layer, ogr_feature)) + check_int(OGR_L_CreateFeature(ogr_layer, ogr_feature)) except CPLE_BaseError as exc: raise FeatureError(f"Could not add feature to layer at index {i}: {exc}") from None @@ -2629,7 +2629,7 @@ def ogr_write_arrow( break if not OGR_L_WriteArrowBatch(ogr_layer, &schema, &array, options): - exc = exc_check() + exc = check_last_error() gdal_msg = f": {str(exc)}" if exc else "." raise DataLayerError( f"Error while writing batch to OGR layer{gdal_msg}" @@ -2756,7 +2756,7 @@ cdef create_fields_from_arrow_schema( continue if not OGR_L_CreateFieldFromArrowSchema(destLayer, child, options): - exc = exc_check() + exc = check_last_error() gdal_msg = f" ({str(exc)})" if exc else "" raise FieldError( f"Error while creating field from Arrow for field {i} with name " diff --git a/pyogrio/_ogr.pyx b/pyogrio/_ogr.pyx index 1c2d5f64..e2ae7ff5 100644 --- a/pyogrio/_ogr.pyx +++ b/pyogrio/_ogr.pyx @@ -3,7 +3,7 @@ import sys from uuid import uuid4 import warnings -from pyogrio._err cimport exc_wrap_int, exc_wrap_ogrerr, exc_wrap_pointer +from pyogrio._err cimport check_int, check_ogrerr, check_pointer from pyogrio._err import CPLE_BaseError, NullPointerError from pyogrio.errors import DataSourceError @@ -183,7 +183,7 @@ def has_proj_data(): cdef OGRSpatialReferenceH srs = OSRNewSpatialReference(NULL) try: - exc_wrap_ogrerr(exc_wrap_int(OSRImportFromEPSG(srs, 4326))) + check_ogrerr(check_int(OSRImportFromEPSG(srs, 4326))) except CPLE_BaseError: return False else: @@ -282,7 +282,7 @@ def _get_driver_metadata_item(driver, metadata_item): cdef void *cogr_driver = NULL try: - cogr_driver = exc_wrap_pointer(GDALGetDriverByName(driver.encode('UTF-8'))) + cogr_driver = check_pointer(GDALGetDriverByName(driver.encode('UTF-8'))) except NullPointerError: raise DataSourceError( f"Could not obtain driver: {driver} (check that it was installed " From ca423060ddbcd514e20b5cbe4fa6a855325a8bab Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 22:43:20 +0100 Subject: [PATCH 09/27] Remove unneeded import logging --- pyogrio/_err.pyx | 1 - 1 file changed, 1 deletion(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index b003dc8f..6eef962f 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -4,7 +4,6 @@ Ported from fiona::_err.pyx """ import contextlib -import logging import warnings from contextvars import ContextVar from enum import IntEnum From ad91d2e080d3ed9a7ec14c0da1182c0fdeb508a9 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Tue, 12 Nov 2024 23:00:01 +0100 Subject: [PATCH 10/27] Remove unused GDALErrCtxManager --- pyogrio/_err.pyx | 39 --------------------------------------- 1 file changed, 39 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 6eef962f..a387d726 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -162,43 +162,6 @@ exception_map = { 17: CPLE_AWSError } -cdef dict _CODE_MAP = { - 0: 'CPLE_None', - 1: 'CPLE_AppDefined', - 2: 'CPLE_OutOfMemory', - 3: 'CPLE_FileIO', - 4: 'CPLE_OpenFailed', - 5: 'CPLE_IllegalArg', - 6: 'CPLE_NotSupported', - 7: 'CPLE_AssertionFailed', - 8: 'CPLE_NoWriteAccess', - 9: 'CPLE_UserInterrupt', - 10: 'ObjectNull', - 11: 'CPLE_HttpResponse', - 12: 'CPLE_AWSBucketNotFound', - 13: 'CPLE_AWSObjectNotFound', - 14: 'CPLE_AWSAccessDenied', - 15: 'CPLE_AWSInvalidCredentials', - 16: 'CPLE_AWSSignatureDoesNotMatch', - 17: 'CPLE_AWSError' -} - - -cdef class GDALErrCtxManager: - """A manager for GDAL error handling contexts.""" - - def __enter__(self): - CPLErrorReset() - return self - - def __exit__(self, exc_type=None, exc_val=None, exc_tb=None): - cdef int err_type = CPLGetLastErrorType() - cdef int err_no = CPLGetLastErrorNo() - cdef const char *msg = CPLGetLastErrorMsg() - # TODO: warn for err_type 2? - if err_type >= 2: - raise exception_map[err_no](err_type, err_no, msg) - cdef inline object check_last_error(): """Checks if the last GDAL error was a fatal or non-fatal error. @@ -350,8 +313,6 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) noexc def _register_error_handler(): CPLPushErrorHandler(error_handler) -cpl_errs = GDALErrCtxManager() - cdef class ErrorHandler: From 2a30dd13a44f8a04c0695588311139fa0e495a46 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 13 Nov 2024 07:57:38 +0100 Subject: [PATCH 11/27] Remove unused GDALError enum --- pyogrio/_err.pyx | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index a387d726..9073373a 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -19,19 +19,6 @@ _ERROR_STACK = ContextVar("error_stack") _ERROR_STACK.set([]) -# CPL Error types as an enum. -class GDALError(IntEnum): - """GDAL error types/classes. - - GDAL doc: https://gdal.org/en/latest/doxygen/cpl__error_8h.html#a463ba7c7202a505416ff95b1aeefa2de - """ - none = CE_None - debug = CE_Debug - warning = CE_Warning - failure = CE_Failure - fatal = CE_Fatal - - class CPLE_BaseError(Exception): """Base CPL error class. From abe8577bef403d2847dc1afdd93d4c1e14c6782d Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 13 Nov 2024 08:00:04 +0100 Subject: [PATCH 12/27] Remove obsolete import --- pyogrio/_err.pyx | 1 - 1 file changed, 1 deletion(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 9073373a..910d2b78 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -6,7 +6,6 @@ Ported from fiona::_err.pyx import contextlib import warnings from contextvars import ContextVar -from enum import IntEnum from itertools import zip_longest from pyogrio._ogr cimport ( From 160ddc949cadffa9a1220a47458e4e25287a42ab Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 13 Nov 2024 08:27:13 +0100 Subject: [PATCH 13/27] Use nogil when no error is logged in stacking_error_handler --- pyogrio/_err.pyx | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 910d2b78..a05b5fe5 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -358,7 +358,7 @@ cdef void stacking_error_handler( CPLErr err_class, int err_no, const char* err_msg -) noexcept with gil: +) noexcept nogil: """Custom CPL error handler that adds non-fatal errors to a stack. All non-fatal errors (CE_Failure) are not printed to stderr (behaviour @@ -367,31 +367,30 @@ cdef void stacking_error_handler( Warnings are converted to Python warnings. """ - global _ERROR_STACK - if err_class == CE_Fatal: # If the error class is CE_Fatal, we want to have a message issued # because the CPL support code does an abort() before any exception # can be generated CPLDefaultErrorHandler(err_class, err_no, err_msg) - return elif err_class == CE_Failure: # For Failures, add them to the error exception stack - stack = _ERROR_STACK.get() - stack.append( - exception_map.get(err_no, CPLE_BaseError)( - err_class, err_no, clean_error_message(err_msg) - ), - ) - _ERROR_STACK.set(stack) + with gil: + global _ERROR_STACK + stack = _ERROR_STACK.get() + stack.append( + exception_map.get(err_no, CPLE_BaseError)( + err_class, err_no, clean_error_message(err_msg) + ), + ) + _ERROR_STACK.set(stack) return elif err_class == CE_Warning: - warnings.warn(clean_error_message(err_msg), RuntimeWarning) - + with gil: + warnings.warn(clean_error_message(err_msg), RuntimeWarning) return # Fall back to the default handler for non-failure messages since From 52032445897ad528f47681685a8a40bd094cda3a Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 11:41:48 +0100 Subject: [PATCH 14/27] Feedback: use CPLPushErrorHandler istead of Ex variant --- pyogrio/_err.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index a05b5fe5..96f726e5 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -414,7 +414,7 @@ def capture_errors(): # stacking_error_handler records GDAL errors in the order they occur and # converts them to exceptions. - CPLPushErrorHandlerEx(stacking_error_handler, NULL) + CPLPushErrorHandler(stacking_error_handler) # Run code in the `with` block. yield ErrorHandler(_ERROR_STACK) From f76fc566aba7409cf256351175f6b3919c34745c Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 12:47:15 +0100 Subject: [PATCH 15/27] Move exception squashing to _err.pyx --- pyogrio/_err.pxd | 4 ++-- pyogrio/_err.pyx | 55 ++++++++++++++++++++++++++++++++++++++++++++++-- pyogrio/_io.pyx | 15 ++++--------- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index c3890613..927e3c9e 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -5,5 +5,5 @@ cdef void *check_pointer(void *ptr) except NULL cdef class ErrorHandler: cdef object error_stack - cdef int check_int(self, int retval) except -1 - cdef void *check_pointer(self, void *ptr) except NULL + cdef int check_int(self, int retval, bint squash_errors) except -1 + cdef void *check_pointer(self, void *ptr, bint squash_errors) except NULL diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 96f726e5..d25053f1 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -305,7 +305,7 @@ cdef class ErrorHandler: def __init__(self, error_stack=None): self.error_stack = error_stack or {} - cdef int check_int(self, int err) except -1: + cdef int check_int(self, int err, bint squash_errors) except -1: """Check the CPLErr (int) value returned by a GDAL/OGR function. If `err` is a nonzero value, an exception inheriting from @@ -313,6 +313,20 @@ cdef class ErrorHandler: When a non-fatal GDAL/OGR error was captured in the error stack, the exception raised will be customized appropriately. Otherwise, a CPLError is raised. + + Parameters + ---------- + err : int + The CPLErr returned by a GDAL/OGR function. + squash_errors : bool + True to squash all errors captured to one error with the exception type of + the last error and all error messages concatenated. + + Returns + ------- + int + The `err` input parameter if it is zero. Otherwise an exception is raised. + """ if err: stack = self.error_stack.get() @@ -323,13 +337,24 @@ cdef class ErrorHandler: if stack: last = stack.pop() if last is not None: + if squash_errors: + # Concatenate all error messages, and raise a single exception + errmsg = str(last) + inner = last.__cause__ + while inner is not None: + errmsg = f"{errmsg}; {inner}" + inner = inner.__cause__ + + raise type(last)(-1, -1, errmsg) + raise last + else: raise CPLError(CE_Failure, err, "Unspecified OGR / GDAL error") return err - cdef void *check_pointer(self, void *ptr) except NULL: + cdef void *check_pointer(self, void *ptr, bint squash_errors) except NULL: """Check the pointer returned by a GDAL/OGR function. If `ptr` is `NULL`, an exception inheriting from CPLE_BaseError is @@ -337,6 +362,21 @@ cdef class ErrorHandler: When a non-fatal GDAL/OGR error was captured in the error stack, the exception raised will be customized appropriately. Otherwise, a NullPointerError is raised. + + Parameters + ---------- + ptr : pointer + The pointer returned by a GDAL/OGR function. + squash_errors : bool + True to squash all errors captured to one error with the exception type of + the last error and all error messages concatenated. + + Returns + ------- + pointer + The `ptr` input parameter if it is not `NULL`. Otherwise an exception is + raised. + """ if ptr == NULL: stack = self.error_stack.get() @@ -347,7 +387,18 @@ cdef class ErrorHandler: if stack: last = stack.pop() if last is not None: + if squash_errors: + # Concatenate all error messages, and raise a single exception + errmsg = str(last) + inner = last.__cause__ + while inner is not None: + errmsg = f"{errmsg}; {inner}" + inner = inner.__cause__ + + raise type(last)(-1, -1, errmsg) + raise last + else: raise NullPointerError(-1, -1, "NULL pointer error") diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index c08eafe6..b71cd129 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -210,7 +210,7 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: # instead of raising an error with capture_errors() as errors: ogr_dataset = GDALOpenEx(path_c, flags, NULL, options, NULL) - return errors.check_pointer(ogr_dataset) + return errors.check_pointer(ogr_dataset, True) except NullPointerError: raise DataSourceError( @@ -218,20 +218,13 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: ) from None except CPLE_BaseError as exc: - # If there are inner exceptions, append their errmsg to the errmsg - errmsg = str(exc) - inner = exc.__cause__ - while inner is not None: - errmsg = f"{errmsg}; {inner}" - inner = inner.__cause__ - - if exc.errmsg.endswith("a supported file format."): + if " not recognized as a supported file format." in exc.errmsg: raise DataSourceError( - f"{errmsg}; It might help to specify the correct driver explicitly by " + f"{exc.errmsg}; It might help to specify the correct driver explicitly by " "prefixing the file path with ':', e.g. 'CSV:path'." ) from None - raise DataSourceError(errmsg) from None + raise DataSourceError(exc.errmsg) from None cdef ogr_close(GDALDatasetH ogr_dataset): From 26fefecc567c63b25ad61feae18ac5f5bacde54e Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 12:55:44 +0100 Subject: [PATCH 16/27] Remove global _ERROR_STACK declarations --- pyogrio/_err.pyx | 2 -- 1 file changed, 2 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index d25053f1..84465367 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -428,7 +428,6 @@ cdef void stacking_error_handler( elif err_class == CE_Failure: # For Failures, add them to the error exception stack with gil: - global _ERROR_STACK stack = _ERROR_STACK.get() stack.append( exception_map.get(err_no, CPLE_BaseError)( @@ -460,7 +459,6 @@ def capture_errors(): if any were captured. """ CPLErrorReset() - global _ERROR_STACK _ERROR_STACK.set([]) # stacking_error_handler records GDAL errors in the order they occur and From 0b85d344c9c5d5198320c76416228b1cc3291153 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 13:07:58 +0100 Subject: [PATCH 17/27] Fix tests again --- pyogrio/_io.pyx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index b71cd129..d2fae0d8 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -218,7 +218,9 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: ) from None except CPLE_BaseError as exc: - if " not recognized as a supported file format." in exc.errmsg: + if " a supported file format." in exc.errmsg: + # In gdal 3.9, this error message was slightly changed, so we can only check + # on this part of the error message. raise DataSourceError( f"{exc.errmsg}; It might help to specify the correct driver explicitly by " "prefixing the file path with ':', e.g. 'CSV:path'." From 723ce9af6ede525000a58021f030e43b5c728a4f Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 13:53:07 +0100 Subject: [PATCH 18/27] Remove check_ogrerr and use OGRERR_NONE where applicable --- pyogrio/_err.pxd | 1 - pyogrio/_err.pyx | 27 ++++++++------------------- pyogrio/_io.pyx | 9 ++++----- pyogrio/_ogr.pyx | 18 +++++++++--------- 4 files changed, 21 insertions(+), 34 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index 927e3c9e..71119e5e 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -1,6 +1,5 @@ cdef object check_last_error() cdef int check_int(int retval) except -1 -cdef int check_ogrerr(int retval) except -1 cdef void *check_pointer(void *ptr) except NULL cdef class ErrorHandler: diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 84465367..23a85494 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -10,7 +10,7 @@ from itertools import zip_longest from pyogrio._ogr cimport ( CE_None, CE_Debug, CE_Warning, CE_Failure, CE_Fatal, CPLErrorReset, - CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, + CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, OGRERR_NONE, CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPopErrorHandler, CPLPushErrorHandler, CPLPushErrorHandlerEx) @@ -237,13 +237,12 @@ cdef void *check_pointer(void *ptr) except NULL: cdef int check_int(int err) except -1: """Check the CPLErr (int) value returned by a GDAL/OGR function. - If `err` is a nonzero value, an exception inheriting from CPLE_BaseError is - raised. + If `err` is not OGRERR_NONE, an exception inheriting from CPLE_BaseError is raised. When the last error registered by GDAL/OGR was a non-fatal error, the exception raised will be customized appropriately. Otherwise a CPLError is raised. """ - if err: + if err != OGRERR_NONE: exc = check_last_error() if exc: raise exc @@ -254,17 +253,6 @@ cdef int check_int(int err) except -1: return err -cdef int check_ogrerr(int err) except -1: - """Check the OGRErr (int) value returned by a GDAL/OGR function. - - If `err` is a nonzero value, a CPLE_BaseError is raised. - """ - if err != 0: - raise CPLE_BaseError(CE_Failure, err, f"OGR Error code {err}") - - return err - - cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) noexcept nogil: """Custom CPL error handler to match the Python behaviour. @@ -308,8 +296,8 @@ cdef class ErrorHandler: cdef int check_int(self, int err, bint squash_errors) except -1: """Check the CPLErr (int) value returned by a GDAL/OGR function. - If `err` is a nonzero value, an exception inheriting from - CPLE_BaseError is raised. + If `err` is not OGRERR_NONE, an exception inheriting from CPLE_BaseError is + raised. When a non-fatal GDAL/OGR error was captured in the error stack, the exception raised will be customized appropriately. Otherwise, a CPLError is raised. @@ -325,10 +313,11 @@ cdef class ErrorHandler: Returns ------- int - The `err` input parameter if it is zero. Otherwise an exception is raised. + The `err` input parameter if it is OGRERR_NONE. Otherwise an exception is + raised. """ - if err: + if err != OGRERR_NONE: stack = self.error_stack.get() for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): if error is not None and cause is not None: diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index d2fae0d8..2a55398c 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -26,7 +26,7 @@ import numpy as np from pyogrio._ogr cimport * from pyogrio._err cimport ( - check_last_error, check_int, check_ogrerr, check_pointer, ErrorHandler + check_last_error, check_int, check_pointer, ErrorHandler ) from pyogrio._vsi cimport * from pyogrio._err import ( @@ -478,13 +478,12 @@ cdef get_total_bounds(OGRLayerH ogr_layer, int force): """ cdef OGREnvelope ogr_envelope - try: - check_ogrerr(OGR_L_GetExtent(ogr_layer, &ogr_envelope, force)) + + if OGR_L_GetExtent(ogr_layer, &ogr_envelope, force) == OGRERR_NONE: bounds = ( ogr_envelope.MinX, ogr_envelope.MinY, ogr_envelope.MaxX, ogr_envelope.MaxY ) - - except CPLE_BaseError: + else: bounds = None return bounds diff --git a/pyogrio/_ogr.pyx b/pyogrio/_ogr.pyx index e2ae7ff5..ea02678e 100644 --- a/pyogrio/_ogr.pyx +++ b/pyogrio/_ogr.pyx @@ -3,7 +3,7 @@ import sys from uuid import uuid4 import warnings -from pyogrio._err cimport check_int, check_ogrerr, check_pointer +from pyogrio._err cimport check_int, check_pointer from pyogrio._err import CPLE_BaseError, NullPointerError from pyogrio.errors import DataSourceError @@ -182,15 +182,15 @@ def has_proj_data(): """ cdef OGRSpatialReferenceH srs = OSRNewSpatialReference(NULL) - try: - check_ogrerr(check_int(OSRImportFromEPSG(srs, 4326))) - except CPLE_BaseError: - return False - else: + retval = OSRImportFromEPSG(srs, 4326) + if srs != NULL: + OSRRelease(srs) + + if retval == OGRERR_NONE: + # Succesfull return, so PROJ data files are correctly found return True - finally: - if srs != NULL: - OSRRelease(srs) + else: + return False def init_gdal_data(): From 8562c9f54b7921fd7fccf0f8b8a75ee5ce2c1019 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 13:56:36 +0100 Subject: [PATCH 19/27] Remove get_last_error_msg again as it is only used in one location and all logic was moved to clean_error_message --- pyogrio/_err.pyx | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 23a85494..6eecef1b 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -162,7 +162,7 @@ cdef inline object check_last_error(): """ err_type = CPLGetLastErrorType() err_no = CPLGetLastErrorNo() - err_msg = get_last_error_msg() + err_msg = clean_error_message(CPLGetLastErrorMsg()) if err_type == CE_Failure: CPLErrorReset() @@ -176,17 +176,6 @@ cdef inline object check_last_error(): return -cdef get_last_error_msg(): - """Checks GDAL error stack for the latest error message. - - Returns - ------- - An error message or empty string - - """ - return clean_error_message(CPLGetLastErrorMsg()) - - cdef clean_error_message(const char* err_msg): """Cleans up error messages from GDAL. From 68f79086bbd5050b552ab76adcf536855ed3dafb Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Thu, 14 Nov 2024 14:24:34 +0100 Subject: [PATCH 20/27] Return "No error message." as error message if there is none + avoid duplicate code in ErrorHandler --- pyogrio/_err.pxd | 1 + pyogrio/_err.pyx | 63 ++++++++++++++++++++++-------------------------- 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/pyogrio/_err.pxd b/pyogrio/_err.pxd index 71119e5e..49d321ea 100644 --- a/pyogrio/_err.pxd +++ b/pyogrio/_err.pxd @@ -6,3 +6,4 @@ cdef class ErrorHandler: cdef object error_stack cdef int check_int(self, int retval, bint squash_errors) except -1 cdef void *check_pointer(self, void *ptr, bint squash_errors) except NULL + cdef void _handle_error_stack(self, bint squash_errors) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 6eecef1b..1dc3c076 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -163,6 +163,8 @@ cdef inline object check_last_error(): err_type = CPLGetLastErrorType() err_no = CPLGetLastErrorNo() err_msg = clean_error_message(CPLGetLastErrorMsg()) + if err_msg == "": + err_msg = "No error message." if err_type == CE_Failure: CPLErrorReset() @@ -308,25 +310,9 @@ cdef class ErrorHandler: """ if err != OGRERR_NONE: stack = self.error_stack.get() - for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): - if error is not None and cause is not None: - error.__cause__ = cause if stack: - last = stack.pop() - if last is not None: - if squash_errors: - # Concatenate all error messages, and raise a single exception - errmsg = str(last) - inner = last.__cause__ - while inner is not None: - errmsg = f"{errmsg}; {inner}" - inner = inner.__cause__ - - raise type(last)(-1, -1, errmsg) - - raise last - + self._handle_error_stack(squash_errors) else: raise CPLError(CE_Failure, err, "Unspecified OGR / GDAL error") @@ -358,29 +344,38 @@ cdef class ErrorHandler: """ if ptr == NULL: stack = self.error_stack.get() - for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): - if error is not None and cause is not None: - error.__cause__ = cause if stack: - last = stack.pop() - if last is not None: - if squash_errors: - # Concatenate all error messages, and raise a single exception - errmsg = str(last) - inner = last.__cause__ - while inner is not None: - errmsg = f"{errmsg}; {inner}" - inner = inner.__cause__ - - raise type(last)(-1, -1, errmsg) - - raise last - + self._handle_error_stack(squash_errors) else: raise NullPointerError(-1, -1, "NULL pointer error") return ptr + + cdef void _handle_error_stack(self, bint squash_errors): + """Handle the errors in `error_stack`.""" + stack = self.error_stack.get() + + for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): + if error is not None and cause is not None: + error.__cause__ = cause + + last = stack.pop() + if last is not None: + if squash_errors: + # Concatenate all error messages, and raise a single exception + errmsg = str(last) + inner = last.__cause__ + while inner is not None: + errmsg = f"{errmsg}; {inner}" + inner = inner.__cause__ + + if errmsg == "": + errmsg = "No error message." + + raise type(last)(-1, -1, errmsg) + + raise last cdef void stacking_error_handler( From ad742bc2c073ab01c555a7855751582dfc64de67 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Sat, 16 Nov 2024 20:49:12 +0100 Subject: [PATCH 21/27] Remove include of CPLPushErrorHandlerEx --- pyogrio/_err.pyx | 6 +++--- pyogrio/_ogr.pxd | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index 1dc3c076..a220e863 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -10,9 +10,9 @@ from itertools import zip_longest from pyogrio._ogr cimport ( CE_None, CE_Debug, CE_Warning, CE_Failure, CE_Fatal, CPLErrorReset, - CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, OGRERR_NONE, - CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, CPLPopErrorHandler, - CPLPushErrorHandler, CPLPushErrorHandlerEx) + CPLGetLastErrorType, CPLGetLastErrorNo, CPLGetLastErrorMsg, OGRErr, + OGRERR_NONE, CPLErr, CPLErrorHandler, CPLDefaultErrorHandler, + CPLPopErrorHandler, CPLPushErrorHandler) _ERROR_STACK = ContextVar("error_stack") _ERROR_STACK.set([]) diff --git a/pyogrio/_ogr.pxd b/pyogrio/_ogr.pxd index 3a797925..8ce6a578 100644 --- a/pyogrio/_ogr.pxd +++ b/pyogrio/_ogr.pxd @@ -33,7 +33,6 @@ cdef extern from "cpl_error.h" nogil: ctypedef void (*CPLErrorHandler)(CPLErr, int, const char*) void CPLDefaultErrorHandler(CPLErr, int, const char *) void CPLPushErrorHandler(CPLErrorHandler handler) - void CPLPushErrorHandlerEx(CPLErrorHandler handler, void *userdata) void CPLPopErrorHandler() From 4a436849f39368476634cc314285f5908717be7b Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Sun, 17 Nov 2024 16:34:10 +0100 Subject: [PATCH 22/27] Cast exception to string instead of using errmsg --- pyogrio/_io.pyx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index 2a55398c..49df8441 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -218,15 +218,15 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: ) from None except CPLE_BaseError as exc: - if " a supported file format." in exc.errmsg: + if " a supported file format." in str(exc): # In gdal 3.9, this error message was slightly changed, so we can only check # on this part of the error message. raise DataSourceError( - f"{exc.errmsg}; It might help to specify the correct driver explicitly by " + f"{str(exc)}; It might help to specify the correct driver explicitly by " "prefixing the file path with ':', e.g. 'CSV:path'." ) from None - raise DataSourceError(exc.errmsg) from None + raise DataSourceError(str(exc)) from None cdef ogr_close(GDALDatasetH ogr_dataset): From 59d7315f2f108fb578e1d8ad087161e24b3dcf3d Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 4 Dec 2024 00:22:39 +0100 Subject: [PATCH 23/27] Update _io.pyx --- pyogrio/_io.pyx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyogrio/_io.pyx b/pyogrio/_io.pyx index 49df8441..7f6b5f64 100644 --- a/pyogrio/_io.pyx +++ b/pyogrio/_io.pyx @@ -214,7 +214,7 @@ cdef void* ogr_open(const char* path_c, int mode, char** options) except NULL: except NullPointerError: raise DataSourceError( - f"Failed to open dataset (mode={mode}): {path_c.decode('utf-8')}" + f"Failed to open dataset ({mode=}): {path_c.decode('utf-8')}" ) from None except CPLE_BaseError as exc: From 518204c388b99652a05a3933463c92e2d4ce6d51 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 4 Dec 2024 00:52:33 +0100 Subject: [PATCH 24/27] Try fixing tests for gdal 3.4 --- pyogrio/tests/test_geopandas_io.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pyogrio/tests/test_geopandas_io.py b/pyogrio/tests/test_geopandas_io.py index c32b8424..b915235f 100644 --- a/pyogrio/tests/test_geopandas_io.py +++ b/pyogrio/tests/test_geopandas_io.py @@ -229,14 +229,15 @@ def test_read_force_2d(tmp_path, use_arrow): def test_read_geojson_error(naturalearth_lowres_geojson, use_arrow): - set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": 0.01}) - with pytest.raises( - DataSourceError, - match="Failed to read GeoJSON data; .* GeoJSON object too complex", - ): - read_dataframe(naturalearth_lowres_geojson, use_arrow=use_arrow) - - set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": None}) + try: + set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": 0.01}) + with pytest.raises( + DataSourceError, + match="Failed to read GeoJSON data; .* GeoJSON object too complex", + ): + read_dataframe(naturalearth_lowres_geojson, use_arrow=use_arrow) + finally: + set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": None}) def test_read_layer(tmp_path, use_arrow): From 01619004a9349bf8133909ad5398cb8a1609d58b Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 4 Dec 2024 01:13:39 +0100 Subject: [PATCH 25/27] Skip test_read_geojson_error test for gdal < 2.5.2 Support for OGR_GEOJSON_MAX_OBJ_SIZE with float values only for gdal >= 3.5.2 by PR https://github.com/OSGeo/gdal/pull/6177/files#diff-821a801ab3a6e3e8ad5fa9883c48daf282b2ea5d4150408c57a62adff626652b --- pyogrio/_compat.py | 1 + pyogrio/tests/test_geopandas_io.py | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/pyogrio/_compat.py b/pyogrio/_compat.py index 6c39ad90..acfea471 100644 --- a/pyogrio/_compat.py +++ b/pyogrio/_compat.py @@ -40,6 +40,7 @@ PANDAS_GE_20 = pandas is not None and Version(pandas.__version__) >= Version("2.0.0") PANDAS_GE_22 = pandas is not None and Version(pandas.__version__) >= Version("2.2.0") +GDAL_GE_352 = __gdal_version__ >= (3, 5, 2) GDAL_GE_38 = __gdal_version__ >= (3, 8, 0) HAS_GDAL_GEOS = __gdal_geos_version__ is not None diff --git a/pyogrio/tests/test_geopandas_io.py b/pyogrio/tests/test_geopandas_io.py index b915235f..96d9e3a0 100644 --- a/pyogrio/tests/test_geopandas_io.py +++ b/pyogrio/tests/test_geopandas_io.py @@ -16,7 +16,7 @@ vsi_listtree, vsi_unlink, ) -from pyogrio._compat import HAS_ARROW_WRITE_API, HAS_PYPROJ, PANDAS_GE_15 +from pyogrio._compat import GDAL_GE_352, HAS_ARROW_WRITE_API, HAS_PYPROJ, PANDAS_GE_15 from pyogrio.errors import DataLayerError, DataSourceError, FeatureError, GeometryError from pyogrio.geopandas import PANDAS_GE_20, read_dataframe, write_dataframe from pyogrio.raw import ( @@ -228,6 +228,10 @@ def test_read_force_2d(tmp_path, use_arrow): assert not df.iloc[0].geometry.has_z +@pytest.mark.skipif( + not GDAL_GE_352, + reason="gdal >= 3.5.2 needed to use OGR_GEOJSON_MAX_OBJ_SIZE with a float value", +) def test_read_geojson_error(naturalearth_lowres_geojson, use_arrow): try: set_gdal_config_options({"OGR_GEOJSON_MAX_OBJ_SIZE": 0.01}) From e2f57555627607b78338140ba702069bf1971e4c Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Wed, 4 Dec 2024 01:33:57 +0100 Subject: [PATCH 26/27] Update CHANGES.md --- CHANGES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index 0b4f63fb..1223ea03 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,7 +20,7 @@ - Silence warning from `write_dataframe` with `GeoSeries.notna()` (#435). - Enable mask & bbox filter when geometry column not read (#431). -- Raise NotImplmentedError when user attempts to write to an open file handle (#442). +- Raise `NotImplementedError` when user attempts to write to an open file handle (#442). - Prevent seek on read from compressed inputs (#443). ### Packaging From d5a8c005a516ddcd9ed90c800ab76510718ccec1 Mon Sep 17 00:00:00 2001 From: Pieter Roggemans Date: Fri, 6 Dec 2024 00:41:49 +0100 Subject: [PATCH 27/27] Process feedback --- pyogrio/_err.pyx | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/pyogrio/_err.pyx b/pyogrio/_err.pyx index a220e863..d280101b 100644 --- a/pyogrio/_err.pyx +++ b/pyogrio/_err.pyx @@ -174,9 +174,6 @@ cdef inline object check_last_error(): if err_type == CE_Fatal: return SystemExit("Fatal error: {0}".format((err_type, err_no, err_msg))) - else: - return - cdef clean_error_message(const char* err_msg): """Cleans up error messages from GDAL. @@ -222,6 +219,7 @@ cdef void *check_pointer(void *ptr) except NULL: else: # null pointer was passed, but no error message from GDAL raise NullPointerError(-1, -1, "NULL pointer error") + return ptr @@ -260,12 +258,12 @@ cdef void error_handler(CPLErr err_class, int err_no, const char* err_msg) noexc CPLDefaultErrorHandler(err_class, err_no, err_msg) return - elif err_class == CE_Failure: + if err_class == CE_Failure: # For Failures, do nothing as those are explicitly caught # with error return codes and translated into Python exceptions return - elif err_class == CE_Warning: + if err_class == CE_Warning: with gil: warnings.warn(clean_error_message(err_msg), RuntimeWarning) return @@ -309,9 +307,7 @@ cdef class ErrorHandler: """ if err != OGRERR_NONE: - stack = self.error_stack.get() - - if stack: + if self.error_stack.get(): self._handle_error_stack(squash_errors) else: raise CPLError(CE_Failure, err, "Unspecified OGR / GDAL error") @@ -343,9 +339,7 @@ cdef class ErrorHandler: """ if ptr == NULL: - stack = self.error_stack.get() - - if stack: + if self.error_stack.get(): self._handle_error_stack(squash_errors) else: raise NullPointerError(-1, -1, "NULL pointer error") @@ -355,7 +349,6 @@ cdef class ErrorHandler: cdef void _handle_error_stack(self, bint squash_errors): """Handle the errors in `error_stack`.""" stack = self.error_stack.get() - for error, cause in zip_longest(stack[::-1], stack[::-1][1:]): if error is not None and cause is not None: error.__cause__ = cause @@ -398,7 +391,7 @@ cdef void stacking_error_handler( CPLDefaultErrorHandler(err_class, err_no, err_msg) return - elif err_class == CE_Failure: + if err_class == CE_Failure: # For Failures, add them to the error exception stack with gil: stack = _ERROR_STACK.get() @@ -411,7 +404,7 @@ cdef void stacking_error_handler( return - elif err_class == CE_Warning: + if err_class == CE_Warning: with gil: warnings.warn(clean_error_message(err_msg), RuntimeWarning) return