Add api version and tls to adapter #610
Conversation
WalkthroughThe changes introduced in this pull request enhance various adapter contexts by adding methods to retrieve TLS connection states and HTTP protocol version details. A new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (13)
adapters/humamux/humamux.go (1)
71-77: LGTM: Version() method implementation is correct with a minor optimization possible.The
Version()method correctly returns ahuma.ProtoVersionstruct containing the HTTP protocol version details from the request. This addition enhances the adapter's capabilities by providing access to protocol information, which can be useful for version-specific handling.Consider a minor optimization by directly returning the struct:
func (c *gmuxContext) Version() huma.ProtoVersion { return huma.ProtoVersion{ Proto: c.r.Proto, ProtoMajor: c.r.ProtoMajor, ProtoMinor: c.r.ProtoMinor, } }This avoids the need for a named return value and is slightly more concise.
adapters/humahttprouter/humahttprouter.go (1)
115-121: LGTM: Version method implementation looks good. Consider a minor optimization.The
Version()method correctly constructs and returns ahuma.ProtoVersionstruct with the protocol version details from thehttp.Request. This implementation aligns with the PR objectives and enhances the adapter's functionality by providing access to HTTP version information.Consider a minor optimization to reduce struct creation overhead:
func (c *httprouterContext) Version() huma.ProtoVersion { - return huma.ProtoVersion{ - Proto: c.r.Proto, - ProtoMajor: c.r.ProtoMajor, - ProtoMinor: c.r.ProtoMinor, - } + return huma.ProtoVersion{Proto: c.r.Proto, ProtoMajor: c.r.ProtoMajor, ProtoMinor: c.r.ProtoMinor} }This change reduces the number of lines and makes the struct initialization more concise without affecting readability.
adapters/humagin/humagin.go (1)
112-118: LGTM: Version method implementation looks good. Consider adding a comment.The
Version()method is correctly implemented, returning ahuma.ProtoVersionstruct with the appropriate protocol version details from the original request. This aligns with the PR objective.Consider adding a brief comment explaining the purpose of this method, for example:
// Version returns the HTTP protocol version used in the request. func (c *ginCtx) Version() huma.ProtoVersion { // ... (existing implementation) }adapters/humaecho/humaecho.go (1)
112-119: LGTM: Version method implementation looks good.The
Version()method correctly constructs and returns ahuma.ProtoVersionobject with the appropriate protocol version details from the original request. This implementation aligns with the PR objective to add API version to the adapter.Consider a minor optimization for better readability:
func (c *echoCtx) Version() huma.ProtoVersion { - r := c.orig.Request() + req := c.orig.Request() return huma.ProtoVersion{ - Proto: r.Proto, - ProtoMajor: r.ProtoMajor, - ProtoMinor: r.ProtoMinor, + Proto: req.Proto, + ProtoMajor: req.ProtoMajor, + ProtoMinor: req.ProtoMinor, } }This change uses a more descriptive variable name
reqinstead ofr, which slightly improves code readability.adapters/adapters_test.go (2)
93-107: LGTM with suggestions for improvementThe
wrapfunction is well-implemented and aligns with the PR objectives. It correctly adds middleware to check TLS and HTTP version, handling the Fiber adapter differently as needed. However, consider the following suggestions:
- Add more descriptive error messages to the assertions to aid in debugging if they fail.
- Consider handling the case where TLS might be present, even though it's not expected in the test environment.
Example of improved assertion:
assert.Nil(t, ctx.TLS(), "Expected TLS to be nil in test environment")
113-120: LGTM with a suggestion for improvementThe modifications to the adapter initialization are well-implemented. All adapters now consistently use the
wrapfunction, ensuring that they are tested with the new middleware. The Fiber adapter is correctly identified withtrueas the second parameter.Suggestion for improvement:
Consider replacing the boolean parameter with a more type-safe approach. This could help prevent potential errors and improve code readability. For example:type AdapterType int const ( StandardAdapter AdapterType = iota FiberAdapter ) wrap := func(h huma.API, adapterType AdapterType) huma.API { // ... (rest of the function remains the same) } // Usage: {"fiber", func() huma.API { return wrap(humafiber.New(fiber.New(), config()), FiberAdapter) }}, // For other adapters: {"chi", func() huma.API { return wrap(humachi.New(chi.NewMux(), config()), StandardAdapter) }},This approach makes the code more self-documenting and reduces the chance of passing incorrect boolean values.
adapters/humaflow/humaflow.go (3)
107-109: LGTM: TLS method added correctlyThe
TLS()method is implemented correctly, providing access to the TLS connection state from the HTTP request. This aligns with the PR objectives.Consider adding a comment to explain the purpose of this method, especially for cases where the connection might not be using TLS:
// TLS returns the TLS connection state if the connection // is using TLS, otherwise it returns nil. func (c *goContext) TLS() *tls.ConnectionState { return c.r.TLS }
111-117: LGTM: Version method added correctlyThe
Version()method is implemented correctly, providing access to the HTTP protocol version information from the request. This aligns with the PR objectives.Consider adding a comment to explain the purpose of this method and the structure it returns:
// Version returns the HTTP protocol version information // encapsulated in a huma.ProtoVersion struct. func (c *goContext) Version() huma.ProtoVersion { return huma.ProtoVersion{ Proto: c.r.Proto, ProtoMajor: c.r.ProtoMajor, ProtoMinor: c.r.ProtoMinor, } }
5-5: Summary: Successful implementation of TLS and Version methodsThe changes in this file successfully implement the new
TLS()andVersion()methods for thegoContextstruct. These additions enhance the adapter's functionality by providing access to TLS connection states and HTTP protocol version details, which aligns with the PR objectives.The implementation is correct and the changes are well-integrated into the existing code structure. The new methods will allow users of this adapter to access important connection and protocol information, improving the overall capability of the system to handle HTTP requests with detailed context.
Consider updating the package documentation or README to highlight these new capabilities, ensuring that users are aware of the enhanced functionality provided by the adapter.
Also applies to: 107-117
api.go (3)
70-73: LGTM: Enhanced Context interface with TLS and Version methodsThe addition of TLS() and Version() methods to the Context interface is well-implemented and aligns with the PR objectives. These methods will provide valuable information about the TLS connection state and HTTP protocol version.
Consider adding brief documentation comments for these new methods to improve code readability:
// TLS returns the TLS connection state, or nil if the connection is not TLS. TLS() *tls.ConnectionState // Version returns the HTTP protocol version used for the request. Version() ProtoVersion
125-130: LGTM: New ProtoVersion struct for HTTP protocol versionThe ProtoVersion struct is well-designed to represent HTTP protocol version information. The field names and types are appropriate and consistent with Go conventions.
Consider enhancing the documentation comment for better clarity:
// ProtoVersion represents the HTTP protocol version used in a request. // It includes the full protocol string (e.g., "HTTP/1.1") and separate // fields for major and minor version numbers. type ProtoVersion struct { Proto string // The protocol string (e.g., "HTTP/1.1") ProtoMajor int // The major version number ProtoMinor int // The minor version number }
Line range hint
1-130: Summary: Successful implementation of TLS and Version supportThe changes in this file effectively implement the PR objectives of adding API version and TLS support to the adapter. The modifications include:
- Adding the necessary "crypto/tls" import
- Enhancing the Context interface with TLS() and Version() methods
- Introducing a new ProtoVersion struct to represent HTTP protocol version information
These changes are well-integrated into the existing codebase and provide valuable additional information for request handling. The implementation follows Go best practices and conventions.
Consider updating any existing adapter implementations to support these new methods, ensuring consistent behavior across different HTTP routers and frameworks.
adapters/humafiber/humafiber.go (1)
115-117: Handle potential nil value from TLSConnectionStateThe
TLS()method correctly retrieves the TLS connection state usingc.orig.Context().TLSConnectionState(). Please be aware that this method may returnnilif the connection is not secured with TLS. Ensure that any code usingTLS()checks for anilvalue to avoid potentialnilpointer dereferences.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- adapters/adapters_test.go (1 hunks)
- adapters/humabunrouter/humabunrouter.go (3 hunks)
- adapters/humachi/humachi.go (2 hunks)
- adapters/humaecho/humaecho.go (2 hunks)
- adapters/humafiber/humafiber.go (2 hunks)
- adapters/humaflow/humaflow.go (2 hunks)
- adapters/humagin/humagin.go (2 hunks)
- adapters/humago/humago.go (2 hunks)
- adapters/humahttprouter/humahttprouter.go (2 hunks)
- adapters/humamux/humamux.go (2 hunks)
- api.go (3 hunks)
🧰 Additional context used
🔇 Additional comments (26)
adapters/humamux/humamux.go (3)
5-5: LGTM: Import addition is correct and necessary.The addition of the
crypto/tlsimport is appropriate for the newTLS()method implementation.
67-69: LGTM: TLS() method implementation is correct.The
TLS()method correctly returns the TLS connection state from the HTTP request. This addition enhances the adapter's capabilities by providing access to TLS information, which can be crucial for security-related operations.
Line range hint
1-145: Overall, the changes look good and align with the PR objectives.The additions of
TLS()andVersion()methods to thegmuxContextstruct enhance the adapter's capabilities by providing access to TLS connection states and HTTP protocol version details. These changes are well-implemented and will be useful for security-related operations and protocol-specific handling.A minor optimization was suggested for the
Version()method, but it's not critical. The code is clean, well-structured, and maintains the existing style of the file.adapters/humahttprouter/humahttprouter.go (2)
111-113: LGTM: TLS method implementation looks good.The
TLS()method correctly returns the TLS connection state from the underlyinghttp.Request. This implementation aligns with the PR objectives and enhances the adapter's functionality by providing access to TLS information.
111-121: Consider the implications on API design and backward compatibility.The addition of
TLS()andVersion()methods to thehttprouterContextstruct enhances the adapter's functionality by providing access to TLS and HTTP version information. These changes align well with the PR objectives and the AI-generated summary.However, it's important to consider the following:
API Design: Ensure that these additions are consistent with the overall API design philosophy. Are similar methods available in other adapters? If not, consider whether they should be added for consistency across the project.
Backward Compatibility: While these additions don't break existing code, they may impact the interface contract. Ensure that this expansion of the interface is documented and communicated to users, especially if
httprouterContextis part of the public API.Usage Guidelines: Consider providing documentation or examples on when and how to use these new methods effectively.
To ensure consistency across the project, let's check if similar methods exist in other adapters:
This will help determine if similar methods exist in other adapters and whether further changes are needed for consistency.
✅ Verification successful
API Design and Backward Compatibility Verified
The addition of
TLS()andVersion()methods to thehttprouterContextstruct is consistent with other adapters in the codebase. Similar methods exist across various adapters, ensuring uniform API design and maintaining backward compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for TLS() and Version() methods in other adapter files # Test: Search for TLS() method in other adapter files echo "Searching for TLS() method in other adapters:" rg --type go -g 'adapters/**/*.go' -n 'func \(.+\) TLS\(\)' --ignore-file adapters/humahttprouter/humahttprouter.go echo "\nSearching for Version() method in other adapters:" rg --type go -g 'adapters/**/*.go' -n 'func \(.+\) Version\(\)' --ignore-file adapters/humahttprouter/humahttprouter.goLength of output: 14139
Script:
#!/bin/bash # Description: Corrected script to check for TLS() and Version() methods in other adapter files # Find all .go files in adapters/ excluding humahttprouter/humahttprouter.go GO_FILES=$(fd --type f --extension go --search-path adapters/ --exclude humahttprouter/humahttprouter.go) echo "Searching for TLS() method in other adapters:" echo "$GO_FILES" | xargs rg -n 'func \(.+\) TLS\(\)' echo -e "\nSearching for Version() method in other adapters:" echo "$GO_FILES" | xargs rg -n 'func \(.+\) Version\(\)'Length of output: 3844
adapters/humachi/humachi.go (4)
5-5: LGTM: Import addition is correct and necessary.The addition of the
crypto/tlsimport is appropriate for the newTLS()method implementation.
110-112: LGTM: TLS() method implementation is correct.The
TLS()method correctly returns the TLS connection state from the HTTP request. This implementation aligns with the PR objectives and enhances the adapter's functionality for handling secure connections.
114-120: LGTM: Version() method implementation is correct and comprehensive.The
Version()method correctly constructs and returns ahuma.ProtoVersionstruct containing all relevant protocol version details from the HTTP request. This implementation aligns with the PR objectives and provides valuable information about the HTTP protocol version used in the request.
5-5: Summary: Excellent implementation of TLS and Version methods.The changes in this file successfully implement the new
TLS()andVersion()methods for thechiContextstruct. These additions, along with the necessarycrypto/tlsimport, enhance the adapter's capabilities by providing access to TLS connection states and HTTP protocol version details. The implementations are concise, correct, and align perfectly with the PR objectives.These changes will improve the ability to handle HTTP requests with detailed connection and protocol information, which is valuable for various use cases, including security checks and protocol-specific handling.
Also applies to: 110-120
adapters/humagin/humagin.go (2)
108-110: LGTM: TLS method implementation looks good.The
TLS()method is correctly implemented, returning the TLS connection state from the original request. This aligns with the PR objective and matches the expected signature for thehuma.Contextinterface.
108-118: Summary: New TLS and Version methods successfully implemented.The changes in this file successfully add the
TLS()andVersion()methods to theginCtxstruct, fulfilling the PR objectives. Both methods are correctly implemented and enhance the adapter's functionality.To ensure completeness, please verify:
- That corresponding unit tests have been added or updated for these new methods.
- That any relevant documentation (e.g., README, API docs) has been updated to reflect these new capabilities.
You can use the following script to check for test updates:
adapters/humaecho/humaecho.go (1)
108-110: LGTM: TLS method implementation looks good.The
TLS()method correctly returns the TLS connection state from the original request. This implementation aligns with the PR objective to add TLS to the adapter.adapters/adapters_test.go (1)
Line range hint
1-120: Overall assessment: Well-implemented changes with minor suggestions for improvementThe changes in this file successfully implement the PR objectives of adding API version and TLS checks to the adapters. The new
wrapfunction and its usage in adapter initialization ensure consistent behavior across all adapters. The implementation correctly handles the unique behavior of the Fiber adapter.Key points:
- The
wrapfunction effectively adds middleware to check TLS and HTTP version.- All adapters now use the
wrapfunction consistently.- The changes align well with the PR objectives and enhance the testing of adapters.
The suggestions provided (more descriptive assertion messages, handling potential TLS presence, and using a type-safe approach for adapter type) are minor improvements that could further enhance the code quality and maintainability.
Great job on implementing these changes!
adapters/humaflow/humaflow.go (1)
5-5: LGTM: Import statement added correctlyThe addition of the
crypto/tlsimport is necessary and correctly placed for the newTLS()method.adapters/humago/humago.go (4)
5-5: LGTM: Import addition for TLS functionalityThe addition of the
crypto/tlsimport is appropriate and necessary for the newTLS()method. This change aligns with the PR objectives of adding TLS functionality to the adapter.
116-118: LGTM: TLS method implementationThe
TLS()method is correctly implemented, providing access to the TLS connection state from the underlying HTTP request. This addition enhances the adapter's capabilities for handling secure connections and aligns with the PR objectives.
120-126: LGTM: Version method implementationThe
Version()method is well-implemented, providing a clean interface to access HTTP protocol version details. It correctly constructs and returns ahuma.ProtoVersionstruct with the relevant information from the underlying HTTP request. This addition enhances the adapter's functionality and aligns with the PR objectives.
5-5: Summary: Excellent additions enhancing adapter functionalityThe changes introduced in this PR successfully add TLS and HTTP version information access to the
goContextstruct. These additions are well-implemented and align perfectly with the PR objectives. The new methods,TLS()andVersion(), provide valuable information that can be used for security-related features and version-specific handling. The code is clean, concise, and maintains the existing structure of the adapter.Great job on enhancing the adapter's capabilities!
Also applies to: 116-126
adapters/humabunrouter/humabunrouter.go (5)
5-5: LGTM: Import added correctly.The
crypto/tlsimport is correctly added and is necessary for the newTLS()method implementations.
111-121: LGTM: TLS and Version methods implemented correctly.Both
TLS()andVersion()methods are implemented correctly:
TLS()returns the TLS connection state from the request.Version()constructs and returns ahuma.ProtoVersionstruct with the correct protocol version details.These implementations align with the PR objectives of adding API version and TLS to the adapter.
214-216: LGTM: TLS method for bunCompatContext implemented correctly.The
TLS()method forbunCompatContextis implemented correctly:
- It returns the TLS connection state from the request.
- The implementation is consistent with the
bunContextversion.
218-224: LGTM: Version method for bunCompatContext implemented correctly.The
Version()method forbunCompatContextis implemented correctly:
- It constructs and returns a
huma.ProtoVersionstruct with the correct protocol version details.- The implementation is consistent with the
bunContextversion.
Line range hint
1-300: Summary: PR objectives successfully implemented.The changes in this file successfully implement the PR objectives of adding API version and TLS to the adapter:
- The
crypto/tlspackage is correctly imported.- Both
bunContextandbunCompatContextnow haveTLS()andVersion()methods.- The implementations are consistent across both context types.
- The new methods provide access to TLS connection state and HTTP protocol version details.
These additions enhance the adapter's functionality and align well with the stated PR objectives.
api.go (1)
5-5: LGTM: Import addition for TLS supportThe addition of the "crypto/tls" import is appropriate and necessary for the new TLS() method in the Context interface.
adapters/humafiber/humafiber.go (2)
6-6: Importing the "crypto/tls" package is necessary for TLS supportThe addition of the
"crypto/tls"import is appropriate to support the newTLS()method, which returns the TLS connection state.
119-123: Version() method accurately implements protocol version retrievalThe
Version()method appropriately constructs ahuma.ProtoVersionwith the protocol obtained fromc.orig.Protocol(). This implementation aligns with the expected interface and provides accurate protocol version information.
| func (c *echoCtx) TLS() *tls.ConnectionState { | ||
| return c.orig.Request().TLS | ||
| } | ||
|
|
||
| func (c *echoCtx) Version() huma.ProtoVersion { | ||
| r := c.orig.Request() | ||
| return huma.ProtoVersion{ | ||
| Proto: r.Proto, | ||
| ProtoMajor: r.ProtoMajor, | ||
| ProtoMinor: r.ProtoMinor, | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Verification Complete: Methods are currently used only in test files.
The newly added TLS() and Version() methods are utilized within adapters/adapters_test.go for testing purposes. To fully leverage their functionality, consider integrating these methods into the production codebase where appropriate.
🔗 Analysis chain
Summary: New methods enhance adapter functionality.
The addition of TLS() and Version() methods to the echoCtx struct successfully implements the PR objectives. These methods provide valuable access to TLS connection state and HTTP protocol version information, enhancing the adapter's capabilities.
To ensure consistency, please verify the usage of these new methods across the codebase. Run the following script to check for potential usage:
This will help identify any existing code that might benefit from using these new methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential usage of new TLS() and Version() methods
# Test: Search for potential usage of TLS() method
echo "Potential usage of TLS() method:"
rg --type go "\.TLS\(\)" --glob '!adapters/humaecho/humaecho.go'
# Test: Search for potential usage of Version() method
echo "Potential usage of Version() method:"
rg --type go "\.Version\(\)" --glob '!adapters/humaecho/humaecho.go'
Length of output: 397
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #610 +/- ##
=======================================
Coverage 92.83% 92.83%
=======================================
Files 22 22
Lines 3923 3923
=======================================
Hits 3642 3642
Misses 236 236
Partials 45 45 ☔ View full report in Codecov by Sentry. |
danielgtaylor
left a comment
There was a problem hiding this comment.
This looks great, thank you! 👍
Hi,
I added new methods: Version/TLS to adapter (issue: #590)
Summary by CodeRabbit
New Features
ProtoVersiontype for encapsulating HTTP protocol details.Bug Fixes