Adding a simple dataref extension #1018

matzew · 2024-02-20T07:43:16Z

Inspired by the java sdk, adding a go implementation for dataref extension

Fixes #198

duglin · 2024-02-20T12:29:52Z

v2/extensions/dataref_extension.go

+const DataRefExtensionKey = "dataref"
+
+type DataRefExtension struct {
+	DataRef string `json:"dataref"`


Not that it matters much but I'm curious why it's a struct instead of just a string? Are you leaving the option open that one day we may need to include extra metadata along with the URL?

matzew · 2024-02-20T12:55:33Z

yeah, to have it more flexible 🤷‍♂️ Sent from Gmail Mobile

…

On Tue 20. Feb 2024 at 13:30, Doug Davis ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In v2/extensions/dataref_extension.go <#1018 (comment)>: > +/* + Copyright 2024 The CloudEvents Authors + SPDX-License-Identifier: Apache-2.0 +*/ + +package extensions + +import ( + "github.com/cloudevents/sdk-go/v2/event" + "net/url" +) + +const DataRefExtensionKey = "dataref" + +type DataRefExtension struct { + DataRef string `json:"dataref"` Not that it matters much but I'm curious why it's a struct instead of just a string? Are you leaving the option open that one day we may need to include extra metadata along with the URL? — Reply to this email directly, view it on GitHub <#1018 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABGPTUZAHNWNQI7YPPTCZDYUSJMZAVCNFSM6AAAAABDQULMHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJQGMYDSMZTGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

duglin

LGTM

embano1 · 2024-02-20T13:57:12Z

v2/extensions/dataref_extension_test.go

+func TestAddDataRefExtensionValidURL(t *testing.T) {
+	e := event.New()
+	expectedDataRef := "https://example.com/data"
+
+	err := AddDataRefExtension(&e, expectedDataRef)
+	if err != nil {
+		t.Fatalf("Failed to add DataRefExtension with valid URL: %s", err)
+	}
+}
+
+func TestAddDataRefExtensionInvalidURL(t *testing.T) {
+	e := event.New()
+	invalidDataRef := "://invalid-url"
+
+	err := AddDataRefExtension(&e, invalidDataRef)
+	if err == nil {
+		t.Fatal("Expected error when adding DataRefExtension with invalid URL, but got none")
+	}
+}
+
+func TestGetDataRefExtensionNotFound(t *testing.T) {
+	e := event.New()
+
+	_, ok := GetDataRefExtension(e)
+	if ok {
+		t.Fatal("Expected not to find DataRefExtension, but did")
+	}
+}


Looks like Java unit tests :)

nit: can we use table-driven tests so those tests are easier to modify/add later? Example here:

sdk-go/v2/protocol/http/protocol_retry_test.go

Line 25 in 4cc6c2d

func TestRequestWithRetries_linear(t *testing.T) {

Just noticed you might be following the distributed tracing implementation which uses same style...whatever you prefer

@matzew I'd like to merge this PR - did you want to update something based on this comment?

ping @matzew

duglin · 2024-03-22T16:30:14Z

rebase needed

duglin · 2025-03-21T11:48:41Z

@matzew @embano1 tweaked the tests to be table driven per @embano1 's request.

@embano1 for the review/merge.

duglin · 2025-03-21T12:00:13Z

Added ref to #198 so it'll close it once we merge

embano1 · 2025-03-22T16:56:42Z

v2/extensions/dataref_extension_test.go

+func TestGetDataRefExtensionNotFound(t *testing.T) {
+	e := event.New()
+
+	_, ok := GetDataRefExtension(e)


nit: missing positive case getting extension

duglin · 2025-03-22T17:41:54Z

Added more tests.
I changed the getter to return "nil" instead of an empty struct when it's not set. I think checking for nil is more appropriate, more go-ish.

embano1 · 2025-03-23T19:09:03Z

v2/extensions/dataref_extension.go

+		}
+		return &DataRefExtension{DataRef: dataRefStr}, true
+	}
+	return nil, false


Sorry Doug for this nit, but let's keep the value semantics here because using pointer can lead to (ownership) surprises (without checking the rest of the code, I assume we mostly use value semantics for core CE fields).

