877 Supporting Arbitrary ORTB Params

[jsligh]

Closes #877

Follows: https://docs.google.com/document/d/1cRWDNO6i7wLyQdQA9ZzmK0QumIr2vvC40r5GhHhOSz4/edit?usp=sharing

Old PR: #941

[YuriyVelichkoPI]

Sorry for the long silence. I was thinking about this method for a while. In general, it does its job - merges the objects of NSDictionary . But we can't use it here because we have cases that should be applied differently for different objects.

Let's assume that SDK generates the RTB like:
https://github.com/openrtb/examples/blob/master/brandscreen/example-request-mobile.json

And let's consider the case that publisher wants to rewrite banner.api field to [1,2].

publisher provides

{
  "imp": [
    {
      "banner": {
        "api": [
          1,
          2
        ]
      }
    }
  ],
}

According to the current implementation, if I'm not mistaken, the whole imp object will be added to the imp array. In the best way values 1, 2 will be added to 3 in the original one.

So we can't merge arrays.

On the other hand, we can't replace the elements of arrays if they are other RTB objects because, with the current example, the partial imp object will be added to the resulting RTB.

Maybe we can recursively work with arrays of objects. But I'm still can't be sure that the implementation will be correct for all cases.

Instead, I recommend moving the harder (more code to write), but still clear (easy code) way:

• serialize PBMORTBBidRequest object and all nested objects from the given JSON
• implement the merge:PBMORTBBidRequest obj function where each property is processed separately.

This way will give us assurance that we don't break any of the given structures and will give us a clear understanding of how exactly particular objects are merging. Each implementation can be covered by clear unit tests.

Looking at the current approach, I am not sure that all RTB objects are supported respectively. We can easily lose some data or produce incorrect results on merging.

[jsligh]

Sorry for the long silence. I was thinking about this method for a while. In general, it does its job - merges the objects of NSDictionary . But we can't use it here because we have cases that should be applied differently for different objects.

Let's assume that SDK generates the RTB like: https://github.com/openrtb/examples/blob/master/brandscreen/example-request-mobile.json

And let's consider the case that publisher wants to rewrite banner.api field to [1,2].

publisher provides

{
  "imp": [
    {
      "banner": {
        "api": [
          1,
          2
        ]
      }
    }
  ],
}

According to the current implementation, if I'm not mistaken, the whole imp object will be added to the imp array. In the best way values 1, 2 will be added to 3 in the original one.

So we can't merge arrays.

On the other hand, we can't replace the elements of arrays if they are other RTB objects because, with the current example, the partial imp object will be added to the resulting RTB.

Maybe we can recursively work with arrays of objects. But I'm still can't be sure that the implementation will be correct for all cases.

Instead, I recommend moving the harder (more code to write), but still clear (easy code) way:

• serialize PBMORTBBidRequest object and all nested objects from the given JSON
• implement the merge:PBMORTBBidRequest obj function where each property is processed separately.

This way will give us assurance that we don't break any of the given structures and will give us a clear understanding of how exactly particular objects are merging. Each implementation can be covered by clear unit tests.

Looking at the current approach, I am not sure that all RTB objects are supported respectively. We can easily lose some data or produce incorrect results on merging.

This issue with the specificity of your example is the following:

As far as checks go, besides it being valid JSON and not changing "protected" fields, this is all Prebid JS/Server does according to @bretg and I believe we should follow-suit and let them "shoot themselves in the foot" if they want to. We want to give users the ability to modify ANY non-protected part of the BidRequest JSON.

If we do go into specifics on checking everything about ORTB formatting, then we eliminate the "arbitrary" part of this issue.

In the above example, if they want to modify a JSONObject INSIDE a JSON Array, we have no logical way of knowing (with merging logic) whether they wanted to add the new object to an array or merge it with an existing. I would say we should err on the side of caution when assuming. I would say that unless the two objects are exactly the same (and then we only have 1 to eliminate duplicates, but even this is questionable) and they're going into an array then there's no definitive way to know what to do except merging the two arrays.

[YuriyVelichkoPI]

I understand it. I just can't predict the behavior and output of the algorithm depending on the different formats and structure of the input data. I don't understand why we should not use the existing RTB objects and structure and instead rely on pure JSON operations.

But if you think it is enough - no objections from me.

[bretg]

I do not believe we should worry about developers supplying an imp array at the global level.

The point of the design is that impression-level ORTB needs to be specified per adunit. We can document this.

So what should we do it someone foolishly supplies an imp array at the global level? I think it's fair to overwrite the imp array. It's likely to fail spectacularly when hits Prebid Server, and assuming they test it, they'll figure it out and actually read the documentation.

But if this team thinks app developers would be turned off by such responsibility, I'm not against a check that specifically rejects setting imp at the global level.

I don't think it's necessary to take on the complexity of merging arrays at this point.

[bretg]

@YuriyVelichkoPI - you asked my review here -- was it just the issue about the possibility of specifying imps at the global level or something else?

[YuriyVelichkoPI]

Yes, I was asking about merge requirements. The current implementation relies on the correctness of the input and doesn't validate the RTB structure. In the current implementation the merge is based not on RTB but on JSON. Publishers can provide whatever they want. If we delegate all the final validation to the server in the current scope - ok, let's try.

[bretg]

My review indicates that I agree with the "low-validation" approach. Checking for valid JSON is a good idea. Checking for valid ORTB is not necessary.

I'm open to the idea of validating that imp is not specified in the global ORTB, but this is not necessary in my opinion.