FIX: accept memoryview in Binary() - #741
Conversation
Binary() rejected memoryview with a TypeError, so any DB-API caller that hands the driver a buffer-protocol value failed. Django's BinaryField gives the driver a memoryview, so BinaryField writes and its serializer roundtrips all broke. Accept memoryview via tobytes(), matching pyodbc and the DB-API convention. (GH-739) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The runtime API change isn’t reflected in the public type stubs (mssql_python/mssql_python.pyi), which will cause type-checking inconsistencies for downstream users.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes DB-API compatibility for binary parameters by allowing mssql_python.Binary() to accept memoryview, which is commonly passed by Django’s BinaryField (GH-739), aligning behavior with pyodbc and typical buffer-protocol usage.
Changes:
- Extend
Binary()to acceptmemoryviewviatobytes(), and update the error message and docstring accordingly. - Add unit coverage for
memoryviewinputs and update existing error-message assertions in type tests.
File summaries
| File | Description |
|---|---|
mssql_python/type.py |
Accepts memoryview in Binary() and updates docs/error message to reflect the expanded supported input types. |
tests/test_002_types.py |
Adds a memoryview test case for Binary() and updates TypeError message expectations for unsupported types. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Match the runtime signature so type checkers accept Binary(memoryview(...)). (GH-739) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 58.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.4%
mssql_python.logging.py: 85.5%
mssql_python.connection.py: 85.9%🔗 Quick Links
|
type.py ships py.typed, so annotate the runtime parameter directly (Union[str, bytes, bytearray, memoryview]) to make the accepted-input contract explicit at the source, matching the stub. (GH-739) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.
Details
Binary()normalizesmemoryviewto an exactbytesvia.tobytes(), preserving the existingbytesreturn contract. This keeps the downstream binding path unchanged, since the binder already mapsbytestoSQL_VARBINARY/SQL_C_BINARY(including large-value streaming)..tobytes()produces an owned copy in logical C order, so non-contiguous, bytearray-backed, and empty memoryviews all round-trip correctly with no aliasing/lifetime concern.- Accepting
memoryviewaligns with pyodbc's acceptance of buffer-protocol values (pyodbc materializes tobytearray; this driver keeps itsbytescontract) and resolves GH-739 (DjangoBinaryFieldhands the driver amemoryview). - Runtime signature and the PEP 561
.pyistub are kept in sync; a focused regression test covers memoryview inputs and the updated error message; Black passes.
Recommendation: Approve
Point the Windows PR-validation legs at the mssql-python wheel from Build-Release-Package-Pipeline dev build 172271, which already carries the two pyodbc-parity fixes (microsoft/mssql-python#741 Binary(memoryview) and microsoft/mssql-python#742 Decimal SQL_NUMERIC) with the native core rebuilt. The published PyPI 1.14.0 wheel does not have these yet, so the two Decimal and BinaryField gaps would otherwise still fail. - tox.ini: allow the mssql-python requirement to be overridden by MSSQL_PYTHON_WHEEL, defaulting to the PyPI requirement when unset. - azure-pipelines-steps-windows.yml: download the matching per-Python wheel from build 172271 and hand its path to tox. - azure-pipelines.yml: gate Linux_Core, Linux_Legacy and Windows_Legacy off for this run. Build 172271 produced no Linux wheels, and the EOL/py3.8-3.9 legs have no wheel, so only the supported Windows matrix is validated here. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-project artifact download from the mssql-python project is blocked for the public project's build identity (VS800075), so commit the Windows wheels directly under ci/mssql-python-wheels/ and install the per-Python wheel from there. Wheels are from Build-Release-Package-Pipeline dev build 172271 and carry microsoft/mssql-python#741 (Binary(memoryview)) and microsoft/mssql-python#742 (Decimal SQL_NUMERIC). Temporary: drop once mssql-python 1.15.0 ships to PyPI with both fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The full Build-Release-Package-Pipeline run 173176 (main, commit b426da4e) produced Linux wheels as well, so extend the wheel validation to the Linux supported matrix: - Refresh ci/mssql-python-wheels/ with the Windows and Linux x86_64 wheels from a single build (173176). Both still carry microsoft/mssql-python#741 (Binary(memoryview)) and microsoft/mssql-python#742 (Decimal SQL_NUMERIC). - azure-pipelines-steps-linux.yml: install the per-Python manylinux wheel from ci/mssql-python-wheels/ via MSSQL_PYTHON_WHEEL, mirroring the Windows steps. - azure-pipelines.yml: re-enable Linux_Core and raise its timeout to 180 min to match Windows_Core (mssql-python runs the suite ~1.5-2x slower). Windows_Legacy and Linux_Legacy stay gated off (no wheels for EOL py3.8/3.9). Temporary: drop once mssql-python 1.15.0 ships to PyPI with both fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Work Item / Issue Reference
Summary
Binary()rejectedmemoryviewwith aTypeError, so any DB-API caller thatpasses a buffer-protocol value failed. Django's
BinaryFieldhands the driver amemoryview, which brokeBinaryFieldwrites and the serializer roundtrips.Accept
memoryviewviatobytes(), matching pyodbc and the DB-API convention.