search

google / openrtb

OpenRTB model for Java and other languages via protobuf; Helper OpenRTB libraries for Java including JSON serialization
Apache License 2.0
398 stars 190 forks source link

Exchange-specific AdUnitId causes NullPointerException #69

Closed bryanwagner closed 8 years ago

bryanwagner commented 8 years ago

The OpenRTB Native Ads Specification 1.0 defines adunit as an integer value referencing section 7.2 Native Ad Unit IDs, which defines integer values 500 and above as "Reserved for Exchange specific formats". In the openrtb library this integer is implemented as a protobuf enum up to value 5 (there is a comment in the protobuf acknowledging the specification values of 500 and above). When exchange-specific integer values are parsed by the library, the parse fails with a NullPointerException. While this is correct according to the protobuf implementation, it is incorrect with respect to the OpenRTB specification. There are several other fields following the same pattern.

Since we have partners using these exchange-specific values, I was tempted to create a pull request with a fix, but I realized it's more of a design-level issue. One workaround could be to add an optional int32 to the protobuf for these fields named exchAdunit for example. Doing so, however, would require a modification to the JSON parse that would need to consider both the adunit and this new value in serialization/deserialization, most likely giving priority to the presence of the defined enums.

I'm interested in knowing what the preferred direction would be for a bug fix; I would say it's a bug due to the declaration in the specification.

opinali commented 8 years ago

The NullPointerException was not intended, it's a bug that was fixed in v0.9.3, so you can update to at least avoid this exception. (If you're using the library via Open Bidder, that will have a release later this week, catching up with the current OpenRTB libs.) I agree this representation of enums doesn't support the extended values allowed by the spec, but it's a tradeoff we've made for a more convenient, strong-typed API. (And frankly this is a bad feature of the spec IMO, so we don't plan to ever use it in AdX' upcoming OpenRTB callout support; but that's a separate discussion.)

However there was always a plan to support extended values via Protobuf's unknown field feature (which makes reading/writing the extended values less convenient, but that's OK for a corner case -- you are the first user ever who reports this limitation as a problem, confirming my impression that actual usage of extended values is uncommon). But it's not a hard change and now there's demand for it, so maybe a good time to work on that. Another solution is forking the proto from this library and adding new values to the enum, this is a good workaround with the latest lib which will just ignore unknown values so you only need to add the ext values you care to process.

bryanwagner commented 8 years ago

Ah, the fix you provided in 0.9.3 works perfectly enough for me! Thank you!

I had a transitive dependency that was blocking my attempt at using the latest build (our project is on Maven 2 still). Knowing there was a fix in 0.9.3 helped me track it down, and 0.9.8 is working for me. I agree; I think in practice codes like these are remnants of system built prior to the spec formalization and it's always been enough for us to discard them or refer people to the ext fields. I agree that the strongly typed API in terms of enums is much more robust. This library has been extremely valuable to us; thanks again.