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

OpenRTB Native Request - Issue with "native" field #73

Closed morpheusTheCoder closed 8 years ago

morpheusTheCoder commented 8 years ago

This is related to the discussion at : https://groups.google.com/forum/#!topic/openrtb-native/ecrjonP-rIs

There are 2 flavors of the "native" field under "imp" in Bid request. A) "native": {"request": "{\"native\": {\"adunit\": 3, \"layout\": 3, \"assets\": [...] }}"} B) "native": {"request": "{\"adunit\": 3, \"layout\": 3, \"assets\": [...]}"}

[Actual] When I run the UTs under OpenRtbJsonTest, I see that json is getting serialized to option#(A) above. [Expected] However, the documentation for this project seems to suggest that it should be serialized to option# (B).

Any insights into this discrepancy? Here is a link to the doc: https://github.com/google/openrtb/wiki#native-ads (Note: I am using openrtb-core-0.9.3. I haven't tried running UT on the latest version of openrtb-core because of Java 8 dependencies).

A a side note, Please let me know if there is a way for me to generate requests as shown in option# (B) above using this library.

opinali commented 8 years ago

You are right, this is an incomplete and not well documented part of the JSON support. The current implementation favor what seemed to me to be the prevailing interpretation of the spec, but the "right thing" is not having that top-level field anymore, it's past due time to change it. Except that I can't just change, so I will be adding a boolean flag you can set in the factory to choose if the JSON write will emit this field or not, so people needing the legacy-compatible behavior can do that. (The reader already works with or without that field.)

morpheusTheCoder commented 8 years ago

Thanks a bunch opinali.

BTW, currently we are using version 0.9.0 of the library. And we were not planning to move to latest version as it requires Java 8.

On which version would you plan to apply the patch?

opinali commented 8 years ago

The plan so far did not include any further updates for lower JDK releases, but I didn't expect the demand :) I think it's fair to backport at least bugfixes and compatibility improvements like this to a 0.9.9 release.

morpheusTheCoder commented 8 years ago

Thanks opinali.

Just to get some idea, would you be comfortable providing a very rough timeframe on when can we expect this. Sincerely appreciate your help.

opinali commented 8 years ago

This would normally be a priority (at least releasing this fix if not the JDK8 port), but bad timing this week with SFO AdX Developer Day ahead :) so I think I can get this out early next week, and the porting maybe later that week, or following week worst case. If there's urgency you could fork the last release you can use and patch in only this fix.

morpheusTheCoder commented 8 years ago

Thanks opinali. I see that this issue fixed in the current branch. Please let me know if you can get around to backport it for pre-java8 branch. Thanks a bunch in advance.

opinali commented 8 years ago

Yes this is released now as 1.0.3. A backport is planned next, should be available later this month!

morpheusTheCoder commented 8 years ago

Thanks Opinali, appreciate your help.

morpheusTheCoder commented 8 years ago

Hi Opinali, just wanted to check on this. Have a great day.

opinali commented 8 years ago

There's a new minor release coming, so the jdk7-compatible branch is planned to be part of that release, thanks for some extra patience :)

morpheusTheCoder commented 8 years ago

Hi opinali,

Just wanted to check if the fix for this eventually made it to the jdk-compatible branch. Thanks a bunch, and appreciate your help.

opinali commented 8 years ago

Oh sure, sorry for not updating this issue but yes the compat branch is updated, I'm keeping it in sync with every new release.