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

[TT-13257] Fix websocket upgrade with multiple values in Connection header #6619

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Darkness4
Copy link

Description

To check for Websocket connection upgrade, replaces the "!=" check with the "not contains" check.

Related Issue

Closes #6449.

How This Has Been Tested

A test case has been added.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

@titpetric titpetric changed the title Fix websocket upgrade with multiple values in Connection header [TT-13257] Fix websocket upgrade with multiple values in Connection header Oct 9, 2024
@titpetric
Copy link
Contributor

Hi,

thank you for the contribution. The approach looks good for the most part, two requests raised from review:

  • can you use testify/assert in tests (we're standardising on it, but have some fragmentation)
  • can you please extend the internal/httputil location with a unit test (ideally that's black box)

Thank you :)

@Darkness4
Copy link
Author

@titpetric You can re-review. 🙂

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.

[TT-13257] Websocket connection is not upgraded when keep-alive is added to Connection
2 participants