[Django] #36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve Last Inserted ID

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: ybjeon01 | Type: Bug
Status: new | Component: Database
| layer (models, ORM)
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
When running tests in `django-docker-box` with Oracle, setting the
`"use_returning_into": False` option in `settings.py` prevents Django from
retrieving the last inserted ID. The issue can be reproduced by modifying
the database options as shown below:

{{{
if engine.endswith(".oracle"):
entry |= {
"NAME": name,
"OPTIONS": {
"use_returning_into": False,
}
}
...
}}}

**Cause of the Issue**
The issue originates from the `last_insert_id()` method in
`django/db/backends/oracle/operations.py`. The SQL executed in this method
does not properly handle the query format, leading to a failure in
retrieving the sequence value.

**Current Implementation (Buggy Code)**

{{{
def last_insert_id(self, cursor, table_name, pk_name):
sq_name = self._get_sequence_name(cursor, strip_quotes(table_name),
pk_name)
cursor.execute('"%s".currval' % sq_name)
return cursor.fetchone()[0]

}}}


**Proposed Fix**
The issue can be resolved by modifying the query to include the correct
SQL syntax:

{{{
def last_insert_id(self, cursor, table_name, pk_name):
sq_name = self._get_sequence_name(cursor, strip_quotes(table_name),
pk_name)
template = 'SELECT "%s".currval' +
self.connection.features.bare_select_suffix

cursor.execute(template % sq_name)
return cursor.fetchone()[0]

}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36189>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------

Reporter: ybjeon01 | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Amaan-ali03):

* cc: Amaan-ali03 (added)
* easy: 1 => 0
* owner: (none) => Amaan-ali03
* status: new => assigned

Comment:

"Hey Django Team,
I’d love to work on this issue! From what I understand, the problem comes
from the last_insert_id() method in oracle/operations.py, where the query
fails when "use_returning_into": False is set.

Plan to Fix:
Reproduce the issue in django-docker-box with Oracle.
Update the query to correctly fetch currval, as proposed.
Add tests to ensure the fix works with and without "use_returning_into".
Run the full Django test suite to confirm no regressions.
Could you assign this to me? Looking forward to contributing!

Thanks,
Amaan Ali
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:1>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------

Reporter: Yeongbae Jeon | Owner: Amaan-

| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* cc: Mariusz Felisiak (added)
* stage: Unreviewed => Accepted

Comment:

I can replicate the issue but we should agree on a solution before
proceeding here.

This relates to #11706 (which introduced `use_returning_into`) and #27789
which changed the way `last_insert_id` works.

I tried applying the proposed patch and there are many tests that fail
with it against (Oracle `23.5.0.24.07`) so I wonder if there is something
else at play here. Given `use_returning_into` has been untested since its
introduction 15 years ago and now appears to be broken in non trivial ways
I wonder if we should just simply remove support for it instead of trying
to resolve this issue.

Any thoughts on the current situation Mariusz?
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:2>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

I dug a bit more in the reason behind why so many tests were failing even
with the SQL adjusted to be valid on Oracle 23 by adding the `SELECT`
prefix and the reason is quite clear; using `SELECT sequence_name.currval`
doesn't account for explicitly specified primary key values which Django
allows.

In other words, `AutoField` are defined using `IDENTITY` but not
`GENERATED ALWAYS` which means that an explicit value can be specified and
must be supported. When it's the case returning the current value of the
associated sequence is simply wrong. I think this is a significant
oversight from 1f68dc4ad4ddc67831c6aa047683a5b53fa33a37 and another
argument to deprecate this option altogether.
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:3>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):

Another reason why `use_returning_into=False` is broken is that it `SELECT
sequence_name.currval` doesn't account for concurrent `INSERT`. From what
I understand, just like in Postgres, Oracle sequences are not tied to
transactions which means that nothing prevents a concurrent `INSERT` from
being executed in a separate transaction and increasing the sequence
before it can be `SELECT`ed.

e.g.

- session0: `INSERT`
- session1: `INSERT`
- session0: `SELECT sequence_name.currval`
- session1: `SELECT sequence_name.currval`

Both session 0 and 1 assume that their `last_insert_id` is the same;
session 0 is wrong.
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:4>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yeongbae Jeon):

Replying to [comment:4 Simon Charette]:

