Fixed #30128 -- Fixed handling timedelta timezone in database functions. #10910

cansarigol · 2019-01-28T08:12:14Z

cansarigol · 2019-01-28T08:16:20Z

django/db/backends/sqlite3/base.py

+        if '+' in tzname:
+            tzname = tzname.split('+')[0]
+        elif '-' in tzname:
+            tzname = tzname.split('-')[0]


after that, I want to add a code block whose purpose is to convert the string to time. Is there another way to solve this?

timgraham · 2019-02-09T22:04:30Z

Does one test cover all the new code? Oracle also needs to be addressed. Possibly add something to the 3.0 release notes under "Database backend API" about how third party databases need to adapt.

carltongibson

Bar release note change, looks OK I think.

buildbot, test on oracle.

carltongibson · 2019-02-28T10:19:48Z

docs/releases/3.0.txt

@@ -213,6 +213,15 @@ backends.

 * ``DatabaseIntrospection.get_field_type()`` may no longer return tuples.

+* ``DatabaseOperations._prepare_tzname_delta()`` is used to customize the database spesific time zone delta function.


I don't think this block quite makes sense as it is: _prepare_tzname_delta() is a private helper used only in the already private _convert_field_to_tz() — there's no guarantee at all that a backend will implement either. (The SQLite backend doesn't, for example...)

Maybe just something like:

Supported database backends now correctly handle ``timezone`` formats when created using `timedelta` instances: >>> from datetime import timezone, timedelta >>> str(timezone(timedelta(hours=5))) 'UTC+05:00' Third-party backends should ensure that they correctly handle this format when preparing ``datetime`` fields in ``datetime_cast_date_sql()`` & co.

(@felixxm, @timgraham?)

Docs should be wrapped to 79 chars.

carltongibson · 2019-02-28T14:04:01Z

New test fails on Oracle.

cansarigol · 2019-02-28T15:10:09Z

I had checked on Oracle. I've checked again and I can see that the test is successful. Could you share something with me to create your env on my test env

charettes · 2019-02-28T15:11:34Z

@cansarigol it fails with ORA-01882: timezone region not found on CI and with the same error when run locally.

carltongibson · 2019-02-28T15:13:22Z

Time to call in the fire service... 🚒

@felixxm: ORACLE ISSUE. PING. 😀

cansarigol · 2019-02-28T15:15:57Z

@cansarigol it fails with ORA-01882: timezone region not found on CI and with the same error when run locally.

I use OracleVM and I guess it is a version problem. I use orcl12c

cansarigol · 2019-02-28T15:28:52Z

I found this page, maybe would be helpful. https://community.oracle.com/thread/841802

timgraham

As I tried to suggest in a previous comment, test coverage doesn't seem sufficient. I think the '-' in tzname: branches aren't tested.

timgraham · 2019-02-28T15:36:01Z

django/db/backends/sqlite3/base.py

+                    time_delta_params = {}
+                    for attr in {'second', 'minute', 'hour', 'millisecond'}:
+                        if hasattr(_offset_date, attr):
+                            time_delta_params["%ss" % attr] = _offset_date.__getattribute__(attr)


getattr() rather than calling a dunder method

felixxm · 2019-02-28T15:45:32Z

@carltongibson I'll check in next few hours 🕵️‍♂️ , but Oracle version shouldn't be an issue.

cansarigol · 2019-02-28T16:29:15Z

As I tried to suggest in a previous comment, test coverage doesn't seem sufficient. I think the '-' in tzname: branches aren't tested.

I added the test_operations in mysql so far. Is it ok? I can add tests for all backends and all operations funcs (datetime_extract_sql, datetime_trunc_sql etc.) and also sqlite3.base. we can publish with ref commit @timgraham

carltongibson · 2019-02-28T16:40:32Z

@cansarigol I think the point is to include them here first time, rather than later. Any reason not to do that?
(Sorry, I missed that part of your earlier comment Tim.)

timgraham · 2019-02-28T17:03:59Z

I didn't expect a low-level test. Is testing extract with, e.g. timedelta(hours-5) insufficient?

cansarigol · 2019-02-28T18:07:32Z

@cansarigol I think the point is to include them here first time, rather than later. Any reason not to do that?
(Sorry, I missed that part of your earlier comment Tim.)

Sorry i couldn't understand that "include them" 😀

cansarigol · 2019-02-28T18:51:35Z

I didn't expect a low-level test. Is testing extract with, e.g. timedelta(hours-5) insufficient?

Sorry, you are right I added it for negative timedelta. there was a problem on sqlite3 and I could fix it through the test.

felixxm · 2019-02-28T19:14:25Z

Oracle failure is a great mystery for me, at least for now 🕵️‍♂️. I checked on Oracle 12c, 18c and in the same Oracle DB that we use in Jenkins, all passed without failures. This can be related with some env variables. I need to take a closer look ... 🔍

cansarigol · 2019-03-27T09:29:07Z

hi @felixxm did you have a chance to take a look? and is there any problem with the second commit?
Thanks

felixxm · 2019-03-28T12:17:21Z

@cansarigol Please send a fix to the 28373 as a separate PR, it is harder to trace fixes for multiple tickets in a single PR. Can you also rebase this branch from master and fix conflicts?

cansarigol · 2019-03-28T13:23:41Z

hi, commits are separated and rebased.

auvipy · 2019-04-06T17:37:00Z

sqlite failures related?

cansarigol · 2019-04-06T18:40:54Z

as i remember, it is unrelated. We just have problem about oracle tests.

felixxm · 2019-04-08T09:37:17Z

@cansarigol Please rebase from master and resolve conflicts, after that I will take another round.

cansarigol · 2019-04-08T09:59:52Z

@cansarigol Please rebase from master and resolve conflicts, after that I will take another round.

ok I will

felixxm

@cansarigol I left few comments.

django/db/backends/mysql/operations.py

django/db/backends/sqlite3/base.py

tests/db_functions/datetime/test_extract_trunc.py

django/db/backends/sqlite3/base.py

docs/releases/3.0.txt

claudep · 2019-06-13T18:10:44Z

Sorry, late to the party, but is there a reason _prepare_tzname_delta wasn't added in the BaseDatabaseOperations class?

felixxm · 2019-06-13T18:41:24Z

It is a part of internal API that is not implemented for all backends and it is used only in _convert_field_to_tz() that is also a part on internal API that is not implemented for all backends.

claudep · 2019-06-13T19:56:03Z

Thanks for explaining!

cansarigol commented Jan 28, 2019

View reviewed changes

cansarigol force-pushed the ticket-30128 branch from 4cd43ed to a7a7bc3 Compare February 12, 2019 07:18

cansarigol changed the title ~~Fixed #30128 -- Fixed timedelta for extract db model functions (myssql, postgresql, sqlite3)~~ Fixed #30128 -- Fixed timedelta for extract db model functions (myssql, postgresql, sqlite3, oracle) Feb 12, 2019

timgraham changed the title ~~Fixed #30128 -- Fixed timedelta for extract db model functions (myssql, postgresql, sqlite3, oracle)~~ Fixed #30128 -- Allowed using a timedelta timezone for extract db functions. Feb 14, 2019

carltongibson reviewed Feb 28, 2019

View reviewed changes

timgraham reviewed Feb 28, 2019

View reviewed changes

cansarigol force-pushed the ticket-30128 branch from a7a7bc3 to 8f9dc0c Compare February 28, 2019 16:25

cansarigol force-pushed the ticket-30128 branch from 8f9dc0c to 5dd2f83 Compare February 28, 2019 16:31

cansarigol force-pushed the ticket-30128 branch from 5dd2f83 to fd7e1e6 Compare February 28, 2019 18:46

cansarigol force-pushed the ticket-30128 branch from 088cf0d to ab4f168 Compare March 19, 2019 12:01

cansarigol force-pushed the ticket-30128 branch 2 times, most recently from a1bd9db to e2e6f6f Compare March 28, 2019 13:19

cansarigol force-pushed the ticket-30128 branch from e2e6f6f to 107dc9b Compare April 8, 2019 09:57

cansarigol force-pushed the ticket-30128 branch from 107dc9b to 16ef895 Compare April 8, 2019 10:28

carltongibson force-pushed the ticket-30128 branch from 16ef895 to 86ef63f Compare June 12, 2019 08:51

felixxm reviewed Jun 12, 2019

View reviewed changes

cansarigol force-pushed the ticket-30128 branch from 86ef63f to 35df0de Compare June 12, 2019 13:35

felixxm changed the title ~~Fixed #30128 -- Allowed using a timedelta timezone for extract db functions.~~ Fixed #30128 -- Fixed handling timedelta timezone in database functions. Jun 13, 2019

felixxm force-pushed the ticket-30128 branch from 35df0de to 509d0d0 Compare June 13, 2019 07:20

carltongibson reviewed Jun 13, 2019

View reviewed changes

docs/releases/3.0.txt Outdated Show resolved Hide resolved

Fixed #30128 -- Fixed handling timedelta timezone in database functions.

fde9b7d

felixxm force-pushed the ticket-30128 branch from 509d0d0 to fde9b7d Compare June 13, 2019 07:30

felixxm merged commit fde9b7d into django:master Jun 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed #30128 -- Fixed handling timedelta timezone in database functions. #10910

Fixed #30128 -- Fixed handling timedelta timezone in database functions. #10910

cansarigol commented Jan 28, 2019 •

edited

cansarigol Jan 28, 2019

timgraham commented Feb 9, 2019

carltongibson left a comment

carltongibson Feb 28, 2019

carltongibson commented Feb 28, 2019

cansarigol commented Feb 28, 2019 •

edited

charettes commented Feb 28, 2019

carltongibson commented Feb 28, 2019

cansarigol commented Feb 28, 2019

cansarigol commented Feb 28, 2019

timgraham left a comment

timgraham Feb 28, 2019

felixxm commented Feb 28, 2019

cansarigol commented Feb 28, 2019

carltongibson commented Feb 28, 2019

timgraham commented Feb 28, 2019

cansarigol commented Feb 28, 2019

cansarigol commented Feb 28, 2019

felixxm commented Feb 28, 2019

cansarigol commented Mar 27, 2019

felixxm commented Mar 28, 2019

cansarigol commented Mar 28, 2019

auvipy commented Apr 6, 2019

cansarigol commented Apr 6, 2019

felixxm commented Apr 8, 2019

cansarigol commented Apr 8, 2019

felixxm left a comment

claudep commented Jun 13, 2019

felixxm commented Jun 13, 2019

claudep commented Jun 13, 2019

		@@ -213,6 +213,15 @@ backends.

		* ``DatabaseIntrospection.get_field_type()`` may no longer return tuples.

		* ``DatabaseOperations._prepare_tzname_delta()`` is used to customize the database spesific time zone delta function.

Fixed #30128 -- Fixed handling timedelta timezone in database functions. #10910

Fixed #30128 -- Fixed handling timedelta timezone in database functions. #10910

Conversation

cansarigol commented Jan 28, 2019 • edited

cansarigol Jan 28, 2019

Choose a reason for hiding this comment

timgraham commented Feb 9, 2019

carltongibson left a comment

Choose a reason for hiding this comment

carltongibson Feb 28, 2019

Choose a reason for hiding this comment

carltongibson commented Feb 28, 2019

cansarigol commented Feb 28, 2019 • edited

charettes commented Feb 28, 2019

carltongibson commented Feb 28, 2019

cansarigol commented Feb 28, 2019

cansarigol commented Feb 28, 2019

timgraham left a comment

Choose a reason for hiding this comment

timgraham Feb 28, 2019

Choose a reason for hiding this comment

felixxm commented Feb 28, 2019

cansarigol commented Feb 28, 2019

carltongibson commented Feb 28, 2019

timgraham commented Feb 28, 2019

cansarigol commented Feb 28, 2019

cansarigol commented Feb 28, 2019

felixxm commented Feb 28, 2019

cansarigol commented Mar 27, 2019

felixxm commented Mar 28, 2019

cansarigol commented Mar 28, 2019

auvipy commented Apr 6, 2019

cansarigol commented Apr 6, 2019

felixxm commented Apr 8, 2019

cansarigol commented Apr 8, 2019

felixxm left a comment

Choose a reason for hiding this comment

claudep commented Jun 13, 2019

felixxm commented Jun 13, 2019

claudep commented Jun 13, 2019

cansarigol commented Jan 28, 2019 •

edited

cansarigol commented Feb 28, 2019 •

edited