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

JSON responses inconsistent in terms of uri prefix #1723

Closed
otsakir opened this Issue Jan 16, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@otsakir
Contributor

otsakir commented Jan 16, 2017

When using the API to retrieve an Account the uri fields is inconsistent with the subresource_uris fields as the path they contain starts from a different level.

uri:

"uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf"

subresource_uris all start with:

"/restcomm/2012-04-24/Accounts/...

The same applies to Calls API and potentially to all remaining rest apis.

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 17, 2017

@otsakir Looks like the rooturi is also present in Gateways endpoint
imo, the Account subresource_uris are all incorrectly implemented and documented, i assume it should all be relative to root uri eg http://host/restcomm/
additionally Twilio's subresourceuri is consistent
https://www.twilio.com/docs/api/rest/account#output
The fix seems to be removing all the append(rootUri) calls where appropriate (though i might be wrong, again!)

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 18, 2017

Hmmm, conceptually, /restcomm prefix of the url is more a part of the web application. In order to change it one should rename the application and not reconfigure the way the responses are generated. I think your thesis is right @abdulazizali77. Twilio confirms it too.

All uris should be relative to .../restcomm

@deruelle , @gvagenas any objections here ?
@gvagenas do you have in mind functionality that will break if we strip the '/restcomm' part form the subresource_uris? I think inside restcomm we don't really use them so far. We should be ok to refactor.

@abdulazizali77 apart from Accounts, Calls, Gateways have you checked other APIs too?

@deruelle

This comment has been minimized.

Member

deruelle commented Jan 19, 2017

@otsakir @abdulazizali77 agreed. no objections on my side

@abdulazizali77

This comment has been minimized.

Contributor

abdulazizali77 commented Jan 23, 2017

apologies for the late update
@otsakir It seems like a lot more APIs incorrectly use the root-uri, most of them are simple fixes
Accounts
Accounts OutgoingCallerIds
Applications
Calls
Clients
Conferences
ConferenceParticipants
IncomingPhoneNumbers
SMSMessages
GatewaysManagement

Theres few places in the *Convertor classes which call getRequestURI().getPath() which incorrectly return the root uri, we have 3 options to fix this

  1. Relativize the UriInfo to root-uri
  2. Catenate strings
    2a) "/" + getAPiVersion() + "/" +info.getPath() - this seems to be how most EPs do it, with StringBuffer
    2b) request.getSevletPath() + "/" + info.getPath()

Ill try put a PR tonight highly likely with option 2a

@otsakir

This comment has been minimized.

Contributor

otsakir commented Jan 24, 2017

np @abdulazizali77 .

Thanks for the thorough investigation on the matter.

abdulazizali77 added a commit to abdulazizali77/Restcomm-Connect that referenced this issue Jan 24, 2017

@otsakir otsakir added this to the 8.1.0 milestone Jan 26, 2017

@otsakir otsakir added the tech_debt label Jan 26, 2017

abdulazizali77 added a commit to abdulazizali77/Restcomm-Connect that referenced this issue Jan 26, 2017

otsakir added a commit that referenced this issue Jan 30, 2017

gvagenas added a commit that referenced this issue Feb 2, 2017

@gvagenas

This comment has been minimized.

Collaborator

gvagenas commented Feb 2, 2017

Closed by PR #1735

Thanks for the contribution @abdulazizali77

@gvagenas gvagenas closed this Feb 2, 2017

@gvagenas gvagenas added the highlight label Feb 2, 2017

maria-farooq added a commit that referenced this issue Feb 2, 2017

Merge branch 'master' into zd-33884
* master:
  Remove unnecessary rootUri parameter This refer to #1723
  Fix for init MySQL/MariaDB script
  Issue #1723: Update documentation for endpoints. Remove context path from uris
  Issue #1723: Endpoint uri inconsistencies - remove rootUri from uri and subresource uris

maria-farooq added a commit that referenced this issue Feb 7, 2017

Merge branch 'master' into issue-1572
* master: (63 commits)
  Fix #1763 by changing the response code to 569
  Restcomm testsuite fixes
  Merged with master This refer to #1339
  Added documentation for IMS GW feature. This refer to #1339
  Automatically complete value extractor for ES 'value' properties
  Fix for maven checkstyle plugin
  Hide empty urls from number list in Dashboard
  Added ussd request url to numbers list
  fixed 'Voice Request' typo in Dashboard
  added exception msg in warn log
  changed logging level of AskTimeoutException while waiting for outbound call info
  Remove unnecessary rootUri parameter This refer to #1723
  Fix for init MySQL/MariaDB script
  Refurbished testsuite for having each HTTP test scenario into different methods as requested. Issue #637, PR #1717
  #1752 Fixing zipping of Media Server files during RC build.
  HTTP response from GMLC read via the response entity. Issue #637, PR #1717
  Fixes for testsuite with mocked GMLC stub, all tests passing again now. Issue #637, PR #1717
  Added Wiremock to mock GMLC for Geolocation testsuite. This refer to #637
  #1752 Made MS_HOME/.autoconfig scripts executable.
  Fixes for auto-config scripts
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment