FEAT: Adding Ouput Converter APIs #190
Conversation
| converted_values[i] = converter(value_bytes) | ||
| else: | ||
| converted_values[i] = converter(value) | ||
| except Exception as e: |
There was a problem hiding this comment.
All exceptions are currently suppressed silently. While it's important not to leak sensitive data, it would be beneficial to at least log the exception at a debug or warning level for troubleshooting purposes. This way, issues with converter functions can be diagnosed without exposing sensitive information.
except Exception: log('debug', 'Exception occurred in output converter', exc_info=True)
This will help with debugging, especially if custom converter functions are used, without leaking sensitive data.
| @@ -208,6 +208,76 @@ def execute(self, sql, *args): | |||
| cursor = self.cursor() | |||
| cursor.execute(sql, *args) | |||
| return cursor | |||
|
|
|||
| def add_output_converter(self, sqltype, func) -> None: | |||
There was a problem hiding this comment.
The use of self._output_converters for storing and managing output converters is clear and straightforward for single-threaded scenarios. However, if the Connection object is accessed or modified from multiple threads, operations like adding, removing, or clearing converters could result in race conditions or inconsistent state.
If multi-threaded access to the Connection object is expected or possible, please consider protecting accesses to self._output_converters with a threading lock (e.g., threading.Lock). This will ensure thread safety during concurrent modifications.
| tup[0], tup[1], tup[2], tup[3], tup[4], tup[5], tup[6] // 1000, | ||
| timezone(timedelta(hours=tup[7], minutes=tup[8])) | ||
| ) | ||
|
|
There was a problem hiding this comment.
As an additional suggestion, please consider adding a test case where a converter function intentionally raises an exception. The test should verify that:
The value for that column falls back to the original (unconverted) value,
No sensitive information is leaked.
This will help ensure the robustness of error handling in the converter logic.
|
|
||
| def add_output_converter(self, sqltype, func) -> None: | ||
| """ | ||
| Register an output converter function that will be called whenever a value |
There was a problem hiding this comment.
The output converter registration API allows arbitrary Python functions to be registered and executed on data retrieved from the database. If this API is public or can be accessed by untrusted users, there is a risk of code injection and arbitrary code execution. This risk is mitigated if only trusted application developers register converters. It is important to document this behavior clearly in the API documentation and warn users not to register converters from untrusted sources.
For the safety of all users, please consider updating the documentation (and relevant docstrings) with a clear "WARNING" that registering output converters is equivalent to executing arbitrary Python code. This should include explicit guidance that only trusted code should be registered as converters, and that converter registration should never be exposed to untrusted or external input.
Example warning for documentation:
⚠️ Warning: Registering an output converter will cause the supplied Python function to be executed on every matching database value. Do not register converters from untrusted sources, as this can result in arbitrary code execution and security vulnerabilities.
Making this risk explicit will help prevent misuse and keep downstream users secure.
Work Item / Issue Reference
Summary
This pull request adds a flexible output converter system to the database connection, allowing custom Python functions to be registered for converting SQL data types when fetching results. It introduces new methods for managing these converters and updates the row handling logic to apply them automatically. Comprehensive tests are added to verify correct registration, retrieval, removal, and integration of output converters, including edge cases and chaining behavior.
Core output converter system:
Connection(add_output_converter,get_output_converter,remove_output_converter,clear_output_converters) for registering and managing output converter functions for specific SQL types. These converters are called when values of the registered SQL type are read from the database. (mssql_python/connection.py)Rowclass to apply registered output converters automatically to values fetched from the database, with fallback logic for string types and robust error handling. (mssql_python/row.py)Testing and validation:
NULLvalues and using multiple converters at once. (tests/test_003_connection.py)DATETIMEOFFSETand custom string handling) to support and validate the new converter system in tests. (tests/test_003_connection.py)Test and utility enhancements:
struct,datetime,timezone, and constants) to support new test cases and converter logic. (tests/test_003_connection.py)