Skip to content

Conversation

jahnvi480
Copy link
Contributor

@jahnvi480 jahnvi480 commented Aug 5, 2025

Work Item / Issue Reference

AB#34897


Summary

This pull request adds support for the lastrowid attribute to the Cursor class, allowing users to retrieve the row ID of the last inserted row after single-row INSERT operations. The implementation ensures compliance with DB-API semantics and includes comprehensive tests to verify correct behavior in various scenarios.

Enhancements to Cursor functionality:

  • Added a read-only lastrowid property to the Cursor class, tracking the row ID of the last modified row, and ensuring it is set only for single-row INSERT operations.
  • Updated the execute method to set lastrowid using the result of SELECT @@IDENTITY after a successful single-row INSERT, and reset it to None at the start of each execution.
  • Ensured that lastrowid is reset to None at the start of executemany, as semantics are undefined for multi-row operations.

Testing improvements:

  • Added a comprehensive suite of tests for lastrowid, covering single and multiple inserts, executemany, non-INSERT operations, tables without identity columns, and enforcement of the read-only property.

@Copilot Copilot AI review requested due to automatic review settings August 5, 2025 08:37
@github-actions github-actions bot added the pr-size: medium Moderate update size label Aug 5, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request implements the lastrowid attribute for the Cursor class, providing DB-API 2.0 compliance by allowing retrieval of the last inserted row's identity value. The implementation ensures lastrowid is only set for single-row INSERT operations and remains None for other operations or batch executions.

  • Added a read-only lastrowid property that queries @@IDENTITY after successful single-row INSERTs
  • Reset lastrowid to None for non-INSERT operations, multi-row INSERTs, and executemany calls
  • Comprehensive test coverage for various scenarios including identity and non-identity tables

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
mssql_python/cursor.py Implements lastrowid property with logic to detect single-row INSERTs and query @@IDENTITY
tests/test_004_cursor.py Adds extensive test cases covering all lastrowid scenarios and edge cases
Comments suppressed due to low confidence (1)

tests/test_004_cursor.py:1359

  • This test doesn't verify that the INSERT actually succeeded and affected multiple rows. Consider adding an assertion to check cursor.rowcount == 3 to ensure the test is validating the correct scenario.
        cursor.execute("INSERT INTO #test_lastrowid (name) VALUES ('test1'), ('test2'), ('test3')")

@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 5, 2025
Copy link
Collaborator

@bewithgaurav bewithgaurav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few limitations on ODBC w.r.t implementation and usage of this attribute, added comments in detail. I believe we can discuss this in detail and park it for now.
Additionally, this is an optional DBAPI extension - https://peps.python.org/pep-0249/#lastrowid and is not implemented by pyodbc due to the above mentioned limitations. Lets discuss this in next meeting.

@jahnvi480 jahnvi480 marked this pull request as draft August 6, 2025 12:27
@github-actions github-actions bot added pr-size: medium Moderate update size and removed pr-size: medium Moderate update size labels Aug 13, 2025
Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment for @@IDENTITY VS SCOPE_IDENTITY() .. Let's discuss where the challenge is @jahnvi480 @@IDENTITY can return unexpected results if there are triggers on the table that perform additional inserts.

@jahnvi480 jahnvi480 marked this pull request as ready for review October 6, 2025 09:48
Copy link

github-actions bot commented Oct 6, 2025

📊 Code Coverage Report

🔥 Diff Coverage

82%


🎯 Overall Coverage

74%


📈 Total Lines Covered: 4204 out of 5624
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/cursor.py (82.4%): Missing lines 1073,1075-1076

Summary

  • Total: 17 lines
  • Missing: 3 lines
  • Coverage: 82%

mssql_python/cursor.py

Lines 1069-1080

  1069                 if (fetch_ret == ddbc_sql_const.SQL_SUCCESS.value and 
  1070                     row_data and row_data[0] is not None):
  1071                     self._lastrowid = int(row_data[0])
  1072 
! 1073         except Exception:
  1074             # If we can't get the identity, leave lastrowid as None
! 1075             self._lastrowid = None
! 1076             log('debug', "Could not retrieve lastrowid: %s", e)
  1077 
  1078         # Return self for method chaining
  1079         return self


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.connection.connection.cpp: 68.3%
mssql_python.ddbc_bindings.py: 68.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.7%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.cursor.py: 79.7%
mssql_python.connection.py: 81.7%
mssql_python.helpers.py: 84.7%
mssql_python.auth.py: 85.3%
mssql_python.type.py: 86.8%
mssql_python.pooling.py: 87.5%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@jahnvi480 jahnvi480 requested a review from sumitmsft October 6, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants