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

Issue #11648 - Introducing HttpDateTime class. #11672

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Apr 19, 2024

  • Introduces HTTP (and Cookie) Date/Time parsing according to spec algorithms.
  • Introduces formatting according to spec mandated preferred RFC 1123 format.
  • Deprecate DateParser

This an alternative implementation for PR #11658

Fixes: #11648

+ Introduces HTTP (and Cookie) Date/Time parsing
  according to spec algorithms.
+ Introduces formatting according to spec
  mandated preferred RFC 1123 format.
+ Deprecate DateParser
@joakime joakime added the Bug For general bugs on Jetty side label Apr 19, 2024
@joakime joakime requested review from gregw and sbordet April 19, 2024 16:18
@joakime joakime self-assigned this Apr 19, 2024
@joakime
Copy link
Contributor Author

joakime commented Apr 19, 2024

As of commit f101889 these are the performance numbers.

Benchmark                                         Mode  Cnt        Score       Error  Units
HttpDateTimeParseBenchmark.testParseDateTimeOld  thrpt   10   348595.650 ± 18537.894  ops/s
HttpDateTimeParseBenchmark.testParseNew          thrpt   10  2028949.555 ± 81101.954  ops/s

The testParseDateTimeOld is the old DateParser.parseDate() behavior.
The testParseNew is the new HttpDateTime.parse() behavior.

@joakime joakime added the Specification For all industry Specifications (IETF / Servlet / etc) label Apr 19, 2024
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

sorry, on re-review, a couple of things...

Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

another niggle

continue; // skip blank tokens

// RFC 6265 - Section 5.1.1 - Step 2.1 - time (00:00:00)
if (hour == (-1) && token.length() == 8 && token.charAt(2) == ':' && token.charAt(5) == ':')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid of parens around -1 everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@joakime joakime requested review from sbordet and gregw April 23, 2024 13:33
sbordet
sbordet previously approved these changes Apr 23, 2024
sbordet
sbordet previously approved these changes Apr 23, 2024
*/
public static String format(TemporalAccessor datetime)
{
return DateTimeFormatter.RFC_1123_DATE_TIME
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting! IT does the hard coded "GMT" for us, even if we pass in "UTC". Nice! Well not nice... ugly history in evidence... but at least it is consistent. The time is in UTC, but just reported as "GMT"!

@joakime joakime merged commit 4cc9384 into jetty-12.0.x Apr 23, 2024
10 checks passed
@joakime joakime deleted the fix/12.0.x/http-date-time branch April 23, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side Specification For all industry Specifications (IETF / Servlet / etc)
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Introduce new HttpDateTime class for parsing obsolete Date formats in HTTP and Cookie
3 participants