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

Ignoring unknown bid request extensions in OpenRtbJsonReader #32

Closed dhamilton-nanigans closed 9 years ago

dhamilton-nanigans commented 9 years ago

Is there a way to set the OpenRtbJsonReader to ignore unknown RTB extensions in an exchange bid request? I am concerned that if an exchange adds additional extensions to their requests this could lead to exceptions and a halt in bidding if new extension readers/writers aren't created and deployed in time. Also it places additional burden on a DSP using the library to require extension handling for extensions that may not be of interest to them.

opinali commented 9 years ago

There is no way to do that right now, but this is an improvement that was already being considered - not just ignoring unknown extensions, but ignoring any unknown field. We'll try to get this into the next update.

dhamilton-nanigans commented 9 years ago

Thanks for getting back to me, that's good to hear. I have one follow up question. I am working on using your library to support MoPub's OpenRTB protocol. Extending the protobuf objects and implementing JSON serializers for the extensions has been a pretty smooth process. However, MoPub defines a non-standard "crtype" field that is not in their bid response that is not mapped to the "ext" field. I am able to serialize and deserialize a bid response with this field without any failures (not sure if that's expected or not), however I can't find a way to register a JSON reader/writer to handle a field like this. Is there a way to do this?

Here's a link to the MoPub spec for reference, note the "crtype" field highlighted in blue under section 3: https://dl.dropboxusercontent.com/u/10601557/RTB2.1spec.html

opinali commented 9 years ago

Adding support for non-standard fields is possible, just not very clean... you can fork the library and adding that field to the proto. Then it should also be easy to patch the JSON read/writer classes with support for this field. The problem then is continued merging to upstream, since we can't accept a change that adds uncompliant exchange-specific fields to the main library (even if it's tempting because we have the exact same problem internally at Google for interop with MoPub's protocol, and we do the same thing, adding the crtype field...). But it's not a lot of work either since the model and the JSON libs are pretty stable.

opinali commented 9 years ago

Final status on this: the next release will have a small fix in the classes OpenRtb*JsonWriter so you can write subclasses that extend field writing for regular fields. (This was already possible for OpenRtb*JsonReader.) This will make a new solution possible: keep the standard proto unchanged (no crtype field there), but create an extension for that, and subclass the writer class so it maps regular fields in the JSON into/from proper extension fields in the protobuf-based model. You may still consider the forking option simpler, but it will be nicer to have this alternative.