Skip to content

Commit

Permalink
Merge pull request kubernetes#486 from yugui/fix/404-if-notfound
Browse files Browse the repository at this point in the history
Make RESTful operations return 404 Not Found when the target resource does not exist.
  • Loading branch information
lavalamp committed Jul 18, 2014
2 parents a6e7e47 + 2aa3de1 commit 2188907
Show file tree
Hide file tree
Showing 7 changed files with 433 additions and 70 deletions.
56 changes: 52 additions & 4 deletions pkg/apiserver/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,39 @@ import (
"github.com/golang/glog"
)

// errNotFound is an error which indicates that a specified resource is not found.
type errNotFound string

// Error returns a string representation of the err.
func (err errNotFound) Error() string {
return string(err)
}

// IsNotFound determines if the err is an error which indicates that a specified resource was not found.
func IsNotFound(err error) bool {
_, ok := err.(errNotFound)
return ok
}

// NewNotFoundErr returns a new error which indicates that the resource of the kind and the name was not found.
func NewNotFoundErr(kind, name string) error {
return errNotFound(fmt.Sprintf("%s %q not found", kind, name))
}

// RESTStorage is a generic interface for RESTful storage services
// Resources whicih are exported to the RESTful API of apiserver need to implement this interface.
type RESTStorage interface {
// List selects resources in the storage which match to the selector.
List(labels.Selector) (interface{}, error)

// Get finds a resource in the storage by id and returns it.
// Although it can return an arbitrary error value, IsNotFound(err) is true for the returned error value err when the specified resource is not found.
Get(id string) (interface{}, error)

// Delete finds a resource in the storage and deletes it.
// Although it can return an arbitrary error value, IsNotFound(err) is true for the returned error value err when the specified resource is not found.
Delete(id string) (<-chan interface{}, error)

Extract(body []byte) (interface{}, error)
Create(interface{}) (<-chan interface{}, error)
Update(interface{}) (<-chan interface{}, error)
Expand Down Expand Up @@ -260,12 +288,12 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re
server.write(http.StatusOK, list, w)
case 2:
item, err := storage.Get(parts[1])
if err != nil {
server.error(err, w)
if IsNotFound(err) {
server.notFound(req, w)
return
}
if item == nil {
server.notFound(req, w)
if err != nil {
server.error(err, w)
return
}
server.write(http.StatusOK, item, w)
Expand All @@ -283,11 +311,19 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re
return
}
obj, err := storage.Extract(body)
if IsNotFound(err) {
server.notFound(req, w)
return
}
if err != nil {
server.error(err, w)
return
}
out, err := storage.Create(obj)
if IsNotFound(err) {
server.notFound(req, w)
return
}
if err != nil {
server.error(err, w)
return
Expand All @@ -299,6 +335,10 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re
return
}
out, err := storage.Delete(parts[1])
if IsNotFound(err) {
server.notFound(req, w)
return
}
if err != nil {
server.error(err, w)
return
Expand All @@ -314,11 +354,19 @@ func (server *APIServer) handleREST(parts []string, req *http.Request, w http.Re
server.error(err, w)
}
obj, err := storage.Extract(body)
if IsNotFound(err) {
server.notFound(req, w)
return
}
if err != nil {
server.error(err, w)
return
}
out, err := storage.Update(obj)
if IsNotFound(err) {
server.notFound(req, w)
return
}
if err != nil {
server.error(err, w)
return
Expand Down
122 changes: 102 additions & 20 deletions pkg/apiserver/apiserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type SimpleList struct {
}

type SimpleRESTStorage struct {
err error
errors map[string]error
list []Simple
item Simple
deleted string
Expand All @@ -69,17 +69,17 @@ func (storage *SimpleRESTStorage) List(labels.Selector) (interface{}, error) {
result := &SimpleList{
Items: storage.list,
}
return result, storage.err
return result, storage.errors["list"]
}

func (storage *SimpleRESTStorage) Get(id string) (interface{}, error) {
return storage.item, storage.err
return storage.item, storage.errors["get"]
}

func (storage *SimpleRESTStorage) Delete(id string) (<-chan interface{}, error) {
storage.deleted = id
if storage.err != nil {
return nil, storage.err
if storage.errors["delete"] != nil {
return nil, storage.errors["delete"]
}
return MakeAsync(func() (interface{}, error) {
if storage.injectedFunction != nil {
Expand All @@ -92,13 +92,13 @@ func (storage *SimpleRESTStorage) Delete(id string) (<-chan interface{}, error)
func (storage *SimpleRESTStorage) Extract(body []byte) (interface{}, error) {
var item Simple
api.DecodeInto(body, &item)
return item, storage.err
return item, storage.errors["extract"]
}

func (storage *SimpleRESTStorage) Create(obj interface{}) (<-chan interface{}, error) {
storage.created = obj.(Simple)
if storage.err != nil {
return nil, storage.err
if storage.errors["create"] != nil {
return nil, storage.errors["create"]
}
return MakeAsync(func() (interface{}, error) {
if storage.injectedFunction != nil {
Expand All @@ -110,8 +110,8 @@ func (storage *SimpleRESTStorage) Create(obj interface{}) (<-chan interface{}, e

func (storage *SimpleRESTStorage) Update(obj interface{}) (<-chan interface{}, error) {
storage.updated = obj.(Simple)
if storage.err != nil {
return nil, storage.err
if storage.errors["update"] != nil {
return nil, storage.errors["update"]
}
return MakeAsync(func() (interface{}, error) {
if storage.injectedFunction != nil {
Expand Down Expand Up @@ -141,15 +141,15 @@ func TestSimpleList(t *testing.T) {
resp, err := http.Get(server.URL + "/prefix/version/simple")
expectNoError(t, err)

if resp.StatusCode != 200 {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp)
if resp.StatusCode != http.StatusOK {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp)
}
}

func TestErrorList(t *testing.T) {
storage := map[string]RESTStorage{}
simpleStorage := SimpleRESTStorage{
err: fmt.Errorf("test Error"),
errors: map[string]error{"list": fmt.Errorf("test Error")},
}
storage["simple"] = &simpleStorage
handler := New(storage, "/prefix/version")
Expand All @@ -158,8 +158,8 @@ func TestErrorList(t *testing.T) {
resp, err := http.Get(server.URL + "/prefix/version/simple")
expectNoError(t, err)

if resp.StatusCode != 500 {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp)
if resp.StatusCode != http.StatusInternalServerError {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp)
}
}

Expand All @@ -179,8 +179,8 @@ func TestNonEmptyList(t *testing.T) {
resp, err := http.Get(server.URL + "/prefix/version/simple")
expectNoError(t, err)

if resp.StatusCode != 200 {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, 200, resp)
if resp.StatusCode != http.StatusOK {
t.Errorf("Unexpected status: %d, Expected: %d, %#v", resp.StatusCode, http.StatusOK, resp)
}

var listOut SimpleList
Expand Down Expand Up @@ -215,6 +215,22 @@ func TestGet(t *testing.T) {
}
}

func TestGetMissing(t *testing.T) {
storage := map[string]RESTStorage{}
simpleStorage := SimpleRESTStorage{
errors: map[string]error{"get": NewNotFoundErr("simple", "id")},
}
storage["simple"] = &simpleStorage
handler := New(storage, "/prefix/version")
server := httptest.NewServer(handler)

resp, err := http.Get(server.URL + "/prefix/version/simple/id")
expectNoError(t, err)
if resp.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", resp)
}
}

func TestDelete(t *testing.T) {
storage := map[string]RESTStorage{}
simpleStorage := SimpleRESTStorage{}
Expand All @@ -232,6 +248,25 @@ func TestDelete(t *testing.T) {
}
}

func TestDeleteMissing(t *testing.T) {
storage := map[string]RESTStorage{}
ID := "id"
simpleStorage := SimpleRESTStorage{
errors: map[string]error{"delete": NewNotFoundErr("simple", ID)},
}
storage["simple"] = &simpleStorage
handler := New(storage, "/prefix/version")
server := httptest.NewServer(handler)

client := http.Client{}
request, err := http.NewRequest("DELETE", server.URL+"/prefix/version/simple/"+ID, nil)
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}

func TestUpdate(t *testing.T) {
storage := map[string]RESTStorage{}
simpleStorage := SimpleRESTStorage{}
Expand All @@ -254,6 +289,30 @@ func TestUpdate(t *testing.T) {
}
}

func TestUpdateMissing(t *testing.T) {
storage := map[string]RESTStorage{}
ID := "id"
simpleStorage := SimpleRESTStorage{
errors: map[string]error{"update": NewNotFoundErr("simple", ID)},
}
storage["simple"] = &simpleStorage
handler := New(storage, "/prefix/version")
server := httptest.NewServer(handler)

item := Simple{
Name: "bar",
}
body, err := api.Encode(item)
expectNoError(t, err)
client := http.Client{}
request, err := http.NewRequest("PUT", server.URL+"/prefix/version/simple/"+ID, bytes.NewReader(body))
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}

func TestBadPath(t *testing.T) {
handler := New(map[string]RESTStorage{}, "/prefix/version")
server := httptest.NewServer(handler)
Expand All @@ -263,7 +322,7 @@ func TestBadPath(t *testing.T) {
expectNoError(t, err)
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != 404 {
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}
Expand All @@ -277,7 +336,7 @@ func TestMissingPath(t *testing.T) {
expectNoError(t, err)
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != 404 {
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}
Expand All @@ -293,7 +352,7 @@ func TestMissingStorage(t *testing.T) {
expectNoError(t, err)
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != 404 {
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}
Expand Down Expand Up @@ -324,6 +383,29 @@ func TestCreate(t *testing.T) {
}
}

func TestCreateNotFound(t *testing.T) {
simpleStorage := &SimpleRESTStorage{
// storage.Create can fail with not found error in theory.
// See https://github.com/GoogleCloudPlatform/kubernetes/pull/486#discussion_r15037092.
errors: map[string]error{"create": NewNotFoundErr("simple", "id")},
}
handler := New(map[string]RESTStorage{
"foo": simpleStorage,
}, "/prefix/version")
server := httptest.NewServer(handler)
client := http.Client{}

simple := Simple{Name: "foo"}
data, _ := api.Encode(simple)
request, err := http.NewRequest("POST", server.URL+"/prefix/version/foo", bytes.NewBuffer(data))
expectNoError(t, err)
response, err := client.Do(request)
expectNoError(t, err)
if response.StatusCode != http.StatusNotFound {
t.Errorf("Unexpected response %#v", response)
}
}

func TestParseTimeout(t *testing.T) {
if d := parseTimeout(""); d != 30*time.Second {
t.Errorf("blank timeout produces %v", d)
Expand Down
Loading

0 comments on commit 2188907

Please sign in to comment.