This is an automated email from the ASF dual-hosted git repository.
baerwang pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/dubbo-go-pixiu.git
The following commit(s) were added to refs/heads/develop by this push:
new 153c1fe0f fix(dubboproxy): replace panic with error returns in filter
construction (#996)
153c1fe0f is described below
commit 153c1fe0fcc03aeeae19094285ad1b4112c026c4
Author: aias00 <[email protected]>
AuthorDate: Mon Aug 24 10:46:10 2026 +0800
fix(dubboproxy): replace panic with error returns in filter construction
(#996)
* fix(dubboproxy): replace panic with error returns in filter construction
Fixes #989 Issue 3: network filter/plugin construction path uses panic
for type assertion failures where normal error propagation would be safer.
Changes in plugin.go:
- CreateFilter: Replace panic with error return when config type assertion
fails
- Already had error return in signature but was panicking instead
- Now returns proper error: "expected
*model.DubboProxyConnectionManagerConfig, got %T"
Changes in manager.go:
- OnData: Replace panic with error return when data type assertion fails
- Already had error return in signature but was panicking instead
- Now returns proper error: "expected *invocation.RPCInvocation, got %T"
Classification:
- Both panics were type assertion failures during request/startup handling
- Neither was a true invariant violation (programming errors should be
caught)
- Both methods already had error returns - just not using them
- Returning errors allows graceful handling without crashing the server
Note: The recover() in handleRpcInvocation (line 182-187) is kept as is -
that's a proper defensive pattern that catches panics from filter chain.
Co-Authored-By: Claude <[email protected]>
* fix(dubboproxy): improve error message clarity
- Change verbose error message in CreateFilter to concise type-mismatch
format
- Change verbose error message in OnData to concise type-mismatch format
Co-Authored-By: Claude <[email protected]>
* fix(dubboproxy): fix import formatting for CI
Co-Authored-By: Claude <[email protected]>
* fix(dubboproxy): replace panics with error returns in OnTripleData
- Check metadata value array length before accessing first element
- Use safe type assertion for interface key extraction
- Return descriptive errors instead of panicking
Co-Authored-By: Claude <[email protected]>
* test(dubboproxy): add unit tests for panic-to-error fixes
Add comprehensive tests for dubboproxy network filter:
plugin_test.go:
- Test CreateFilter with valid config (success)
- Test CreateFilter with invalid config type (error, not panic)
manager_test.go:
- Test OnData with invalid type returns error (not panic)
- Test OnData with valid RPCInvocation passes type check
- Test OnTripleData with empty metadata value returns error
- Test OnTripleData with missing interface key returns error
- Test OnTripleData with valid metadata passes type check
- Test OnEncode with invalid type returns error
Coverage improvements:
- CreateFilter: 100%
- OnData: 63.2%
- OnTripleData: 56.0%
These tests verify the fixes in PR #996 that replace panics
with proper error returns for type assertion failures.
Co-Authored-By: Claude <[email protected]>
* fix: add missing newline at end of test files
CI gofmt check requires newline at end of files.
* fix: format imports in test files for CI
imports-formatter requires specific grouping of imports:
- Standard library
- Third-party packages
- Project packages
* style: replace interface{} with any in test files
golangci-lint v2.4.0 requires interface{} to be replaced with any
---------
Co-authored-by: Claude <[email protected]>
---
pkg/filter/network/dubboproxy/manager.go | 13 +-
pkg/filter/network/dubboproxy/manager_test.go | 227 ++++++++++++++++++++++++++
pkg/filter/network/dubboproxy/plugin.go | 8 +-
pkg/filter/network/dubboproxy/plugin_test.go | 86 ++++++++++
4 files changed, 329 insertions(+), 5 deletions(-)
diff --git a/pkg/filter/network/dubboproxy/manager.go
b/pkg/filter/network/dubboproxy/manager.go
index e16950269..3ae4d5e00 100644
--- a/pkg/filter/network/dubboproxy/manager.go
+++ b/pkg/filter/network/dubboproxy/manager.go
@@ -110,10 +110,17 @@ func (dcm *DubboProxyConnectionManager) OnTripleData(ctx
context.Context, method
md, ok := metadata.FromIncomingContext(ctx)
if ok {
for k := range md {
- dubboAttachment[k] = md.Get(k)[0]
+ values := md.Get(k)
+ if len(values) == 0 {
+ return nil, errors.Errorf("empty metadata value
for key: %s", k)
+ }
+ dubboAttachment[k] = values[0]
}
}
- interfaceName := dubboAttachment[constant.InterfaceKey].(string)
+ interfaceName, ok := dubboAttachment[constant.InterfaceKey].(string)
+ if !ok {
+ return nil, errors.Errorf("missing or invalid interface key in
metadata: expected string, got %T", dubboAttachment[constant.InterfaceKey])
+ }
ra, err := dcm.routerCoordinator.RouteByPathAndName(interfaceName,
methodName)
@@ -144,7 +151,7 @@ func (dcm *DubboProxyConnectionManager) OnTripleData(ctx
context.Context, method
func (dcm *DubboProxyConnectionManager) OnData(data any) (any, error) {
old_invoc, ok := data.(*invocation.RPCInvocation)
if !ok {
- panic("create invocation occur some exception for the type is
not suitable one.")
+ return nil, errors.Errorf("invalid invocation type: expected
*invocation.RPCInvocation, got %T", data)
}
// need reconstruct RPCInvocation ParameterValues witch is same with
arguments. refer to dubbogo/common/proxy/proxy.makeDubboCallProxy
arguments := old_invoc.Arguments()
diff --git a/pkg/filter/network/dubboproxy/manager_test.go
b/pkg/filter/network/dubboproxy/manager_test.go
new file mode 100644
index 000000000..658d11d8b
--- /dev/null
+++ b/pkg/filter/network/dubboproxy/manager_test.go
@@ -0,0 +1,227 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package dubboproxy
+
+import (
+ "context"
+ "testing"
+)
+
+import (
+ "dubbo.apache.org/dubbo-go/v3/protocol/invocation"
+
+ "github.com/dubbogo/grpc-go/metadata"
+
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "github.com/apache/dubbo-go-pixiu/pkg/model"
+)
+
+func TestDubboProxyConnectionManager_OnData_InvalidType(t *testing.T) {
+ // Create a minimal manager with required dependencies
+ dcm := &DubboProxyConnectionManager{
+ config: &model.DubboProxyConnectionManagerConfig{},
+ routerCoordinator: nil, // not needed for this test
+ }
+
+ // Test with nil data
+ result, err := dcm.OnData(nil)
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "invalid invocation type")
+ assert.Contains(t, err.Error(), "expected *invocation.RPCInvocation")
+
+ // Test with wrong type - string
+ result, err = dcm.OnData("invalid")
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "invalid invocation type")
+
+ // Test with wrong type - int
+ result, err = dcm.OnData(123)
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "invalid invocation type")
+
+ // Test with wrong pointer type
+ result, err = dcm.OnData(&struct{}{})
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "invalid invocation type")
+}
+
+func TestDubboProxyConnectionManager_OnData_ValidTypePassesTypeCheck(t
*testing.T) {
+ // This test verifies that OnData accepts valid RPCInvocation type
+ // without panicking on type assertion (the fix in this PR)
+ // Note: Full route functionality requires mocking routerCoordinator
which is complex
+
+ // Create a manager with proper initialization
+ cfg := &model.DubboProxyConnectionManagerConfig{
+ RouteConfig: model.RouteConfiguration{},
+ }
+ dcm := CreateDubboProxyConnectionManager(cfg)
+
+ // Create valid RPCInvocation
+ invoc := invocation.NewRPCInvocationWithOptions(
+ invocation.WithMethodName("testMethod"),
+ invocation.WithArguments([]any{}),
+ )
+
+ // OnData should not panic on type assertion (the key fix in this PR)
+ // It will return an error about route not found, which is acceptable
+ result, err := dcm.OnData(invoc)
+
+ // The key assertion: we get a proper error, not a panic
+ // Type assertion passed, so we should not see "invalid invocation
type" error
+ if err != nil {
+ assert.NotContains(t, err.Error(), "invalid invocation type",
+ "Type assertion should have passed for valid
RPCInvocation")
+ }
+ // Result may be nil due to route error, which is expected
+ _ = result // We don't care about result for this test
+}
+
+func TestDubboProxyConnectionManager_OnTripleData_EmptyMetadataValue(t
*testing.T) {
+ dcm := &DubboProxyConnectionManager{
+ config: &model.DubboProxyConnectionManagerConfig{},
+ routerCoordinator: nil,
+ }
+
+ // Create context with metadata where the values slice is empty
+ // This tests the "if len(values) == 0" check in manager.go:114
+ md := metadata.MD{
+ "test-key": []string{}, // Empty slice triggers the error
+ }
+ ctx := metadata.NewIncomingContext(context.Background(), md)
+
+ result, err := dcm.OnTripleData(ctx, "testMethod", []any{})
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "empty metadata value for key:
test-key")
+}
+
+func TestDubboProxyConnectionManager_OnTripleData_MissingInterfaceKey(t
*testing.T) {
+ dcm := &DubboProxyConnectionManager{
+ config: &model.DubboProxyConnectionManagerConfig{},
+ routerCoordinator: nil,
+ }
+
+ // Create context with metadata but no interface key
+ md := metadata.Pairs("some-key", "some-value")
+ ctx := metadata.NewIncomingContext(context.Background(), md)
+
+ result, err := dcm.OnTripleData(ctx, "testMethod", []any{})
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "missing or invalid interface key")
+}
+
+func TestDubboProxyConnectionManager_OnTripleData_InvalidInterfaceKeyType(t
*testing.T) {
+ dcm := &DubboProxyConnectionManager{
+ config: &model.DubboProxyConnectionManagerConfig{},
+ routerCoordinator: nil,
+ }
+
+ // Create context with metadata where interface key exists but is not a
string
+ // Note: metadata.Pairs always stores string values, so we need to
manually construct
+ md := metadata.MD{
+ "interface": []string{}, // Empty slice - this will trigger
empty metadata error
+ }
+ ctx := metadata.NewIncomingContext(context.Background(), md)
+
+ result, err := dcm.OnTripleData(ctx, "testMethod", []any{})
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ // Should get empty metadata value error
+ assert.Contains(t, err.Error(), "empty metadata value")
+}
+
+func TestDubboProxyConnectionManager_OnTripleData_NoMetadata(t *testing.T) {
+ dcm := &DubboProxyConnectionManager{
+ config: &model.DubboProxyConnectionManagerConfig{},
+ routerCoordinator: nil,
+ }
+
+ // Create context without metadata
+ ctx := context.Background()
+
+ result, err := dcm.OnTripleData(ctx, "testMethod", []any{})
+ assert.Error(t, err)
+ assert.Nil(t, result)
+ assert.Contains(t, err.Error(), "missing or invalid interface key")
+}
+
+func TestCreateDubboProxyConnectionManager(t *testing.T) {
+ cfg := &model.DubboProxyConnectionManagerConfig{
+ RouteConfig: model.RouteConfiguration{},
+ TimeoutStr: "5s",
+ }
+
+ dcm := CreateDubboProxyConnectionManager(cfg)
+ assert.NotNil(t, dcm)
+ assert.NotNil(t, dcm.config)
+ assert.NotNil(t, dcm.routerCoordinator)
+ assert.NotNil(t, dcm.codec)
+ assert.NotNil(t, dcm.filterManager)
+}
+
+func
TestDubboProxyConnectionManager_OnTripleData_ValidMetadataTypeCheckPasses(t
*testing.T) {
+ // This test verifies that OnTripleData accepts valid metadata without
type assertion panic
+ cfg := &model.DubboProxyConnectionManagerConfig{
+ RouteConfig: model.RouteConfiguration{},
+ }
+ dcm := CreateDubboProxyConnectionManager(cfg)
+
+ // Create context with valid metadata including interface key
+ md := metadata.Pairs(
+ "interface", "com.example.TestService",
+ "version", "1.0.0",
+ "group", "testGroup",
+ )
+ ctx := metadata.NewIncomingContext(context.Background(), md)
+
+ // OnTripleData should not panic on type assertions (the fix in this PR)
+ // It will return an error about route not found, which is acceptable
+ result, err := dcm.OnTripleData(ctx, "testMethod", []any{})
+
+ // The key assertion: we get a proper error, not a panic
+ // Type assertions passed, so we should not see "missing or invalid
interface key" error
+ if err != nil {
+ assert.NotContains(t, err.Error(), "missing or invalid
interface key",
+ "Type assertion for interface key should have passed")
+ assert.NotContains(t, err.Error(), "empty metadata value",
+ "Metadata value check should have passed")
+ }
+ // Result may be nil due to route error, which is expected
+ _ = result
+}
+
+func TestDubboProxyConnectionManager_OnEncode(t *testing.T) {
+ // Test OnEncode with invalid type
+ cfg := &model.DubboProxyConnectionManagerConfig{
+ RouteConfig: model.RouteConfiguration{},
+ }
+ dcm := CreateDubboProxyConnectionManager(cfg)
+
+ // Test with invalid type
+ _, err := dcm.OnEncode("invalid")
+ assert.Error(t, err)
+ assert.Contains(t, err.Error(), "invalid rpc response")
+}
diff --git a/pkg/filter/network/dubboproxy/plugin.go
b/pkg/filter/network/dubboproxy/plugin.go
index 3e8d99f4e..5a9999e2f 100644
--- a/pkg/filter/network/dubboproxy/plugin.go
+++ b/pkg/filter/network/dubboproxy/plugin.go
@@ -17,6 +17,10 @@
package dubboproxy
+import (
+ "fmt"
+)
+
import (
"github.com/apache/dubbo-go-pixiu/pkg/common/constant"
"github.com/apache/dubbo-go-pixiu/pkg/common/extension/filter"
@@ -45,10 +49,10 @@ func (p *Plugin) Kind() string {
// CreateFilter create dubbo networkfilter
func (p *Plugin) CreateFilter(config any) (filter.NetworkFilter, error) {
hcmc, ok := config.(*model.DubboProxyConnectionManagerConfig)
- hcmc.Timeout = stringutil.ResolveTimeStr2Time(hcmc.TimeoutStr,
constant.DefaultReqTimeout)
if !ok {
- panic("CreateFilter occur some exception for the type is not
suitable one.")
+ return nil, fmt.Errorf("invalid config type: expected
*model.DubboProxyConnectionManagerConfig, got %T", config)
}
+ hcmc.Timeout = stringutil.ResolveTimeStr2Time(hcmc.TimeoutStr,
constant.DefaultReqTimeout)
return CreateDubboProxyConnectionManager(hcmc), nil
}
diff --git a/pkg/filter/network/dubboproxy/plugin_test.go
b/pkg/filter/network/dubboproxy/plugin_test.go
new file mode 100644
index 000000000..f8f195421
--- /dev/null
+++ b/pkg/filter/network/dubboproxy/plugin_test.go
@@ -0,0 +1,86 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package dubboproxy
+
+import (
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+)
+
+import (
+ "github.com/apache/dubbo-go-pixiu/pkg/model"
+)
+
+func TestPlugin_Kind(t *testing.T) {
+ p := &Plugin{}
+ assert.Equal(t, "dgp.filter.network.dubboconnectionmanager", p.Kind())
+}
+
+func TestPlugin_Config(t *testing.T) {
+ p := &Plugin{}
+ cfg := p.Config()
+ _, ok := cfg.(*model.DubboProxyConnectionManagerConfig)
+ assert.True(t, ok, "Config should return
*model.DubboProxyConnectionManagerConfig")
+}
+
+func TestPlugin_CreateFilter_Success(t *testing.T) {
+ p := &Plugin{}
+ cfg := &model.DubboProxyConnectionManagerConfig{
+ RouteConfig: model.RouteConfiguration{},
+ TimeoutStr: "3s",
+ }
+
+ filter, err := p.CreateFilter(cfg)
+ assert.NoError(t, err)
+ assert.NotNil(t, filter)
+
+ // Verify timeout was set
+ assert.Greater(t, cfg.Timeout.Seconds(), float64(0))
+}
+
+func TestPlugin_CreateFilter_InvalidConfigType(t *testing.T) {
+ p := &Plugin{}
+
+ // Test with nil config
+ filter, err := p.CreateFilter(nil)
+ assert.Error(t, err)
+ assert.Nil(t, filter)
+ assert.Contains(t, err.Error(), "invalid config type")
+ assert.Contains(t, err.Error(), "expected
*model.DubboProxyConnectionManagerConfig")
+
+ // Test with wrong type - string
+ filter, err = p.CreateFilter("invalid")
+ assert.Error(t, err)
+ assert.Nil(t, filter)
+ assert.Contains(t, err.Error(), "invalid config type")
+
+ // Test with wrong type - map
+ filter, err = p.CreateFilter(map[string]any{"key": "value"})
+ assert.Error(t, err)
+ assert.Nil(t, filter)
+ assert.Contains(t, err.Error(), "invalid config type")
+
+ // Test with wrong pointer type
+ filter, err = p.CreateFilter(&model.HttpConnectionManagerConfig{})
+ assert.Error(t, err)
+ assert.Nil(t, filter)
+ assert.Contains(t, err.Error(), "invalid config type")
+}
