refactor(RSSHandlerRoutine): switch to Instant#1365
refactor(RSSHandlerRoutine): switch to Instant#1365christolis wants to merge 5 commits intoTogether-Java:developfrom
Conversation
Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
tj-wazei
left a comment
tj-wazei
left a comment
There was a problem hiding this comment.
Looks reasonable to me.
application/src/main/java/org/togetherjava/tjbot/features/rss/RSSHandlerRoutine.java
Outdated
Show resolved
Hide resolved
ankitsmt211
left a comment
ankitsmt211
left a comment
There was a problem hiding this comment.
everything looks good but we should handle the outdated timestamps stored in DB
tj-wazei
dismissed
their stale review
Dismissing until we figure out a way to migrate the existing data for this to work.
|
there's only two ways to handle this atleast the ones i can think of
second one has more moving pieces and requires more effort from someone with access to VPS, first one runs and handles things on the fly we can later remove the logic which handles thing on exception once we stop getting the INFO logs for |
| * @throws DateTimeParseException if the date cannot be parsed | ||
| */ | ||
| private static ZonedDateTime getZonedDateTime(@Nullable String date, String format) | ||
| private static Instant getInstantTimeFromFormat(@Nullable String date, String datePattern) |
There was a problem hiding this comment.
btw, having the need to rename this on a type-change indicates that the method name is flawed.
it should be renamed into getTimeFromFormat or more idiomatic parseTime.
Also note the inconsistency with the parameter names called date instead of time. To avoid confusion it should proibably be renamed into parseDateTime and String dateTime, String pattern`
Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
Handle rewriting timestamps from the specific RSS feed's format to a more unified Instant string format. If a DateTimeParseException is thrown while attempting to work with the date format, use that opportunity to attempt to parse that original date with the given date format pattern, then convert it into a string using 'Instant#toString'. If the conversion fails, simply return an empty Optional and let the rest of the code handle it from there since the new value will be overwritten in any case. Signed-off-by: Chris Sdogkos <work@chris-sdogkos.com>
|
@ankitsmt211 After some testing with the recently pushed changes, I was able to successfully handle outdated date-time strings in the database. No Flyway magic or offline scripts on the VPS needed. A review will be appreciated. |
|
Here's more context on the discussion around this PR https://discord.com/channels/272761734820003841/895717328359153664/1459639871726420020 |
ankitsmt211
left a comment
ankitsmt211
left a comment
There was a problem hiding this comment.
It seems my assumptions were wrong, time parsing wouldn't blow up
I have looked at the database, rss_feed stores Instant friendly timestamps for our active rss_feeds using "yyyy-MM-dd'T'HH:mm:ssXXX" datetime format from configuration.
So I have pointed out the error handling that we wouldn't need anymore. Apologies for the confusion, should have looked into this more carefully before putting out any comments.
| } catch (DateTimeParseException _) { | ||
| Optional<Instant> convertedDate = convertDateTimeToInstant(feedConfig); | ||
|
|
||
| if (lastSavedDate.isEmpty()) { | ||
| return Optional.empty(); | ||
| if (convertedDate.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| lastSavedDate = convertedDate.get(); |
There was a problem hiding this comment.
this wouldnt be raised, if there's an error then database is corrupted maybe just log this and move on (silent handling is no longer needed)
4 participants
From a recent conversation: