Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NXDRIVE-2967: Upgrade the deprecated datetime adapter #5313

Open
wants to merge 5 commits into
base: wip-NXDRIVE-2929-upgrade-python-from-3.9.5-to-3.12.3
Choose a base branch
from
+27 −12
Open

NXDRIVE-2967: Upgrade the deprecated datetime adapter #5313

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
aa11d4e
NXDRIVE-2967: Upgrade the deprecated datetime adapter
gitofanindya Oct 7, 2024
853392b
NXDRIVE-2967: Upgrade the deprecated datetime adapter --001
gitofanindya Oct 8, 2024
88f8a6f
Update test_utils.py
gitofanindya Oct 22, 2024
62a40e6
Update base.py
gitofanindya Oct 25, 2024
1fc22e7
Update base.py
gitofanindya Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion nxdrive/dao/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,24 @@ def execute(self, sql: str, parameters: Iterable[Any] = ()) -> Cursor:
while True:
count += 1
try:
return super().execute(sql, parameters)
if parameters:
import datetime
import sqlite3
new_param = []
for param in parameters:
if type(param) == datetime.datetime:
def adapt_datetime_iso(val):
return datetime.datetime.fromtimestamp(str(val), datetime.UTC)
mtime = sqlite3.register_adapter(param, adapt_datetime_iso)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: Improve datetime adaptation for SQLite

The current implementation of adapt_datetime_iso is incorrect and the adapter registration is inefficient. Consider using isoformat() for datetime conversion and register the adapter once outside the loop. For example:

def adapt_datetime_iso(val):
return val.isoformat()

sqlite3.register_adapter(datetime.datetime, adapt_datetime_iso)

In the loop

if isinstance(param, datetime.datetime):
new_param.append(param) # SQLite will use the registered adapter

new_param.append(mtime)
else:
new_param.append(param)

new_param = tuple(new_param)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (performance): Optimize parameter processing

Instead of creating a list and then converting it to a tuple, consider using a generator expression for better performance. For example:

new_param = tuple(adapt_datetime_iso(param) if isinstance(param, datetime.datetime) else param for param in parameters)

def adapt_datetime_iso(val):
    return datetime.datetime.fromtimestamp(str(val), datetime.UTC)

sqlite3.register_adapter(datetime.datetime, adapt_datetime_iso)

new_param = tuple(
    adapt_datetime_iso(param) if isinstance(param, datetime.datetime) else param
    for param in parameters
)

res = super().execute(sql, new_param)
else:
res = super().execute(sql, parameters)
return res
except OperationalError as exc:
log.info(
f"Retry locked database #{count}, {sql=}, {parameters=}",
Expand Down
4 changes: 0 additions & 4 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,6 @@ def no_warnings(recwarn):
continue
elif "Cryptography will be significantly faster" in message:
continue
elif "The default datetime adapter is deprecated as of Python 3.12" in message:
continue
Comment on lines -98 to -99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Removal of warning suppression for deprecated datetime adapter

The removal of this warning suppression suggests that the issue with the deprecated datetime adapter has been addressed. However, it would be beneficial to add a test that verifies the new datetime handling in the execute method of nxdrive/dao/base.py.

@pytest.fixture(autouse=True)
def ignore_warnings(request):
    warnings.filterwarnings("ignore", category=DeprecationWarning)
    warnings.filterwarnings("ignore", category=ResourceWarning)
    yield
    warnings.resetwarnings()

def test_datetime_handling():
    from nxdrive.dao.base import execute
    # Add test logic here to verify datetime handling

elif "datetime.datetime" in message:
continue

warn = f"{warning.filename}:{warning.lineno} {message}"
print(warn, file=sys.stderr)
Expand Down
Loading