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

Fixes server ignores request to gzip data #14982

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

rose2221
Copy link

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

This PR fixes an issue where the Prysm beacon chain server ignores the Accept-Encoding: gzip header in HTTP requests and always returns uncompressed JSON responses.

Fix Details:

  • Updated middleware to properly respect the Accept-Encoding header.
  • Ensured that responses are compressed when requested.
  • Added error handling to prevent silent failures.

Which issue(s) does this PR fix?

Fixes #14593

Other Notes for Review

Acknowledgements

  • [✅] I have read CONTRIBUTING.md.
  • [✅] I have included a uniquely named changelog fragment file.
  • [✅] I have added a description to this PR with sufficient context for reviewers to understand this PR.

@rose2221 rose2221 requested a review from a team as a code owner February 24, 2025 10:11
@CLAassistant
Copy link

CLAassistant commented Feb 24, 2025

CLA assistant check
All committers have signed the CLA.

@rose2221 rose2221 changed the title Fixes #14593 Fixes server ignores request to gzip data Feb 24, 2025
@@ -112,6 +114,40 @@ func AcceptHeaderHandler(serverAcceptedTypes []string) Middleware {
}
}

func GzipMiddleware() Middleware {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you rename this to AcceptEncodingHeaderHandler because we already have AcceptHeaderHandler?

@rkapka
Copy link
Contributor

rkapka commented Feb 24, 2025

Hey @rose2221 , thanks for the contribution! This PR only adds gzip to the blobs endpoint, but it would make much more sense to have support on all endpoints that return data. Which means all endpoint that use middleware.AcceptHeaderHandler.

Before merging the PR you will also have to sign the Contributor License Agreement and add a changelog file.

@rose2221
Copy link
Author

rose2221 commented Feb 25, 2025

Thank you, @rkapka, for your suggestions! I've implemented the requested changes. Let me know if there's anything else that needs adjustment. Also, should I add tests for this as well?

@rkapka
Copy link
Contributor

rkapka commented Feb 25, 2025

Also, should I add tests for this as well?

That would be very helpful. Perhaps you could write a unit test where you:

  • have a dummy handler that returns some value
  • register the gzip middleware for this handler
  • execute the handler and check that the returned data is NOT equal to the value that the handler returns (meaning that it got zipped - this is to prevent a scenario where nothing gets zipped and the next step does nothing, but the whole test passes)
  • unzip the returned data and see that it's equal to the value returned from the handler

@rose2221
Copy link
Author

rose2221 commented Feb 26, 2025

Also, should I add tests for this as well?

That would be very helpful. Perhaps you could write a unit test where you:

  • have a dummy handler that returns some value
  • register the gzip middleware for this handler
  • execute the handler and check that the returned data is NOT equal to the value that the handler returns (meaning that it got zipped - this is to prevent a scenario where nothing gets zipped and the next step does nothing, but the whole test passes)
  • unzip the returned data and see that it's equal to the value returned from the handler

Thank you, I've implemented the test as suggested and I'm open to any further feedback. @rkapka

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

server ignores request to gzip data
3 participants