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.

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

ASCII_Date_Time_* do not sufficiently check valid days of a month or seconds #434

Closed
jordanpadams opened this issue Feb 15, 2022 · 8 comments · Fixed by #462
Closed

ASCII_Date_Time_* do not sufficiently check valid days of a month or seconds #434

jordanpadams opened this issue Feb 15, 2022 · 8 comments · Fixed by #462
Assignees
Labels
B13.0 bug Something isn't working Epic invalid This doesn't seem right needs:receivable s.medium

Comments

@jordanpadams
Copy link
Member

jordanpadams commented Feb 15, 2022

🐛 Describe the bug

Per NASA-PDS/harvest#81, it looks like the PDS4 IM schematron rules do not sufficiently check the proper days for each month.

In the example in the ticket above,

<stop_date_time>2012-06-31T02:22:47.956Z</stop_date_time> 

is valid.

Additionally, this looks to validate successfully:

<stop_date_time>2012-02-30T02:22:47.956Z</stop_date_time> 

New update found 2022-02-17, issue with allowing 60 seconds. This comes back valid and it shouldn't:

<stop_date_time>2012-02-01T02:22:60.956Z</stop_date_time> 

🕵️ Expected behavior

If an invalid day of month is given, the schematron should trigger an error.

For additional test cases see #487

@rsjoyner
Copy link

I have not tested this against every variation of PDS date and date-time (e.g., ASCII_Date_Time_YMD_UTC, ASCII_Date_YMD, etc).
But this new and improved schematron rule appears to work to validate the above month-day conditions including leap-year.

<sch:pattern>
  <sch:rule context="pds:Time_Coordinates/pds:start_date_time">
    <sch:let name="type-date" value="if (contains(.,'T')) then (xs:dateTime(.)) else (xs:date(.))"/>
    <sch:let name="date-sansZ" value="translate(., 'Z', '')"/>
    
    <sch:assert test="$type-date = .">
      <title>pds:Time_Coordinates/pds:start_date_time</title>
      The value for "pds:Time_Coordinates/pds:start_date_time" does not appear to be a valid date-time: '<sch:value-of select="."/>' - '<sch:value-of select="$type-date"/>' </sch:assert>
  </sch:rule>
  <sch:rule context="pds:Time_Coordinates/pds:stop_date_time">
    <sch:let name="type-date" value="if (contains(.,'T')) then (xs:dateTime(.)) else (xs:date(.))"/>
    <sch:let name="date-sansZ" value="translate(., 'Z', '')"/>
    
    <sch:assert test="$type-date = .">
      <title>pds:Time_Coordinates/pds:stop_date_time</title>
      The value for "pds:Time_Coordinates/pds:stop_date_time" does not appear to be a valid date-time: '<sch:value-of select="."/>' - '<sch:value-of select="$type-date"/>' </sch:assert>
  </sch:rule>      
</sch:pattern>

Apparently, I cannot drop the SCH file here. So, I will do what I have done for millennia and email the updated SCH to J.Hughes.

@jordanpadams jordanpadams changed the title ASCII_Date_Time_* do not sufficiently check valid days of a month ASCII_Date_Time_* do not sufficiently check valid days of a month or seconds Feb 18, 2022
@tloubrieu-jpl
Copy link
Member

@rsjoyner @jshughes let us know when you can make progress on these tickets. Thanks

jshughes pushed a commit that referenced this issue Apr 12, 2022
Issue #434 ASCII_Date_Time_* do not sufficiently check valid days of a month or seconds

Per NASA-PDS/harvest#81, it looks like the PDS4 IM schematron rules do not sufficiently check the proper days for each month.

Resolves #434
jshughes added a commit that referenced this issue Apr 12, 2022
Issue #434 ASCII_Date_Time_* do not sufficiently check valid days of a month or seconds

Per NASA-PDS/harvest#81, it looks like the PDS4 IM schematron rules do not sufficiently check the proper days for each month.

Resolves #434

Co-authored-by: John Hughes <jsh416@gmail.com>
@jordanpadams jordanpadams reopened this Jun 3, 2022
@jordanpadams
Copy link
Member Author

Reverted changes in order to build a more sufficient test suite to ensure the solution does not break any other time formats.

@jpl-jengelke
Copy link
Contributor

This is verified with the new files provided today. There is now no exception. Diff-ing the schematron and schema files shows that exactly that section identified above by @rchenatjpl was removed from the new files.

See attached test execution report.
434_execution_new_I_files.txt

PASS

@jordanpadams
Copy link
Member Author

@jimmie
Copy link
Member

jimmie commented Aug 11, 2022

CCB discussions continuing on this.

@matthewtiscareno
Copy link

CCB discussions continuing on this.

You mean DDWG, not CCB.

The DDWG is responsible for drafting proposals for implementation. The CCB only gives an up-or-down vote to what the DDWG produces.

@jordanpadams
Copy link
Member Author

Closing this as invalid. will be replaced by CCB-350 and #502

@jordanpadams jordanpadams added the invalid This doesn't seem right label Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B13.0 bug Something isn't working Epic invalid This doesn't seem right needs:receivable s.medium
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants