Implemented review changes (pass-http-status-codes PR)

composition/composition_handler.go

@@ -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, res.HttpStatus, w, r)
+			res.Def.ErrHandler.Handle(res.Err, res.Content.HttpStatusCode(), w, r)
 			return
 		} else {
 			log.WithField("fetchResult", res).Warnf("optional content not loaded: %v", res.Def.URL)

composition/composition_handler_test.go

@@ -173,10 +173,9 @@ 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),
-				HttpStatus: 502,
+				Def:     NewFetchDefinition("/foo"),
+				Content: &MemoryContent{httpStatusCode: 502},
+				Err:     errors.New(errorString),
 			},
 		}
 	}

composition/content_fetcher.go

@@ -5,7 +5,6 @@ import (
 	"sync"
 
 	"github.com/tarent/lib-compose/logging"
-	"net/http"
 	"strings"
 )
 
@@ -16,11 +15,10 @@ func (def *FetchDefinition) IsFetchable() bool {
 }
 
 type FetchResult struct {
-	Def        *FetchDefinition
-	Err        error
-	Content    Content
-	HttpStatus int
-	Hash       string // the hash of the FetchDefinition
+	Def     *FetchDefinition
+	Err     error
+	Content Content
+	Hash    string // the hash of the FetchDefinition
 }
 
 // ContentFetcher is a type, which can fetch a set of Content pages in parallel.
@@ -77,7 +75,7 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) {
 
 	fetcher.activeJobs.Add(1)
 
-	fetchResult := &FetchResult{Def: d, Hash: hash, Err: errors.New("not fetched"), HttpStatus: http.StatusBadGateway}
+	fetchResult := &FetchResult{Def: d, Hash: hash, Err: errors.New("not fetched")}
 	fetcher.r.results = append(fetcher.r.results, fetchResult)
 
 	go func() {
@@ -96,7 +94,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, fetchResult.HttpStatus = fetcher.fetch(&definitionCopy)
+		fetchResult.Content, fetchResult.Err = fetcher.fetch(&definitionCopy)
 
 		if fetchResult.Err == nil {
 			fetcher.addMeta(fetchResult.Content.Meta())
@@ -114,7 +112,7 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) {
 	}()
 }
 
-func (fetcher *ContentFetcher) fetch(fd *FetchDefinition) (Content, error, int) {
+func (fetcher *ContentFetcher) fetch(fd *FetchDefinition) (Content, error) {
 	if strings.HasPrefix(fd.URL, FileURLPrefix) {
 		return fetcher.fileContentLoader.Load(fd)
 	}

composition/content_fetcher_test.go

@@ -68,9 +68,9 @@ func Test_ContentFetcher_FetchingWithDependency(t *testing.T) {
 	a := assert.New(t)
 
 	loader := NewMockContentLoader(ctrl)
-	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{}{})
+	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{}{})
 
 	fetcher := NewContentFetcher(nil)
 	fetcher.httpContentLoader = loader
@@ -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, requiredStatusCode int, loaderBlocking time.Duration, metaJSON map[string]interface{}) *FetchDefinition {
+func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoader, url string, requiredContent []*FetchDefinition, loaderBlocking time.Duration, metaJSON map[string]interface{}) *FetchDefinition {
 	fd := NewFetchDefinition(url)
 	fd.Timeout = time.Second * 42
 
@@ -110,7 +110,7 @@ func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoad
 			func(fetchDefinition *FetchDefinition) {
 				time.Sleep(loaderBlocking)
 			}).
-		Return(content, nil, requiredStatusCode)
+		Return(content, nil)
 
 	return fd
 }

composition/file_content_loader.go

@@ -21,14 +21,14 @@ func NewFileContentLoader() *FileContentLoader {
 	}
 }
 
-func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error, int) {
+func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error) {
 	if fd.RespProc != nil {
-		return nil, ResponseProcessorsNotApplicable, 502
+		return nil, ResponseProcessorsNotApplicable
 	}
 	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), 502
+		return nil, fmt.Errorf("error opening file %v: %v", fd.URL, err)
 	}
 
 	c := NewMemoryContent()
@@ -43,9 +43,9 @@ func (loader *FileContentLoader) Load(fd *FetchDefinition) (Content, error, int)
 			WithField("duration", time.Since(parsingStart)).
 			Debug("content parsing")
 		f.Close()
-		return c, err, c.httpStatusCode
+		return c, err
 	}
 
 	c.reader = f
-	return c, nil, c.httpStatusCode
+	return c, nil
 }

composition/file_content_loader_test.go

@@ -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())
@@ -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())
@@ -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)
 }
 
@@ -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)
 }
 

composition/http_content_loader.go

