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

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

Closed
erfaan opened this issue Feb 2, 2018 · 13 comments
Closed

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

erfaan opened this issue Feb 2, 2018 · 13 comments

Comments

@erfaan
Copy link

erfaan commented Feb 2, 2018

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

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

grokify commented Feb 2, 2018

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:

  • platform.get("/scim/v2/Users") becomes https://platform.devtest.ringcentral.com/scim/v2/Users
  • platform.get("account/~") becomes https://platform.devtest.ringcentral.com/restapi/v1.0/account/~

API Reference:

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

scim_api_reference

Postman:

scim_postman2

@kirill-konshin
Copy link
Contributor

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

grokify commented Feb 2, 2018

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

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

grokify commented Feb 3, 2018

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

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

grokify commented Feb 8, 2018

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.

ringcentral/ringcentral-js#95

@grokify
Copy link
Contributor

grokify commented Mar 3, 2018

Here is another proposal presented for this:

  • Allow leading slashes to mean root, except when known /restapi/v1.0 endpoints are used, then prepend /restapi/v1.0.

This approach accomplishes two goals:

  • Supports expected behavior
  • Does not break backward compatibility

Expected behavior allows us to use the following principles:

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

  • account
  • client-info
  • dictionary
  • glip
  • number-parser
  • subscription

Of note, we also have the following endpoints:

  • /restapi/{version}
  • /restapi/v1.0

@kirill-konshin
Copy link
Contributor

kirill-konshin commented Mar 3, 2018

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

grokify commented Mar 4, 2018

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

grokify commented Mar 4, 2018

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:

  • it provides a cleaner DWIM/POLA URL behavior
  • it better communicates the status of this SDK

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:

#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.

@grokify grokify assigned grokify and tylerlong and unassigned grokify Mar 5, 2018
@kirill-konshin
Copy link
Contributor

kirill-konshin commented Mar 5, 2018

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.

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

4 participants