Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#31301 closed Bug (fixed)

bulk_create with ForeignKey fields with mixed filled/None values fails.

Reported by: Hans Aarne Liblik Owned by: Hans Aarne Liblik
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: oracle sql
Cc: Marti Raudsepp Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

I have a model with ForeignKey (null=True) to another model

class A(models.Model):
  name = models.CharField()

class B(models.Model):
  name = models.CharField()
  a = models.ForeignKey(A, on_delete=models.CASCADE, null=True)

When I try to use bulk_create as follows

B.objects.bulk_create([
  B(name='first', a_id=1)
  B(name='seconds', a_id=None)
])

the query fails with error

django.db.utils.DatabaseError: ORA-01790: expression must have same datatype as corresponding expression

I got bulk_create working if the ForeignKey references are either All None or All NOT None. It seems like the fix introduced in https://code.djangoproject.com/ticket/22669 fixes the case for NumberFields and CharFields, but ForeignKey field 'target_field' for me is 'AutoField' which is not defined in

django/db/backends/oracle/utils.py#52  class BulkInsertMapper:

Manually editing that this file and adding

class BulkInsertMapper:
  ...
  types: {
    ...
    ...
    'AutoField': NUMBER
  }

fixes the issue for me.

Change History (21)

comment:1 Changed 3 years ago by Simon Charette

Easy pickings: set
Triage Stage: UnreviewedAccepted

That seems like the correct approach. Would you be interested in submitting a Github PR that addresses the issue?

Based on c4e2fc5d9872c9a0c9c052a2e124f8a9b87de9b4 adding a nullable foreign key on NullableFields to another test model of the same module should be sufficient to augment the test_bulk_insert_nullable_fields test to cover this case.

comment:2 Changed 3 years ago by Hardik Patel

Owner: changed from nobody to Hardik Patel
Status: newassigned

comment:3 Changed 3 years ago by Hans Aarne Liblik

Any progress on this? If not, I can take over.

comment:4 in reply to:  3 Changed 3 years ago by Hardik Patel

Replying to Hans Aarne Liblik:

Any progress on this? If not, I can take over.

It is looking harder than I expected. I am just a newbie. I will come back later. Feel free to take over

comment:5 Changed 3 years ago by Hardik Patel

Owner: Hardik Patel deleted
Status: assignednew

comment:6 Changed 3 years ago by Simon Charette

Owner: set to Hans Aarne Liblik
Status: newassigned

comment:7 Changed 3 years ago by Marti Raudsepp

Cc: Marti Raudsepp added

comment:8 Changed 3 years ago by Mariusz Felisiak

Hans, Please remember to add also SmallAutoField and BigAutoField.

comment:9 Changed 3 years ago by Hans Aarne Liblik

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:10 Changed 3 years ago by Hans Aarne Liblik

Triage Stage: Ready for checkinAccepted

comment:11 Changed 3 years ago by Mariusz Felisiak

Patch needs improvement: set

comment:12 Changed 3 years ago by Mariusz Felisiak

Needs tests: set

comment:13 Changed 3 years ago by Mariusz Felisiak

Needs tests: unset
Patch needs improvement: unset
Summary: bulk_create with ForeignKey fields with mixed filled/None values failsbulk_create with ForeignKey fields with mixed filled/None values fails.
Triage Stage: AcceptedReady for checkin
Version: 3.0master

comment:14 Changed 3 years ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In a21f7b91:

Fixed #31301 -- Fixed crash of QuerySet.bulk_create() with mixed empty and set ForeignKeys to AutoFields on Oracle.

comment:15 Changed 3 years ago by Hans Aarne Liblik

This didn't get into 3.0.4 release?

comment:16 Changed 3 years ago by Mariusz Felisiak

No, this doesn't qualify for a backport, see https://docs.djangoproject.com/en/3.0/internals/release-process/ for more details.

comment:17 Changed 3 years ago by Marti Raudsepp

@felixxm, are you sure this doesn't qualify?

It seems like a significant issue to me; the backport criteria does list "Crashing bugs".

If I understand correctly, it causes different database backends to behave differently? Which is always a bad surprise.

Last edited 3 years ago by Marti Raudsepp (previous) (diff)

comment:18 Changed 3 years ago by Mariusz Felisiak

It doesn't qualify for a backport from the same reason that #22669 didn't qualify. It's not a regression it existed since 9fe050a27598cdce7f2ef7a332b4818b493a8702 (Django 1.4) so it's not crucial.

comment:19 Changed 3 years ago by Marti Raudsepp

What's your suggested solution for users who are hitting this bug now? I can think of:

  • Fork Django, backport the fix manually, and continue to maintain the fork until Django 3.1.0 is released and upgrading is viable
  • Upgrade to Django development "master" branch, which sometimes breaks third-party Django addons
  • Stop using Django until August 2020 rolls around (just sarcastic, sorry!)

Am I missing something? This is behavior that everyone agrees is a bug, and a helpful user provided a pull request to fix it. Yet you are leaving users hanging without a good solution for ~5 months.

Seems like a bug in the backport policy itself :)

Last edited 3 years ago by Marti Raudsepp (previous) (diff)

comment:20 Changed 3 years ago by Mariusz Felisiak

I'm sorry Marti but that's our policy. We're fixing dozens of bugs in each month and only few of them are backported. We don't backport all bugfixes to minimize the risk of regressions.

comment:21 in reply to:  19 Changed 3 years ago by Claude Paroz

Replying to Marti Raudsepp:

  • Fork Django, backport the fix manually, and continue to maintain the fork until Django 3.1.0 is released and upgrading is viable

I think that's clearly the way to go if the fix is critical to your application. You may have to update your fork once a month if Django periodic minor release contains security fixes, but that's a price to pay for Django general stability where you can almost blindly apply minor upgrades without worrying. And even with this strict backport policy, it has happened in the past that such fixes have introduced regressions, so I don't think we should relax this policy.

Note: See TracTickets for help on using tickets.
Back to Top