Typically (convention?), if an error is returned, the value should not be used (we can add a comment to make it even more explicit). Alternatively, If needed, users can check for the zero value with https://pkg.go.dev/reflect#Value.IsZero.

this is actually one of the reasons I prefer to return nil - I find the entire Zero value stuff annoying when nil is much more clear . If we return a Zero struct then it's too easy for someone to think the dataref is set but with "" - which is a valid URL. But I see at least one of the other extensions does it without a pointer so I'll just hold my nose and switch it back.

IIRC, Go is moving to making it easier to understand zero values (recently they did it with time for example) because nil has so many edges, most importantly surprising side effects for SDK users due to ownership/pointer semantics.

nit: we should add comments to these new public methods.

embano1 · 2025-03-23T19:13:06Z

v2/extensions/dataref_extension_test.go

+				t.Fatalf("Retrieved dataref(%v) doesn't match set value(%s)",
+					dr, test.dataref)
+			}
+		} else {


nit: I think it's safe to remove the else and directly jump to the next if without changing test semantics?

I think we need it because we only want to fail when the dataref is set on the "failed" case, not when the pass is expected to pass.

duglin · 2025-03-23T23:19:00Z

@embano1 updated

Signed-off-by: Matthias Wessendorf <[email protected]> Signed-off-by: Doug Davis <[email protected]>

embano1

LGTM

matzew force-pushed the add_dataref_extension branch from 7c1fbd1 to e84d5f0 Compare February 20, 2024 08:28

duglin reviewed Feb 20, 2024

View reviewed changes

duglin previously approved these changes Feb 20, 2024

View reviewed changes

embano1 reviewed Feb 20, 2024

View reviewed changes

duglin force-pushed the add_dataref_extension branch from e84d5f0 to 40188ae Compare July 16, 2024 12:12

duglin requested a review from a team as a code owner July 16, 2024 12:12

duglin force-pushed the add_dataref_extension branch from 40188ae to 58f801f Compare September 26, 2024 13:47

duglin force-pushed the add_dataref_extension branch from 58f801f to f0848db Compare March 19, 2025 14:00

duglin added the AwaitingAuthor label Mar 19, 2025

duglin dismissed their stale review via 09c9b2c March 21, 2025 11:47

duglin force-pushed the add_dataref_extension branch from f0848db to 09c9b2c Compare March 21, 2025 11:47

duglin removed the AwaitingAuthor label Mar 21, 2025

duglin force-pushed the add_dataref_extension branch from 09c9b2c to 26ade34 Compare March 22, 2025 13:33

embano1 reviewed Mar 22, 2025

View reviewed changes

duglin force-pushed the add_dataref_extension branch from 26ade34 to 9c287f1 Compare March 22, 2025 17:40

ivan-penchev mentioned this pull request Mar 22, 2025

Release changes from main #1132

Closed

embano1 reviewed Mar 23, 2025

View reviewed changes

duglin force-pushed the add_dataref_extension branch from 9c287f1 to 435db4a Compare March 23, 2025 23:18

Adding a simple dataref extension, similar to the java sdk

535da92

Signed-off-by: Matthias Wessendorf <[email protected]> Signed-off-by: Doug Davis <[email protected]>

duglin force-pushed the add_dataref_extension branch from 435db4a to 535da92 Compare March 24, 2025 09:35

embano1 enabled auto-merge March 24, 2025 10:14

embano1 approved these changes Mar 24, 2025

View reviewed changes

embano1 merged commit 69cfc2d into cloudevents:main Mar 24, 2025
9 checks passed

dependabot bot mentioned this pull request Mar 25, 2025

Bump github.com/cloudevents/sdk-go/v2 from 2.15.2 to 2.16.0 octo-sts/app#820

Merged

Adding a simple dataref extension #1018

Adding a simple dataref extension #1018

Uh oh!

Conversation

matzew commented Feb 20, 2024 • edited by duglin Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matzew commented Feb 20, 2024 via email

Uh oh!

duglin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Mar 22, 2024

Uh oh!

duglin commented Mar 21, 2025

Uh oh!

duglin commented Mar 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Mar 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Mar 23, 2025

Uh oh!

embano1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matzew commented Feb 20, 2024 •

edited by duglin

Loading