Fix issue with duplicate parsing of Cache-Control header#424
Closed
arturobernalg wants to merge 1 commit intoapache:5.3.xfrom
Closed
Fix issue with duplicate parsing of Cache-Control header#424arturobernalg wants to merge 1 commit intoapache:5.3.xfrom
arturobernalg wants to merge 1 commit intoapache:5.3.xfrom
Conversation
arturobernalg
force-pushed
the
feature/redundant-cache-control-validations
branch
from
4ad2ad5 to
3a4c595
Compare
ok2c
requested changes
Mar 14, 2023
Member
ok2c
left a comment
ok2c
left a comment
There was a problem hiding this comment.
@arturobernalg Two directives: no-cache and private should not be represented as a boolean as they may have a list of field names as a parameter. For now, if we do not use those parameters fro caching policies we can keep it like that.
| * The set of characters that can delimit a value in the header. | ||
| */ | ||
| private static final BitSet VALUE_DELIMS = Tokenizer.INIT_BITSET(EQUAL_CHAR, ','); | ||
| private static final BitSet VALUE_DELIMS = Tokenizer.INIT_BITSET(EQUAL_CHAR, ',', SEMICOLON_CHAR); |
Member
There was a problem hiding this comment.
@arturobernalg Why? Why did you put it back? Where is it being used?
|
|
||
| while (!cursor.atEnd()) { | ||
| final String name = tokenParser.parseToken(buffer, cursor, TOKEN_DELIMS); | ||
|
|
Member
There was a problem hiding this comment.
@arturobernalg Oh, man. This looks ugly. There should be a better way of doing the same.
Member
Author
There was a problem hiding this comment.
@ok2c
Perhaps, however, I am open to any help or advice as it has been challenging to fit all the pieces together. Nevertheless, I am determined to find the optimal solution.
Member
Author
There was a problem hiding this comment.
@arturobernalg Oh, man. This looks ugly. There should be a better way of doing the same.
@ok2c what about now?
Previously, the same Cache-Control header was being parsed twice, once by isExplicitlyNonCacheable and again by calculateFreshnessLifetime. The parsing code was extracted from calculateFreshnessLifetime and enhanced to include the main cache control directive that isExplicitlyNonCacheable could use to make its decision. This improves the efficiency and accuracy of the caching logic.
arturobernalg
force-pushed
the
feature/redundant-cache-control-validations
branch
from
d4995b6 to
10191fa
Compare
Member
|
Committed as 8ba0b17 with some changes to the parsing code. @arturobernalg The trick was to reverse the parsing flow and make it simple and linear. Fewer lines of code, fewer intermediate garbage as a result. And why on earth did you choose to use deprecated |
Member
Author
Member
|
Member
Author
damn. TY |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug where the same Cache-Control f3f07a3 header is parsed twice, leading to incorrect performance issues. The parsing code has been extracted from the calculateFreshnessLifetime method and is now enhanced to include the main cache control directive that the isExplicitlyNonCacheable method uses to make its decision. This makes the decision based on the main cache control directive instead of parsing the header twice.
Additionally, this PR adds a new method parseCacheControlHeader to the CacheControlParser class which parses the Cache-Control header and returns a CacheControl instance. This new method improves the parsing of the Cache-Control header and provides more accurate parsing of the different directives.
This PR also includes some code cleanup and refactoring.