Added integration type support for host company #2082

AlexBVolcy · 2021-11-15T01:53:46Z

This PR partially addresses the Integration Type and Channel issue #1428

There will be a series of smaller PRs to address this issue, another PR is currently out for adding channel support here #2037

This PR is the first one dedicated towards the Integration part of the issue. What this PR does is it adds the functions that allow us to get the integration type (getIntegrationTypeFromRequest()) value found in a openRTB request, and to validate that the integration value in the request is allowed by the host company (validateIntegrationType()).

Future integration type focused PRs will add support for publishers to define a defaultIntegration value, and if that value can be overridden.

bsardo · 2021-11-17T17:08:44Z

config/config.go

@@ -84,6 +84,9 @@ type Configuration struct {
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
 	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	// IntegrationTypes will hold the integration type values that the host will allow.
+	// Map structure isn't included here because the integration type feature is still being worked on, but once complete, this variable will be updated
+	IntegrationTypes map[string]bool


If I understand correctly, this config variable will only contain the integration type values that are allowed by the host, is that correct?
If so, in that case, map[string]struct{} might make more sense since the map values are insignificant and the empty struct is a typical go construct used for this purpose. It does not appear as though the Java version has this configuration setting but we could still add it so we can detect invalid values and notify the publisher.

I also understand that a future PR will handle populating this map from what I imagine will be a slice called IntegrationTypes on the Configuration object so you might want to call this one you've defined here IntegrationTypesMap instead.

Thank you for this comment, I'll update this maps value accordingly!

bsardo · 2021-11-17T17:14:54Z

openrtb_ext/request.go

@@ -33,6 +33,7 @@ type ExtRequestPrebid struct {
 	Data                 *ExtRequestPrebidData     `json:"data,omitempty"`
 	Debug                bool                      `json:"debug,omitempty"`
 	Events               json.RawMessage           `json:"events,omitempty"`
+	IntegrationType      string                    `json:"integration,omitempty"`


I think we might want to call this Integration instead. Our struct field names typically match the json.

Will update!

bsardo · 2021-11-17T17:16:29Z

openrtb_ext/request.go

@@ -33,6 +33,7 @@ type ExtRequestPrebid struct {
 	Data                 *ExtRequestPrebidData     `json:"data,omitempty"`
 	Debug                bool                      `json:"debug,omitempty"`
 	Events               json.RawMessage           `json:"events,omitempty"`
+	IntegrationType      string                    `json:"integration,omitempty"`


I assume you'll also convert this from a string to some integration-specific custom string type in a future PR, is that right?

Actually maybe I have that wrong. Can the integration values be whatever the host wants? In that case string makes sense.

Yeah the integration values allowed are set by the host to be whatever they want!

bsardo · 2021-11-17T17:56:14Z

endpoints/openrtb2/auction.go

+		return "", err
+	}
+	reqPrebid := reqExt.GetPrebid()
+	return reqPrebid.IntegrationType, nil


reqPrebid could be nil so you'll want to check for that before you try to access .IntegrationType.

Will update!

bsardo · 2021-11-17T18:12:56Z

endpoints/openrtb2/auction.go

+	if _, ok := integrationTypes[integrationType]; ok {
+		return nil
+	}


If the integration has not been specified in the request, I believe that's valid according to the corresponding issue, is that correct? It looks like the integration would be set to some default value specified in the config in this case. If I have that right, you'll want to handle that case here as I don't imagine specifying an empty string or nil value in the slice of valid integration values in the config.

You're right in bringing up this point! However, I'll handle this Default Integration in a future PR, since we're breaking up my implementation of Integration Type and Channel into multiple smaller PRs.

bsardo · 2021-11-17T18:13:41Z

endpoints/openrtb2/auction.go

+	return reqPrebid.IntegrationType, nil
+}
+
+func validateIntegrationType(req *openrtb_ext.RequestWrapper, integrationTypes map[string]bool) error {


Are you planning to add test coverage for these functions in this PR or a future related PR?

Yeah I'll add more test coverage in this PR for the getIntegrationTypeFromRequest function, the validateIntegrationType function has a test that I wrote already in this PR!

hhhjort · 2021-11-23T19:29:03Z

config/config.go

@@ -84,6 +84,9 @@ type Configuration struct {
 	GenerateBidID bool `mapstructure:"generate_bid_id"`
 	// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
 	GenerateRequestID bool `mapstructure:"generate_request_id"`
+	// IntegrationTypesMap will hold the integration type values that the host will allow.
+	// Map structure isn't included here because the integration type feature is still being worked on, but once complete, this variable will be updated
+	IntegrationTypesMap map[string]struct{}


Two questions. First is how do you plan on populating IntegrationTypesMap? For the above Blacklist maps, we have two entries, one a slice of strings, and the second is the actual map. In your configuration you would provide the list of values, and then config code builds the map from the list.

The second question is how do we default to integration types not being required? We don't want to break every install of PBS that is not currently requiring integration types to be sent in requests.

To your first question, I like the idea of copying how the Blacklist maps work, so I could have one variable IntegrationTypes that is just a slice of strings indicating what IntegrationTypes the host company allows, and this slice can be filled with another SetDefault of that SetupViper function. And then that slice can be apart of the IntegrationTypesMap. And then we can continue to utilize the fast lookup the map allows us.

To your second question, integration types are not being required in this PR since we never actually call any of the validate/get functions set up for integration types, so the flow of current PBS instances won't be broken. But in the future, we could create a simple boolean (i.e. AllowIntegrationTypes), that if false, avoids any evaluation of integration type information. Or host company's could choose to not populate the IntegrationTypes slice, and then we could add a check that makes it if IntegrationTypes slice is empty, we just continue evaluating the request ignoring IntegrationType related validation.

Oh, yes, you don't actually call these new functions. Seems like a small enough addition, was there a reason that the actual activation was left to another PR?

Since we still have to implement the integration type features for accounts (i.e. Default Integration), we can leave the activation for that PR!

hhhjort

Looks good, even if it just adds code but doesn't activate it. Will need a followup PR to activate this functionality, but will go ahead and set my approval now.

AlexBVolcy added 2 commits November 14, 2021 17:35

Get/validate integration type, allow host config

545a4e2

Minor update to integration tests

406567c

mansinahar requested review from VeronikaSolovei9 and bsardo November 15, 2021 18:34

mansinahar assigned bsardo and VeronikaSolovei9 Nov 15, 2021

bsardo reviewed Nov 17, 2021

View reviewed changes

Address minor comments, increased test coverage to get integration func

2066cdc

mansinahar requested review from hhhjort and removed request for VeronikaSolovei9 November 23, 2021 18:11

mansinahar assigned hhhjort and unassigned VeronikaSolovei9 Nov 23, 2021

hhhjort reviewed Nov 23, 2021

View reviewed changes

mansinahar requested review from mansinahar and bsardo December 1, 2021 18:10

mansinahar assigned mansinahar and unassigned bsardo Dec 1, 2021

hhhjort approved these changes Dec 2, 2021

View reviewed changes

mansinahar closed this Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added integration type support for host company #2082

Added integration type support for host company #2082

AlexBVolcy commented Nov 15, 2021 •

edited

Loading

bsardo Nov 17, 2021

AlexBVolcy Nov 22, 2021

bsardo Nov 17, 2021

AlexBVolcy Nov 22, 2021

bsardo Nov 17, 2021

bsardo Nov 17, 2021 •

edited

Loading

AlexBVolcy Nov 22, 2021 •

edited

Loading

bsardo Nov 17, 2021

AlexBVolcy Nov 22, 2021

bsardo Nov 17, 2021

AlexBVolcy Nov 22, 2021

bsardo Nov 17, 2021

AlexBVolcy Nov 22, 2021 •

edited

Loading

hhhjort Nov 23, 2021

AlexBVolcy Nov 30, 2021 •

edited

Loading

hhhjort Nov 30, 2021

AlexBVolcy Dec 2, 2021

hhhjort left a comment

Added integration type support for host company #2082

Added integration type support for host company #2082

Conversation

AlexBVolcy commented Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

AlexBVolcy Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhhjort left a comment

Choose a reason for hiding this comment

AlexBVolcy commented Nov 15, 2021 •

edited

Loading

bsardo Nov 17, 2021 •

edited

Loading

AlexBVolcy Nov 22, 2021 •

edited

Loading

AlexBVolcy Nov 22, 2021 •

edited

Loading

AlexBVolcy Nov 30, 2021 •

edited

Loading