Closed titpetric closed 17 hours ago
API Changes
no api changes detected
โฑ๏ธ Estimated effort to review: 4 ๐ต๐ต๐ต๐ตโช |
๐งช PR contains tests |
๐ No security concerns identified |
โก Key issues to review **Possible Bug:** The changes in `gateway/reverse_proxy.go` modify the behavior of how proxy responses are handled, specifically with the removal of conditionals around `IsGrpcStreaming`. This could affect the functionality of gRPC streaming in the system. It's important to ensure that these changes are thoroughly tested, especially in scenarios involving gRPC streaming. **Refactoring Concern:** The refactoring in `gateway/reverse_proxy.go` and `gateway/handler_success.go` to use `httputil.IsStreamingRequest` instead of `IsGrpcStreaming` should be carefully reviewed to ensure that it covers all previous use cases. The change in method signature and logic could lead to unexpected behaviors if not properly handled. **Test Coverage:** The modifications and additions in the test files, such as `gateway/reverse_proxy_test.go`, need to be reviewed to ensure they adequately cover the new logic introduced in the PR. It's crucial that the tests validate the new streaming checks and handle all edge cases. |
Category | Suggestion | Score |
Possible bug |
Restore conditional logic for wrapping HTTP requests based on their type___ **TheServeHTTP method always passes true as the third parameter to WrappedServeHTTP , which might not be appropriate for all requests, especially if some should not be wrapped based on certain conditions. Consider restoring the condition to check if the request is a gRPC streaming request or similar conditions that were previously in place.** [gateway/reverse_proxy.go [518]](https://github.com/TykTechnologies/tyk/pull/6392/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01bR518-R518) ```diff -resp := p.WrappedServeHTTP(rw, req, true) +var shouldWrap bool +if httputil.IsGrpcStreaming(req) { + shouldWrap = false +} else { + shouldWrap = true +} +resp := p.WrappedServeHTTP(rw, req, shouldWrap) ``` Suggestion importance[1-10]: 9Why: The suggestion addresses a significant issue where the logic for determining whether to wrap HTTP requests was removed. Restoring this logic ensures that requests are handled appropriately based on their type, which is crucial for correct functionality. | 9 |
Possible issue |
Maintain the original order of return types in
___
**The | 8 |
Best practice |
Replace
___
**The use of | 7 |
Failed conditions
0.0% Coverage on New Code (required โฅ 80%)
User description
Playing around with getting more data from tests per function; it could help us diagnose bottlenecks and form our test strategy further based on the findings. Currently we know that 173 functions out of 501 functions in scope are unit tests for the area, and the rest are integration tests; we can separate coverage based on that, which likely yields some numbers for the following:
we can hypothetize the statement to be true, as the number of unit tests is 173/501, meaning that integration test cover a larger area, largely focused on middleware as that is our biggest area by code size as a group;
we already can use the presence of
StartTest
in individual test functions to categorize a test as an integration test as this will create a gateway lifecycle, but that only covers those particular cases. As I am very fond of venn diagrams, i expect a number of tests to be in the integration tests group, and StartTest a subset of that group. The integration tests without StartTest are the remaining code smells, as these should:Essentially the solution is the same, but the concerns are mainly around two outcomes:
PR Type
Enhancement, Tests
Description
httputil
package.TestStartTest
.httputil
.task lint
.Changes walkthrough ๐
5 files
handler_success.go
Refactor gRPC streaming check to use httputil package
gateway/handler_success.go - Replaced `IsGrpcStreaming` with `httputil.IsStreamingRequest`.
reverse_proxy.go
Refactor reverse proxy to use httputil for streaming checks
gateway/reverse_proxy.go
IsGrpcStreaming
withhttputil.IsStreamingRequest
.IsUpgrade
function to usehttputil.IsUpgrade
.testutil.go
Add CPU profiling rate setting in test initialization
gateway/testutil.go - Added CPU profiling rate setting in `InitTestMain`.
streaming.go
Add utility functions for streaming checks in httputil
internal/httputil/streaming.go - Added utility functions for streaming checks (gRPC, SSE, Upgrade).
Makefile
Simplify linting process using task lint
Makefile - Simplified linting process by using `task lint`.
3 files
reverse_proxy_test.go
Improve SSE test reliability and add context timeout
gateway/reverse_proxy_test.go
testutil_test.go
Add baseline integration test for StartTest
gateway/testutil_test.go - Added a baseline integration test `TestStartTest`.
Taskfile.inspect.yml
Add Taskfile for inspecting test coverage and profiling
gateway/Taskfile.inspect.yml - Added Taskfile for inspecting test coverage and profiling data.
3 files
docker-compose.yml
Update docker-compose with project name and additional services
docker-compose.yml
docker-compose.yml
Include httpbin service in docker services
docker/services/docker-compose.yml - Included httpbin service.
httpbin.yml
Add httpbin service configuration
docker/services/httpbin.yml - Added configuration for httpbin service.