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.

Sign up for GitHub

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

Implementation of Categories Http fetcher #882

Merged
merged 7 commits into from
May 20, 2019
Merged

Implementation of Categories Http fetcher #882

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b735807
Implementation of Categories Http fetcher
Apr 19, 2019
7bf0358
Moved common Category struct to stored_request
Apr 19, 2019
e617bd2
Added comments
Apr 19, 2019
be41c4b
Minor refactoring
May 3, 2019
925cc9f
Minor refactoring
May 14, 2019
2528a08
Minor refactoring
May 16, 2019
20d6f7f
Minor refactoring
May 17, 2019
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
1 change: 1 addition & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("datacache.ttl_seconds", 0)
v.SetDefault("category_mapping.filesystem.enabled", true)
v.SetDefault("category_mapping.filesystem.directorypath", "./static/category-mapping")
v.SetDefault("category_mapping.http.endpoint", "")
v.SetDefault("stored_requests.filesystem", false)
v.SetDefault("stored_requests.directorypath", "./stored_requests/data/by_id")
v.SetDefault("stored_requests.postgres.connection.dbname", "")
Expand Down
6 changes: 3 additions & 3 deletions exchange/exchange.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
conversions := e.currencyConverter.Rates()

adapterBids, adapterExtra := e.getAllBids(auctionCtx, cleanRequests, aliases, bidAdjustmentFactors, blabels, conversions)
bidCategory, adapterBids, err := applyCategoryMapping(requestExt, adapterBids, *categoriesFetcher, targData)
bidCategory, adapterBids, err := applyCategoryMapping(ctx, requestExt, adapterBids, *categoriesFetcher, targData)
auc := newAuction(adapterBids, len(bidRequest.Imp))
if err != nil {
return nil, fmt.Errorf("Error in category mapping : %s", err.Error())
Expand Down Expand Up @@ -321,7 +321,7 @@ func (e *exchange) buildBidResponse(ctx context.Context, liveAdapters []openrtb_
return bidResponse, err
}

func applyCategoryMapping(requestExt openrtb_ext.ExtRequest, seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, categoriesFetcher stored_requests.CategoryFetcher, targData *targetData) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, error) {
func applyCategoryMapping(ctx context.Context, requestExt openrtb_ext.ExtRequest, seatBids map[openrtb_ext.BidderName]*pbsOrtbSeatBid, categoriesFetcher stored_requests.CategoryFetcher, targData *targetData) (map[string]string, map[openrtb_ext.BidderName]*pbsOrtbSeatBid, error) {
res := make(map[string]string)

type bidDedupe struct {
Expand Down Expand Up @@ -375,7 +375,7 @@ func applyCategoryMapping(requestExt openrtb_ext.ExtRequest, seatBids map[openrt
continue
} else {
//if unique IAB category is present then translate it to the adserver category based on mapping file
category, err = categoriesFetcher.FetchCategories(primaryAdServer, publisher, bidIabCat[0])
category, err = categoriesFetcher.FetchCategories(ctx, primaryAdServer, publisher, bidIabCat[0])
if err != nil || category == "" {
//TODO: add metrics
//if mapping required but no mapping file is found then discard the bid
Expand Down
4 changes: 2 additions & 2 deletions exchange/exchange_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ func TestCategoryMapping(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, err := applyCategoryMapping(requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, err := applyCategoryMapping(nil, requestExt, adapterBids, categoriesFetcher, targData)

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, "10.00_Electronics_30s", bidCategory["bid_id1"], "Category mapping doesn't match")
Expand Down Expand Up @@ -652,7 +652,7 @@ func TestCategoryDedupe(t *testing.T) {

adapterBids[bidderName1] = &seatBid

bidCategory, adapterBids, err := applyCategoryMapping(requestExt, adapterBids, categoriesFetcher, targData)
bidCategory, adapterBids, err := applyCategoryMapping(nil, requestExt, adapterBids, categoriesFetcher, targData)

assert.Equal(t, nil, err, "Category mapping error should be empty")
assert.Equal(t, 2, len(adapterBids[bidderName1].bids), "Bidders number doesn't match")
Expand Down
2 changes: 1 addition & 1 deletion stored_requests/backends/db_fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func (fetcher *dbFetcher) FetchRequests(ctx context.Context, requestIDs []string
return storedRequestData, storedImpData, errs
}

func (fetcher *dbFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (fetcher *dbFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
return "", nil
}

Expand Down
2 changes: 1 addition & 1 deletion stored_requests/backends/empty_fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@ func (fetcher EmptyFetcher) FetchRequests(ctx context.Context, requestIDs []stri
return
}

func (fetcher EmptyFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (fetcher EmptyFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
return "", nil
}
13 changes: 4 additions & 9 deletions stored_requests/backends/file_fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,7 @@ func NewFileFetcher(directory string) (stored_requests.AllFetcher, error) {

type eagerFetcher struct {
FileSystem FileSystem
Categories map[string]map[string]Category
}

type Category struct {
Id string
Name string
Categories map[string]map[string]stored_requests.Category
}

func (fetcher *eagerFetcher) FetchRequests(ctx context.Context, requestIDs []string, impIDs []string) (map[string]json.RawMessage, map[string]json.RawMessage, []error) {
Expand All @@ -38,15 +33,15 @@ func (fetcher *eagerFetcher) FetchRequests(ctx context.Context, requestIDs []str
return storedRequests, storedImpressions, errs
}

func (fetcher *eagerFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (fetcher *eagerFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
fileName := primaryAdServer

if len(publisherId) != 0 {
fileName = primaryAdServer + "_" + publisherId
}

if fetcher.Categories == nil {
fetcher.Categories = make(map[string]map[string]Category)
fetcher.Categories = make(map[string]map[string]stored_requests.Category)
}
if data, ok := fetcher.Categories[fileName]; ok {
return data[iabCategory].Id, nil
Expand All @@ -56,7 +51,7 @@ func (fetcher *eagerFetcher) FetchCategories(primaryAdServer, publisherId, iabCa

if file, ok := primaryAdServerDir.Files[fileName]; ok {

tmp := make(map[string]Category)
tmp := make(map[string]stored_requests.Category)

if err := json.Unmarshal(file, &tmp); err != nil {
return "", fmt.Errorf("Unable to unmarshal categories for adserver: '%s', publisherId: '%s'", primaryAdServer, publisherId)
Expand Down
10 changes: 5 additions & 5 deletions stored_requests/backends/file_fetcher/fetcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func TestCategoriesFetcherWithPublisher(t *testing.T) {
if err != nil {
t.Errorf("Failed to create a category Fetcher: %v", err)
}
category, err := fetcher.FetchCategories("test", "categories", "IAB1-1")
category, err := fetcher.FetchCategories(nil, "test", "categories", "IAB1-1")
assert.Equal(t, nil, err, "Categories were loaded incorrectly")
assert.Equal(t, "Beverages", category, "Categories were loaded incorrectly")
}
Expand All @@ -124,7 +124,7 @@ func TestCategoriesFetcherWithoutPublisher(t *testing.T) {
if err != nil {
t.Errorf("Failed to create a category Fetcher: %v", err)
}
category, err := fetcher.FetchCategories("test", "", "IAB1-1")
category, err := fetcher.FetchCategories(nil, "test", "", "IAB1-1")
assert.Equal(t, nil, err, "Categories were loaded incorrectly")
assert.Equal(t, "VideoGames", category, "Categories were loaded incorrectly")
}
Expand All @@ -134,7 +134,7 @@ func TestCategoriesFetcherNoCategory(t *testing.T) {
if err != nil {
t.Errorf("Failed to create a category Fetcher: %v", err)
}
_, fetchingErr := fetcher.FetchCategories("test", "", "IAB1-100")
_, fetchingErr := fetcher.FetchCategories(nil, "test", "", "IAB1-100")
assert.Equal(t, fmt.Errorf("Unable to find category for adserver 'test', publisherId: '', iab category: 'IAB1-100'"),
fetchingErr, "Categories were loaded incorrectly")
}
Expand All @@ -144,7 +144,7 @@ func TestCategoriesFetcherBrokenJson(t *testing.T) {
if err != nil {
t.Errorf("Failed to create a category Fetcher: %v", err)
}
_, fetchingErr := fetcher.FetchCategories("test", "broken", "IAB1-100")
_, fetchingErr := fetcher.FetchCategories(nil, "test", "broken", "IAB1-100")
assert.Equal(t, fmt.Errorf("Unable to unmarshal categories for adserver: 'test', publisherId: 'broken'"),
fetchingErr, "Categories were loaded incorrectly")
}
Expand All @@ -154,7 +154,7 @@ func TestCategoriesFetcherNoCategoriesFile(t *testing.T) {
if err != nil {
t.Errorf("Failed to create a category Fetcher: %v", err)
}
_, fetchingErr := fetcher.FetchCategories("test", "not_exists", "IAB1-100")
_, fetchingErr := fetcher.FetchCategories(nil, "test", "not_exists", "IAB1-100")
assert.Equal(t, fmt.Errorf("Unable to find mapping file for adserver: 'test', publisherId: 'not_exists'"),
fetchingErr, "Categories were loaded incorrectly")
}
48 changes: 43 additions & 5 deletions stored_requests/backends/http_fetcher/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ func NewFetcher(client *http.Client, endpoint string) *HttpFetcher {
}

type HttpFetcher struct {
client *http.Client
Endpoint string
hasQuery bool
client *http.Client
Endpoint string
hasQuery bool
Categories map[string]map[string]stored_requests.Category
}

func (fetcher *HttpFetcher) FetchRequests(ctx context.Context, requestIDs []string, impIDs []string) (requestData map[string]json.RawMessage, impData map[string]json.RawMessage, errs []error) {
Expand All @@ -80,8 +81,45 @@ func (fetcher *HttpFetcher) FetchRequests(ctx context.Context, requestIDs []stri
return
}

func (fetcher *HttpFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
return "", nil
func (fetcher *HttpFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
if fetcher.Categories == nil {
fetcher.Categories = make(map[string]map[string]stored_requests.Category)
}

if publisherId == "" {
publisherId = primaryAdServer
Copy link
Collaborator

Choose a reason for hiding this comment

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

This means we could have something like freewheel_freewheel as the dataName below. Is this correct, or should it resolve to just freewheel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, fixed!

}

dataName := fmt.Sprintf("%s_%s", primaryAdServer, publisherId)
if data, ok := fetcher.Categories[dataName]; ok {
return data[iabCategory].Id, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Overall the PR LGTM. I just have one small question though: Are we sure that the data will always have a value for iabCategory? Should we rather just do a small check before we return? Both here and towards the end of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mansinahar yes, it's good to have this check. Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Kalypsonika !

}
//in NewFetcher function there is a code to add "?" at the end of url
//in case of categories we don't expect to have any parameters, that's why we need to remove "?"
url := fmt.Sprintf("%s/%s/%s.json", strings.Replace(fetcher.Endpoint, "?", "", -1), primaryAdServer, publisherId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably just removing the ? isn't going to work if the fetcher.Endpoint contains query params in the URL, we should just have this function throw an appropriate error if the endpoint has a ? here (instead of trying to handle that case).

Copy link
Contributor Author

@VeronikaSolovei9 VeronikaSolovei9 Apr 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I made it because in NewFetcher function contains this code:

func NewFetcher(client *http.Client, endpoint string) *HttpFetcher {
	// Do some work up-front to figure out if the (configurable) endpoint has a query string or not.
	// When we build requests, we'll either want to add `?request-ids=...&imp-ids=...` _or_
	// `&request-ids=...&imp-ids=...`, depending.
	urlPrefix := endpoint
	if strings.Contains(endpoint, "?") {
		urlPrefix = urlPrefix + "&"
	} else {
		urlPrefix = urlPrefix + "?"
	}

	...

	return &HttpFetcher{
		client:   client,
		Endpoint: urlPrefix,
	}
}

because of this there is always ? at the end of url.
In my understanding in case of Categories fetcher we expect to have url without parameters, so I have to handle this explicitly.
Do you think I need to refactor existing functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ok I see why that's there then. I guess the difference is category fetcher is expecting it's variable parts to be in the URL path where as other fetchers are passing them in URL query params. Maybe this is fine then, but please add a comment to the effect that you are stripping away the question mark that gets added in NewFetcher code. Also we're assuming endpoints used for category mappings aren't going to contain query params, which maybe is ok since they'll probably just be statically hosted files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added.


httpReq, err := http.NewRequest("GET", url, nil)
if err != nil {
return "", err
}

httpResp, err := ctxhttp.Do(ctx, fetcher.client, httpReq)
if err != nil {
return "", err
}
defer httpResp.Body.Close()

respBytes, err := ioutil.ReadAll(httpResp.Body)
tmp := make(map[string]stored_requests.Category)

if err := json.Unmarshal(respBytes, &tmp); err != nil {
return "", fmt.Errorf("Unable to unmarshal categories for adserver: '%s', publisherId: '%s'", primaryAdServer, publisherId)
}
fetcher.Categories[dataName] = tmp

resultCategory := tmp[iabCategory].Id

return resultCategory, nil
}

func buildRequest(endpoint string, requestIDs []string, impIDs []string) (*http.Request, error) {
Expand Down
11 changes: 8 additions & 3 deletions stored_requests/fetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ type Fetcher interface {

type CategoryFetcher interface {
// FetchCategories fetches the ad-server/publisher specific category for the given IAB category
FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error)
FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error)
}

// AllFetcher is an iterface that encapsulates both the original Fetcher and the CategoryFetcher
type AllFetcher interface {
FetchRequests(ctx context.Context, requestIDs []string, impIDs []string) (requestData map[string]json.RawMessage, impData map[string]json.RawMessage, errs []error)
FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error)
FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error)
}

// NotFoundError is an error type to flag that an ID was not found by the Fetcher.
Expand All @@ -44,6 +44,11 @@ type NotFoundError struct {
DataType string
}

type Category struct {
Id string
Name string
}

func (e NotFoundError) Error() string {
return fmt.Sprintf(`Stored %s with ID="%s" not found.`, e.DataType, e.ID)
}
Expand Down Expand Up @@ -176,7 +181,7 @@ func (f *fetcherWithCache) FetchRequests(ctx context.Context, requestIDs []strin
return
}

func (f *fetcherWithCache) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (f *fetcherWithCache) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
return "", nil
}

Expand Down
2 changes: 1 addition & 1 deletion stored_requests/fetcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func (f *mockFetcher) FetchRequests(ctx context.Context, requestIDs []string, im
return args.Get(0).(map[string]json.RawMessage), args.Get(1).(map[string]json.RawMessage), args.Get(2).([]error)
}

func (f *mockFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (f *mockFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
return "", nil
}

Expand Down
4 changes: 2 additions & 2 deletions stored_requests/multifetcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ func (mf MultiFetcher) FetchRequests(ctx context.Context, requestIDs []string, i
return
}

func (mf MultiFetcher) FetchCategories(primaryAdServer, publisherId, iabCategory string) (string, error) {
func (mf MultiFetcher) FetchCategories(ctx context.Context, primaryAdServer, publisherId, iabCategory string) (string, error) {
for _, f := range mf {
if cf, ok := f.(CategoryFetcher); ok {
iabCategory, _ := cf.FetchCategories(primaryAdServer, publisherId, iabCategory)
iabCategory, _ := cf.FetchCategories(ctx, primaryAdServer, publisherId, iabCategory)
if iabCategory != "" {
return iabCategory, nil
}
Expand Down