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

Pass http status codes #5

Merged
merged 6 commits into from
Jun 21, 2016
Merged
Show file tree
Hide file tree
Changes from 5 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
2 changes: 1 addition & 1 deletion composition/composition_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (agg *CompositionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)

} else if res.Def.Required {
log.WithField("fetchResult", res).Errorf("error loading content from: %v", res.Def.URL)
res.Def.ErrHandler.Handle(res.Err, w, r)
res.Def.ErrHandler.Handle(res.Err, res.HttpStatus, w, r)
return
} else {
log.WithField("fetchResult", res).Warnf("optional content not loaded: %v", res.Def.URL)
Expand Down
9 changes: 5 additions & 4 deletions composition/composition_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,10 @@ func Test_CompositionHandler_ErrorInFetching(t *testing.T) {
contentFetcherFactory := func(r *http.Request) FetchResultSupplier {
return MockFetchResultSupplier{
&FetchResult{
Def: NewFetchDefinition("/foo"),
Content: nil,
Err: errors.New(errorString),
Def: NewFetchDefinition("/foo"),
Content: nil,
Err: errors.New(errorString),
HttpStatus: 502,
},
}
}
Expand All @@ -185,7 +186,7 @@ func Test_CompositionHandler_ErrorInFetching(t *testing.T) {
r, _ := http.NewRequest("GET", "http://example.com", nil)
aggregator.ServeHTTP(resp, r)

a.Equal("Bad Gateway: "+errorString+"\n", string(resp.Body.Bytes()))
a.Equal("Error: "+errorString+"\n", string(resp.Body.Bytes()))
a.Equal(502, resp.Code)
}

Expand Down
16 changes: 9 additions & 7 deletions composition/content_fetcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"sync"

"github.com/tarent/lib-compose/logging"
"net/http"
"strings"
)

Expand All @@ -15,10 +16,11 @@ func (def *FetchDefinition) IsFetchable() bool {
}

type FetchResult struct {
Def *FetchDefinition
Err error
Content Content
Hash string // the hash of the FetchDefinition
Def *FetchDefinition
Err error
Content Content
HttpStatus int
Copy link
Member

Choose a reason for hiding this comment

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

The Http Status Code was already part of Content.httpStatusCode
so, there is no need to double it here in the fetch result.

Hash string // the hash of the FetchDefinition
}

// ContentFetcher is a type, which can fetch a set of Content pages in parallel.
Expand Down Expand Up @@ -75,7 +77,7 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) {

fetcher.activeJobs.Add(1)

fetchResult := &FetchResult{Def: d, Hash: hash, Err: errors.New("not fetched")}
fetchResult := &FetchResult{Def: d, Hash: hash, Err: errors.New("not fetched"), HttpStatus: http.StatusBadGateway}
fetcher.r.results = append(fetcher.r.results, fetchResult)

go func() {
Expand All @@ -94,7 +96,7 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) {
// want to override the original URL with expanded values
definitionCopy := *d
definitionCopy.URL = url
fetchResult.Content, fetchResult.Err = fetcher.fetch(&definitionCopy)
fetchResult.Content, fetchResult.Err, fetchResult.HttpStatus = fetcher.fetch(&definitionCopy)

if fetchResult.Err == nil {
fetcher.addMeta(fetchResult.Content.Meta())
Expand All @@ -112,7 +114,7 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) {
}()
}

func (fetcher *ContentFetcher) fetch(fd *FetchDefinition) (Content, error) {
func (fetcher *ContentFetcher) fetch(fd *FetchDefinition) (Content, error, int) {
if strings.HasPrefix(fd.URL, FileURLPrefix) {
return fetcher.fileContentLoader.Load(fd)
}
Expand Down
10 changes: 5 additions & 5 deletions composition/content_fetcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ func Test_ContentFetcher_FetchingWithDependency(t *testing.T) {
a := assert.New(t)

loader := NewMockContentLoader(ctrl)
barFd := getFetchDefinitionMock(ctrl, loader, "/bar", nil, time.Millisecond*2, map[string]interface{}{"foo": "bar"})
fooFd := getFetchDefinitionMock(ctrl, loader, "/foo", []*FetchDefinition{barFd}, time.Millisecond*2, map[string]interface{}{"bli": "bla"})
bazzFd := getFetchDefinitionMock(ctrl, loader, "/bazz", []*FetchDefinition{barFd}, time.Millisecond, map[string]interface{}{})
barFd := getFetchDefinitionMock(ctrl, loader, "/bar", nil, http.StatusOK, time.Millisecond*2, map[string]interface{}{"foo": "bar"})
fooFd := getFetchDefinitionMock(ctrl, loader, "/foo", []*FetchDefinition{barFd}, http.StatusOK, time.Millisecond*2, map[string]interface{}{"bli": "bla"})
bazzFd := getFetchDefinitionMock(ctrl, loader, "/bazz", []*FetchDefinition{barFd}, http.StatusOK, time.Millisecond, map[string]interface{}{})

fetcher := NewContentFetcher(nil)
fetcher.httpContentLoader = loader
Expand All @@ -91,7 +91,7 @@ func Test_ContentFetcher_FetchingWithDependency(t *testing.T) {
a.Equal("bla", meta["bli"])
}

func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoader, url string, requiredContent []*FetchDefinition, loaderBlocking time.Duration, metaJSON map[string]interface{}) *FetchDefinition {
func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoader, url string, requiredContent []*FetchDefinition, requiredStatusCode int, loaderBlocking time.Duration, metaJSON map[string]interface{}) *FetchDefinition {
fd := NewFetchDefinition(url)
fd.Timeout = time.Second * 42

Expand All @@ -110,7 +110,7 @@ func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoad
func(fetchDefinition *FetchDefinition) {
time.Sleep(loaderBlocking)
}).
Return(content, nil)
Return(content, nil, requiredStatusCode)

return fd
}
4 changes: 2 additions & 2 deletions composition/fetch_definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,6 @@ func NewDefaultErrorHandler() *DefaultErrorHandler {
return deh
}

func (der *DefaultErrorHandler) Handle(err error, w http.ResponseWriter, r *http.Request) {
http.Error(w, "Bad Gateway: "+err.Error(), 502)
func (der *DefaultErrorHandler) Handle(err error, status int, w http.ResponseWriter, r *http.Request) {
http.Error(w, "Error: "+err.Error(), status)
}
2 changes: 1 addition & 1 deletion composition/fetch_definition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,5 @@ func Test_FetchDefinition_use_DefaultErrorHandler_if_not_set(t *testing.T) {
a := assert.New(t)

fd := NewFetchDefinitionWithErrorHandler("http://upstream:8080/", nil)
a.Equal(NewDefaultErrorHandler(), fd.ErrHandler)
a.Equal(NewDefaultErrorHandler(), fd.ErrHandler)
}
10 changes: 5 additions & 5 deletions composition/file_content_loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@ func NewFileContentLoader() *FileContentLoader {
}
}

func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error) {
func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error, int) {
if fd.RespProc != nil {
return nil, ResponseProcessorsNotApplicable
return nil, ResponseProcessorsNotApplicable, 502
}
filename := strings.TrimPrefix(fd.URL, FileURLPrefix)
f, err := os.Open(filename)
if err != nil {
return nil, fmt.Errorf("error opening file %v: %v", fd.URL, err)
return nil, fmt.Errorf("error opening file %v: %v", fd.URL, err), 502
}

c := NewMemoryContent()
Expand All @@ -43,9 +43,9 @@ func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error) {
WithField("duration", time.Since(parsingStart)).
Debug("content parsing")
f.Close()
return c, err
return c, err, c.httpStatusCode
}

c.reader = f
return c, nil
return c, nil, c.httpStatusCode
}
8 changes: 4 additions & 4 deletions composition/file_content_loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ func Test_FileContentLoader_LoadHTML(t *testing.T) {
a.NoError(err)

loader := NewFileContentLoader()
c, err := loader.Load(NewFetchDefinition(FileURLPrefix + fileName))
c, err, _ := loader.Load(NewFetchDefinition(FileURLPrefix + fileName))
a.NoError(err)
a.NotNil(c)
a.Nil(c.Reader())
Expand All @@ -42,7 +42,7 @@ func Test_FileContentLoader_LoadStream(t *testing.T) {
a.NoError(err)

loader := NewFileContentLoader()
c, err := loader.Load(NewFetchDefinition(FileURLPrefix + fileName))
c, err, _ := loader.Load(NewFetchDefinition(FileURLPrefix + fileName))
a.NoError(err)
a.NotNil(c)
body, err := ioutil.ReadAll(c.Reader())
Expand All @@ -54,7 +54,7 @@ func Test_FileContentLoader_LoadError(t *testing.T) {
a := assert.New(t)

loader := NewFileContentLoader()
_, err := loader.Load(NewFetchDefinition("/tmp/some/non/existing/path"))
_, err, _ := loader.Load(NewFetchDefinition("/tmp/some/non/existing/path"))
a.Error(err)
}

Expand All @@ -66,7 +66,7 @@ func Test_FileContentLoader_RequestProcessor(t *testing.T) {
fd := NewFetchDefinition("/tmp/some/non/existing/path")
fd.RespProc = NewMockResponseProcessor(ctrl)

_, err := NewFileContentLoader().Load(fd)
_, err, _ := NewFileContentLoader().Load(fd)
a.Equal(ResponseProcessorsNotApplicable, err)
}

Expand Down
23 changes: 14 additions & 9 deletions composition/http_content_loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,14 @@ func NewHttpContentLoader() *HttpContentLoader {
}

// TODO: Should we filter the headers, which we forward here, or is it correct to copy all of them?
func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error) {
func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error, int) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above:
Since the Http Status Code is already part of Content.httpStatusCode,
there is no need to double it here in the method return parameters.

client := &http.Client{Timeout: fd.Timeout}
statusCode := 502

var err error
request, err := http.NewRequest(fd.Method, fd.URL, fd.Body)
if err != nil {
return nil, err
return nil, err, 0
}
request.Header = fd.Header
if request.Header == nil {
Expand All @@ -41,26 +42,30 @@ func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error) {

resp, err := client.Do(request)

if resp != nil {
statusCode = resp.StatusCode
}

logging.Call(request, resp, start, err)
if err != nil {
return nil, err
return nil, err, statusCode
}

if fd.RespProc != nil {
err = fd.RespProc.Process(resp, fd.URL)
}
if err != nil {
return nil, err
return nil, err, statusCode
}

if resp.StatusCode < 200 || resp.StatusCode > 399 {
return nil, fmt.Errorf("(http %v) on loading url %q", resp.StatusCode, fd.URL)
if statusCode < 200 || statusCode > 399 {
return nil, fmt.Errorf("(http %v) on loading url %q", statusCode, fd.URL), statusCode
}

c := NewMemoryContent()
c.url = fd.URL
c.httpHeader = resp.Header
c.httpStatusCode = resp.StatusCode
c.httpStatusCode = statusCode

// take the first parser for the content type
// direct access to the map does not work, because the
Expand All @@ -81,11 +86,11 @@ func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error) {
WithField("full_url", c.URL()).
WithField("duration", time.Since(parsingStart)).
Debug("content parsing")
return c, err
return c, err, statusCode
}
}
}

