From 7ea369f3bbe2e9eb47979333e8ac8ffc70393edb Mon Sep 17 00:00:00 2001 From: tanyamadaan Date: Wed, 2 Apr 2025 13:26:12 +0530 Subject: [PATCH 1/4] bug fix - request body not read properly --- .../reqpreprocessor/reqpreprocessor.go | 64 ++++++------------- 1 file changed, 18 insertions(+), 46 deletions(-) diff --git a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go index 24ffa67..b9da290 100644 --- a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go +++ b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go @@ -10,88 +10,60 @@ import ( "net/http" "github.com/beckn/beckn-onix/pkg/log" - "github.com/google/uuid" ) -// Config holds the configuration settings for the application. type Config struct { - ContextKeys []string // ContextKeys is a list of context keys used for request processing. - Role string // Role specifies the role of the entity (e.g., subscriber, gateway). -} - -type becknRequest struct { - Context map[string]any `json:"context"` + ContextKeys []string + Role string } const contextKey = "context" const subscriberIDKey = "subscriber_id" -// NewPreProcessor creates a middleware that processes incoming HTTP requests by extracting -// and modifying the request context based on the provided configuration. func NewPreProcessor(cfg *Config) (func(http.Handler) http.Handler, error) { if err := validateConfig(cfg); err != nil { return nil, err } return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - body, _ := io.ReadAll(r.Body) - var req becknRequest + body, err := io.ReadAll(r.Body) + if err != nil { + http.Error(w, "Failed to read request body", http.StatusBadRequest) + return + } + var req map[string]interface{} ctx := r.Context() if err := json.Unmarshal(body, &req); err != nil { http.Error(w, "Failed to decode request body", http.StatusBadRequest) return } - if req.Context == nil { - http.Error(w, fmt.Sprintf("%s field not found.", contextKey), http.StatusBadRequest) + + // Extract context from request + reqContext, ok := req["context"].(map[string]interface{}) + if !ok { + http.Error(w, fmt.Sprintf("%s field not found or invalid.", contextKey), http.StatusBadRequest) return } var subID any switch cfg.Role { case "bap": - subID = req.Context["bap_id"] + subID = reqContext["bap_id"] case "bpp": - subID = req.Context["bpp_id"] + subID = reqContext["bap_id"] } if subID != nil { log.Debugf(ctx, "adding subscriberId to request:%s, %v", subscriberIDKey, subID) - // TODO: Add a ContextKey type in model and use it here instead of raw context key. ctx = context.WithValue(ctx, subscriberIDKey, subID) } - for _, key := range cfg.ContextKeys { - value := uuid.NewString() - updatedValue := update(req.Context, key, value) - ctx = context.WithValue(ctx, key, updatedValue) - } - reqData := map[string]any{"context": req.Context} - updatedBody, _ := json.Marshal(reqData) - r.Body = io.NopCloser(bytes.NewBuffer(updatedBody)) - r.ContentLength = int64(len(updatedBody)) + + r.Body = io.NopCloser(bytes.NewBuffer(body)) + r.ContentLength = int64(len(body)) r = r.WithContext(ctx) next.ServeHTTP(w, r) }) }, nil } -func update(wrapper map[string]any, key string, value any) any { - field, exists := wrapper[key] - if !exists || isEmpty(field) { - wrapper[key] = value - return value - } - - return field -} -func isEmpty(v any) bool { - switch v := v.(type) { - case string: - return v == "" - case nil: - return true - default: - return false - } -} - func validateConfig(cfg *Config) error { if cfg == nil { return errors.New("config cannot be nil") From 87329bfb9d79d8a73f7922233aaa11d385193ccf Mon Sep 17 00:00:00 2001 From: tanyamadaan Date: Thu, 3 Apr 2025 16:08:57 +0530 Subject: [PATCH 2/4] Bug Fix for ReqPreProcessor plugin --- .../reqpreprocessor/cmd/plugin.go | 4 - .../reqpreprocessor/cmd/plugin_test.go | 27 ++- .../reqpreprocessor/reqpreprocessor.go | 17 +- .../reqpreprocessor/reqpreprocessor_test.go | 169 ++++++++++++------ 4 files changed, 140 insertions(+), 77 deletions(-) diff --git a/pkg/plugin/implementation/reqpreprocessor/cmd/plugin.go b/pkg/plugin/implementation/reqpreprocessor/cmd/plugin.go index b89b650..331ec80 100644 --- a/pkg/plugin/implementation/reqpreprocessor/cmd/plugin.go +++ b/pkg/plugin/implementation/reqpreprocessor/cmd/plugin.go @@ -3,7 +3,6 @@ package main import ( "context" "net/http" - "strings" "github.com/beckn/beckn-onix/pkg/plugin/implementation/reqpreprocessor" ) @@ -12,9 +11,6 @@ type provider struct{} func (p provider) New(ctx context.Context, c map[string]string) (func(http.Handler) http.Handler, error) { config := &reqpreprocessor.Config{} - if contextKeysStr, ok := c["contextKeys"]; ok { - config.ContextKeys = strings.Split(contextKeysStr, ",") - } if role, ok := c["role"]; ok { config.Role = role } diff --git a/pkg/plugin/implementation/reqpreprocessor/cmd/plugin_test.go b/pkg/plugin/implementation/reqpreprocessor/cmd/plugin_test.go index 6044c44..20cc426 100644 --- a/pkg/plugin/implementation/reqpreprocessor/cmd/plugin_test.go +++ b/pkg/plugin/implementation/reqpreprocessor/cmd/plugin_test.go @@ -32,9 +32,9 @@ func TestProviderNew(t *testing.T) { }, }, { - name: "With Check Keys", + name: "Success with BPP role", config: map[string]string{ - "contextKeys": "message_id,transaction_id", + "role": "bpp", }, expectedError: false, expectedStatus: http.StatusOK, @@ -42,6 +42,29 @@ func TestProviderNew(t *testing.T) { // Add headers matching the check keys. req.Header.Set("context", "test-context") req.Header.Set("transaction_id", "test-transaction") + req.Header.Set("bpp_id", "bpp-456") + }, + }, + { + name: "Missing role configuration", + config: map[string]string{ + // No role specified + }, + expectedError: true, + prepareRequest: func(req *http.Request) { + req.Header.Set("context", "test-context") + req.Header.Set("transaction_id", "test-transaction") + }, + }, + { + name: "Invalid role configuration", + config: map[string]string{ + "role": "invalid-role", + }, + expectedError: true, + prepareRequest: func(req *http.Request) { + req.Header.Set("context", "test-context") + req.Header.Set("transaction_id", "test-transaction") }, }, } diff --git a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go index b9da290..f687b8a 100644 --- a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go +++ b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go @@ -13,8 +13,7 @@ import ( ) type Config struct { - ContextKeys []string - Role string + Role string } const contextKey = "context" @@ -49,7 +48,7 @@ func NewPreProcessor(cfg *Config) (func(http.Handler) http.Handler, error) { case "bap": subID = reqContext["bap_id"] case "bpp": - subID = reqContext["bap_id"] + subID = reqContext["bpp_id"] } if subID != nil { log.Debugf(ctx, "adding subscriberId to request:%s, %v", subscriberIDKey, subID) @@ -69,16 +68,8 @@ func validateConfig(cfg *Config) error { return errors.New("config cannot be nil") } - // Check if ContextKeys is empty. - if len(cfg.ContextKeys) == 0 { - return errors.New("ContextKeys cannot be empty") - } - - // Validate that ContextKeys does not contain empty strings. - for _, key := range cfg.ContextKeys { - if key == "" { - return errors.New("ContextKeys cannot contain empty strings") - } + if cfg.Role != "bap" && cfg.Role != "bpp" { + return errors.New("role must be either 'bap' or 'bpp'") } return nil } diff --git a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go index d70af8e..c327598 100644 --- a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go +++ b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go @@ -5,48 +5,48 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" ) -func TestNewUUIDSetterSuccessCases(t *testing.T) { +func TestNewPreProcessorSuccessCases(t *testing.T) { tests := []struct { - name string - config *Config - requestBody map[string]any - expectedKeys []string - role string + name string + config *Config + requestBody map[string]interface{} + expectedID string }{ { - name: "Valid keys, update missing keys with bap role", + name: "BAP role with valid context", config: &Config{ - ContextKeys: []string{"transaction_id", "message_id"}, - Role: "bap", + Role: "bap", }, - requestBody: map[string]any{ - "context": map[string]any{ - "transaction_id": "", - "message_id": nil, - "bap_id": "bap-123", + requestBody: map[string]interface{}{ + "context": map[string]interface{}{ + "bap_id": "bap-123", + "message_id": "msg-123", + }, + "message": map[string]interface{}{ + "key": "value", }, }, - expectedKeys: []string{"transaction_id", "message_id", "bap_id"}, - role: "bap", + expectedID: "bap-123", }, { - name: "Valid keys, do not update existing keys with bpp role", + name: "BPP role with valid context", config: &Config{ - ContextKeys: []string{"transaction_id", "message_id"}, - Role: "bpp", + Role: "bpp", }, - requestBody: map[string]any{ - "context": map[string]any{ - "transaction_id": "existing-transaction", - "message_id": "existing-message", - "bpp_id": "bpp-456", + requestBody: map[string]interface{}{ + "context": map[string]interface{}{ + "bpp_id": "bpp-456", + "message_id": "msg-456", + }, + "message": map[string]interface{}{ + "key": "value", }, }, - expectedKeys: []string{"transaction_id", "message_id", "bpp_id"}, - role: "bpp", + expectedID: "bpp-456", }, } @@ -54,29 +54,40 @@ func TestNewUUIDSetterSuccessCases(t *testing.T) { t.Run(tt.name, func(t *testing.T) { middleware, err := NewPreProcessor(tt.config) if err != nil { - t.Fatalf("Unexpected error while creating middleware: %v", err) + t.Fatalf("NewPreProcessor() error = %v", err) } - bodyBytes, _ := json.Marshal(tt.requestBody) + bodyBytes, err := json.Marshal(tt.requestBody) + if err != nil { + t.Fatalf("Failed to marshal request body: %v", err) + } req := httptest.NewRequest(http.MethodPost, "/test", bytes.NewReader(bodyBytes)) req.Header.Set("Content-Type", "application/json") rec := httptest.NewRecorder() + var gotSubID interface{} + dummyHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() + gotSubID = ctx.Value(subscriberIDKey) w.WriteHeader(http.StatusOK) - subID, ok := ctx.Value(subscriberIDKey).(string) - if !ok { - http.Error(w, "Subscriber ID not found", http.StatusInternalServerError) + // Verify subscriber ID + subID := ctx.Value(subscriberIDKey) + if subID == nil { + t.Errorf("Expected subscriber ID but got none %s", ctx) return } - response := map[string]any{"subscriber_id": subID} - if err := json.NewEncoder(w).Encode(response); err != nil { - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - return + // Verify the correct ID was set based on role + expectedKey := "bap_id" + if tt.config.Role == "bpp" { + expectedKey = "bpp_id" + } + expectedID := tt.requestBody["context"].(map[string]interface{})[expectedKey] + if subID != expectedID { + t.Errorf("Expected subscriber ID %v, got %v", expectedID, subID) } }) @@ -87,71 +98,113 @@ func TestNewUUIDSetterSuccessCases(t *testing.T) { return } - var responseBody map[string]any - if err := json.Unmarshal(rec.Body.Bytes(), &responseBody); err != nil { - t.Fatal("Failed to unmarshal response body:", err) - } - - expectedSubIDKey := "bap_id" - if tt.role == "bpp" { - expectedSubIDKey = "bpp_id" - } - - subID, ok := responseBody["subscriber_id"].(string) - if !ok { - t.Error("subscriber_id not found in response") + // Verify subscriber ID + if gotSubID == nil { + t.Error("Expected subscriber_id to be set in context but got nil") return } - expectedSubID := tt.requestBody["context"].(map[string]any)[expectedSubIDKey] - if subID != expectedSubID { - t.Errorf("Expected subscriber_id %v, but got %v", expectedSubID, subID) + subID, ok := gotSubID.(string) + if !ok { + t.Errorf("Expected subscriber_id to be string, got %T", gotSubID) + return + } + + if subID != tt.expectedID { + t.Errorf("Expected subscriber_id %q, got %q", tt.expectedID, subID) } }) } } -func TestNewUUIDSetterErrorCases(t *testing.T) { +func TestNewPreProcessorErrorCases(t *testing.T) { tests := []struct { name string config *Config - requestBody map[string]any + requestBody interface{} expectedCode int + expectErr bool + errMsg string }{ { - name: "Missing context key", + name: "Missing context", config: &Config{ - ContextKeys: []string{"transaction_id"}, + Role: "bap", }, requestBody: map[string]any{ "otherKey": "value", }, expectedCode: http.StatusBadRequest, + expectErr: false, + errMsg: "context field not found or invalid", }, { name: "Invalid context type", config: &Config{ - ContextKeys: []string{"transaction_id"}, + Role: "bap", }, requestBody: map[string]any{ "context": "not-a-map", }, expectedCode: http.StatusBadRequest, + expectErr: false, + errMsg: "context field not found or invalid", }, { name: "Nil config", config: nil, requestBody: map[string]any{}, expectedCode: http.StatusInternalServerError, + expectErr: true, + errMsg: "config cannot be nil", + }, + { + name: "Invalid role", + config: &Config{ + Role: "invalid-role", + }, + requestBody: map[string]interface{}{ + "context": map[string]interface{}{ + "bap_id": "bap-123", + }, + }, + expectedCode: http.StatusInternalServerError, + expectErr: true, + errMsg: "role must be either 'bap' or 'bpp'", + }, + { + name: "Missing subscriber ID", + config: &Config{ + Role: "bap", + }, + requestBody: map[string]interface{}{ + "context": map[string]interface{}{ + "message_id": "msg-123", + }, + }, + expectedCode: http.StatusOK, + expectErr: false, + }, + { + name: "Invalid JSON body", + config: &Config{ + Role: "bap", + }, + requestBody: "{invalid-json}", + expectedCode: http.StatusBadRequest, + expectErr: false, + errMsg: "failed to decode request body", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { middleware, err := NewPreProcessor(tt.config) - if tt.config == nil { + if tt.expectErr { if err == nil { - t.Error("Expected an error for nil config, but got none") + t.Errorf("Expected an error for NewPreProcessor(%s), but got none", tt.config) + } else if tt.errMsg != "" && !strings.Contains(err.Error(), tt.errMsg) { + t.Errorf("Expected error to contain %q, got %v", tt.errMsg, err) } return } From 159ff956d3a48edc66399afa4b0517a91061dc9f Mon Sep 17 00:00:00 2001 From: tanyamadaan Date: Thu, 3 Apr 2025 16:18:26 +0530 Subject: [PATCH 3/4] Bug Fix for ReqPreProcessor plugin --- .../implementation/reqpreprocessor/reqpreprocessor.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go index f687b8a..6d30c38 100644 --- a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go +++ b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor.go @@ -16,8 +16,12 @@ type Config struct { Role string } -const contextKey = "context" -const subscriberIDKey = "subscriber_id" +type keyType string + +const ( + contextKey keyType = "context" + subscriberIDKey keyType = "subscriber_id" +) func NewPreProcessor(cfg *Config) (func(http.Handler) http.Handler, error) { if err := validateConfig(cfg); err != nil { From 1ae7a164613d46800380aa53885b72593885e99e Mon Sep 17 00:00:00 2001 From: tanyamadaan Date: Fri, 4 Apr 2025 11:06:54 +0530 Subject: [PATCH 4/4] fix in type --- .../implementation/reqpreprocessor/reqpreprocessor_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go index c327598..960d7ea 100644 --- a/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go +++ b/pkg/plugin/implementation/reqpreprocessor/reqpreprocessor_test.go @@ -9,11 +9,12 @@ import ( "testing" ) +// ToDo Separate Middleware creation and execution. func TestNewPreProcessorSuccessCases(t *testing.T) { tests := []struct { name string config *Config - requestBody map[string]interface{} + requestBody map[string]any expectedID string }{ {