microsoft/mssql-django

bulk_update_with_default fails when field is not int

federicoemartinez opened this issue · 5 comments

federicoemartinez commented

If you have a bulk update of different fields, when all the values of a case are null, a default value is added.
By the default, the value is 0; but it fails when the data type of the column is not an int.

Example:
django.db.utils.DataError: ('22018', '[22018] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Operand type clash: int is incompatible with datetimeoffset (206) (SQLExecDirectW)')

When the bulk update affects different fields of different data types, there is not a valid default value.

I think we can fix this by changing functions.py:

case_statement = Case(*when_statements, output_field=field, default=default)
with
case_statement = Case(*when_statements, output_field=field, default=F(field.attname))

That way the data type will match and as the default is not used, there wouldn't be a problem.
I can create a PR with this if you think it is correct.

dauinsight commented

Hi @federicoemartinez, specifying the default value, e.g bulk_update(... default=) to match your data type should fix your issue.

federicoemartinez commented

Hi @dauinsight but If I'm updating more than one field, with different types (for instance a datetime and an integer) there is no default value that can be used for both. That is why I suggest using the current value of the row with F(field.attname)

dauinsight commented

@federicoemartinez seems like that approach would work.

We will continue to have default as an optional parameter.

If default is specified we will execute Case(*when_statements, output_field=field, default=default)

If default is not specified we will execute Case(*when_statements, output_field=field, default=F(field.attname))

Let me know what you think, thank you.

federicoemartinez commented

You would change the definition to have default=None instead of default=0 and use F(field.attname) unless default is not None. That sounds good.

In general, queryset.bulk_update does not expose a default value (if I'm not wrong), so I think default=None is better, and it should work always, instead of default=0 which fails if the column is not numeric.

I can create a PR if you want.

dauinsight commented

Thank you for bringing this to our attention Federico. We have a PR up for the update and it will be featured in our next release.