Skip to content

Convert MS-format datetimes without platform-bound fromtimestamp (#59)#229

Open
CedricConday wants to merge 1 commit into
XeroAPI:masterfrom
CedricConday:fix/59-datetime-ms-platform-independent
Open

Convert MS-format datetimes without platform-bound fromtimestamp (#59)#229
CedricConday wants to merge 1 commit into
XeroAPI:masterfrom
CedricConday:fix/59-datetime-ms-platform-independent

Conversation

@CedricConday

Copy link
Copy Markdown

Fixes #59

get_employees (and other calls) intermittently fail for some organisations with:

File .../api_client/deserializer.py, in deserialize_datetime_ms
    return datetime.datetime.fromtimestamp(timestamp_s, tz=tz_info)
OSError: [Errno 22] Invalid argument

datetime.fromtimestamp delegates to the platform C library and is not portable at the edges:

  • on Windows it raises OSError for pre-1970 (negative) timestamps;
  • for out-of-range timestamps it leaks OSError / OverflowError instead of the ValueError this function documents ("Raise ValueError if the input is well formatted but not a valid datetime").

Some Xero responses carry such /Date(...)/ values, so the SDK raises an unexpected error type that callers can't sensibly catch.

Change

Build the datetime from the Unix epoch with timedelta, which is platform independent, and coerce genuinely unrepresentable values to the documented ValueError:

epoch = datetime.datetime(1970, 1, 1, tzinfo=tz.UTC)
try:
    utc_datetime = epoch + datetime.timedelta(milliseconds=timestamp_ms)
except (OverflowError, OSError) as error:
    raise ValueError("Invalid datetime value {!r}".format(data)) from error
return utc_datetime.astimezone(tz_info)

This is behaviour-preserving for all in-range values (verified against the existing test_deserialize_datetime_ms cases, including the +1300 offset).

Tests

Added to test_deserializer.py:

  • test_deserialize_datetime_ms_pre_epoch — a pre-1970 timestamp now converts cleanly (this is the Windows scenario from the issue; it can't fail in Linux CI but is what users hit).
  • test_deserialize_datetime_ms_out_of_range_raises_value_error — an out-of-range timestamp now raises the documented ValueError. This fails on master (leaks OSError) and passes with the fix.

Full test_api_client suite (149 tests) stays green.

…oAPI#59)

deserialize_datetime_ms used datetime.fromtimestamp, which is bounded by
the platform C library: it raises OSError on Windows for pre-1970
(negative) timestamps and leaks OSError/OverflowError for out-of-range
values. Some Xero organisations return such timestamps, turning a normal
API call (e.g. get_employees) into an uncaught OSError.

Build the datetime from the Unix epoch with timedelta instead, which is
platform independent, and surface the ValueError the function already
documents for values python's datetime cannot represent.

Closes XeroAPI#59.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

get_employees causing error in deserializer.py

1 participant