BingAds / BingAds-Java-SDK

Other
42 stars 47 forks source link

Rest: uploading OfflineConversionAdjustments fails #187

Closed schabe77 closed 4 months ago

schabe77 commented 4 months ago

When trying to upload an offline conversion adjustment, the request fails with an

OperationError code=100,details=Invalid JSON at line 0 position XXX. Path: $.OfflineConversionAdjustments[0].AdjustmentTime

After looking at the request I found out, that an example payload looks like this

{
     "OfflineConversionAdjustments": [{
         "AdjustmentCurrencyCode": "EUR",
         "AdjustmentTime": "2024-05-07T10:51:52Europe/Berlin",
         "AdjustmentType": "Restate",
         "AdjustmentValue": 0.0,
         "ConversionName": "MyConversionName",
         "ConversionTime": "2024-05-01T19:57:12",
         "MicrosoftClickId": "MyMicrosoftClickId"
     }]
 }

The AdjustmentTIme somehow got the TimeZone of the Calendar attached. I can do a workaround by explicitly setting the adjustment time's Calendar with TimeZone UTC, but with SOAP requests it was no problem to just use a standard calendar Calendar.getInstance() (with uses my standard time zone Europe/Berlin).

By the way: You should update your troubleshooting section. With the current version of BingAds all the dependency versions should be updated to 4.0.2, otherwise NoClassDefFoundErrors occur.

I had a hard time to log the requests. I didn't manage it to do it "the SOAP way" that is explained in the troubleshooting section. The only way I was able to log the communcation was to modify my spring-configuration the way it is mentioned here and here and to log the data provided for loggers under org.apache.cxf.services.

schabe77 commented 4 months ago

The problem seems to be com.microsoft.bingads.internal.restful.adaptor.CalendarSerializer maybe the serializer should work the same way as in BulkOnlineConversionAdjustment:

    SimpleDateFormat format = new SimpleDateFormat("MM/dd/yyyy HH:mm:ss");
    format.setTimeZone(TimeZone.getTimeZone("UTC"));

    return format.format(value.getTime());

I am not familiar with jackson's serializers - Is it safe to use SimpleDateFormat as field in CalendarSerializer? It's not thread safe if multiple serialization tasks use the same serializer object, this could be problematic.

dennis-yemelyanov commented 4 months ago

thank you for raising this issue!

SOAP requests use jakarta.xml.bind.DatatypeConverter.printDateTime(), for example here https://github.com/BingAds/BingAds-Java-SDK/blob/e66576e1756eb8ddebac8e13862112b088ba14e4/proxies/com/microsoft/bingads/v13/campaignmanagement/Adapter1.java#L20

Looks like DatatypeConverter writes the timezone as a number instead of its string name, that way it can be parsed successfully by the service.

I'm checking if we can just use the same DatatypeConverter in the serializer or if there were any issues with using it.

I had a hard time to log the requests. I didn't manage it to do it "the SOAP way" that is explained in the troubleshooting section.

Yeah the example in that section should be fixed or cleaned up. At least for recent CXF versions it should be enough to set com.sun.xml.ws.transport.http.client.HttpTransportPipe.dump to true and add slf4j-simple as described here: https://learn.microsoft.com/en-us/advertising/guides/get-started-java?view=bingads-13#logging-service-calls

Although I'm not sure if some logging filters set in the application might be causing the request logs to not be printed (at least it seems to work in a simple application with default settings)

schabe77 commented 4 months ago

Your servers seem to need the time in format "yyyy-MM-dd'T'HH:mm:ss". That's why I provided the pull request https://github.com/BingAds/BingAds-Java-SDK/pull/188 for com.microsoft.bingads.internal.restful.adaptor.CalendarSerializer (the time format is correct and it is thread safe).

This JSON-serializer is registered here

https://github.com/BingAds/BingAds-Java-SDK/blob/b25e458bedb890f5d27f02f1dd1a0aada71733d4/src/main/java/com/microsoft/bingads/internal/restful/adaptor/AdaptorUtil.java#L35

dennis-yemelyanov commented 4 months ago

Well looks like for writing it as a UTC string there is an even simpler solution using Jackson only, we just need to disable one default feature it has:

ObjectMapper mapper = new ObjectMapper();
mapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS);
System.out.println(mapper.writeValueAsString(Calendar.getInstance(TimeZone.getTimeZone("Europe/Berlin"))));
// "2024-05-09T17:20:50.989+00:00"

I don't know why this is not the default behavior and instead it writes timestamps by default...

But in any case, to fully match SOAP behavior it's probably better to send the time the same way, including the time zone that was set on the object (probably local time zone in most cases).

System.out.println(jakarta.xml.bind.DatatypeConverter.printDateTime(Calendar.getInstance(TimeZone.getTimeZone("Europe/Berlin"))));
// 2024-05-09T19:41:23.601+02:00

We also wanted to update the deserializer so it has similar logic to the serializer (in this case using DatatypeConverter).

13.0.20.2 has these changes. Hopefully it works for now, but we can think more about alternatives (using Jackson only, if possible)

schabe77 commented 4 months ago

It seems to work. My test-request containing "AdjustmentTime":"2024-05-10T06:48:16.972Z" "AdjustmentTime":"2024-05-10T08:55:43.400+02:00" "AdjustmentTime":"2024-05-10T09:02:29+02:00" produced no error.

I don't know if it's a problem that milliseconds are being sent now. With the previous formatter, only seconds were part of the timestamp. I will deploy my software next Monday. Then I can tell you if it works in production.

schabe77 commented 4 months ago

uploading offline conversion works without problems with version 13.0.20.2