search

ringcentral / ringcentral-python

RingCentral Connect Platform Python SDK
MIT License
45 stars 35 forks source link

/restapi/v1.0 is prepended to SCIM urls #13

Closed erfaan closed 6 years ago

erfaan commented 6 years ago

When I try to fetch Users list via SCIM URI using:

platform.get("/scim/v2/Users")

It is automatically converted to https://platform.devtest.ringcentral.com/restapi/v1.0/scim/v2/Users

i.e. /restapi/v1.0 prepended to the URL.

and results in "Invalid URI" exception.

kirill-konshin commented 6 years ago

This SDK is intended to be used with RingCentral, that's why all URLs are prefixed. Are you using it with a different backend?

grokify commented 6 years ago

The RingCentral SCIM API in beta and is rooted at /scim/v2. For example:

https://platform.devtest.ringcentral.com/scim/v2/Users

The SCIM API is in the API Reference and accessible via Postman, both shown below.

A generic way to support this may be to prepend restapi/v1.0 only with partial URLs without a leading slash? For example:

API Reference:

https://developer.ringcentral.com/api-docs/latest/index.html#!#RefScim.html

scim_api_reference

Postman:

scim_postman2
kirill-konshin commented 6 years ago

I don't think that it's a good idea to change logic of slash handling. Instead, I can add a /scim/ prefix handler.

grokify commented 6 years ago

I like the idea of changing the slash handling logic because it's a more generic and easily understandable approach where a leading slash means a base path. This is how the ringcentral_sdk Ruby gem is implemented.

That being said, it is a breaking change here, so special /scim/ handling may be the way to go for a minor release.

kirill-konshin commented 6 years ago

It's a breaking change, so it's a no go. Unless we want to release a new major version. But for such change major version is a bit too much.

grokify commented 6 years ago

Yeah, let's stay at a minor version update for now.

We can considering changing for the next major upgrade. The benefit will be that the SDK will be ready to support future endpoints like this, but it does mean changing URLs.

tylerlong commented 6 years ago

Hi, for those who are stuck on this. Please try https://github.com/tylerlong/ringcentral-python. It doesn't prepend anything to the url you specified. It is unofficial. And I only recommend it as a temporary workaround before the official SDK fix this issue.

grokify commented 6 years ago

Given the need to support the https://platform.ringcentral.com/rcvideo/v1 endpoint now per the following pull request, I think we should move to a major version release and generically fix this issue. It will be a breaking change, but it will ensure the SDK generically works for upcoming APIs.

https://github.com/ringcentral/ringcentral-js/pull/95

grokify commented 6 years ago

Here is another proposal presented for this:

This approach accomplishes two goals:

Expected behavior allows us to use the following principles:

The following path parts exist under /restapi/v1.0 today:

Of note, we also have the following endpoints:

kirill-konshin commented 6 years ago

Nice proposal, but it's not safe. If a new endpoint will be introduced and used under /restapi/v1.0 then behavior will be inconsistent. Imagine the astonishment in this case. Since /restapi/v1.0 for now is the main entry point I think we should be very careful.

grokify commented 6 years ago

Both of the proposals that provide special processing for specific URL paths are less ideal because when new affected APIs are released, a new SDK will need to be released with a code change and/or documentation change. We rebuild statically-typed language SDKs for each release, so either approach would also require this SDK to be reviewed per-release. The advantage of allowing a leading slash to default to root is that it is DWIM (Do What I Mean) and more POLA (Principle of Least Astonishment) because of that.

That being said, the simplest, cleanest, and most easily-understood solution to avoid a major version release may be to add a SDK constructor and/or request parameter that simply disables URL-building magic (similar to how a leading slash often behaves). This won't impact back-compat since it's a new, optional parameter. This solution is more maintainable and easier to understand because there's no need to keep track of URL paths for special treatment per release, in the code and/or documentation.

grokify commented 6 years ago

Regarding a major version release, I think releasing a major version bump is a good idea if we intend to treat this as a production GA SDK, which there seems to be some desire to do here. This will also result in the cleanest implementation.

The original stated goal of keeping this a sub-1.0 release was to keep flexibility for making breaking changes. If this is no longer the case and we want to treat this as a production GA release, we should bump the version to 1.0 to communicate this, at which time we can also introduce the leading slash at root breaking change behavior.

The advantages of making a breaking change and bumping the major version are:

For a 1.0 release, we should also add Authorization Code flow support websites and bot apps as well as to be inline with other official RingCentral SDKs as described here:

https://github.com/ringcentral/ringcentral-python/issues/14

I don't think we need many other changes for a major version bump to 1.0 because this is really to communicate moving from beta to GA. We've had this SDK available for a while and if we want to treat it as GA, we should communicate it as such.

kirill-konshin commented 6 years ago

Released 0.7.8 with the quick fix. Now since the issue is fixed we can discuss what else we want in 1.0.0 or any other update. Developers are now unblocked and can start using the feature, no impact on existing users.