NotificationFilterUrlMonetaryAccount create responds with incorrect data, doesn't match spec, create fails

timvisee commented 4 years ago

It appears there's an API specification and server response inconsistency.

The API specification clearly states that NotificationFilterUrlMonetaryAccount::create should respond with an Id object. Instead, the server responds with a list of NotificationFilterUrl objects.

Steps to reproduce:

Set a monetary account ID in $id.

$filters = [
    new NotificationFilterUrl('PAYMENT', 'https://test'),
    new NotificationFilterUrl('BUNQME_TAB', 'https://test'),
];
NotificationFilterUrlMonetaryAccount::create($id, $filters);

What should happen:

Filters should be configured.
API client should respond successfully.

What happens:

API client throws an error, an Id is not found. See 'Extra info'.

Traceback

Symfony\Component\Debug\Exception\FatalThrowableError thrown with message "Return value of bunq\Model\Core\Id::getId() must be of the type int, null returned"

Stacktrace:
#76 Symfony\Component\Debug\Exception\FatalThrowableError in /home/timvisee/git/barbapappa/webapp/vendor/bunq/sdk_php/src/Model/Core/Id.php:18
#75 bunq\Model\Core\Id:getId in /home/timvisee/git/barbapappa/webapp/vendor/bunq/sdk_php/src/Model/Core/BunqModel.php:346
#74 bunq\Model\Core\BunqModel:processForId in /home/timvisee/git/barbapappa/webapp/vendor/bunq/sdk_php/src/Model/Generated/Endpoint/NotificationFilterUrlMonetaryAccount.php:85
#73 bunq\Model\Generated\Endpoint\NotificationFilterUrlMonetaryAccount:create in /home/timvisee/git/barbapappa/webapp/app/Models/BunqAccount.php:206
#72 App\Models\BunqAccount:updateBunqAccountSettings in /home/timvisee/git/barbapappa/webapp/app/Http/Controllers/BunqAccountController.php:260
#71 App\Http\Controllers\BunqAccountController:doHousekeep in /home/timvisee/git/barbapappa/webapp/vendor/laravel/framework/src/Illuminate/Routing/Controller.php:54

# -- snip --

SDK version and environment

Tested on 1.12.1 (PHP)
[x] Sandbox
[x] Production

Extra info:

Here is a prettified JSON response, that is received from the server when invoking the create function:

{
  "Response":[
    {"NotificationFilterUrl":{
      "id":76401046,
      "created":"2019-11-11 13:58:17.057469",
      "updated":"2019-11-11 13:58:17.057469",
      "category":"PAYMENT",
      "notification_target":"https:\/\/test"
    }},
    {"NotificationFilterUrl":{
      "id":76401047,
      "created":"2019-11-11 13:58:17.061249",
      "updated":"2019-11-11 13:58:17.061249",
      "category":"BUNQME_TAB",
      "notification_target":"https:\/\/test"
    }}
  ]
}

This clearly shows the API responds with NotificationFilterUrl objects, and not an Id object.

Providing a different number of filters doesn't improve things.

It is not clear to me what the response Id should represent.

I see two possible solutions for this:

The API should respond with an Id object, as shown in the specification.
The specification should be updated, and the PHP SDK (and others) must be patched.

timvisee commented 4 years ago

Seems related to #177, though it was marked as fixed.

timvisee commented 4 years ago

You can wrap this in a try/catch block as temporary workaround (in PHP 7+). The callback URLs are set on the server, just the response is faulty.

I do not recommend this approach in a production environment, because this will ignore real errors as well. But yes, this works:

try {
    NotificationFilterUrlMonetaryAccount::create($id, $filters);
} catch(\Error $e) {}

sirajea commented 4 years ago

Have a look at the readme, it says:

Note! Due to an in internal change in the way we handle NotificationFilters (Callbacks), you should not use the default classes included in this SDK. Please make sure you make use of the associated Internal-classes. For example when you need NotificationFilterUrlUser, make use of NotificationFilterUrlUserInternal. You can use every method of these classes, except for the create() method. Always use createWithListResponse() instead.

So it works as long as you use NotificationFilterUrlMonetaryAccountInternal::createWithListResponse() instead of NotificationFilterUrlMonetaryAccount::create() because createWithListResponse() does have the correct return type.

timvisee commented 4 years ago

Thanks a bunch for mentioning this! This different approach using these internal objects does work fine indeed.

But I think it's weird the regular endpoint object doesn't work, even if you'd consider this to be because of 'compatibility purposes', because the actual implementation is broken. Therefore I'll leave this open.

hurnell commented 3 years ago

NotificationFilterUrlMonetaryAccountInternal::createWithListResponse returns an empty result set as does NotificationFilterUrlMonetaryAccountInternal::listing: ‌...... 'notificationFilters' => NULL, 'notificationFiltersFieldForRequest' => NULL,

createWithListResponse does create the notification correctly but I cannot check if it has been made using sdk_php.

I am able to check using the direct api https://public-api.sandbox.bunq.com/v1/user/xxxxxx/monetary-account/xxxxxx/notification-filter-url or production equivalent.

Seems to get to Guzzle ok but the response is wrong!

dqos commented 2 years ago

Ugh, is this still not fixed? I am unable to see my notifications or callbacks whatever bunq calls them. It would be convenient to offer a way to manage these "webhooks" through a web interface, like Wise, Mollie, Stripe etc does.

@hurnell how did you create a call to this direct API? Because I need to see if I created a notification or not :P

bunq / sdk_php