Skip to content

Conversation

kmpm
Copy link
Contributor

@kmpm kmpm commented Apr 11, 2025

I had some issues with a single subject regardless of event. Added a function to add an alternate subject via context.

This will also possibly close #1115

@kmpm kmpm requested a review from a team as a code owner April 11, 2025 14:00
@kmpm kmpm force-pushed the fix/alt-subject-jsmv3 branch from 1d08816 to 9b37c1a Compare April 14, 2025 06:02
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can you please squash and clean up your commits?

@kmpm kmpm force-pushed the fix/alt-subject-jsmv3 branch from 9b37c1a to 1c14031 Compare April 14, 2025 13:47
duglin
duglin previously approved these changes Apr 14, 2025
@duglin duglin self-requested a review April 14, 2025 14:28
@duglin
Copy link
Contributor

duglin commented Apr 14, 2025

Get we get a testcase?

@duglin duglin dismissed their stale review April 14, 2025 14:29

premature

@embano1
Copy link
Member

embano1 commented Apr 16, 2025

Fair ask @duglin . @kmpm do you mind adding a test for coverage of the existing/new behavior?

@kmpm
Copy link
Contributor Author

kmpm commented Apr 16, 2025

Protocol.Send looks like a mess to mock in any meaningful way.
I could add some unit tests for Protocol.getSendSubject and also look at modifying the integration tests if it's enough?

@embano1
Copy link
Member

embano1 commented Apr 18, 2025

Yeah sorry for the code base...I created a small unit test to verify the behavior, patch below

diff --git a/protocol/nats_jetstream/v3/protocol_test.go b/protocol/nats_jetstream/v3/protocol_test.go
new file mode 100644
index 0000000..b857a94
--- /dev/null
+++ b/protocol/nats_jetstream/v3/protocol_test.go
@@ -0,0 +1,85 @@
+/*
+ Copyright 2024 The CloudEvents Authors
+ SPDX-License-Identifier: Apache-2.0
+*/
+
+package nats_jetstream
+
+import (
+	"context"
+	"testing"
+
+	"github.com/cloudevents/sdk-go/v2/test"
+	"github.com/nats-io/nats.go"
+	"github.com/nats-io/nats.go/jetstream"
+)
+
+// mockJetStream implements jetstream.JetStream interface for testing
+type mockJetStream struct {
+	streamNameBySubjectFunc func(ctx context.Context, subject string) (string, error)
+	publishMsgFunc          func(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error)
+	jetstream.JetStream
+}
+
+func (m *mockJetStream) StreamNameBySubject(ctx context.Context, subject string) (string, error) {
+	return m.streamNameBySubjectFunc(ctx, subject)
+}
+
+func (m *mockJetStream) PublishMsg(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error) {
+	return m.publishMsgFunc(ctx, msg, opts...)
+}
+
+func TestSend(t *testing.T) {
+	tests := []struct {
+		name        string
+		sendSubject string
+		ctx         context.Context
+		wantSubject string
+	}{
+		{
+			name:        "using p.sendSubject",
+			sendSubject: "test.subject",
+			ctx:         context.Background(),
+			wantSubject: "test.subject",
+		},
+		{
+			name:        "using WithSubject",
+			sendSubject: "",
+			ctx:         WithSubject(context.Background(), "test.ctxSubject"),
+			wantSubject: "test.ctxSubject",
+		},
+		{
+			name:        "using WithSubject and p.sendSubject",
+			sendSubject: "test.subject",
+			ctx:         WithSubject(context.Background(), "test.ctxSubject"),
+			wantSubject: "test.ctxSubject",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			mockJS := &mockJetStream{
+				streamNameBySubjectFunc: func(ctx context.Context, subject string) (string, error) {
+					if subject != tt.wantSubject {
+						t.Errorf("unexpected subject: got %s, want %s", subject, tt.wantSubject)
+					}
+					return "test-stream", nil
+				},
+				publishMsgFunc: func(ctx context.Context, msg *nats.Msg, opts ...jetstream.PublishOpt) (*jetstream.PubAck, error) {
+					if msg.Subject != tt.wantSubject {
+						t.Errorf("unexpected subject in publish: got %s, want %s", msg.Subject, tt.wantSubject)
+					}
+					return nil, nil
+				},
+			}
+
+			p := &Protocol{
+				jetStream:   mockJS,
+				sendSubject: tt.sendSubject,
+			}
+
+			msg := test.FullMessage()
+			_ = p.Send(tt.ctx, msg)
+		})
+	}
+}

@embano1
Copy link
Member

embano1 commented May 1, 2025

@kmpm mind just copy and pasting the test so we can close this PR?

@kmpm
Copy link
Contributor Author

kmpm commented May 2, 2025

@embano1 , I will fix it. Life got in the way. Thanks.

@embano1 embano1 force-pushed the fix/alt-subject-jsmv3 branch from aa2bf56 to 7986922 Compare May 2, 2025 06:45
@embano1
Copy link
Member

embano1 commented May 2, 2025

Thank you! I rebased onto the most recent commit. Can you please squash the test commit into one final commit? Should also fix the DCO warning (you need to -s sign-off all commits).

added tests from @embano1

Signed-off-by: Peter Magnusson <me@kmpm.se>
@kmpm kmpm force-pushed the fix/alt-subject-jsmv3 branch from 7986922 to 9318872 Compare May 2, 2025 06:55
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@embano1 embano1 merged commit c2f73fb into cloudevents:main May 2, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why so inconvenient protocol binding for NATS?
3 participants