Serialize webhook's release payload published_at property as a string #5966
Conversation
way if need to forward.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5966 +/- ##
==========================================
Coverage 74.20% 74.20%
- Complexity 5399 5402 +3
==========================================
Files 378 379 +1
Lines 19568 19577 +9
Branches 2036 2037 +1
==========================================
+ Hits 14520 14527 +7
- Misses 4072 4073 +1
- Partials 976 977 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -104,7 +104,7 @@ public static void handleGitHubBranchDeletion(WorkflowsApi workflowsApi, String | |||
| */ | |||
| public static void handleGitHubTaggedRelease(WorkflowsApi workflowsApi, String repository, String tagName, Date date, String username) { | |||
| final ReleasePayload 8000 releasePayload = new ReleasePayload(); | |||
| releasePayload.setRelease(new WebhookRelease().tagName(tagName).publishedAt(date.getTime())); | |||
| releasePayload.setRelease(new WebhookRelease().tagName(tagName).publishedAt(date)); | |||
There was a problem hiding this comment.
Changed this because before it was a long in the generated code.
There was a problem hiding this comment.
No need to change, but do you think it would be easy to tweak Jackson so that, by default, it would serialize/deserialize Timestamps using the desired patterns/code? Per RFC 3339, fractional seconds are allowed, which probably is why the generated OpenAPI code includes them:
https://datatracker.ietf.org/doc/html/rfc3339#section-5.6
Yes, they're allowed. It looks like Gary had tried to change it globally but commented it out for some reason. We'd have to do something like the 3rd answer (the first 2 answers, including the accepted one, didn't work!). I preferred, to keep the fix localized in case it broke something by changing it globally. Plus we're already passing around Timestamps as longs elsewhere, and we shouldn't change those (it'll break the CLI). |
Description
Ran into this when trying to redeliver events in dockstore-support.
published_atfield with a date in string form, e.g.,2024-07-23T21:30:12Zint64).In dockstore-support, when trying to read in a raw GitHub event we'd stored in s3 into an OpenAPI generated Java class, it would fail, because the OpenAPI generated code expected a long, but got a string.
So serialize
published_atas a string, so the type of the field is the same both when coming from GitHub and when being invoked with the OpenAPI.Next problem I ran into was into is that the OpenAPI client code serializes the date with milliseconds, e.g.,
2024-07-23T21:30:12.123Z, whereas the GitHub payload does not have milliseconds. The default Jackson deserializer cannot handle both cases, so I had to write a custom deserializer.Review Instructions
Verify that in https://qa.dockstore.org/api/static/swagger-ui/index.html#/workflows/handleGitHubTaggedRelease, the schema for ReleasePayload|WebHookRelease|published_at is a string.
Issue
SEAB-6466
Security and Privacy
If there are any concerns that require extra attention from the security team, highlight them here and check the box when complete.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install@RolesAllowedannotation