From ef0f8134c678e9cac707dac8f5293f80991d51db Mon Sep 17 00:00:00 2001 From: Daniel Zerlett Date: Mon, 11 Jul 2016 15:21:25 +0200 Subject: [PATCH 1/4] Composition handler returns error 500 on empty fetch list --- composition/composition_handler.go | 6 ++++++ composition/composition_handler_test.go | 5 +++++ composition/content_fetcher.go | 6 ++++++ composition/content_fetcher_test.go | 15 +++++++++++++++ composition/fetch_definition.go | 2 ++ composition/interfaces.go | 3 +++ 6 files changed, 37 insertions(+) diff --git a/composition/composition_handler.go b/composition/composition_handler.go index 9cb0dad..4aff627 100644 --- a/composition/composition_handler.go +++ b/composition/composition_handler.go @@ -32,6 +32,12 @@ func (agg *CompositionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) fetcher := agg.contentFetcherFactory(r) + if fetcher.Empty() { + w.WriteHeader(500) + w.Write([]byte("Internal server error")) + return + } + // fetch all contents results := fetcher.WaitForResults() diff --git a/composition/composition_handler_test.go b/composition/composition_handler_test.go index 081ecb0..541c0ff 100644 --- a/composition/composition_handler_test.go +++ b/composition/composition_handler_test.go @@ -265,3 +265,8 @@ func (m MockFetchResultSupplier) WaitForResults() []*FetchResult { func (m MockFetchResultSupplier) MetaJSON() map[string]interface{} { return nil } + +func (m MockFetchResultSupplier) Empty() bool { + return false +} + diff --git a/composition/content_fetcher.go b/composition/content_fetcher.go index 2029d88..025ecf0 100644 --- a/composition/content_fetcher.go +++ b/composition/content_fetcher.go @@ -108,6 +108,12 @@ func (fetcher *ContentFetcher) AddFetchJob(d *FetchDefinition) { }() } +func (fetcher *ContentFetcher) Empty() bool { + fetcher.r.mutex.Lock() + defer fetcher.r.mutex.Unlock() + return len(fetcher.r.results) == 0 +} + // isAlreadySheduled checks, if there is already a job for a FetchDefinition, or it is already fetched. // The method has to be called in a locked mutex block. func (fetcher *ContentFetcher) isAlreadySheduled(fetchDefinitionHash string) bool { diff --git a/composition/content_fetcher_test.go b/composition/content_fetcher_test.go index 84109f8..a7a92eb 100644 --- a/composition/content_fetcher_test.go +++ b/composition/content_fetcher_test.go @@ -34,6 +34,21 @@ func Test_ContentFetcher_FetchingWithDependency(t *testing.T) { meta := fetcher.MetaJSON() a.Equal("bar", meta["foo"]) a.Equal("bla", meta["bli"]) + + a.False(fetcher.Empty()) +} + +func Test_ContentFetcher_Empty(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + a := assert.New(t) + + loader := NewMockContentLoader(ctrl) + + fetcher := NewContentFetcher(nil) + fetcher.Loader = loader + + a.True(fetcher.Empty()) } func getFetchDefinitionMock(ctrl *gomock.Controller, loaderMock *MockContentLoader, url string, requiredContent []*FetchDefinition, loaderBlocking time.Duration, metaJSON map[string]interface{}) *FetchDefinition { diff --git a/composition/fetch_definition.go b/composition/fetch_definition.go index 422a29a..7ccd9e2 100644 --- a/composition/fetch_definition.go +++ b/composition/fetch_definition.go @@ -65,6 +65,8 @@ type FetchDefinition struct { RespProc ResponseProcessor ErrHandler ErrorHandler CacheStrategy CacheStrategy + + IsStaticError bool //ServeResponseHeaders bool //IsPrimary bool //FallbackURL string diff --git a/composition/interfaces.go b/composition/interfaces.go index e0a17e4..bdcb5ac 100644 --- a/composition/interfaces.go +++ b/composition/interfaces.go @@ -34,6 +34,9 @@ type FetchResultSupplier interface { // MetaJSON returns the composed meta JSON object MetaJSON() map[string]interface{} + + // True, if no fetch jobs were added + Empty() bool } type CacheStrategy interface { From d76cc0ed83317227770db0455607535aba3b5c10 Mon Sep 17 00:00:00 2001 From: Daniel Zerlett Date: Mon, 11 Jul 2016 15:38:42 +0200 Subject: [PATCH 2/4] removed obsolete var --- composition/fetch_definition.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/composition/fetch_definition.go b/composition/fetch_definition.go index 7ccd9e2..422a29a 100644 --- a/composition/fetch_definition.go +++ b/composition/fetch_definition.go @@ -65,8 +65,6 @@ type FetchDefinition struct { RespProc ResponseProcessor ErrHandler ErrorHandler CacheStrategy CacheStrategy - - IsStaticError bool //ServeResponseHeaders bool //IsPrimary bool //FallbackURL string From bd3628f48a05714868fe1a651616ba8b38b53ea3 Mon Sep 17 00:00:00 2001 From: Daniel Zerlett Date: Mon, 11 Jul 2016 15:48:22 +0200 Subject: [PATCH 3/4] Error log message added --- composition/composition_handler.go | 1 + 1 file changed, 1 insertion(+) diff --git a/composition/composition_handler.go b/composition/composition_handler.go index 4aff627..b03ba3a 100644 --- a/composition/composition_handler.go +++ b/composition/composition_handler.go @@ -35,6 +35,7 @@ func (agg *CompositionHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) if fetcher.Empty() { w.WriteHeader(500) w.Write([]byte("Internal server error")) + logging.Application(r.Header).Error("No fetchers available for composition, throwing error 500") return } From e38d6f6dd6aa32eb7254c465baa306442a2804a5 Mon Sep 17 00:00:00 2001 From: Daniel Zerlett Date: Mon, 11 Jul 2016 16:09:16 +0200 Subject: [PATCH 4/4] TOOM-200: test coverage --- composition/composition_handler_test.go | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/composition/composition_handler_test.go b/composition/composition_handler_test.go index 541c0ff..136190f 100644 --- a/composition/composition_handler_test.go +++ b/composition/composition_handler_test.go @@ -190,6 +190,24 @@ func Test_CompositionHandler_ErrorInFetching(t *testing.T) { a.Equal(502, resp.Code) } +func Test_CompositionHandler_ErrorEmptyFetchersList(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + a := assert.New(t) + + contentFetcherFactory := func(r *http.Request) FetchResultSupplier { + return MockFetchResultSupplier{} + } + aggregator := NewCompositionHandler(ContentFetcherFactory(contentFetcherFactory)) + + resp := httptest.NewRecorder() + r, _ := http.NewRequest("GET", "http://example.com", nil) + aggregator.ServeHTTP(resp, r) + + a.Equal("Internal server error", string(resp.Body.Bytes())) + a.Equal(500, resp.Code) +} + func Test_metadataForRequest(t *testing.T) { a := assert.New(t) r, _ := http.NewRequest("GET", "https://example.com/nothing?foo=bar", nil) @@ -267,6 +285,6 @@ func (m MockFetchResultSupplier) MetaJSON() map[string]interface{} { } func (m MockFetchResultSupplier) Empty() bool { - return false + return len([]*FetchResult(m)) == 0 }