c.reader = resp.Body
return c, nil
return c, nil, statusCode
}
32 changes: 25 additions & 7 deletions composition/http_content_loader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func Test_HttpContentLoader_Load(t *testing.T) {

loader.parser["text/html"] = mockParser

c, err := loader.Load(NewFetchDefinition(server.URL))
c, err, _ := loader.Load(NewFetchDefinition(server.URL))
a.NoError(err)
a.NotNil(c)
a.Nil(c.Reader())
Expand Down Expand Up @@ -64,7 +64,7 @@ func Test_HttpContentLoader_Load_ResponseProcessor(t *testing.T) {

mockResponseProcessor := NewMockResponseProcessor(ctrl)
mockResponseProcessor.EXPECT().Process(gomock.Any(), gomock.Any())
c, err := loader.Load(NewFetchDefinitionWithResponseProcessor(server.URL, mockResponseProcessor))
c, err, _ := loader.Load(NewFetchDefinitionWithResponseProcessor(server.URL, mockResponseProcessor))
a.NoError(err)
a.NotNil(c)
a.Nil(c.Reader())
Expand Down Expand Up @@ -105,7 +105,7 @@ func Test_HttpContentLoader_Load_POST(t *testing.T) {
fd.Method = "POST"
fd.Body = strings.NewReader("post content")

c, err := loader.Load(fd)
c, err, _ := loader.Load(fd)
a.NoError(err)
a.NotNil(c)
a.Nil(c.Reader())
Expand All @@ -124,7 +124,7 @@ func Test_HttpContentLoader_LoadStream(t *testing.T) {
defer server.Close()

loader := &HttpContentLoader{}
c, err := loader.Load(NewFetchDefinition(server.URL))
c, err, _ := loader.Load(NewFetchDefinition(server.URL))
a.NoError(err)
a.NotNil(c.Reader())
body, err := ioutil.ReadAll(c.Reader())
Expand All @@ -143,14 +143,31 @@ func Test_HttpContentLoader_LoadStream_No_Composition_Header(t *testing.T) {
defer server.Close()

loader := &HttpContentLoader{}
c, err := loader.Load(NewFetchDefinition(server.URL))
c, err, _ := loader.Load(NewFetchDefinition(server.URL))
a.NoError(err)
a.NotNil(c.Reader())
body, err := ioutil.ReadAll(c.Reader())
a.NoError(err)
a.Equal("{}", string(body))
}

func Test_HttpContentLoader_Pass_404(t *testing.T) {
a := assert.New(t)

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte("{}"))
}))

defer server.Close()

loader := &HttpContentLoader{}
c, err, status := loader.Load(NewFetchDefinition(server.URL))
a.Error(err)
a.Nil(c)
a.Equal(404, status)
}

