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

fix: date parsing #426

Merged

Conversation

sabejensen
Copy link
Contributor

Summary of Changes

Added transform function for parsing dates from multiple formats. Added transform function to basic messaging.

Related Issues

N/A

Pull Request Checklist

  • All commits contain a DCO Signed-off-by line (we use the DCO GitHub app to enforce this).
  • Updated LICENSE-3RD-PARTY.md for any added dependencies or vendored components.
  • Added tests for changed code (run scripts/preflight to run tests and check code style).
  • Prefixed code comments with GitHub nick and an appropriate prefix.
  • Coding style is consistent with the rest of the framework.
  • Updated documentation for changed code and new or modified features.

PR template adapted from the Python attrs project.

Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
Signed-off-by: seajensen <seajensen@gmail.com>
@sabejensen sabejensen requested a review from a team as a code owner August 17, 2021 16:51
@TimoGlastra
Copy link
Contributor

@seajensen What format is not working with the JS date? Basic message uses ISO8601, which I think JS supports

@TimoGlastra TimoGlastra changed the title Feature/date parsing fix: date parsing Aug 17, 2021
@sabejensen
Copy link
Contributor Author

@seajensen What format is not working with the JS date? Basic message uses ISO8601, which I think JS supports

Apologies, I forgot to link the issue. This is in reference to #301. @JamesKEbert might be able to fill in some additional details.

@JamesKEbert
Copy link
Contributor

Yes! I had meant to follow up on the issue. So, from my findings, ISO 8601 dates do not allow spaces in the name, but RFC 3339 that is an extension to ISO 8601 (as I currently understand it) would allow a space. You are correct that we can parse ISO 8601 strings in both React Native and Node (and I assume Electron), however what is being received from ACA-Py isn't strictly ISO 8601, which maybe is something I should raise on that side.
Regardless, I was unable to find any polyfill for this unfortunately, and date issues are not exactly a rare occurrence. Using Luxon gives additional flexibility and assurance of parsing across different environments. Happy to hear your thoughts though!

@berendsliedrecht
Copy link
Contributor

berendsliedrecht commented Aug 18, 2021

ISO 3339 is indeed an extension of ISO 8601. Both ISO's specify that you are allowed to use another delimiter if ALL interacting parties know this ( quite odd IMO ).

The Basic messaging RFC also gives an example for an ISO 8601 date with a space as delimiter between date and time.

My suggestion is for ACA-Py to change it and change it also in the RFC. However, since both RFCs technically allow it, I am not opposed to this fix.

@@ -1,7 +1,8 @@
import { Expose, Type } from 'class-transformer'
import { Expose, Transform, Type } from 'class-transformer'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { Expose, Transform, Type } from 'class-transformer'
import { Expose, Transform } from 'class-transformer'

Signed-off-by: seajensen <seajensen@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #426 (b05dc78) into main (0e2338f) will decrease coverage by 0.68%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #426      +/-   ##
==========================================
- Coverage   86.24%   85.55%   -0.69%     
==========================================
  Files         249      249              
  Lines        5264     5352      +88     
  Branches      784      800      +16     
==========================================
+ Hits         4540     4579      +39     
- Misses        724      773      +49     
Impacted Files Coverage Δ
packages/core/src/utils/transformers.ts 72.72% <55.55%> (-11.89%) ⬇️
...rc/modules/basic-messages/messages/BasicMessage.ts 100.00% <100.00%> (ø)
packages/core/src/agent/MessageSender.ts 71.92% <0.00%> (-2.30%) ⬇️
packages/core/src/agent/Agent.ts 94.68% <0.00%> (-2.20%) ⬇️
.../core/src/modules/connections/ConnectionsModule.ts 72.15% <0.00%> (-1.88%) ⬇️
...ckages/core/src/transport/WsOutboundTransporter.ts 3.94% <0.00%> (-1.32%) ⬇️
...ckages/core/src/modules/routing/RecipientModule.ts 63.04% <0.00%> (-0.96%) ⬇️
.../modules/connections/models/did/service/Service.ts 100.00% <0.00%> (ø)
.../modules/connections/services/ConnectionService.ts 98.83% <0.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e2338f...b05dc78. Read the comment docs.

Copy link
Contributor

@JamesKEbert JamesKEbert left a comment

Choose a reason for hiding this comment

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

LGTM

@TimoGlastra TimoGlastra linked an issue Aug 20, 2021 that may be closed by this pull request
@TimoGlastra
Copy link
Contributor

Yes! I had meant to follow up on the issue. So, from my findings, ISO 8601 dates do not allow spaces in the name, but RFC 3339 that is an extension to ISO 8601 (as I currently understand it) would allow a space. You are correct that we can parse ISO 8601 strings in both React Native and Node (and I assume Electron), however what is being received from ACA-Py isn't strictly ISO 8601, which maybe is something I should raise on that side.
Regardless, I was unable to find any polyfill for this unfortunately, and date issues are not exactly a rare occurrence. Using Luxon gives additional flexibility and assurance of parsing across different environments. Happy to hear your thoughts though!

@JamesKEbert As I understand it from this thread, ACA-Py does send ISO 8601.

https://chat.hyperledger.org/group/os2iqWFM5k2usnrEP?msg=RR5mMYJdxFcYFBLMQ

And after doing a bit more research, it seems that JS Date only supports a simplified version of ISO8601. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse#parameters

So ACA-Py is not necessarily the issue here, which makes this PR more an appropriate fix imo.

Is basic message the only date that should be in ISO8601 format? No other properties/messages?

@swcurran
Copy link
Contributor

Not quite, Timo. ACA-Py does send ISO860, but is not sending timezone because the default call in Python doesn't send it. So, yes, we are going to fix ACA-Py to include the timezone. However, all recipients of W3C proofs have to handle cases with and without timezones.

@JamesKEbert
Copy link
Contributor

@TimoGlastra it's worth noting @blu3beri's thoughts above:

ISO 3339 is indeed an extension of ISO 8601. Both ISO's specify that you are allowed to use another delimiter if ALL interacting parties know this ( quite odd IMO ).

The Basic messaging RFC also gives an example for an ISO 8601 date with a space as delimiter between date and time.

My suggestion is for ACA-Py to change it and change it also in the RFC. However, since both RFCs technically allow it, I am not opposed to this fix.

I'd recommend the Aries RFC changing to not include the space for additional clarity--however, you are correct that this is technically feasible under ISO 8601 according to Berend's findings, since it's defined in the Aries basic messaging RFC. I am not actually immediately sure on other date fields in Aries (I hadn't gotten to it), but I can look into it.

@JamesKEbert JamesKEbert merged commit 2d31b87 into openwallet-foundation:main Aug 24, 2021
@JamesKEbert JamesKEbert deleted the feature/dateParsing branch August 24, 2021 19:27
@JamesKEbert
Copy link
Contributor

Ahh I just identified/realized in RFC 74 DIDComm Best Practices that the use of a space instead of "T" is allowed and called out as supported behavior.

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.

ISO Date Parsing Failure in React Native
6 participants