Incorrect "battr" value produces null pointer on JSON deseriailzation

[Spikhalskiy]

Hello,

If I try to deserialize OpenRtb.BidRequest from json

OpenRtb.BidRequest bidRequest = OpenRtbJsonFactory.create().newReader().readBidRequest(json);

with banner request that has incorrect (by OpenRTB specification) battr, for example

"battr": [2, 17]  //1-16 by specification

I get

java.lang.NullPointerException
    at com.google.openrtb.OpenRtb$BidRequest$Imp$Banner$Builder.addBattr(OpenRtb.java:11190)
    at com.google.openrtb.json.OpenRtbJsonReader.readBannerField(OpenRtbJsonReader.java:566)

Two problems:

1. NullPointerException - should be more detailed diagnostic info I think
2. You think and write in Wiki about extensions. We should think about them here too. For example, save non-standard attributes codes to another int array. Maybe it should be configurable with extension. Example of Rubicon BidRequest where they use non-standard creative attributes.

[opinali]

You have a good point/find that deserializing this JSON should never fail with a NullPointerException. I guess the best option is not failing at all, just ignoring the unsupported values. Will be fixed in next release.

Now on the second problem of providing some kind of support for non-standard values: this is one explicit tradeoff made by the library, we wanted both the safety and convenience of fields typed as enumerations, instead of having tons of int32 fields everywhere. The flipside is that supporting extended enum values is not possible, at least not in any elegant way (yes I could play with protobuf's support for "unknown values"... it would require reserving separate tag IDs and hacks in several places). But the most important factor is that this tradeoff is actually minimal, because this kind of extension is not generally allowed; the only enum explicitly documented to allow exchange-specific values is the at (Auction Type) IIRC; and even there you have to use ordinal numbers above 500, you can't just pick the first "free" number. This is highly likely to clash with new standard creative attributes that may be added in future versions of the spec (unless the spec just adopts as standard this new attribute from Rubicon; but that doesn't scale to multiple vendors doing the same hack). In the native spec this is similar, vendor-specific enums all above 500, except that this is explicitly allowed for all enums from that spec.

I suggest that you fork the library and just add a new enum element for attribute 17, if not losing it is critical.

[Spikhalskiy]

Hi,

17 is just a value to localize error for you, that we have an error on values above maximum provided by specification. I have no need to handle this specific value. Rubicon used values above 10000 in attached example. Maybe it's an outdated sample.

Your solution is absolutely fine with me if using values not from specification is prohibited be spec for this attribute. In that case - look at it issue as a report about NullPointer.

One general note - it would be great to have two options dealing with elements not from schema/specification - "ignore" and "fail whole process with exception".

Thank you, Dmitry

[opinali]

Happy to know Rubicon doesn't break the spec in this way, that seemed surprising considering their role creating the standard :) The NPE was an easy fix... can't wait for Protobuf 3 GA so we can stop writing handcoded JSON parsers for this :(

Optional failure is a good idea too.

[opinali]

Just got in the code to fix this, you can test with master but we should have a release soon.