Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks ago

#36133 closed Bug (invalid)

Passing expression with wrongly attributed text output_field to Concat crashes on Postgres due the lack of casting

Reported by: Siburg Owned by:
Component: Database layer (models, ORM) Version: 5.1
Severity: Normal Keywords: Cast, ExpressionWrapper
Cc: Siburg Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

It seems that output_field for an ExpressionWrapper no longer works in Django 5.1 as in previous versions. Example code and test is below.

class PartnerQuerySet(models.QuerySet):

    def order_by_nick_name(self):
        qs = self.annotate(
            nick_name_for_ordering=Case(
                When(
                    nick_name='',
                    # The starting 'zzzzzzzz' is intended to ensure they
                    # will be after records with nicknames. As ugly
                    # as it is, this should work on all databases.
                    # Subtracting the pk from a large number provides a
                    # reverse ordering by pk. Let's assume we never
                    # reach 900,000 records.
                    then=Concat(
                        Value('zzzzzzzz'),
                        ExpressionWrapper(999_999 - F('pk'), output_field=models.CharField()),
                        # This is probably a bug in Django 5.1, but it no longer
                        # worked when using `CharField` as output_field for the
                        # ExpressionWrapper. Amazingly, the 2-stage casting
                        # does work.
                        # Cast(
                        #     ExpressionWrapper(999_999 - F('pk'), output_field=models.BigAutoField()),
                        #     output_field=models.CharField()
                        # ),
                    ),
                ),
                default=Lower('nick_name'),
            )
        )
        return qs.order_by('nick_name_for_ordering')


class Partner(models.Model):
    name = models.CharField(max_length=128, unique=True)
    nick_name = models.CharField(max_length=64, default='', blank=True)

    objects = PartnerQuerySet().as_manager()


class PartnerQuerySetTests(TestCase):

    def test_ordering_by_nick_name(self):
        # Given
        aaron = Partner.objects.create(name='aaron', nick_name='Zorro')
        bert = Partner.objects.create(name='bert')
        zelda = Partner.objects.create(name='Zelda', nick_name='ace')
        ernie = Partner.objects.create(name='Ernie')
        # When
        qs = Partner.objects.order_by_nick_name()
        # Then
        self.assertEqual(list(qs), [zelda, aaron, ernie, bert])
        # And
        self.assertEqual(qs[2].nick_name_for_ordering, 'zzzzzzzz999995')

I'm not especially proud of that code, but it worked in Django 4.2 and 5.0 and passed the test. It fails in 5.1. My workaround for it, wrapping the ExpressionWrapper itself in a Cast solves the problem; as in the commented out snippet above. However, I don't see why that should have become necessary.

I don't know what causes this change in behaviour. I think this ticket may be related to https://code.djangoproject.com/ticket/26650

Change History (6)

comment:1 by Simon Charette, 4 weeks ago

In order to help with triaging could you please provide the exception you are encountering and the database backend you as using.

comment:2 by Siburg, 4 weeks ago

In order to help with triaging could you please provide the exception you are encountering and the database backend you as using.

Thank you for fast response. I'm running it on PostgreSQL. Exception from the test is

psycopg.errors.InvalidTextRepresentation: invalid input syntax for type bigint: ""
LINE 1: ... '') || COALESCE((999999 - "calls_partner"."id"), '')) ELSE ...

From various trials and errors that I undertook I learned that the problem is not actually with the COALESCE though. The problem is caused by the ExpressionWrapper.

comment:3 by Simon Charette, 4 weeks ago

Well ExpressionWrapper should not change the SQL generated at all so unless you are getting a Django level crash about issues resolving output_field, which doesn't seem to be case here, there is likely something else at play.

Could you provide the SQL that was previously generated, your Postgres version (if it changed between Django upgrades), and which version of psycopg2 or psycopg you are using?

comment:4 by Simon Charette, 4 weeks ago

Resolution: invalid
Status: newclosed

The issue you are running into has more to do with Concat changes than ExpressionWrapper

The pre-5.1 SQL was along the lines of

CONCAT(('zzzzzzzz')::text, ((999999 - "test_34444_partner"."id"))::text)

and the post 5.1 SQL is

COALESCE('zzzzzzzz', '') || COALESCE((999999 - "test_34444_partner"."id"), '')

due to the change to string concatenation on Postgres to the immutable || in #34955.

The reason why your code broke is that you explicitly tell the ORM that ExpressionWrapper(999_999 - F('pk'), output_field=models.CharField()) resolves to CharField when it actually resolves to BigIntegerField which prevents the ORM from implicitly applying adequate casting.

Simply removing all usage of ExpressionWrapper should solve your issue as it's doing more harm than good in its current form.

Concat(
    Value("zzzzzzzz"),
    999_999 - F("pk"),
    output_field=models.CharField(),
)

comment:5 by Simon Charette, 4 weeks ago

Summary: ExpressionWrapper output_field no longer worksPassing expression with wrongly attributed text output_field to Concat crashes on Postgres due the lack of casting

comment:6 by Siburg, 4 weeks ago

Thank you. I'm using PostgreSQL 14 and psycopg == 3.2.4 . Just tried it out and the test passes with SQLite, so it is indeed a PostgreSQL thing.

I appreciate your simplification of removing the ExpressionWrapper altogether. Happy to confirm that indeed works, and will adopt that in my code.

But, still surprised by the unexpected breakage in Django 5.1. I naively expected that specifying output_field=models.CharField() for an ExpressionWrapper would lead to the intended result. I think that is the gist of https://code.djangoproject.com/ticket/26650 as well. So it's annoying that such a specification is useless, and the ExpressionWrapper will provide a bigint anyway. Perhaps it would be good to clarify the documentation in https://docs.djangoproject.com/en/5.1/ref/models/expressions/#output-field.

Thinking a little more about it, I realize that output_field is a very trick thing and can depend on the database. For example, I am aware from painful experience that SQLite causes problems with DecimalField. So I don't have any clear solution myself for this breakage in 5.1.

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