> Another reason why `use_returning_into=False` is broken is that it
`SELECT sequence_name.currval` doesn't account for concurrent `INSERT`.
From what I understand, just like in Postgres, Oracle sequences are not
tied to transactions which means that nothing prevents a concurrent
`INSERT` from being executed in a separate transaction and increasing the
sequence before it can be `SELECT`ed.
>
> e.g.
>
> - session0: `INSERT`
> - session1: `INSERT`
> - session0: `SELECT sequence_name.currval`
> - session1: `SELECT sequence_name.currval`
>
> Both session 0 and 1 assume that their `last_insert_id` is the same;
session 0 is wrong.

I agree with you. Another case I can think of (although I haven't tested
it yet, so please correct me if I'm wrong, as this is based on my limited
experience) is when a user explicitly provides a value for the primary key
while creating an object, instead of relying on the nextval from the
sequence.

For example, if the currval of the sequence is 1, but the user manually
assigns 10 as the primary key when creating the object, then
use_returning_into: False will execute the last_insert_id function, which
will return the incorrect value of 1.
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:5>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

> I agree with you. Another case I can think of (although I haven't tested
it yet, so please correct me if I'm wrong, as this is based on my limited
experience) is when a user explicitly provides a value for the primary key
while creating an object, instead of relying on the nextval from the
sequence.

Right this is what is causing many test failures as described in comment:3
and why the solution is not appropriate as don't use `GENERATED ALWAYS`
which would disallow explicit assignment of primary key values.

> Another case that shows sequence is tied to transactions:

Thanks so at least this feature is not completely broken. I had not tried
it it out myself but I read mixed reports online depending on the
isolation level used (e.g. `READ COMMITTED` vs `REPEATABLE READ`).
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:6>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak):

I have no issue with deprecating
[https://docs.djangoproject.com/en/stable/ref/databases/#insert-returning-
into this feature].
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:7>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yeongbae Jeon):

I searched for occurrences of "deprecate" in the Django repository to
understand how deprecations are handled.

In django/db/backends/base/operations.py, I found the following code

{{{
def field_cast_sql(self, db_type, internal_type):
"""
Given a column type (e.g. 'BLOB', 'VARCHAR') and an internal type
(e.g. 'GenericIPAddressField'), return the SQL to cast it before using
it in a WHERE statement. The resulting string should contain a '%s'
placeholder for the column being searched against.
"""
warnings.warn(
(
"DatabaseOperations.field_cast_sql() is deprecated use "
"DatabaseOperations.lookup_cast() instead."
),
RemovedInDjango60Warning,
)
return "%s"
}}}

In django/db/backends/oracle/base.py, I found the following code, and it
seems like a good place to add a deprecation warning


{{{
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
use_returning_into = self.settings_dict["OPTIONS"].get(
"use_returning_into", True
)
self.features.can_return_columns_from_insert = use_returning_into
}}}

Before submitting a merge request, I wonder which Django version this
feature should be deprecated.

Also, does it matter who submits the merge request, or can anyone do it?
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:8>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Jacob Walls):

Contributions are welcomed from all. See this
[https://docs.djangoproject.com/en/dev/internals/contributing/writing-code
/submitting-patches/#deprecating-a-feature guide] on deprecating a
feature. This change would be made in Django 6.0 and use
`RemovedInDjango70Warning`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:9>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Antoliny):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:10>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Antoliny):

[https://github.com/django/django/pull/19247 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:11>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:12>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Amaan-ali03):

* needs_tests: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:13>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 0 => 1
* needs_docs: 0 => 1

* needs_tests: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:14>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Amaan-
| ali03
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Yeongbae Jeon):

Replying to [comment:11 Antoliny]:
> [https://github.com/django/django/pull/19247 PR]

Antoliny, it looks like the PR has been closed. If you are not currently
working with this issue, I can take it over.
--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:15>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------

Reporter: Yeongbae Jeon | Owner: Yeongbae
| Jeon

Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Yeongbae Jeon):

* owner: Amaan-ali03 => Yeongbae Jeon

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:16>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Yeongbae
| Jeon
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Yeongbae Jeon):

* has_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:17>

[Django]

#36189: Oracle Backend with `"use_returning_into": False` Option Fails to Retrieve
Last Inserted ID
-------------------------------------+-------------------------------------
Reporter: Yeongbae Jeon | Owner: Yeongbae
| Jeon
Type: Bug | Status: assigned
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1

Needs tests: 1 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Yeongbae Jeon):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/36189#comment:18>