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.

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

Allow Bidder to Override Callback Type in /setuid #3301

Merged
merged 10 commits into from
Jan 12, 2024

Conversation

AlexBVolcy
Copy link
Contributor

@AlexBVolcy AlexBVolcy commented Nov 15, 2023

Addressing a piece of this larger issue: #2173

This PR will allow a bidder to force override its callback to be either iframe or redirect, based on a configuration they can now set in their .yaml formatOverride

Use Case from issue: client creates iframe, the cookie-family's iframe drops image. The usersync type is 'iframe'. The default f=b response works, but returns blank into an img, which may not be ideal in some scenarios. Use a format override to set f=i instead of the default f=b.

usersync/syncer_test.go Outdated Show resolved Hide resolved
@hhhjort
Copy link
Collaborator

hhhjort commented Nov 15, 2023

I don't understand the use case here. From the linked issue, I could not find a clear explanation of what this is supposed to be implementing. What you have done seems to force the /setuid endpoint to respond with an empty gif or an empty html response reguardless what the URL indicates should be the response. This seems broken, as the call will expect one type based on the 'f' parameter it is using, and will lead to a browser error if the wrong type is returned.

The only place I can make a forced type make sense is in the /cookie_sync endpoint, where you can force the sync type to be a certain type, so the downstream caller can format the call with the appropriate type of tag despite what the cookie_sync sync endpoint is told is preferred.

Possibly I suppose to allow the bidder's sync code to specify the wrong value if the "f=" parameter, but then fix it on the PBS side, but I think the proper response here is to tell the bidder to fix their sync code rather than putting a hack in PBS to correct the sync type.

What am I missing here?

@AlexBVolcy
Copy link
Contributor Author

AlexBVolcy commented Nov 15, 2023

@hhhjort I think I’m the one probably missing something. Scott and I spoke a few weeks ago about various aspects of the linked issue that needed to be handled, and there’s one line in the linked issue referencing what this PR is trying to address: “Allow a bidder to override it's setuid callback type from either iframe or image”

This specific part of the issue we didn’t dive into great detail on, but I figured overriding a setuid callback type would refer to overriding what is being returned by getResponseFormat() within /setuid. However, the concerns that you bring up make sense. @SyntaxNode, do you have any more info for how I should be approaching about this override?

In the meantime, I'm going to investigate the Java code to see if/how they handle address this piece of the issue.

config/bidderinfo.go Outdated Show resolved Hide resolved
usersync/syncer_test.go Outdated Show resolved Hide resolved
usersync/syncer_test.go Outdated Show resolved Hide resolved
@SyntaxNode
Copy link
Contributor

What am I missing here?

The linked issue identifies the use case as:

Client creates iframe, the cookie-family's iframe drops image. The usersync type is 'iframe'. The default f=b response works, but returns blank into an <img>, which may not be ideal in some scenarios. Use the format override to set f=i.

I'm not clear on the situation where a sync iframe would return an image. @bretg is better versed with the lifecycle and edge cases of iframe syncs.

@bretg
Copy link
Contributor

bretg commented Nov 17, 2023

@AlexBVolcy - please help me understand the use case here.

allow a bidder to force override its callback to be either iframe or redirect

At this point, I don't think this is correct or desirable.

Bidders should be able to supply both iframe and pixel syncs, and they can express a preference, but it doesn't make sense to me that a bidder should be able to force it -- this is a page/publisher decision, not a bidder decision.

@SyntaxNode
Copy link
Contributor

Bidders should be able to supply both iframe and pixel syncs, and they can express a preference, but it doesn't make sense to me that a bidder should be able to force it -- this is a page/publisher decision, not a bidder decision.

@bretg This PR is adding the "formatOverride" feature introduced in PBS-Java to PBS-Go. The questions are for it's use case. There doesn't appear to be any bidder using this today. Could you please when a bidder would want to set "formatOverride"?

@bretg
Copy link
Contributor

bretg commented Nov 17, 2023

Right, ok, this is the use case from the issue

2. client creates iframe, the cookie-family's iframe drops image. The usersync type is 'iframe'. The
default f=b response works, but returns blank into an img, which may not be ideal in some
scenarios. Use a format override to set f=i instead of the default f=b.

So formatOverride is a way for iframe-sync URLs to be built with /setuid calls containing f=i instead of the default f=b

@SyntaxNode
Copy link
Contributor

@bretg Gothcha. Why would iframe sync URLs need to return f=i instead of f=b?

@bretg
Copy link
Contributor

bretg commented Nov 17, 2023

There was a scenario where someone was emitting an iframe, and in that iframe they were dropping a pixel back to PBS rather than using AJAX

@hhhjort
Copy link
Collaborator

hhhjort commented Nov 21, 2023

There was a scenario where someone was emitting an iframe, and in that iframe they were dropping a pixel back to PBS rather than using AJAX

In the code delivered by the iframe, why don't they just hardcode the pixel to have f=i in the URL, rather than allowing it to be f=b in the iframe body, but then correct it when it hits PBS to be a pixel? It would seem that any bidder that uses this override feature would lock themselves into always using empty images or blank text on the setuid call for all their sync cases. It would seem to be better to just always use f= in the setuid URL rather than disregarding that setting and assuming that the bidder always knows better than the HTML that the call is coming from.

@bretg
Copy link
Contributor

bretg commented Nov 21, 2023

why don't they just hardcode the pixel to have f=i in the URL

Because PBS generates the redirect URL.

@AlexBVolcy
Copy link
Contributor Author

Just to keep this PR updated: After speaking with @SyntaxNode, this PR is correctly addressing the feature that we want implemented.

I'm just addressing some small comments, and this will be ready for review soon.

@@ -266,3 +279,14 @@ func (s standardSyncer) chooseTemplate(syncType SyncType) *template.Template {
return nil
}
}

func (s standardSyncer) ForceResponseFormat() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer used after the refactor and can be deleted.

config/bidderinfo.go Show resolved Hide resolved
hhhjort
hhhjort previously approved these changes Jan 8, 2024
config/bidderinfo.go Outdated Show resolved Hide resolved
endpoints/cookie_sync_test.go Outdated Show resolved Hide resolved
endpoints/setuid_test.go Outdated Show resolved Hide resolved
usersync/chooser_test.go Outdated Show resolved Hide resolved
config/bidderinfo.go Show resolved Hide resolved
@@ -206,6 +209,11 @@ type InfoReaderFromDisk struct {
Path string
}

const (
ResponseFormatIFrame = "b" // b = blank HTML response
ResponseFormatRedirect = "i" // i = image response
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: Since these are moved out of the sync package, consider renaming to SyncResponseFormatIFrame and SyncResponseFormatRedirect to add back that context.

@bsardo bsardo assigned SyntaxNode and unassigned bsardo Jan 11, 2024
@SyntaxNode SyntaxNode merged commit 3789cf9 into prebid:master Jan 12, 2024
3 checks passed
Wazabit pushed a commit to AIDEM-Technologies/prebid-server that referenced this pull request Jan 16, 2024
Wazabit pushed a commit to AIDEM-Technologies/prebid-server that referenced this pull request Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants