Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#18176 closed Bug (fixed)

SQL queries for filtering datetime objects use incorrect format for years <1000

Reported by: rrotaru Owned by: rrotaru
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: datetime, SQL, year
Cc: flo@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Datetime objects with year values <1000 are auto-formatted to 4-digits by adding leading
zeroes (i.e. year 1 becomes 0001). Also, according to the docs, the __year lookup takes a
four digit year (i.e., again, 0000-9999). However, when attempting to use the __year lookup on
years <1000, the SQL generated lacks the leading zeroes and as a result, no matches are made.

Example with Poll objects:

>>> poll1 = Poll(question="This",pub_date=datetime.datetime(0001,01,01))
>>> poll1.save()
>>> Poll.objects.filter(pub_date__year=0001) #should return our poll1
[]

Also, certain years <1000, such as 0999, or years ending in a 9 with at least one leading zero
generate "invalid token" syntax errors

>>> poll2 = Poll(question="That",pub_date=datetime.datetime(0999,01,01))
>>> poll2.save()
>>> Poll.objects.filter(pub_date__year=0999) #should return our poll2
  Poll.objects.filter(pub_date__year=0999)
                                        ^
SyntaxError: invalid token

Note: This bug occurs when using sqlite3, I have not tried replicating it with other databases.
More discussion on this bug: https://groups.google.com/forum/?fromgroups#%21topic/django-developers/xPIQKbc4tiQ

Attachments (1)

sql_query_fix.diff (2.1 KB) - added by rrotaru 3 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 3 years ago by akaariai

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

The main problem I see here is that if 999 is changed to mean '0999' instead of 1999 (or is it 2999?) this could break existing applications.

How about allowing strings in year lookups - document that what 1 means is database dependent like now(?). If you want year 0001 you need to pass '0001'. The problem with this is this can be a little confusing for users. Opinions on this API idea welcome!

comment:2 Changed 3 years ago by rrotaru

It turns out that python interprets numbers with leading zeroes as octal input (hence why anything with a leading zero that also contains an 8 or 9 causes an invalid token error). It seems like allowing strings to be passed in year lookups would solve both issues and might be the most reasonable approach.

Note: __year lookup already supports strings, however something along the lines of

Poll.objects.filter(pub_date__year='0999')

Still generates the following query in SQLite:

{'sql': u'SELECT "polls_poll"."id", "polls_poll"."question", "polls_poll"."pub_date" FROM "polls_poll" WHERE "polls_poll"."pub_date" BETWEEN 999-01-01 and 999-12-31 23:59:59.999999 LIMIT 21',
  'time': '0.000'}]

(Notice how the years still lack the leading zeroes. This seems to be mainly an issue in SQLite.)

Last edited 3 years ago by rrotaru (previous) (diff)

comment:3 Changed 3 years ago by yamochao

Tried a similar test using the postgresql_psycopg2 engine and achieved the following results:

In [28]: poll1 = Poll(question="poll1?",pub_date=datetime.datetime(0001,01,01))

In [29]: poll1.save()

In [30]: Poll.objects.filter(pub_date__year=0001)
Out[30]: []

In [31]: poll2 = Poll(question="poll2?",pub_date=datetime.datetime(1,01,01))

In [32]: poll2.save()

In [33]: Poll.objects.filter(pub_date__year=1)
Out[33]: []

In [34]: poll3 = Poll(question="poll3?",pub_date=datetime.datetime(0999,01,01))
------------------------------------------------------------
   File "<ipython console>", line 1
     poll3 = Poll(question="poll3?",pub_date=datetime.datetime(0999,01,01))
                                                                  ^
SyntaxError: invalid token


In [35]: poll4 = Poll(question="poll4?",pub_date=datetime.datetime(999,01,01))

In [36]: poll4.save()

In [37]: Poll.objects.filter(pub_date__year=999)
Out[37]: [<Poll: poll4?>]

Note that poll4 works while poll3 does not. They should create equally valid queries, but django seems to nip poll3. Here are the generated queries:

{'sql': 'INSERT INTO "polls_poll" ("question", "pub_date") VALUES (E\'poll1?\', E\'0001-01-01 00:00:00\') RETURNING "polls_poll"."id"',
  'time': '0.001'},
 {'sql': 'SELECT "polls_poll"."id", "polls_poll"."question", "polls_poll"."pub_date" FROM "polls_poll" WHERE "polls_poll"."pub_date" BETWEEN E\'1-01-01 00:00:00\' and E\'1-12-31 23:59:59.999999\' LIMIT 21',
  'time': '0.002'},
 {'sql': 'INSERT INTO "polls_poll" ("question", "pub_date") VALUES (E\'poll2?\', E\'0001-01-01 00:00:00\') RETURNING "polls_poll"."id"',
  'time': '0.001'},
 {'sql': 'SELECT "polls_poll"."id", "polls_poll"."question", "polls_poll"."pub_date" FROM "polls_poll" WHERE "polls_poll"."pub_date" BETWEEN E\'1-01-01 00:00:00\' and E\'1-12-31 23:59:59.999999\' LIMIT 21',
  'time': '0.001'},
 {'sql': 'INSERT INTO "polls_poll" ("question", "pub_date") VALUES (E\'poll4?\', E\'0999-01-01 00:00:00\') RETURNING "polls_poll"."id"',
  'time': '0.001'},
 {'sql': 'SELECT "polls_poll"."id", "polls_poll"."question", "polls_poll"."pub_date" FROM "polls_poll" WHERE "polls_poll"."pub_date" BETWEEN E\'999-01-01 00:00:00\' and E\'999-12-31 23:59:59.999999\' LIMIT 21',
  'time': '0.001'}]

comment:4 Changed 3 years ago by anonymous

So, in django/db/backends/sqlite3 there is the following in base.py

def year_lookup_bounds(self, value):
    first = '%s-01-01'
    second = '%s-12-31 23:59:59.999999'
    return [first % value, second % value]

Would it not make more sense to have something like the following?

def year_lookup_bounds(self, value):
    first = datetime.datetime(value,1,1)
    second = datetime.datetime(value,12,31,23,59,59,999999)
    return [first, second]

This fixes the above issue and a similar patch can be applied to the base.py file in django/db/backends/mysql and django/db/backends/oracle

Changed 3 years ago by rrotaru

comment:5 Changed 3 years ago by rrotaru

  • Has patch set
  • Needs documentation set
  • Needs tests set

comment:6 Changed 3 years ago by fhahn

I think this issue is already fixed in master.

#18969 describes a similar issue and the test case provided there tests year lookups with a year < 1000. This test case worked for me with postgres and sqlite. The SQL is generated correctly. String lookups for __year work as well.

We should probably close either this ticket or #18969 as duplicate.

comment:7 Changed 3 years ago by fhahn

  • Needs documentation unset
  • Needs tests unset
  • Version changed from 1.4 to master

I've created a pull request with a test and updated documentation (__year, __month,... accept integer and strings) by #18969

comment:8 Changed 3 years ago by aaugustin

I closed #18969 as a duplicate of this one, I'll copy my comment below:

If the current implementation happens to accept strings, fantastic. But I'm against documenting it — because it means committing to support strings in these lookups forever.

Years, months, and friends are fundamentally integers and "there should be one-- and preferably only one --obvious way to do it."

In my opinion the only requirement to close this ticket is to add a test.

comment:9 Changed 3 years ago by fhahn

  • Cc flo@… added

I've moved the test from my pull request for #18969 and created a new one for this ticket, without the doc update: https://github.com/django/django/pull/847

comment:10 Changed 3 years ago by Florian Hahn <flo@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f28c301a4720ef110643728192c27b2ca0ed193a:

Fixed #18176 -- Added test for year lookups with year < 1000

Thanks Tomas Ehrlich for the initial test

comment:11 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In 86b96038f22237b9d1c5da9f7f9f6aae7afa2578:

Merge pull request #847 from fhahn/ticket_18176

Fixed #18176 -- Added test for year lookups with year < 1000

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