func Test_HttpContentLoader_LoadError500(t *testing.T) {
a := assert.New(t)

Expand All @@ -160,17 +177,18 @@ func Test_HttpContentLoader_LoadError500(t *testing.T) {
defer server.Close()

loader := &HttpContentLoader{}
c, err := loader.Load(NewFetchDefinition(server.URL))
c, err, statusCode := loader.Load(NewFetchDefinition(server.URL))
a.Error(err)
a.Nil(c)
a.Contains(err.Error(), "http 500")
assert.True(t, statusCode == 500)
}

func Test_HttpContentLoader_LoadErrorNetwork(t *testing.T) {
a := assert.New(t)

loader := &HttpContentLoader{}
c, err := loader.Load(NewFetchDefinition("..."))
c, err, _ := loader.Load(NewFetchDefinition("..."))
a.Error(err)
a.Nil(c)
a.Contains(err.Error(), "unsupported protocol scheme")
Expand Down
7 changes: 4 additions & 3 deletions composition/interface_mocks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package composition

import (
gomock "github.com/golang/mock/gomock"

io "io"
http "net/http"
)
Expand Down Expand Up @@ -62,11 +62,12 @@ func (_m *MockContentLoader) EXPECT() *_MockContentLoaderRecorder {
return _m.recorder
}

func (_m *MockContentLoader) Load(_param0 *FetchDefinition) (Content, error) {
func (_m *MockContentLoader) Load(_param0 *FetchDefinition) (Content, error, int) {
ret := _m.ctrl.Call(_m, "Load", _param0)
ret0, _ := ret[0].(Content)
ret1, _ := ret[1].(error)
return ret0, ret1
ret2, _ := ret[2].(int)
return ret0, ret1, ret2
}

func (_mr *_MockContentLoaderRecorder) Load(arg0 interface{}) *gomock.Call {
Expand Down
Loading