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.

Sign up for GitHub

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

Jump to bottom

Only validate header name once (attempt #2) #24499

Merged
merged 3 commits into from
Mar 13, 2023
Merged

Only validate header name once (attempt #2) #24499

merged 3 commits into from
Mar 13, 2023

Conversation

jhanders34
Copy link
Member

@jhanders34 jhanders34 commented Feb 28, 2023 •
edited
Loading

  • Only validate header name when creating HttpHeaderKeys the first time.
  • Correctly validate byte[] header names. byte[].toString doesn't give you a String of the byte[] contents.
  • Make all HttpHeaderKeys constructors private to prevent anyone creating one without validation.
  • For get, contains, and remove do not throw IllegalArgumentException when the header name is invalid. Just ignore the header and do not created a HttpHeaderKeys object. Do the same for CR and LF characters.
  • Update HttpTrailerGeneratorImpl to do the correct equals. The code never would work as it was written.
  • Add test case for invalidate header names and make sure that each operation does what it is supposed to do.

Fixes #24427

This pull request is redo of PR #24382. It is a performance improvement for the changes done in #24157. Also going to tag #24187 since that is the PR associated with #24157.

@jhanders34 jhanders34 self-assigned this Feb 28, 2023
@jhanders34 jhanders34 requested a review from loriadi March 3, 2023 23:42
@jhanders34
Copy link
Member Author

#libby #build

@LibbyBot
Copy link

LibbyBot commented Mar 3, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_zsHoALoUEe2r6re_Zvbagg

Target locations of links might be accessible only to IBM employees.

@jhanders34
Only validate header name once
3830c81 
- Only validate header name when creating HttpHeaderKeys the first time.
- Correctly validate byte[] header names.  byte[].toString doesn't give
you a String of the byte[] contents.
- Make all HttpHeaderKeys constructors private to prevent anyone
creating one without validation.
@jhanders34 jhanders34 requested a review from mrsaldana March 9, 2023 21:49
@jhanders34
Copy link
Member Author

#libby #build

@LibbyBot
Copy link

LibbyBot commented Mar 9, 2023

Your personal build request is at https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_f-6C4L68Ee2o6J5YNcso_g

Target locations of links might be accessible only to IBM employees.

@jhanders34
Update the original commit to not throw exception on non set ops
7c0eda8 
- For get, contains, and remove do not throw IllegalArgumentException
when the header name is invalid.  Just ignore the header and do not
created a HttpHeaderKeys object.
- Update HttpTrailerGeneratorImpl to do the correct equals.  The code
never would work as it was written.
@jhanders34
Add test cases for request, response and trailers to make sure it works
c356deb 
- Add test cases for invalid header names and make sure for containers,
get and remove methods that we don't throw an exception, but for set and
append methods we do throw an exception
@jhanders34
Copy link
Member Author

#libby

@LibbyBot
Copy link

LibbyBot commented Mar 9, 2023

Code analysis and actions

DO NOT DELETE THIS COMMENT.
  • 6 product code files were changed.
  • Please describe in a separate comment how you tested your changes.

@LibbyBot
Copy link

The build jhanders34-24499-20230310-0614
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_NiELUL9EEe2o6J5YNcso_g
completed and has errors or failures.

For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_NiELUL9EEe2o6J5YNcso_g

@LibbyBot
Copy link

The build jhanders34-24499-20230310-0614
https://wasrtc.hursley.ibm.com:9443/jazz/resource/itemOid/com.ibm.team.build.BuildResult/_Tirp8L9EEe2o6J5YNcso_g
completed and has errors or failures.

For help analyzing your personal build, go to https://cognitive.hursley.ibm.com/buildAnalysis.html?uuid=_Tirp8L9EEe2o6J5YNcso_g

mrsaldana
mrsaldana approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@mrsaldana mrsaldana left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the work improving the performance here!

loriadi
loriadi approved these changes Mar 13, 2023
View reviewed changes
Copy link
Contributor

@loriadi loriadi left a comment

Choose a reason for hiding this comment

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

Thanks, Jared!

@jhanders34 jhanders34 merged commit c488ae8 into OpenLiberty:integration Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrsaldana mrsaldana mrsaldana approved these changes

@loriadi loriadi loriadi approved these changes

Assignees

@jhanders34 jhanders34

Labels
CLA Signed performance team:Performance
Projects
None yet
Milestone
[Iteration 23.5] Feb 27 - Mar 10
Development

Successfully merging this pull request may close these issues.

Re-introduce change reverted from 24382
4 participants
@jhanders34 @LibbyBot @loriadi @mrsaldana