TheMediaGrid: Add params.keywords processing

[TheMediaGrid]

Update s2s TheMediaGrid adapter to read imp.ext.bidder.keywords and copy it in ext.keywords

[guscarreon]

Left some comments

[guscarreon]

In order to have less nested statements can we return if error instead?

    if err := json.Unmarshal(request.Ext, &reqExt); err != nil {
        return err
    }

[guscarreon]

Can we please add the keywords parameter in static/bidder-params/grid.json with its proper constraints if applicable? (required or not, minlength or none)

[guscarreon]

Can we refactor the reformatExtKeywords(extKeywords map[string]interface{}) function or subdivide it into subfunctions to make it easier to read?

[guscarreon]

if these couple of lines are of no use, can we remove?

[guscarreon]

Given that the type switch statements in lines 160 and 167 only seem to have one case statement, can we use a type assertion instead?

if v, ok = x.(T); ok {
    // ...
}

Or even the reflect library:

x := 42

xtype := reflect.TypeOf(x).Kind()

if xtype == reflect.Int {
    println("x is int")
}

[SyntaxNode]

I completely agree with the BidMeta changes to PBS-Core to solve the overwriting problem.

[SyntaxNode]

Please include BidMeta alphabetically in the comments and TypedBid object.

[SyntaxNode]

Please include bidMeta alphabetically in the comments and pbsOrtbBid object.

[guscarreon]

Is the error thrown in setImpExtKeywords not worth returning nil? Among the potential errors that setImpExtKeywords may throw are Unmarshal and Marshal errors. In those cases do we still not want to do:

		return nil, errors

?

[guscarreon]

We realize an error scenario here is unlikely but we believe it's better to try to check for it and handle it

[guscarreon]

We realize an error scenario here is unlikely but we believe it's better to try to check for it and handle it

[hhhjort]

Do you have any statistics on how many requests come in currently without a "uid" parameter? There was a design choice that any request that does not pass validation on any one of the bidder JSON validation schemas fails entirely, not just for the one bidder. This was done as when a publisher sets up a new configuration with multiple bidders, a bidder parameter failure on one bidder may be masked by all the successful bids from other bidders. We would rather the requests were correct from the start.

This means any current bidder configs that do not include uid will fail to fetch ads from any bidder. This will break publisher configs across all bidders, not just yours. The preferred way to add a new required field is to initially list it as optional in the schema, and have the adapter return an error if it is missing. An adapter returning an error outside of JSON schema validation will not invalidate the whole request. Then once we all have some confidence that all the publishers have updated their configs, put in a followup PR to change the schema to required.

[TheMediaGrid]

@guscarreon @SyntaxNode @hhhjort any chance you could take a look at the most recent changes that we've made as per Scott's comments? Any insight into why the auto tests are failing would be very useful as well.

[SyntaxNode]

@guscarreon @SyntaxNode @hhhjort any chance you could take a look at the most recent changes that we've made as per Scott's comments? Any insight into why the auto tests are failing would be very useful as well.

I'm looking into it. I didn't see this locally either. I suspect it's a case where the segments array order is indeterminate, but will confirm.

[SyntaxNode]

@guscarreon @SyntaxNode @hhhjort any chance you could take a look at the most recent changes that we've made as per Scott's comments? Any insight into why the auto tests are failing would be very useful as well.

I'm looking into it. I didn't see this locally either. I suspect it's a case where the segments array order is indeterminate, but will confirm.

Found it.

The issue is caused by the indeterminate ordering of the segments when constructed from an array of strings, referred to as the "compatible alternate segment format" in the code.

The ordering relies on the iteration order of keys within a map, which is purposefully inconsistent in Go. I don't believe the ordering matters to your bidding server, but it does matter to the json tests since arrays are always tested as order dependent. I'm not sure how easily or safely we can modify the test layer, but it easy to sort the keys to ensure deterministic order.

Change:

// compose compatible alternate segment format
for potentialSegmentName, potentialSegmentValues := range publisherItem {
	// values must be an array
	if valuesSlice, ok := potentialSegmentValues.([]interface{}); ok {
		for _, value := range valuesSlice {
			if valueAsString, ok := value.(string); ok {
				segments = append(segments, KeywordSegment{Name: potentialSegmentName, Value: valueAsString})
			}
		}
	}
}

To:

// ensure consistent ordering for publisher item map
publisherItemKeys := make([]string, 0, len(publisherItem))
for v := range publisherItem {
	publisherItemKeys = append(publisherItemKeys, v)
}
sort.Strings(publisherItemKeys)

// compose compatible alternate segment format
for _, potentialSegmentName := range publisherItemKeys {
	potentialSegmentValues := publisherItem[potentialSegmentName]
	// values must be an array
	if valuesSlice, ok := potentialSegmentValues.([]interface{}); ok {
		for _, value := range valuesSlice {
			if valueAsString, ok := value.(string); ok {
				segments = append(segments, KeywordSegment{Name: potentialSegmentName, Value: valueAsString})
			}
		}
	}
}

This will then consistently fail until the mixed test is updated to the following order:

"segments": [
    { "name": "segName1", "value": "segVal1" },
    { "name": "segName2", "value": "segVal2" },
    { "name": "bottom", "value": "bottomKey1" },
    { "name": "bottom", "value": "bottomKey2" },
    { "name": "segments", "value": "stringKey" }
]

After which the tests will consistently passes. I don't believe this is a new issue, but was missed earlier just by chance.

[TheMediaGrid]

@SyntaxNode @guscarreon @hhhjort @bsardo can anybody please review this PR? it's been a while

[bsardo]

Can you please add test coverage for this function?

[TheMediaGrid]

@bsardo we added the test as requested

[bsardo]

👍 looks good

[bsardo]

My personal preference here would be to see some of the invalid cases tested in their own small test files as this is pretty hard to digest. At the same time, I recognize this test adds value as written because it exercises just about all of the keyword logic which raises confidence that it works correctly together. I can be swayed either way. @guscarreon @SyntaxNode thoughts?

[guscarreon]

@TheMediaGrid : In order to double-check the expected behavior or your keywords algorithm could you please write 3 more tests where there are keywords:

• In the root.Ext only
• In the root.site only
• In the root.user only

I think your code looks pretty solid and with these three extra tests I'd be good to approve.

[guscarreon]

The code looks pretty solid. It'd be nice to add 3 more tests in a follow-up PR where there are keywords:

• In the root.Ext only
• In the root.site only
• In the root.user only