From 3dac92f5ef33f84915dc8c188cdadb39cdd02518 Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Wed, 13 Dec 2023 19:43:58 -0500 Subject: [PATCH 1/4] Correct uniqueness lookup for nil location Signed-off-by: Peter Broadhurst --- internal/contracts/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index b5a4e51de..1dc327102 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -861,7 +861,7 @@ func (cm *contractManager) AddContractListener(ctx context.Context, listener *co fb := database.ContractListenerQueryFactory.NewFilter(ctx) if existing, _, err := cm.database.GetContractListeners(ctx, cm.namespace, fb.And( fb.Eq("topic", listener.Topic), - fb.Eq("location", listener.Location.String()), + fb.Eq("location", listener.Location), fb.Eq("signature", listener.Signature), )); err != nil { return err From 4c54af52f0f3cc892d099599769073932ab75bf0 Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Wed, 13 Dec 2023 19:56:19 -0500 Subject: [PATCH 2/4] Handle empty string consistently too Signed-off-by: Peter Broadhurst --- internal/contracts/manager.go | 5 +++- internal/contracts/manager_test.go | 41 ++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index 1dc327102..122f07243 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -857,11 +857,14 @@ func (cm *contractManager) AddContractListener(ctx context.Context, listener *co } // Namespace + Topic + Location + Signature must be unique + if len(strings.TrimSpace(listener.Location.String())) == 0 { + listener.Location = nil // consistently store nil + } listener.Signature = cm.blockchain.GenerateEventSignature(ctx, &listener.Event.FFIEventDefinition) fb := database.ContractListenerQueryFactory.NewFilter(ctx) if existing, _, err := cm.database.GetContractListeners(ctx, cm.namespace, fb.And( fb.Eq("topic", listener.Topic), - fb.Eq("location", listener.Location), + fb.Eq("location", listener.Location), // we query nil fb.Eq("signature", listener.Signature), )); err != nil { return err diff --git a/internal/contracts/manager_test.go b/internal/contracts/manager_test.go index 2663cecdd..726ada3d8 100644 --- a/internal/contracts/manager_test.go +++ b/internal/contracts/manager_test.go @@ -788,6 +788,47 @@ func TestAddContractListenerInline(t *testing.T) { mdi.AssertExpectations(t) } +func TestAddContractListenerInlineEmptyString(t *testing.T) { + cm := newTestContractManager() + mbi := cm.blockchain.(*blockchainmocks.Plugin) + mdi := cm.database.(*databasemocks.Plugin) + + sub := &core.ContractListenerInput{ + ContractListener: core.ContractListener{ + Location: fftypes.JSONAnyPtr(" "), + Event: &core.FFISerializedEvent{ + FFIEventDefinition: fftypes.FFIEventDefinition{ + Name: "changed", + Params: fftypes.FFIParams{ + { + Name: "value", + Schema: fftypes.JSONAnyPtr(`{"type": "integer"}`), + }, + }, + }, + }, + Options: &core.ContractListenerOptions{}, + Topic: "test-topic", + }, + } + + mbi.On("NormalizeContractLocation", context.Background(), blockchain.NormalizeListener, sub.Location).Return(sub.Location, nil) + mbi.On("GenerateEventSignature", context.Background(), mock.Anything).Return("changed") + mdi.On("GetContractListeners", context.Background(), "ns1", mock.Anything).Return(nil, nil, nil) + mbi.On("AddContractListener", context.Background(), mock.MatchedBy(func(cl *core.ContractListener) bool { + return cl.Location == nil // converted from empty string + })).Return(nil) + mdi.On("InsertContractListener", context.Background(), &sub.ContractListener).Return(nil) + + result, err := cm.AddContractListener(context.Background(), sub) + assert.NoError(t, err) + assert.NotNil(t, result.ID) + assert.NotNil(t, result.Event) + + mbi.AssertExpectations(t) + mdi.AssertExpectations(t) +} + func TestAddContractListenerNoLocationOK(t *testing.T) { cm := newTestContractManager() mbi := cm.blockchain.(*blockchainmocks.Plugin) From 34b400d376367bec56375041c3db20e1d7b28d90 Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Wed, 13 Dec 2023 21:33:16 -0500 Subject: [PATCH 3/4] Work around complexities of fftypes.JSONAny nil query Signed-off-by: Peter Broadhurst --- internal/contracts/manager.go | 13 +++++++++---- internal/contracts/manager_test.go | 7 +++---- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index 122f07243..16b397ce9 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -19,6 +19,7 @@ package contracts import ( "context" "crypto/sha256" + "database/sql/driver" "encoding/hex" "fmt" "hash" @@ -857,14 +858,18 @@ func (cm *contractManager) AddContractListener(ctx context.Context, listener *co } // Namespace + Topic + Location + Signature must be unique - if len(strings.TrimSpace(listener.Location.String())) == 0 { - listener.Location = nil // consistently store nil - } listener.Signature = cm.blockchain.GenerateEventSignature(ctx, &listener.Event.FFIEventDefinition) + // Above we only call NormalizeContractLocation if the listener is non-nil, and that means + // for an unset location we will have a nil value. Using an fftypes.JSONAny in a query + // of nil does not yield the right result, so we need to do an explicit nil query. + var locationLookup driver.Value = nil + if !listener.Location.IsNil() { + locationLookup = listener.Location + } fb := database.ContractListenerQueryFactory.NewFilter(ctx) if existing, _, err := cm.database.GetContractListeners(ctx, cm.namespace, fb.And( fb.Eq("topic", listener.Topic), - fb.Eq("location", listener.Location), // we query nil + fb.Eq("location", locationLookup), fb.Eq("signature", listener.Signature), )); err != nil { return err diff --git a/internal/contracts/manager_test.go b/internal/contracts/manager_test.go index 726ada3d8..304d05933 100644 --- a/internal/contracts/manager_test.go +++ b/internal/contracts/manager_test.go @@ -788,14 +788,13 @@ func TestAddContractListenerInline(t *testing.T) { mdi.AssertExpectations(t) } -func TestAddContractListenerInlineEmptyString(t *testing.T) { +func TestAddContractListenerInlineNilLocation(t *testing.T) { cm := newTestContractManager() mbi := cm.blockchain.(*blockchainmocks.Plugin) mdi := cm.database.(*databasemocks.Plugin) sub := &core.ContractListenerInput{ ContractListener: core.ContractListener{ - Location: fftypes.JSONAnyPtr(" "), Event: &core.FFISerializedEvent{ FFIEventDefinition: fftypes.FFIEventDefinition{ Name: "changed", @@ -812,11 +811,11 @@ func TestAddContractListenerInlineEmptyString(t *testing.T) { }, } - mbi.On("NormalizeContractLocation", context.Background(), blockchain.NormalizeListener, sub.Location).Return(sub.Location, nil) mbi.On("GenerateEventSignature", context.Background(), mock.Anything).Return("changed") mdi.On("GetContractListeners", context.Background(), "ns1", mock.Anything).Return(nil, nil, nil) mbi.On("AddContractListener", context.Background(), mock.MatchedBy(func(cl *core.ContractListener) bool { - return cl.Location == nil // converted from empty string + // Normalize is not called for this case + return cl.Location == nil })).Return(nil) mdi.On("InsertContractListener", context.Background(), &sub.ContractListener).Return(nil) From a1c2760095a8445ddb83424d8cf7a77c5034fffd Mon Sep 17 00:00:00 2001 From: Peter Broadhurst Date: Wed, 13 Dec 2023 21:38:08 -0500 Subject: [PATCH 4/4] Back to .ToString() for non-nil case Signed-off-by: Peter Broadhurst --- internal/contracts/manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/contracts/manager.go b/internal/contracts/manager.go index 16b397ce9..85ff63eba 100644 --- a/internal/contracts/manager.go +++ b/internal/contracts/manager.go @@ -864,7 +864,7 @@ func (cm *contractManager) AddContractListener(ctx context.Context, listener *co // of nil does not yield the right result, so we need to do an explicit nil query. var locationLookup driver.Value = nil if !listener.Location.IsNil() { - locationLookup = listener.Location + locationLookup = listener.Location.String() } fb := database.ContractListenerQueryFactory.NewFilter(ctx) if existing, _, err := cm.database.GetContractListeners(ctx, cm.namespace, fb.And(