Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rest: uploading OfflineConversionAdjustments fails #187

Closed
schabe77 opened this issue May 7, 2024 · 6 comments
Closed

Rest: uploading OfflineConversionAdjustments fails #187

schabe77 opened this issue May 7, 2024 · 6 comments

Comments

@schabe77
Copy link

schabe77 commented May 7, 2024

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 schabe77 changed the title Rest: OfflineConversionAdjustments fail Rest: uploading OfflineConversionAdjustments fails May 7, 2024
@schabe77
Copy link
Author

schabe77 commented May 7, 2024

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
Copy link
Contributor

thank you for raising this issue!

SOAP requests use jakarta.xml.bind.DatatypeConverter.printDateTime(), for example here

return (jakarta.xml.bind.DatatypeConverter.printDateTime(value));

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
Copy link
Author

schabe77 commented May 8, 2024

Your servers seem to need the time in format "yyyy-MM-dd'T'HH:mm:ss". That's why I provided the pull request #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

public static SimpleModule module = new SimpleModule().addDeserializer(Calendar.class, new CalendarDeserializer()).addSerializer(Calendar.class, new CalendarSerializer())

@dennis-yemelyanov
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

uploading offline conversion works without problems with version 13.0.20.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants