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

Introduce a generic method to query unsupported endpoints. #107

Open
georgestephanis opened this issue Feb 6, 2024 · 5 comments
Open

Introduce a generic method to query unsupported endpoints. #107

georgestephanis opened this issue Feb 6, 2024 · 5 comments

Comments

@georgestephanis
Copy link

There are a number of documented endpoints that are not able to be queried through the sift-php classes presently, due to not having methods to use them.

Such as the Webhooks API, for example: https://sift.com/developers/docs/curl/webhooks-api/overview

Rather than trying to push through methods for everything to handle every endpoint and maintain them as new ones are introduced, it would be useful if there was a generic method available in the SiftClient class to do so, something like --

public function queryEndpoint( $version, $endpoint, $method, $query_args = array() ) {
    $api_url = self::urlPrefix( $version ) . '/' . str_replace( '{accountId}', $this->account_id, $endpoint );
    $endpoint = ltrim( $endpoint, '/' );
    $query_args['auth'] = $this->api_key . ':';

    try {
        $request = new SiftRequest(
            $api_url,
            $method,
            $this->timeout,
            $this->version,
            $query_args
        );
        return $request->send();
    } catch (Exception $e) {
        $this->logError($e->getMessage());
        return null;
    }
}

or something -- I could be off on a couple usages or paradigms, but that feels like it mirrors most of the existing patterns from a quick glance.

I'm unsure if there's other tokens that may need to be able to get str_replaced into the api url for other varied endpoints -- or if it's better to use accessor functions to pull those out of the SiftClient instance and add them in manually prior to invoking.

Once introduced, it could be used something like

$sift = new SiftClient([ 'api_key' => 'my_api_key', 'account_id' => 'my_account_id' ]);
$response = $sift->queryEndpoint( 3, 'accounts/{accountId}/webhooks', 'POST', $data_array );
print_r( $response->body ); 

In short, the big reason for this request is to enable the library to be used for all requests sent to the Sift API, rather than needing to maintain a secondary code path for unsupported endpoints -- at which point it would be easier when building client integrations to ignore this official client library and consolidate around a custom one, which sort of defeats the purpose and benefits of using and supporting an official client, in my opinion.

@viaskal-sift
Copy link
Contributor

@georgestephanis we actually implemented webhooks API support in this PR - https://github.com/SiftScience/sift-php/pull/108/files. Feel free to check it out and leave comments. Thanks!

@georgestephanis
Copy link
Author

Yup, Tyler had let us know. There's other aspects of the API that aren't explicitly included, though, so an ability to have a generic method would be much appreciated, rather than additions to the development cycle of asking for specifics, waiting to hear back, and tons of extra back and forth.

@viaskal-sift
Copy link
Contributor

viaskal-sift commented Feb 23, 2024

@georgestephanis I understand your point, and that could be quite convenient. However, we intentionally keep the lib this way because we would like to control the underlying calls to the Sift API. This will help us to upgrade the version of the API much easier, For example, imagine we need to introduce backward not-compatible changes to some of the endpoints. If people use this API via generic method - it will be quite painful to switch then. But as long as we control the usage in dedicated methods - we can tune the method implementation to handle the change and this doesn't affect client usage itself. Hopefully, that makes sense
You also mentioned:

There's other aspects of the API that aren't explicitly included

Could you pls elaborate a little bit? If you have some other issues (or missing points) we of course can fix that

@georgestephanis
Copy link
Author

georgestephanis commented Feb 23, 2024

If people use this API via generic method - it will be quite painful to switch then. But as long as we control the usage in dedicated methods - we can tune the method implementation to handle the change and this doesn't affect client usage itself.

To an extent, however by not supporting the endpoints, you're driving folks to not use this library at all, and to build their own clients for it, as some other internal teams and other open-source projects have done. This gives even less ability to update and mitigate as the code accessing the documented API endpoints is fully outside of your ecosystem.

Another example that came up the other week was the Fingerprinting API and device labels -- which also isn't supported.

https://developers.sift.com/docs/curl/device-fingerprinting-api/overview


In the end, by not providing a general way to query endpoints, it drives anyone looking to integrate with the Sift API to just build their own methods for firing requests to the endpoint, which give you far less oversight and ability to guide through api changes down the road than including a generic method that could be extended.

Walled gardens on clients for public APIs just incentivize folks to bring ladders or use a different way in.

@viaskal-sift
Copy link
Contributor

Another example that came up the other week was the Fingerprinting API and device labels -- which also isn't supported.

that's true, DFP API is not yet supported, but I wanted to double check if you really would like to leverage it. If yes - we can introduce it in the library as well. So, could you pls confirm this API is something you would like to use exactly from within the library?

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