facebook / facebook-java-business-sdk

Java SDK for Meta Marketing APIs
https://developers.facebook.com/docs/business-sdk
Other
393 stars 324 forks source link

Fix cast error from `AdsPixel` to `APINode` when submitting CAPI requests #416

Closed pmscosta closed 1 year ago

pmscosta commented 1 year ago

The SDK cannot be used to send CAPI requests due to a bug in the sendEventToCAPIOnly() method where it tries to cast an APINode type (returned by the executeAsync() method of a AdsPixel) to an AdsPixel.

Related issues: https://github.com/facebook/facebook-java-business-sdk/issues/408

Related commits: https://github.com/facebook/facebook-java-business-sdk/commit/97b153c3a0719026ed30e21d599b3966cac399d2 fixed this issue only for the sendEventAsyncToCAPIAndCustomEndpoint method.

These changes were tested in a dev environment, and I could successfully send the requests and see them on the Events Manager view of Facebook.

facebook-github-bot commented 1 year ago

Hi @pmscosta!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

facebook-github-bot commented 1 year ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

facebook-github-bot commented 1 year ago

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

judearavinda commented 1 year ago

Can you put a more thorough test plan of sending requests in a before and after code change. Otherwise LGTM!

pmscosta commented 1 year ago

I've tried to create a unit test around the sendEventToCAPIOnly method of EventRequest class, however I couldn't get past this error when decoding the pixel response ({"error":{"message":"Invalid OAuth access token - Cannot parse access token","type":"OAuthException","code":190,"fbtrace_id":"A4Sf-q3hDVN8pI4ktPegAvw"}}).

Nonetheless, we can test the old and new behaviour with something like the following (notice we are not setting the endpointRequest field of the EventRequest):

    final String pixelId = "pixel-id-1";
    final APIContext mockApiContext = mock(APIContext.class);
    final EventRequest eventRequest = new EventRequest(pixelId, mockApiContext);
    final EventResponse reponse = eventRequest.executeAsync().get();

Before this change, this snippet would fail with something like:

Execution java.lang.ClassCastException: com.facebook.ads.sdk.APINode cannot be cast to com.facebook.ads.sdk.AdsPixel

After this code change, we are able to obtain a response.

If you can point me in the direction of providing a valid mock access token in the test I was trying to do, I'll gladly add it as well. Bellow follows my attempt at creating the spec:

Click to open ```java @Test public void sendAsyncToCAPIOnly() throws APIException, ExecutionException, InterruptedException { final String pixelId = "pixel-id-1"; final APIContext mockApiContext = mock(APIContext.class); final EventRequest eventRequest = new EventRequest(pixelId, mockApiContext); final HttpServiceInterface mockClient = mock(HttpServiceInterface.class); final EventResponse mockEventResponse = mock(EventResponse.class); final String accessToken = "access-token-0"; final String appSecretProof = "app-secret-proof-01"; final String expectedUrl = String.format("%s/%s/%s/events", APIConfig.DEFAULT_API_BASE, APIConfig.DEFAULT_API_VERSION, pixelId ); doReturn(accessToken).when(mockApiContext).getAccessToken(); doReturn(expectedUrl).when(mockApiContext).getEndpointBase(); doReturn(true).when(mockApiContext).hasAppSecret(); doReturn(appSecretProof).when(mockApiContext).getAppSecretProof(); doReturn(mockEventResponse).when(mockClient).executeRequest( ArgumentMatchers.any(), ArgumentMatchers.any(), ArgumentMatchers.>any(), ArgumentMatchers.any() ); eventRequest.setHttpServiceClient(mockClient); final Event testEvent1 = new Event(); testEvent1.setEventId("1"); testEvent1.setEventName("testEvent1"); eventRequest.addDataItem(testEvent1); // All the response fields are set to none due to an error with the access token // {"error":{"message":"Invalid OAuth access token - Cannot parse access token","type":"OAuthException","code":190,"fbtrace_id":"A4Sf-q3hDVN8pI4ktPegAvw"}} // This error can be seen by adding a `System.out.println(pixel.getRawResponse());` to the EventRequest.sendEventToCAPIOnly method final EventResponse reponse = eventRequest.executeAsync().get(); assertEquals(response.getEventsReceived(), new Integer(1)); } ```
facebook-github-bot commented 1 year ago

@stcheng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

stcheng commented 1 year ago

@pmscosta could you rename the variable name from pixel to node to match the type change?

pmscosta commented 1 year ago

@pmscosta could you rename the variable name from pixel to node to match the type change?

Yes, done in https://github.com/facebook/facebook-java-business-sdk/pull/416/commits/6fc71c732334ced338086b4c69086cd596709f5f.

facebook-github-bot commented 1 year ago

@stcheng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot commented 1 year ago

@stcheng merged this pull request in facebook/facebook-java-business-sdk@8d205f90b846168ba1e41a9d74482795e6a7d566.