Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ensure get returns a nil and key not found as per spec #844

Merged
merged 1 commit into from Oct 11, 2021
Merged

ensure get returns a nil and key not found as per spec #844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion js.go
View file
Open in desktop
Expand Up @@ -254,16 +254,25 @@ func (opt jsOptFn) configureJSContext(opts *jsOpts) error {

// Domain changes the domain part of JetSteam API prefix.
func Domain(domain string) JSOpt {
if domain == _EMPTY_ {
return APIPrefix(_EMPTY_)
}

return APIPrefix(fmt.Sprintf(jsDomainT, domain))
}

// APIPrefix changes the default prefix used for the JetStream API.
func APIPrefix(pre string) JSOpt {
return jsOptFn(func(js *jsOpts) error {
if pre == _EMPTY_ {
return nil
}

js.pre = pre
if !strings.HasSuffix(js.pre, ".") {
js.pre = js.pre + "."
}

return nil
})
}
Expand Down Expand Up @@ -2430,7 +2439,7 @@ func (m *Msg) checkReply() (*js, *jsSub, error) {
if m == nil || m.Sub == nil {
return nil, nil, ErrMsgNotBound
}
if m.Reply == "" {
if m.Reply == _EMPTY_ {
return nil, nil, ErrMsgNoReply
}
sub := m.Sub
Expand Down
17 changes: 16 additions & 1 deletion kv.go
View file
Open in desktop
Expand Up @@ -333,6 +333,18 @@ func keyValid(key string) bool {

// Get returns the latest value for the key.
func (kv *kvs) Get(key string) (KeyValueEntry, error) {
e, err := kv.get(key)
if err == ErrKeyDeleted {
return nil, ErrKeyNotFound
Copy link
Contributor Author

@ripienaar ripienaar Oct 11, 2021 •
edited

Choose a reason for hiding this comment

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

It's critical that from the perspective of a user of Get() its always found/not found when delete or purge operation, we also discussed returning half baked Entries and agreed to return nil instead.

However we use the invalid behavior of the previous get operation in later functions in Create(), so I retained old behavior for internal use in get() but made Get() comply with spec

Copy link
Contributor Author

@ripienaar ripienaar Oct 11, 2021 •
edited

Choose a reason for hiding this comment

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

Since ErrKeyDeleted is now internal only maybe I should unexport it

}
if err != nil {
return nil, err
}

return e, nil
}

func (kv *kvs) get(key string) (KeyValueEntry, error) {
if !keyValid(key) {
return nil, ErrInvalidKey
}
Expand Down Expand Up @@ -367,6 +379,7 @@ func (kv *kvs) Get(key string) (KeyValueEntry, error) {
entry.op = KeyValuePurge
return entry, ErrKeyDeleted
}

}

return entry, nil
Expand Down Expand Up @@ -400,11 +413,13 @@ func (kv *kvs) Create(key string, value []byte) (revision uint64, err error) {
if err == nil {
return v, nil
}

// TODO(dlc) - Since we have tombstones for DEL ops for watchers, this could be from that
// so we need to double check.
if e, err := kv.Get(key); err == ErrKeyDeleted {
if e, err := kv.get(key); err == ErrKeyDeleted {
return kv.Update(key, value, e.Revision())
}

return 0, err
}

Expand Down
9 changes: 4 additions & 5 deletions test/kv_test.go
View file
Open in desktop
Expand Up @@ -60,7 +60,7 @@ func TestKeyValueBasics(t *testing.T) {
err = kv.Delete("name")
expectOk(t, err)
_, err = kv.Get("name")
expectErr(t, err, nats.ErrKeyDeleted)
expectErr(t, err, nats.ErrKeyNotFound)
r, err = kv.Create("name", []byte("derek"))
expectOk(t, err)
if r != 3 {
Expand Down Expand Up @@ -284,10 +284,9 @@ func TestKeyValueDeleteVsPurge(t *testing.T) {
expectOk(t, err)
// Check marker
e, err := kv.Get("name")
expectErr(t, err, nats.ErrKeyDeleted)
// Also make sure op is purge
if e.Operation() != nats.KeyValuePurge {
t.Fatalf("Expected a purge operation but got %v", e.Operation())
expectErr(t, err, nats.ErrKeyNotFound)
if e != nil {
t.Fatalf("Expected a nil entry but got %v", e)
}
entries, err = kv.History("name")
expectOk(t, err)
Expand Down