-
Notifications
You must be signed in to change notification settings - Fork 1
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: accept date without time #125
Conversation
We were required to accept from CKAN both `issued` and `modified_at`, as they are part of DCAT-AP. The problem now is there are three date format for these fields. This hotfix will accept all these three format, as we have to decouple the deployment of this component. The issue GenomicDataInfrastructure/gdi-userportal-ckanext-gdi-userportal/95 on CKAN was created to unify datetime format The issue ART-9689 was create in Jira to remove this code duplication.
Reviewer's Guide by SourceryThis pull request addresses an issue with date parsing in the CKAN dataset repository. It modifies the code to accept three different date formats for the 'issued' and 'modified_at' fields, including dates without time information. The changes are implemented in two Java classes and one test class. Sequence DiagramsequenceDiagram
participant C as Caller
participant P as Parser
C->>P: parse(date)
alt date is in OffsetDateTime format
P->>P: OffsetDateTime.parse(date)
P-->>C: Return parsed OffsetDateTime
else date is not in OffsetDateTime format
P->>P: Check if date is date-only format
alt date is date-only format
P->>P: Append default time
end
P->>P: LocalDateTime.parse(dateToParse, DATE_FORMATTER)
P->>P: Convert to OffsetDateTime
P-->>C: Return parsed OffsetDateTime
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @brunopacheco1 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more robust date parsing library in the future to handle various date formats more elegantly and avoid potential timezone-related issues.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -162,7 +162,11 @@ private OffsetDateTime parse(String date) { | |||
try { | |||
return OffsetDateTime.parse(date); | |||
} catch (DateTimeParseException e) { | |||
return LocalDateTime.parse(date, DATE_FORMATTER) | |||
var dateToParse = date; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider extracting date parsing logic to a common utility method
This same date parsing logic appears in both CkanDatasetsRepository and PackageShowMapper. To improve maintainability and reduce duplication, consider creating a shared utility method for this date parsing operation.
@@ -194,7 +194,7 @@ void can_parse() { | |||
.id("resource_id") | |||
.title("resource_name") | |||
.description("resource_description") | |||
.createdAt(parse("2025-03-19T13:37:05Z")) | |||
.createdAt(parse("2025-03-19T00:00Z")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Test case updated to cover new date format, but edge cases are missing
While the test has been updated to use the new date format without time, it would be beneficial to add more test cases covering all three date formats mentioned in the PR description. This ensures that the new parsing logic handles all scenarios correctly.
.name("pdf")
.build())
.uri("uri")
.created("2025-03-19")
.lastModified("2025-03-19T13:37:05Z")
.build()))
.contactPoint(List.of(
.id("resource_id")
.title("resource_name")
.description("resource_description")
.createdAt(parse("2025-03-19T00:00Z"))
.createdAt(parse("2025-03-19T13:37:05Z"))
.createdAt(parse("2025-03-19"))
@hcvdwerf I merged the PR, as it is a hotfix to the current version. Let's talk tomorrow or on Monday to find out the best solution for this problem. |
We were required to accept from CKAN both
issued
andmodified_at
, as they are part of DCAT-AP. The problem now is there are three date format for these fields.This hotfix will accept all these three format, as we have to decouple the deployment of this component.
The issue GenomicDataInfrastructure/gdi-userportal-ckanext-gdi-userportal/95 on CKAN was created to unify datetime format.
The issue ART-9689 was create in Jira to remove this code duplication.
Summary by Sourcery
Fix date parsing to accept date strings without time components by appending a default time value, ensuring compatibility with multiple date formats as required by DCAT-AP. Update tests to cover the new date handling logic.
Bug Fixes:
Tests: