backward compatible metadata keys #4738
Conversation
WalkthroughThe changes in this pull request involve modifications to two files within the codebase. In Changes
Poem
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
pkg/lib/ncl/subscriber.go (1)
Line range hint
1-1: Consider implementing a metadata key migration strategyThe removal of "Bacalhau-" prefix from metadata keys across the codebase requires careful handling to maintain backward compatibility. Consider implementing:
- A transition period where both old and new formats are supported
- A migration guide for downstream consumers
- Version detection in message handling code
- Deprecation notices for the old format
pkg/lib/envelope/metadata.go (2)
27-27: Consider adding a safety check for already prefixed keys.While the current implementation works for the simplified keys, it might cause issues if a key already contains the prefix. Consider adding a check to prevent double-prefixing.
- headers[HeaderPrefix+k] = []string{v} + key := k + if !strings.HasPrefix(k, HeaderPrefix) { + key = HeaderPrefix + k + } + headers[key] = []string{v}
Line range hint
10-41: Consider introducing a metadata key management utility.To improve maintainability and reduce duplication, consider extracting the prefix handling logic into utility functions. This would centralize the logic and make it easier to modify the behavior in the future.
Example approach:
// keyutil/keys.go package keyutil func ToHeaderKey(key string) string { if strings.HasPrefix(key, HeaderPrefix) { return key } return HeaderPrefix + key } func FromHeaderKey(key string) string { if strings.HasPrefix(key, HeaderPrefix) { return key[len(HeaderPrefix):] } return key }This would:
- Centralize the prefix handling logic
- Make it easier to modify the behavior
- Reduce the chance of inconsistencies
- Simplify testing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
pkg/lib/envelope/metadata.go(2 hunks)pkg/lib/ncl/constants.go(1 hunks)pkg/lib/ncl/error_response.go(1 hunks)pkg/lib/ncl/subscriber.go(1 hunks)pkg/transport/nclprotocol/dispatcher/constants.go(1 hunks)pkg/transport/nclprotocol/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkg/lib/ncl/constants.go
- pkg/transport/nclprotocol/dispatcher/constants.go
🔇 Additional comments (4)
pkg/lib/ncl/error_response.go (1)
15-15:
Ensure backward compatibility for KeyStatusCode changes
The change from "Bacalhau-StatusCode" to "StatusCode" might break existing systems expecting the old format. Consider:
- Adding a transition period where both formats are supported
- Using the new HeaderPrefix constant mentioned in the summary
Let's verify the impact of this change:
pkg/transport/nclprotocol/types.go (1)
56-56:
Verify sequence number backward compatibility
The change from "Bacalhau-SeqNum" to "SeqNum" could affect message ordering and processing in existing systems. Consider:
- Adding a transition mechanism to handle both old and new sequence number formats
- Documenting the change for downstream consumers
Let's verify the sequence number usage:
pkg/lib/ncl/subscriber.go (1)
78-78: LGTM: Enhanced error logging with handler context
Good improvement to add the handler name in error logs. This will make it easier to identify which subscriber encountered issues during message processing.
pkg/lib/envelope/metadata.go (1)
10-12: LGTM! Clean separation of concerns with the header prefix.
The introduction of HeaderPrefix and simplification of key constants improves code maintainability while preserving backward compatibility through consistent header prefixing.
Let's verify that these are the only metadata keys being used:
✅ Verification successful
No other metadata keys need updating, changes are consistent
Based on the search results, the only metadata-related keys in the codebase that use the "Bacalhau-" prefix are the ones being modified in this PR (Type and PayloadEncoding). The changes are consistent with the codebase:
- The
HeaderPrefixconstant is used correctly in theToHeadersandFromHeadersmethods in the same file - Other message-related keys in the codebase (like
KeySource,KeyEventTime, etc.) follow a similar pattern of not having the prefix in the constant name - The prefix is only added when converting to headers, maintaining backward compatibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other metadata keys that might need similar updates
rg -g '!vendor/*' '"Bacalhau-[A-Za-z]+"'
Length of output: 42
Script:
#!/bin/bash
# Let's try a more comprehensive search for metadata keys
# First, let's look for any string constants containing "Type" or "PayloadEncoding"
rg -g '!vendor/*' -i '(Type|PayloadEncoding).*=.*"'
# Also search for any usage of HeaderPrefix
rg -g '!vendor/*' 'HeaderPrefix'
# And look for any other potential metadata-related constants
rg -g '!vendor/*' 'Key[A-Z][A-Za-z]+\s*='
Length of output: 19519
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/lib/envelope/metadata.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: build / Build Binary (darwin, arm64)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (darwin, amd64)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, armv6)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, armv7)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, arm64)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, amd64)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
pkg/lib/envelope/metadata.go
[failure] 40-40:
undefined: strings (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
🔇 Additional comments (3)
pkg/lib/envelope/metadata.go (3)
10-12: LGTM! Good refactoring of constants.
The introduction of HeaderPrefix and simplification of key names improves code maintainability while preserving backward compatibility through the ToHeaders method.
27-27: LGTM! Correct implementation of prefix handling.
The modification correctly applies the HeaderPrefix to maintain backward compatibility with existing header formats.
Line range hint 10-46: Verify backward compatibility across the codebase.
While the changes maintain backward compatibility through proper prefix handling, let's verify the usage across the codebase.
✅ Verification successful
Let me gather more information about the usage of metadata and headers in the codebase to ensure we have a complete picture.
Let me check one final aspect to ensure we have a complete understanding of the header usage in tests.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Backward compatibility is properly maintained.
The code maintains backward compatibility through:
- Consistent usage of
HeaderPrefix("Bacalhau-") across the codebase - Proper handling of both prefixed and unprefixed keys in
FromHeaders - Consistent metadata key handling in message types and transport layers
- No direct string literals of prefixed headers found that could break
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of the old prefixed constants
rg '"Bacalhau-Type"|"Bacalhau-PayloadEncoding"'
# Search for header key usage to ensure we haven't missed any cases
rg -A 2 'ToHeaders|FromHeaders'
Length of output: 996
Script:
#!/bin/bash
# Search for Metadata type usage and initialization
ast-grep --pattern 'type $_ struct {
$$$
Metadata $$$
$$$
}'
# Search for any direct header manipulation that might bypass ToHeaders/FromHeaders
rg -A 2 'Header.*=.*map\[string\]\[\]string'
# Search for tests involving headers to verify compatibility handling
rg -l 'Test.*Header|Header.*Test' --type go
Length of output: 1474
Script:
#!/bin/bash
# Check the test file content to understand header usage patterns
cat pkg/executor/docker/executor_test.go
# Look for any usage of the specific header keys we're concerned about
rg -A 2 'KeyMessageType|KeyPayloadEncoding'
# Check for any direct manipulation of headers in tests
rg -A 2 'Header.*Bacalhau-' --type go
Length of output: 39174
🧰 Tools
🪛 GitHub Check: build / Build Binary (darwin, arm64)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (darwin, amd64)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, armv6)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, armv7)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, arm64)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: build / Build Binary (linux, amd64)
[failure] 40-40:
undefined: strings
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
[failure] 40-40:
undefined: strings (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
[failure] 40-40:
undefined: strings) (typecheck)
| HeaderPrefix = "Bacalhau-" | ||
| KeyMessageType = "Bacalhau-Type" | ||
| KeyPayloadEncoding = "Bacalhau-PayloadEncoding" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update message type and encoding constants to use HeaderPrefix
The constants KeyMessageType and KeyPayloadEncoding have the "Bacalhau-" prefix hardcoded, while we now have a HeaderPrefix constant. This creates redundancy and potential maintenance issues.
Consider using the HeaderPrefix constant:
const (
HeaderPrefix = "Bacalhau-"
- KeyMessageType = "Bacalhau-Type"
- KeyPayloadEncoding = "Bacalhau-PayloadEncoding"
+ KeyMessageType = "Type"
+ KeyPayloadEncoding = "PayloadEncoding"
)This change would:
- Reduce redundancy
- Make it easier to maintain if the prefix needs to change
- Match the backward compatibility logic in the
Getmethod
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HeaderPrefix = "Bacalhau-" | |
| KeyMessageType = "Bacalhau-Type" | |
| KeyPayloadEncoding = "Bacalhau-PayloadEncoding" | |
| HeaderPrefix = "Bacalhau-" | |
| KeyMessageType = "Type" | |
| KeyPayloadEncoding = "PayloadEncoding" |
| func (m Metadata) Get(key string) string { | ||
| return m[key] | ||
| if v, ok := m[key]; ok { | ||
| return v | ||
| } | ||
| // backward compatible with old headers | ||
| if key == KeyMessageType || key == KeyPayloadEncoding { | ||
| // return keys excluding header prefix | ||
| return m[strings.TrimPrefix(key, HeaderPrefix)] | ||
| } | ||
| return "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance backward compatibility handling in Get method
The current implementation has several limitations:
- Only handles backward compatibility for
KeyMessageTypeandKeyPayloadEncoding - Doesn't handle case where both prefixed and unprefixed keys exist
- Could be more generic to handle all keys with prefix
Consider this more robust implementation:
func (m Metadata) Get(key string) string {
if v, ok := m[key]; ok {
return v
}
- // backward compatible with old headers
- if key == KeyMessageType || key == KeyPayloadEncoding {
- // return keys excluding header prefix
- return m[strings.TrimPrefix(key, HeaderPrefix)]
+ // Check if key has prefix
+ if strings.HasPrefix(key, HeaderPrefix) {
+ // Try without prefix
+ if v, ok := m[strings.TrimPrefix(key, HeaderPrefix)]; ok {
+ return v
+ }
+ } else {
+ // Try with prefix
+ if v, ok := m[HeaderPrefix+key]; ok {
+ return v
+ }
}
return ""
}This implementation:
- Handles all keys, not just specific ones
- Tries both with and without prefix
- Maintains backward compatibility more comprehensively
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m Metadata) Get(key string) string { | |
| return m[key] | |
| if v, ok := m[key]; ok { | |
| return v | |
| } | |
| // backward compatible with old headers | |
| if key == KeyMessageType || key == KeyPayloadEncoding { | |
| // return keys excluding header prefix | |
| return m[strings.TrimPrefix(key, HeaderPrefix)] | |
| } | |
| return "" | |
| func (m Metadata) Get(key string) string { | |
| if v, ok := m[key]; ok { | |
| return v | |
| } | |
| // Check if key has prefix | |
| if strings.HasPrefix(key, HeaderPrefix) { | |
| // Try without prefix | |
| if v, ok := m[strings.TrimPrefix(key, HeaderPrefix)]; ok { | |
| return v | |
| } | |
| } else { | |
| // Try with prefix | |
| if v, ok := m[HeaderPrefix+key]; ok { | |
| return v | |
| } | |
| } | |
| return "" | |
| } |
Summary by CodeRabbit
New Features
Improvements