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.

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

feat(sdk/go): Refactor the key/value Go SDK to be more idiomatic #1845

Merged
merged 1 commit into from
Oct 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions examples/tinygo-key-value/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@ import (
"net/http"

spin_http "github.com/fermyon/spin/sdk/go/http"
"github.com/fermyon/spin/sdk/go/key_value"
"github.com/fermyon/spin/sdk/go/kv"
)

func init() {
// handler for the http trigger
spin_http.Handle(func(w http.ResponseWriter, r *http.Request) {
store, err := key_value.Open("default")
store, err := kv.OpenStore("default")
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
defer key_value.Close(store)
defer store.Close()

body, err := io.ReadAll(r.Body)
if err != nil {
Expand All @@ -26,15 +26,15 @@ func init() {

switch r.Method {
case http.MethodPost:
err := key_value.Set(store, r.URL.Path, body)
err := store.Set(r.URL.Path, body)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}

w.WriteHeader(http.StatusOK)
case http.MethodGet:
value, err := key_value.Get(store, r.URL.Path)
value, err := store.Get(r.URL.Path)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
Expand All @@ -43,15 +43,14 @@ func init() {
w.WriteHeader(http.StatusOK)
w.Write(value)
case http.MethodDelete:
err := key_value.Delete(store, r.URL.Path)
if err != nil {
if err := store.Delete(r.URL.Path); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}

w.WriteHeader(http.StatusOK)
case http.MethodHead:
exists, err := key_value.Exists(store, r.URL.Path)
exists, err := store.Exists(r.URL.Path)
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
Expand Down
6 changes: 3 additions & 3 deletions sdk/go/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,15 @@ GENERATED_OUTBOUND_HTTP = http/wasi-outbound-http.c http/wasi-outbound-http.h
GENERATED_SPIN_HTTP = http/spin-http.c http/spin-http.h
GENERATED_OUTBOUND_REDIS = redis/outbound-redis.c redis/outbound-redis.h
GENERATED_SPIN_REDIS = redis/spin-redis.c redis/spin-redis.h
GENERATED_KEY_VALUE = key_value/key-value.c key_value/key-value.h
GENERATED_KEY_VALUE = kv/key-value.c kv/key-value.h
GENERATED_SQLITE = sqlite/sqlite.c sqlite/sqlite.h
GENERATED_LLM = llm/llm.c llm/llm.h

SDK_VERSION_SOURCE_FILE = sdk_version/sdk-version-go-template.c

# NOTE: Please update this list if you add a new directory to the SDK:
SDK_VERSION_DEST_FILES = config/sdk-version-go.c http/sdk-version-go.c \
key_value/sdk-version-go.c redis/sdk-version-go.c \
kv/sdk-version-go.c redis/sdk-version-go.c \
sqlite/sdk-version-go.c llm/sdk-version-go.c

# NOTE: To generate the C bindings you need to install a forked version of wit-bindgen.
Expand Down Expand Up @@ -84,7 +84,7 @@ $(GENERATED_SPIN_REDIS):
wit-bindgen c --export ../../wit/ephemeral/spin-redis.wit --out-dir ./redis

$(GENERATED_KEY_VALUE):
wit-bindgen c --import ../../wit/ephemeral/key-value.wit --out-dir ./key_value
wit-bindgen c --import ../../wit/ephemeral/key-value.wit --out-dir ./kv

$(GENERATED_SQLITE):
wit-bindgen c --import ../../wit/ephemeral/sqlite.wit --out-dir ./sqlite
Expand Down
128 changes: 0 additions & 128 deletions sdk/go/key_value/key-value.go

This file was deleted.

File renamed without changes.
File renamed without changes.
137 changes: 137 additions & 0 deletions sdk/go/kv/kv.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
// Package kv provides access to key value stores within Spin
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to start only building these when building for WASI and otherwise provide a no-op stub? It'll make building mixed spin/non-spin codebases easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems fine in theory. Would you copy all the function stubs or is there something to automate that?

// components.
package kv

// #include "key-value.h"
import "C"
import (
"errors"
"fmt"
"unsafe"
)

// Store is the Key/Value backend storage.
type Store struct {
name string
active bool
ptr C.key_value_store_t
}

// OpenStore creates a new instance of Store and opens a connection.
func OpenStore(name string) (*Store, error) {
s := &Store{name: name}
if err := s.open(); err != nil {
return nil, err
}
return s, nil
}

// Close terminates the connection to Store.
func (s *Store) Close() {
if s.active {
C.key_value_close(C.uint32_t(s.ptr))
}
s.active = false
}

// Get retrieves a value from Store.
func (s *Store) Get(key string) ([]byte, error) {
ckey := toCStr(key)
var ret C.key_value_expected_list_u8_error_t
C.key_value_get(C.uint32_t(s.ptr), &ckey, &ret)
if ret.is_err {
return nil, toErr((*C.key_value_error_t)(unsafe.Pointer(&ret.val)))
}
list := (*C.key_value_list_u8_t)(unsafe.Pointer(&ret.val))
return C.GoBytes(unsafe.Pointer(list.ptr), C.int(list.len)), nil
}

// Delete removes a value from Store.
func (s *Store) Delete(key string) error {
ckey := toCStr(key)
var ret C.key_value_expected_unit_error_t
C.key_value_delete(C.uint32_t(s.ptr), &ckey, &ret)
if ret.is_err {
return toErr((*C.key_value_error_t)(unsafe.Pointer(&ret.val)))
}
return nil
}

// Set creates a new key/value in Store.
func (s *Store) Set(key string, value []byte) error {
ckey := toCStr(key)
cbytes := toCBytes(value)
var ret C.key_value_expected_unit_error_t
C.key_value_set(C.uint32_t(s.ptr), &ckey, &cbytes, &ret)
if ret.is_err {
return toErr((*C.key_value_error_t)(unsafe.Pointer(&ret.val)))
}
return nil
}

// Exists checks if a key exists within Store.
func (s *Store) Exists(key string) (bool, error) {
ckey := toCStr(key)
var ret C.key_value_expected_bool_error_t
C.key_value_exists(C.uint32_t(s.ptr), &ckey, &ret)
if ret.is_err {
return false, toErr((*C.key_value_error_t)(unsafe.Pointer(&ret.val)))
}
return *(*bool)(unsafe.Pointer(&ret.val)), nil
}

func (s *Store) open() error {
if s.active {
return nil
}
cname := toCStr(s.name)
var ret C.key_value_expected_store_error_t
C.key_value_open(&cname, &ret)
if ret.is_err {
return toErr((*C.key_value_error_t)(unsafe.Pointer(&ret.val)))
}
s.ptr = *(*C.key_value_store_t)(unsafe.Pointer(&ret.val))
s.active = true
return nil
}

func toCBytes(x []byte) C.key_value_list_u8_t {
return C.key_value_list_u8_t{ptr: (*C.uint8_t)(unsafe.Pointer(&x[0])), len: C.size_t(len(x))}
}

func toCStr(x string) C.key_value_string_t {
return C.key_value_string_t{ptr: C.CString(x), len: C.size_t(len(x))}
}

func fromCStrList(list *C.key_value_list_string_t) []string {
var result []string

listLen := int(list.len)
slice := unsafe.Slice(list.ptr, listLen)
for i := 0; i < listLen; i++ {
str := slice[i]
result = append(result, C.GoStringN(str.ptr, C.int(str.len)))
}

return result
}

func toErr(err *C.key_value_error_t) error {
switch err.tag {
case 0:
return errors.New("store table full")
case 1:
return errors.New("no such store")
case 2:
return errors.New("access denied")
case 3:
return errors.New("invalid store")
case 4:
return errors.New("no such key")
Comment on lines +122 to +130
Copy link
Collaborator

@lann lann Oct 17, 2023

Choose a reason for hiding this comment

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

These should be exported Err* constants so that callers can distinguish between error types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to include that in a separate PR to make all the SDK's consistent. I wasn't sure yet if a custom error type would be the way to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, that's fine. As a general approach I would probably do something like:

// Named const for payload-less variants
var ErrAccessDenied = errors.New("access denied")

// Error type for most payload-ful variants (not KV)
type ImaginaryError string

// Anonymous errors for `other(string)` variants
errors.New(otherString)

Copy link
Member Author

@adamreese adamreese Oct 20, 2023

Choose a reason for hiding this comment

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

I was thinking about something like...

type Error struct {
  Code int
  Message string
}

Stdlib example https://github.com/golang/go/blob/master/src/net/textproto/textproto.go#L35

But it's still a little awkward because Code is an internal value that has no real meaning to the user

Copy link
Collaborator

Choose a reason for hiding this comment

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

It depends on whether we care more about trying to closely match the WIT or trying to write a "idiomatic" Go lib I think. For the latter I would say separate var Err*s for no-payloads and type *Errors for yes-payloads most closely matches the ecosystem.

Copy link
Collaborator

@lann lann Oct 20, 2023

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm sold. I was getting distracted by the wit. Thanks for the journey.

case 5:
str := (*C.key_value_string_t)(unsafe.Pointer(&err.val))
return fmt.Errorf("io error: %s", C.GoStringN(str.ptr, C.int(str.len)))
default:
return fmt.Errorf("unrecognized error: %v", err.tag)
}
}