@@ -23,14 +23,17 @@ 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, int) {
+func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error) {
 	client := &http.Client{Timeout: fd.Timeout}
-	statusCode := 502
+
+	c := NewMemoryContent()
+	c.url = fd.URL
+	c.httpStatusCode = 502
 
 	var err error
 	request, err := http.NewRequest(fd.Method, fd.URL, fd.Body)
 	if err != nil {
-		return nil, err, 0
+		return c, err
 	}
 	request.Header = fd.Header
 	if request.Header == nil {
@@ -43,29 +46,26 @@ func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error, int)
 	resp, err := client.Do(request)
 
 	if resp != nil {
-		statusCode = resp.StatusCode
+		c.httpStatusCode = resp.StatusCode
 	}
 
 	logging.Call(request, resp, start, err)
 	if err != nil {
-		return nil, err, statusCode
+		return c, err
 	}
 
 	if fd.RespProc != nil {
 		err = fd.RespProc.Process(resp, fd.URL)
 	}
 	if err != nil {
-		return nil, err, statusCode
+		return c, err
 	}
 
-	if statusCode < 200 || statusCode > 399 {
-		return nil, fmt.Errorf("(http %v) on loading url %q", statusCode, fd.URL), statusCode
+	if c.httpStatusCode < 200 || c.httpStatusCode > 399 {
+		return c, fmt.Errorf("(http %v) on loading url %q", c.httpStatusCode, fd.URL)
 	}
 
-	c := NewMemoryContent()
-	c.url = fd.URL
 	c.httpHeader = resp.Header
-	c.httpStatusCode = statusCode
 
 	// take the first parser for the content type
 	// direct access to the map does not work, because the
@@ -86,11 +86,11 @@ func (loader *HttpContentLoader) Load(fd *FetchDefinition) (Content, error, int)
 					WithField("full_url", c.URL()).
 					WithField("duration", time.Since(parsingStart)).
 					Debug("content parsing")
-				return c, err, statusCode
+				return c, err
 			}
 		}
 	}
 
 	c.reader = resp.Body
-	return c, nil, statusCode
+	return c, nil
 }

composition/http_content_loader_test.go

@@ -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())
@@ -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())
@@ -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())
@@ -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())
@@ -143,7 +143,7 @@ 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())
@@ -162,10 +162,9 @@ func Test_HttpContentLoader_Pass_404(t *testing.T) {
 	defer server.Close()
 
 	loader := &HttpContentLoader{}
-	c, err, status := loader.Load(NewFetchDefinition(server.URL))
+	c, err := loader.Load(NewFetchDefinition(server.URL))
 	a.Error(err)
-	a.Nil(c)
-	a.Equal(404, status)
+	a.Equal(404, c.HttpStatusCode())
 }
 
 func Test_HttpContentLoader_LoadError500(t *testing.T) {
@@ -177,20 +176,18 @@ func Test_HttpContentLoader_LoadError500(t *testing.T) {
 	defer server.Close()
 
 	loader := &HttpContentLoader{}
-	c, err, statusCode := loader.Load(NewFetchDefinition(server.URL))
+	c, err := loader.Load(NewFetchDefinition(server.URL))
 	a.Error(err)
-	a.Nil(c)
 	a.Contains(err.Error(), "http 500")
-	assert.True(t, statusCode == 500)
+	assert.True(t, c.HttpStatusCode() == 500)
 }
 
 func Test_HttpContentLoader_LoadErrorNetwork(t *testing.T) {
 	a := assert.New(t)
 
 	loader := &HttpContentLoader{}
-	c, err, _ := loader.Load(NewFetchDefinition("..."))
+	_, err := loader.Load(NewFetchDefinition("..."))
 	a.Error(err)
-	a.Nil(c)
 	a.Contains(err.Error(), "unsupported protocol scheme")
 }
 

composition/interface_mocks_test.go

@@ -1,12 +1,12 @@
 // Automatically generated by MockGen. DO NOT EDIT!
-// Source: github.com/tarent/lib-compose/composition (interfaces: Fragment,ContentLoader,Content,ContentMerger,ContentParser,ResponseProcessor)
+// Source: lib-compose/composition (interfaces: Fragment,ContentLoader,Content,ContentMerger,ContentParser,ResponseProcessor)
 
 package composition
 
 import (
 	gomock "github.com/golang/mock/gomock"
-
 	io "io"
+
 	http "net/http"
 )
 
@@ -62,12 +62,11 @@ func (_m *MockContentLoader) EXPECT() *_MockContentLoaderRecorder {
 	return _m.recorder
 }
 
-func (_m *MockContentLoader) Load(_param0 *FetchDefinition) (Content, error, int) {
+func (_m *MockContentLoader) Load(_param0 *FetchDefinition) (Content, error) {
 	ret := _m.ctrl.Call(_m, "Load", _param0)
 	ret0, _ := ret[0].(Content)
 	ret1, _ := ret[1].(error)
-	ret2, _ := ret[2].(int)
-	return ret0, ret1, ret2
+	return ret0, ret1
 }
 
 func (_mr *_MockContentLoaderRecorder) Load(arg0 interface{}) *gomock.Call {

composition/interfaces.go

@@ -15,7 +15,7 @@ type Fragment interface {
 type ContentLoader interface {
 	// Load synchronously loads a content.
 	// The loader has to ensure to return the call withing the supplied timeout.
-	Load(fd *FetchDefinition) (content Content, err error, status int)
+	Load(fd *FetchDefinition) (content Content, err error)
 }
 
 type ContentParser interface {