New Adapter: SmartHub #1932

SmartHubSolutions · 2021-07-19T15:27:36Z

No description provided.

SmartHubSolutions · 2021-07-23T12:36:19Z

@VeronikaSolovei9 @AlexBVolcy @SyntaxNode

Hello, guys! :) Could you please confirm that we meet requirements to start a review process? Maybe we've done something wrong and need to fix this or add something else?

There is our PR with docs: prebid/prebid.github.io#3131

mansinahar · 2021-07-27T15:29:34Z

adapters/smarthub/smarthub.go

+	ADAPTER_VER = "1.0.0"
+)
+
+type SmartHubAdapter struct {


Please rename this to adapter. There's no need to repeat the package name in the adapter type. Please review our new adapter docs: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html

Thank you, I've fixed this. Do other things look correct?

VeronikaSolovei9

Hello @SmartHubSolutions, I apologize for the delay in review.
I added some comments, other than that it looks good to me.
Thank you for adding documentation PR!

VeronikaSolovei9 · 2021-07-28T09:42:03Z

adapters/smarthub/smarthub.go

+	reqInfo *adapters.ExtraRequestInfo,
+) (
+	requestsToBidder []*adapters.RequestData,
+	errs []error,


What is the reason behind naming output parameters? In MakeBids function as well.

There is no reason. I've removed in both

VeronikaSolovei9 · 2021-07-28T09:48:05Z

adapters/smarthub/smarthub.go

+		}}
+	}
+
+	bid := bids[0]


Is it always only one bid in response?

Yes, we don't support more than one bid in response

VeronikaSolovei9 · 2021-07-28T09:54:05Z

usersync/usersyncers/syncer.go

