prebid / prebid-mobile-ios

Prebid Mobile SDK for iOS applications
Apache License 2.0
48 stars 93 forks source link

PB SDK ORTB issues #1017

Open bretg opened 4 months ago

bretg commented 4 months ago

Describe the bug

Found a couple of issues while testing first party data (FPD) on PB SDK:

  1. AdUnitConfig.setOrtbConfig() (global-level) is not available in Prebid SDK v2.2.3 despite what the doc says. (we think this probably works for Android but not iOS)
  2. adUnit.setOrtbConfig() is global for iOS rather than imp-level as described in the docs.

Expected behavior

Arbitrary ORTB should work as documented in https://docs.prebid.org/prebid-mobile/pbm-api/ios/pbm-targeting-ios.html#arbitrary-openrtb

jsligh commented 4 months ago

@bretg

  1. AdUnit.setOrtbConfig() and AdUnitConfig.setOrtbConfig() are both available on Android. onIOS it needs to either be AdUnit.setOrtbConfig() or AdUnitConfig.ortbConfig = "new json string". I will update the iOS section in the docs with this code.
  2. Thoughts on just removing the impression level from the docs? I believe we had talked about just doing it at the global level as it allows people to customize the impression level things as well.
bretg commented 4 months ago

Thoughts on just removing the impression level from the docs? I believe we had talked about just doing it at the global level as it allows people to customize the impression level things as well.

How would they do that and merge to an existing imp object in the array? We tried and it ended up creating a new imp object.

jsligh commented 4 months ago

@bretg I guess I didn't realize this would be wanted/needed until now. I thought previously we had just discussed when merging JSON arrays that we would just add to end and remove dupes. I assume that the imp id's are unique? I will create a check that if we are looking at changing an impression, check and see if any of the id's match and then merge those.

bretg commented 4 months ago

It's pretty unfriendly to make the caller figure out the imp.id. We need the imp-level ORTB merge.

jsligh commented 4 months ago

Is there a unique identifier to be able to tell which imps need to be merged? Or do you want it merged into every imp?

bretg commented 4 months ago

The "AdUnit" object translates to an imp object... any method on that object should be able to put the results in the right location.

Let's step back. I'll re-state the original requirements:

  1. we need a way for SDK users to define arbitrary OpenRTB at the global (cross-adunit) level. Example use case: PBS cache commands.
  2. we need a way for SDK users to define arbitrary OpenRTB at the level of a single adunit. Example use case: custom PMP

To those requirements, I'll state a design consistency preference that global ORTB be treated similarly to other global parameters - i.e. that method should be on Targeting.shared.

YuriyVelichkoPI commented 4 months ago

Hi @jono, @bretg, sorry, I remember that I promised to take a look at this ticket and provide implementation spec, but unfortunately, I didn't have the bandwidth. I'll do my best to accomplish it this week.

However, as I told you before, it won't be a simple feature because Swift and JAVA, unlike js, don't manage the JSONs in a generic way. I don't support the implementation of merging, like merging "random" JSONs, which are basically random strings. We need to implement the merging of the internal RTB models (the model is already implemented in the SDK).

So the SDK should deserialize arbitrary RTB param into the internal model, merge this model into the model built by SDK, and serialize the result into the JSON (build OpenRTB request) for the request. If SDK controls the structure and rules we will avoid such issues like invalid ids, etc.

If it makes sense, I'll work on the details later this week.

YuriyVelichkoPI commented 4 months ago

Hi @bretg, @jsligh,

Investigation

I've been experimenting with the current implementation and we can adapt it if we don't want to work on the internal-model level.

The current method adUnit.setOrtbConfig() merges given values starting from the global level.

For instance, the following code:

adUnit.setOrtbConfig("{\"test_rtb\":1, \"app\" : {\"bundle\": \"org.prebid.PrebidDemoSwift_TEST\"}, \"device\": {} }")

changes the request in the following way:

However, if we want to change the impression using the setOrtbConfig: like this:

adUnit.setOrtbConfig("{\"test_rtb\":1, \"app\" : {\"bundle\": \"org.prebid.PrebidDemoSwift_TEST\"}, \"device\": {}, \"imp\": [{\"banner\": {\"api\": [7]}}]}")

The algorithm builds wrong requests. It adds a new object instead of modifying the existing one. And it also breaks the json (❗️). You can try the code snippet to reproduce the error.

In any case, publishers can't use adUnit.setOrtbConfig to modify Imp-level data properly.

Objectives

In order to implement the requirements and keep the current approach (merging of random JSONs), we need to

ankit-thanekar007 commented 3 months ago

Hi @YuriyVelichkoPI , @jsligh Is there an ETA for this change ? This would be really helpful to have.

vietta commented 2 months ago

Hi @YuriyVelichkoPI , @jsligh Is it possible to update the banner.battr by using setOrtbConfig()? Context: I thought it was possible when I looked at the usage examples here However, using the code below, another banner object was created in the imp object without updating the original one. bannerAdUnit.setOrtbConfig("{\"imp\": [{\"banner\": {\"battr\": [[1,3,5,6,8,9,10,11,14,17]]}}]}")