Skip to content

isISO8601Date rejects valid ISO 8601 timestamps, including the form the NWB schema recommends #329

Description

@rly

AQNWB::isISO8601Date (src/Utils.hpp:200) requires a fractional seconds part and a numeric ±HH:MM offset. Both are optional in ISO 8601.

const std::string iso8601Pattern =
    R"(^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+[+-]\d{2}:\d{2}$)";

This has stayed hidden because getCurrentTime() (src/Utils.hpp:174) emits exactly the one accepted shape, so AqNWB generated values always pass. It bites on caller supplied values, at NWBFile.cpp:81 (sessionStartTime), NWBFile.cpp:86 (timestampsReferenceTime), and Subject.cpp:73 (date_of_birth).

Rejected, all valid ISO 8601:

  • 2024-01-15T00:00:00Z, the UTC designator
  • 2024-01-15T00:00:00.000Z, the form the NWB schema's isodatetime doc text describes ("Dates stored in UTC end in "Z" with no timezone offset. Date accuracy is up to milliseconds.")
  • 2024-01-15T00:00:00+00:00, what datetime.isoformat() produces for a whole second
  • 2024-01-15T00:00:00.123+0000, basic format offset

The tests assert the wrong contract

Four assertions in tests/testUtilsFunctions.cpp:21-36 classify valid ISO 8601 as invalid, with comments that do not hold. Fractional seconds are optional, +0200 and -0800 are valid basic format offsets, and Z specifies UTC rather than omitting it.

REQUIRE_FALSE(AQNWB::isISO8601Date("2018-09-28T14:43:54+02:00"));     // Missing fractional seconds
REQUIRE_FALSE(AQNWB::isISO8601Date("2018-09-28T14:43:54.123+0200"));  // Missing colon in timezone
REQUIRE_FALSE(AQNWB::isISO8601Date("2018-09-28T14:43:54.123Z"));      // Missing timezone offset
REQUIRE_FALSE(AQNWB::isISO8601Date("2018-09-28T14:43:54.123-0800"));  // Incorrect timezone format

The other three assertions in that section are correct and should stay: a space in place of T, a string with no timezone information, and free text. Requiring an explicit timezone is a real NWB best practice.

Proposed change

const std::string iso8601Pattern =
    R"(^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?(Z|[+-]\d{2}:?\d{2})$)";

Checked against every string above and every string the current tests accept. It still rejects the hour only offset (+02), which ISO 8601 permits but nothing in the NWB ecosystem emits.

Relatedly, the function name says Date while it validates a datetime. Rename to isISO8601DateTime?

Acceptance criteria

  • The four forms listed above are accepted, and getCurrentTime() output still validates.
  • Strings with no timezone designator, with a space in place of T, and free text remain rejected.
  • tests/testUtilsFunctions.cpp moves the four mislabeled cases to the valid section and drops the incorrect comments.

Out of scope: a bare date such as 2024-01-15 stays rejected, since the NWB dtype for all three fields is isodatetime.

Generated by Claude Code and reviewed by me.

Activity

  1. rly commented on Sep 3, 2026

    @rly
    ContributorAuthor

    Note on provenance: This was found by Claude Opus 5 while reviewing #320. The output below was produced by building and running the code, but I have only briefly reviewed the analysis myself and I am not an expert in this area. Please review it critically.

    Worth recording here because the downstream effect on Subject is larger than a validation helper returning the wrong answer.

    Passing dateOfBirth = "2024-01-15T00:00:00+00:00" to NWBFile::initialize makes it return Status::Failure, even though the file it produced is complete and valid apart from the missing date_of_birth. A caller that treats a non Success return as "recording setup failed" aborts on good data.

    Measured acceptance, calling isISO8601Date directly
    "2024-01-15T00:00:00.000000+00:00"  true    <- the shape getCurrentTime() emits
    "2024-01-15T00:00:00+00:00"         false
    "2024-01-15T00:00:00Z"              false
    "2024-01-15T00:00:00.123456Z"       false
    "2024-01-15"                        false
    "2024-01-15T00:00:00"               false
    "2024-01-15T00:00:00.5+00:00"       true
    

    The resulting file state after that Failure:

    NWBFile::initialize returned Failure
    /general/subject   exists
      subject_id       exists
      species          exists
      date_of_birth    absent
    

    So the group is written, everything except date_of_birth is written, and the caller is told it failed. The partial write, and the fact that a corrected retry reports success while changing nothing, are tracked separately in #338.

  2. added
    category: bugerrors in the code or code behavior
    priority: mediumnon-critical problem and/or affecting only a small set of users
    topic: apiissues related to the core AqNWB API
    on Sep 3, 2026
  3. added a commit that references this issue on Sep 3, 2026
    a6f7365
  4. added this to the 0.4.0 milestone on Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    category: bugerrors in the code or code behaviorpriority: mediumnon-critical problem and/or affecting only a small set of userstopic: apiissues related to the core AqNWB APItopic: testingissues related to testing

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions