prebid / prebid-server

Open-source solution for running real-time advertising auctions in the cloud.
https://prebid.org/product-suite/prebid-server/
Apache License 2.0
431 stars 739 forks source link

Remove validation for native enumerations #1732

Closed YuriyVelichkoOpenX closed 1 year ago

YuriyVelichkoOpenX commented 3 years ago

Issue

According to the Open Measurement (OM) spec, the ad request should contain the event tracker with custom type 555 to signal the demand side that SDK supports OM. See section Native Bid Response in OpenRTB p.28 of Onboarding Guide for Integration Partners

{\"event\":555,\"methods\":[2]}

The bid requests with such a tracker leads to the server error:

Invalid request: json: cannot unmarshal number 555 into Go struct field EventTracker.eventtrackers.event of type native.EventType

Objective

Need to provide publishers an ability to request native ads with any exchange-specific event tracker including the Open Measurement.

Additional info

The IAB spec for native ads: 4.1 Native Markup Request Object 4.7 Event Trackers Request Object 7.6 Event Types Table - the most important section for our case

Example of native markup in the request:

"native": {
        "request": "{\"assets\":[{\"required\":1,\"title\":{\"len\":90}},{\"img\":{\"hmin\":50,\"type\":1,\"wmin\":50},\"required\":1},{\"img\":{\"hmin\":50,\"type\":3,\"wmin\":150},\"required\":1},{\"data\":{\"type\":2},\"required\":1},{\"data\":{\"type\":12},\"required\":1},{\"data\":{\"type\":1},\"required\":1}],\"context\":2,\"contextsubtype\":20,\"eventtrackers\":[{\"event\":1,\"methods\":[1,2]},{\"event\":555,\"methods\":[2]}],\"plcmttype\":1,\"ver\":\"1.2\"}",
        "ver": "1.2"
      }

The fast troubleshooting search has shown that the issue is related to OpenRTB library https://github.com/mxmCherry/openrtb/blob/v11.0.0/native/event_type.go#L4

SyntaxNode commented 3 years ago

Thank you for the detailed report.

We use a third party library to represent the OpenRTB data structures. They have the native.EventType as an 8-bit integer, but more recent updates change that to a larger variable type. We are currently using an older version. I'll put out a PR to bring us up to date.

laurb9 commented 3 years ago

Thanks for picking this up @SyntaxNode. The mxmCherry repo restructured the package in v12 when they added openrtb3, so updating touches a lot of files :)

SyntaxNode commented 3 years ago

Not a problem. Yeah, it's an insane amount of files touched. I think I have 220 files changes so far... Just something we need to do.

bretg commented 3 years ago

@YuriyVelichkoOpenX - what response is expected? In PBS-Java I'm getting an error when sending event type 555 to appnexus:

Invalid request format: request.imp[0].native.request.eventtrackers[1].event is invalid. See section 7.6: https://iabtechlab.com/wp-content/uploads/2016/07/OpenRTB-Native-Ads-Specification-Final-1.2.pdf#page=43

Here's the request I send:

{
            "id": "1001",
            "imp": [
                {
                    "id": "some-impression-id",
                    "native": {
            "request": "{\"assets\":[{\"title\":{\"len\":90},\"required\":1},{\"img\":{\"hmin\":50,\"type\":1,\"wmin\":50},\"required\":1},{\"img\":{\"hmin\":50,\"type\":3,\"wmin\":150},\"required\":1},{\"data\":{\"type\":2},\"required\":1},{\"data\":{\"type\":12},\"required\":1},{\"data\":{\"type\":1},\"required\":1}],\"context\":2,\"contextsubtype\":20,\"eventtrackers\":[{\"event\":1,\"methods\":[1,2]},{\"event\":555,\"methods\":[2]}],\"plcmttype\":1,\"ver\":\"1.2\"}",
            "ver": "1.2"
                    },
                    "secure": 1,
                    "ext": {
                        "appnexus": {
                            "placementId": 13232354
                        }
                    }
                }
            ],
            "site": {
                "domain": "rubiconproject.com",
                "page": "www.rubiconproject.com"
            },
            "device": {
                "ua": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36",
                "ip": "174.57.47.200"
            },
            "test": 1,
            "ext": {
                "prebid": {
                    "cache": {
                        "bids": {}
                    },
                    "targeting": {
                        "pricegranularity": "med"
                    }
                }
            }
        }

It works ok if I send event type 2 instead of 555

SyntaxNode commented 3 years ago

In PBS-Java I'm getting an error when sending event type 555 to appnexus:

@bretg Is PBS-Java itself generating the error?

SyntaxNode commented 3 years ago

@laurb9 Just opened https://github.com/prebid/prebid-server/pull/1733. If you'd like to take a look, I have open questions about how to handle the breaking changes.

bretg commented 3 years ago

Is PBS-Java itself generating the error?

Looks like yes. The validation routine isn't accepting 500+

    private void validateNativeEventTracker(EventTracker eventTracker, int impIndex, int eventIndex)
            throws ValidationException {
        if (eventTracker != null) {
            final int event = eventTracker.getEvent() != null ? eventTracker.getEvent() : 0;

            if (event != 0 && (event < EventType.IMPRESSION.getValue()
                    || event > EventType.VIEWABLE_VIDEO50.getValue())) {
                throw new ValidationException(
                        "request.imp[%d].native.request.eventtrackers[%d].event is invalid. See section 7.6: "
                                + documentationOnPage(43), impIndex, eventIndex
                );
            }

Though this is a straight port of what's in Go

func validateNativeEventTracker(tracker nativeRequests.EventTracker, impIndex int, eventIndex int) error {
    if tracker.Event < native.EventTypeImpression || tracker.Event > native.EventTypeViewableVideo50 {
        return fmt.Errorf("request.imp[%d].native.request.eventtrackers[%d].event is invalid. See section 7.6: https://iabtechlab.com/wp-content/uploads/2016/07/OpenRTB-Native-Ads-Specification-Final-1.2.pdf#page=43", impIndex, eventIndex)
    }

Sounds like this is a problem?

SyntaxNode commented 3 years ago

Yes. Looks like it. The IAB Native 1.2 spec does allow for values greater than 500, so I think our validation in PBS is wrong. The current logic plus allowing event types 500 and larger would satisfy the spec.

There's an additional problem in PBS-Go since the data model is limited to an int8 and unable to represent a 500. That's the immediate error reported in this issue. You're right though, as we need to update this check too.

SyntaxNode commented 3 years ago

I removed the PBS-Go label. I originally thought this was just a PBS-Go data model issue, but both versions also share the validation @bretg mentioned.

SyntaxNode commented 3 years ago

Reverting GitHub's auto close.

SyntaxNode commented 3 years ago

This is implemented in PBS-Go 0.158.0.

We'd like to review the implementation with the Committee to ensure our reading of the Native Spec is correct.

We made validation changes to allow the following Native Ads type ids to have a value of 500+.

The specification has rules for which context sub type ids are valid for which context type ids. There is no mention of how this works for values of 500+, so we treat them as wild cards.

There is confusing wording on what 500+ means. This read to us as "500, 501, 502..." but there is wording in the spec of "above 500". Since the specification is unclear and there is no downside to allowing 500, we choose 500+ to be inclusive.

YuriyVelichkoOpenX commented 3 years ago

I will be able to test it next week when we update PBS. We do not have real native demand - just mocks, so I can't say how fixes will affect demand side that is able to process requests for native ads. But we will check our test requests with 555 event type for Open Measurement.

SyntaxNode commented 3 years ago

Sounds good. We're also going to reevaluate if PBS should be performing this kind of native bid validation.

bretg commented 3 years ago

Had the discussion internally -- Magnite is ok with removing specific numeric range validations from PBS. It's find for it to confirm the value is a number, but validating which specific numbers doesn't appear to bring much value to the ecosystem.

YuriyVelichkoOpenX commented 3 years ago

Checked with new PBS version. We are able to send OM event tracker (555) in the request.

SyntaxNode commented 3 years ago

I've had the discussion internally with Xandr and we are also good with removing the type id validation. I've also sought guidance from the IAB and they do not intend for services to validate against type id enumerations. Let's plan to remove this check entirely, but leave in the other checks as those are verifying proper structure.

YuriyVelichkoPI commented 1 year ago

Hi @bretg ! It looks like the fix was introduced for PBS-Go (desc comment), but I don't know whether it was introduced for PBS-JAVA. If yes, the issue can be closed.

bretg commented 1 year ago

PBS-Java ported the PBS-Go fix in PBS-Java 1.66