Opened 5 years ago

Closed 5 years ago

Last modified 5 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 by Simon Charette, 5 years ago

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 by Hardik Patel, 5 years ago

Owner: changed from nobody to Hardik Patel
Status: newassigned

comment:3 by Hans Aarne Liblik, 5 years ago

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

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

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 by Hardik Patel, 5 years ago

Owner: Hardik Patel removed
Status: assignednew

comment:6 by Simon Charette, 5 years ago

Owner: set to Hans Aarne Liblik
Status: newassigned

comment:7 by Marti Raudsepp, 5 years ago

Cc: Marti Raudsepp added

comment:8 by Mariusz Felisiak, 5 years ago

Hans, Please remember to add also SmallAutoField and BigAutoField.

comment:9 by Hans Aarne Liblik, 5 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:10 by Hans Aarne Liblik, 5 years ago

Triage Stage: Ready for checkinAccepted

comment:11 by Mariusz Felisiak, 5 years ago

Patch needs improvement: set

comment:12 by Mariusz Felisiak, 5 years ago

Needs tests: set

comment:13 by Mariusz Felisiak, 5 years ago

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 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

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 by Hans Aarne Liblik, 5 years ago

This didn't get into 3.0.4 release?

comment:16 by Mariusz Felisiak, 5 years ago

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

comment:17 by Marti Raudsepp, 5 years ago

@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 5 years ago by Marti Raudsepp (previous) (diff)

comment:18 by Mariusz Felisiak, 5 years ago

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 by Marti Raudsepp, 5 years ago

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 5 years ago by Marti Raudsepp (previous) (diff)

comment:20 by Mariusz Felisiak, 5 years ago

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.

in reply to:  19 comment:21 by Claude Paroz, 5 years ago

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