@@ -71,9 +71,10 @@ import (
 	"github.com/prebid/prebid-server/adapters/rhythmone"
 	"github.com/prebid/prebid-server/adapters/rtbhouse"
 	"github.com/prebid/prebid-server/adapters/rubicon"
-	"github.com/prebid/prebid-server/adapters/sa_lunamedia"
+	salunamedia "github.com/prebid/prebid-server/adapters/sa_lunamedia"


Can you please don't add aliases for other bidders? There shouldn't be any name conflicts.

Yes, of course. I've removed them

guscarreon · 2021-07-28T16:39:41Z

adapters/smarthub/smarthub.go

+	errs []error,
+) {
+	if len(openRTBRequest.Imp) == 0 {
+		return nil, []error{&errortypes.BadInput{Message: "Missing Imp object"}}


Nitpick: Prebid server core filters out requests with empty []Imp arrays so there's no need for this check. I realize this isn't a lot overhead so you could leave it or remove it if you want.

Removed this

guscarreon · 2021-07-28T16:55:23Z

adapters/smarthub/smarthub.go

+	endpointParams := macros.EndpointTemplateParams{
+		Host:      params.Host,
+		AccountID: params.Seat,
+		SourceId:  params.Token,


I see you pass all three parameters to your URL. Do you need all three to NOT be empty? As of right now, you marked them as required inside static/bidder-params/smarthub.json but they can still be zero-lenght strings. In case you need that any or all three of your parameters to not be empty you could add the minLength property like:

{ "$schema": "http://json-schema.org/draft-04/schema#", "title": "SmartHub Adapter Params", "description": "A schema which validates params accepted by the SmartHub adapter", "type": "object", "properties": { "host": { "type": "string", "description": "Network host to send request" + "minLength": 1 }, "seat": { "type": "string", "description": "Seat" + "minLength": 1 }, "token": { "type": "string", "description": "Token" + "minLength": 1 } }, "required": [ "host", "seat", "token" ] } static/bidder-params/smarthub.json

Without the additions shown above, an all empty parameter req.Imp[i].Ext would be allowed. In other words, for the moment, a test case like this is currently passing:

var invalidParams = []string{ ``, `null`, `true`, `5`, `[]`, `{}`, `{"anyparam": "anyvalue"}`, `{"host":"smarthub.test"}`, `{"seat":"9Q20EdGxzgWdfPYShScl"}`, `{"token":"Y9Evrh40ejsrCR4EtidUt1cSxhJsz8X1"}`, `{"seat":"9Q20EdGxzgWdfPYShScl", "token":"alNYtemWggraDVbhJrsOs9pXc3Eld32E"}`, `{"host":"smarthub.test", "token":"LNywdP2ebX5iETF8gvBeEoB6Cam64eeq"}`, `{"host":"smarthub.test", "seat":"9Q20EdGxzgWdfPYShScl"}`, + `{"host":"", "seat":"", "token":""}`, // <-- all empty fields test case is passing right now } adapters/smarthub/params_test.go

If that's what you intended, it's fine

Yes, thanks a lot for pointing the mistake! All three parameters mustn't be an empty string. I've fixed this in static/bidder-params/smarthub.json and also added possible cases to test into invalidParams

…y string

VeronikaSolovei9

Thank you for addressing comments. LGTM.

guscarreon · 2021-07-29T15:39:36Z

config/config.go

@@ -946,6 +947,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("adapters.silvermob.endpoint", "http://{{.Host}}.silvermob.com/marketplace/api/dsp/bid/{{.ZoneID}}")
 	v.SetDefault("adapters.smaato.endpoint", "https://prebid.ad.smaato.net/oapi/prebid")
 	v.SetDefault("adapters.smartadserver.endpoint", "https://ssb-global.smartadserver.com")
+	v.SetDefault("adapters.smarthub.endpoint", "http://{{.Host}}/?seat={{.AccountID}}&token={{.SourceId}}")


Hello @SmartHubSolutions . Despite being a proxy server, our policy is that Prebid server can pass subdomains but not whole domains. Can you please modify?
Allowed:

952 v.SetDefault("adapters.smarthub.endpoint", "http://{{.Host}}.restofsmarthubdomain.com/?seat={{.AccountID}}&token={{.SourceId}}") config/config.go

Not-allowed:

952 v.SetDefault("adapters.smarthub.endpoint", "http://{{.Host}}/?seat={{.AccountID}}&token={{.SourceId}}") config/config.go

Unfortunately, impossible for us. Replacing of all host is the only one way how we work with our partners. We need to leave this how it is. BTW, there are a lot of adapters with the same pattern. Several examples:

858 v.SetDefault("adapters.adkernel.endpoint", "http://{{.Host}}/hb?zone={{.ZoneID}}") config/config.go

link

918 v.SetDefault("adapters.invibes.endpoint", "https://{{.Host}}/bid/ServerBidAdContent") config/config.go

link

978 v.SetDefault("adapters.zeroclickfraud.endpoint", "http://{{.Host}}/openrtb2?sid={{.SourceId}}") config/config.go

link

@SmartHubSolutions We're aware that there are a few other adapters that use this approach for the hosts but it's an anti-pattern that we as a Prebid Server committee strongly oppose and are currently working with these adapters to correct their behavior. We're focused on making sure that this pattern isn't used for any other adapters going further. It's odd to ask the publisher to specify the server address of the bidder, it greatly harms Prebid Server's performance which is based on keep alive connections to bidders, and allowing the publisher to pass the host through opens up PBS to a variety of attacks.

Can you please help us understand your use case for this? We'd be happy to work with you to find a better solution for your use case, if possible.

In case the use case here is to have the Publishers specify the host based on the region they are in then the recommended approach for that is to use a default endpoint pointing to your company's global load balancer. If you don't have a global load balancer, consider marking the adapter as disabled by default and add instructions in your bidder docs to let hosts know which endpoints to use for their geo regions, or ask them to contact you directly to setup if you don't want to publicly post those endpoints.

@mansinahar Thank you for this detailed explanation! Now, everything is clear. I'll discuss this with my team to find a solution. We'll definitely find it and back soon.

@mansinahar I've written below how we had changed URL template.Does it meet requirements now? :) Can we continue our review process?

guscarreon · 2021-07-29T17:09:57Z

static/bidder-info/smarthub.yaml

@@ -0,0 +1,13 @@
+maintainer:
+  email: "[email protected]"


Just double-checking: are you registered with IAB Europe? If you don't, that's fine. If you do, IAB's GVL id number must be written into this file as shown below. Just double-checking in case you are.

1 maintainer: 2 email: "[email protected]" + gvlVendorID: <IABs_GVL_ID> 3 capabilities: 4 app: 5 mediaTypes: 6 - banner 7 - video 8 - native 9 site: 10 mediaTypes: 11 - banner 12 - video 13 - native static/bidder-info/smarthub.yaml

SmartHub is the solution for different companies which every has own registration and Vendor ID. Every company which receive traffic will process European data by itself. If a company doesn't have Vendor ID, SmartHub clean all data due to IAB recommendations. SmartHub completely support GDPR, but I think it's incorrect to write our Vendor ID here

SmartHubSolutions · 2021-08-03T10:28:25Z

@guscarreon @mansinahar Hi!
We've fixed the way how to build endpoint URL according your recommendations. Thank you! Now our URL template looks like:

"http://{{.Host}}-prebid.smart-hub.io/?seat={{.AccountID}}&token={{.SourceId}}"

Also, we've updated docs PR: prebid/prebid.github.io#3131
Could we continue the review process?

VeronikaSolovei9

LGTM

guscarreon

@SmartHubSolutions thank you for addressing our feedback. Your code looks pretty good.

adding SmartHub adapter

2573576

SmartHubSolutions mentioned this pull request Jul 19, 2021

SmartHub docs prebid/prebid.github.io#3131

Merged

SyntaxNode requested review from AlexBVolcy and VeronikaSolovei9 July 19, 2021 17:42

SyntaxNode assigned VeronikaSolovei9 and AlexBVolcy Jul 19, 2021

fix typo

e5d4d33

SyntaxNode requested review from guscarreon and removed request for AlexBVolcy July 26, 2021 17:19

SyntaxNode assigned guscarreon and unassigned AlexBVolcy Jul 26, 2021

mansinahar reviewed Jul 27, 2021

View reviewed changes

naming according to docs

e0b7f6f

SmartHubSolutions requested a review from mansinahar July 27, 2021 20:06

VeronikaSolovei9 reviewed Jul 28, 2021

View reviewed changes

SmartHubSolutions added 2 commits July 28, 2021 15:25

removing import aliases

e713bba

removing named return params

7a6c090

SmartHubSolutions requested a review from VeronikaSolovei9 July 28, 2021 12:59

guscarreon reviewed Jul 28, 2021

View reviewed changes

SmartHubSolutions added 2 commits July 28, 2021 21:05

removing extra check on empty []Imp

0f8c117

fixing validation schema to avoid the situation when param is an empt…

44ca904

…y string

SmartHubSolutions requested a review from guscarreon July 28, 2021 18:30

VeronikaSolovei9 previously approved these changes Jul 28, 2021

View reviewed changes

guscarreon reviewed Jul 29, 2021

View reviewed changes

SmartHubSolutions requested a review from guscarreon July 29, 2021 19:46

using SmartHub subdomain as part of the host for building endpoint URL

9ec73d6

SmartHubSolutions dismissed VeronikaSolovei9’s stale review via 9ec73d6 August 3, 2021 10:04

SmartHubSolutions requested a review from VeronikaSolovei9 August 4, 2021 19:58

VeronikaSolovei9 approved these changes Aug 5, 2021

View reviewed changes

guscarreon approved these changes Aug 5, 2021

View reviewed changes

guscarreon merged commit 854ff54 into prebid:master Aug 5, 2021

guscarreon mentioned this pull request Aug 10, 2021

SmartHub: extra test to add coverage for unknown bid type scenario #1954

Merged

javaAndScriptDeveloper mentioned this pull request Aug 25, 2021

Add SmartHub bidder prebid/prebid-server-java#1442

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Adapter: SmartHub #1932

New Adapter: SmartHub #1932

SmartHubSolutions commented Jul 19, 2021

SmartHubSolutions commented Jul 23, 2021

mansinahar Jul 27, 2021

SmartHubSolutions Jul 27, 2021 •

edited

Loading

VeronikaSolovei9 left a comment

VeronikaSolovei9 Jul 28, 2021

SmartHubSolutions Jul 28, 2021

VeronikaSolovei9 Jul 28, 2021

SmartHubSolutions Jul 28, 2021

VeronikaSolovei9 Jul 28, 2021

SmartHubSolutions Jul 28, 2021

guscarreon Jul 28, 2021

SmartHubSolutions Jul 28, 2021

guscarreon Jul 28, 2021

guscarreon Jul 28, 2021

SmartHubSolutions Jul 28, 2021 •

edited

Loading

VeronikaSolovei9 left a comment

guscarreon Jul 29, 2021

SmartHubSolutions Jul 29, 2021

mansinahar Aug 2, 2021

SmartHubSolutions Aug 2, 2021

SmartHubSolutions Aug 4, 2021 •

edited

Loading

guscarreon Jul 29, 2021

SmartHubSolutions Jul 29, 2021

SmartHubSolutions commented Aug 3, 2021

VeronikaSolovei9 left a comment

guscarreon left a comment

New Adapter: SmartHub #1932

New Adapter: SmartHub #1932

Conversation

SmartHubSolutions commented Jul 19, 2021

SmartHubSolutions commented Jul 23, 2021

Choose a reason for hiding this comment

SmartHubSolutions Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

VeronikaSolovei9 left a 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

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

SmartHubSolutions Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

VeronikaSolovei9 left a 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

SmartHubSolutions Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SmartHubSolutions commented Aug 3, 2021

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

SmartHubSolutions Jul 27, 2021 •

edited

Loading

SmartHubSolutions Jul 28, 2021 •

edited

Loading

SmartHubSolutions Aug 4, 2021 •

edited

Loading