search

RestComm / Restcomm-Connect

The Open Source Cloud Communications Platform
http://www.restcomm.com/
GNU Affero General Public License v3.0
244 stars 215 forks source link

Usage Records API isn't working well #2302

Open mithileshkarnati opened 7 years ago

mithileshkarnati commented 7 years ago

When we execute the general GET request to the API, curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json

Its working fine but when we use Filters like curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json?Category=calls

Its returning a server error, HTTP Status 500 - No enum constant org.restcomm.connect.dao.entities.Usage.Category.calls

This problem is getting fixed when we use curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json?Category=CALLS

But when we use a similar command like curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json?Category=SMS

We are getting a response whose, "category": "calls" BUT "uri": "/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json?Category=sms&StartDate=2017-06-29&EndDate=2017-06-30"

I think a "category": "sms", if present, should be returned

scottbarstow commented 7 years ago

Following on to this, the responses are a bit of a mess. It appears that the returned category is always calls, but the data is not for calls. Also, the documentation is not right on the public site. The category documentation says to use the lower case enum values, but you have to use the actual ENUM values to get it to work.

scottbarstow commented 7 years ago

Also, if you don't specify a category it only returns the calls category.

FerUy commented 7 years ago

@mithileshkarnati @scottbarstow we definitely have some problems with this. Anyway, I wanted to point out that, as for latest Restcomm-Connect release, on the lowercase issue, if instead of executing:

curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json?Category=calls

you issue the command like this:

curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json -d "Category=calls"

you don't get the HTTP Status 500 response, but the expected answer, for example:

[ { "category": "calls", "description": "Total Calls", "account_sid": "ACae6e420f425248d6a26948c17a9e2acf", "start_date": "2017-10-11", "end_date": "2017-10-12", "usage": "0", "usage_unit": "minutes", "count": "9", "count_unit": "calls", "price": "0.0", "price_unit": "USD", "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls&StartDate=2017-10-11&EndDate=2017-10-12" } ]

The problem is that if you issue the same command but with sms instead of calls as value for "Category", you get the same result.

So, we have two issues here: 1) Documentation must be improved 2) The returned data is always for calls, even if Category is set to sms

scottbarstow commented 7 years ago

@FerUy It's actually a combination of things. Yes, the category always says calls, but the data changes based on category. It appears the data is correct, but the category label is wrong.

And, as I said above, if you do not specify category, only calls data is returned. I tried a number of options and in many cases the results were unexpected / wrong.

FerUy commented 7 years ago

@scottbarstow true, we are in the same page... today you'll have more news on this as part of current research.

FerUy commented 7 years ago

For the categories issue, the problem is that the following:

String categoryStr = info.getQueryParameters().getFirst("Category");

within protected Response getUsage(final String accountSid, final String subresource, UriInfo info, final MediaType responseType) method in UsageEndpoint.java, always sets categoryStr to null.

Then, following Usage.Category category = categoryStr != null ? Usage.Category.valueOf(categoryStr) : null; sets category to null, making following queries treat "Category" as not provided in the GET command (this also applies to "StartDate" and "EndDate" curl parameters)

Then, we need to fix that in the usage endpoint.

FerUy commented 7 years ago

@scottbarstow commit 5cd90d2 solves the problem @mithileshkarnati reported in the first place. For example, after applying this very simple patch, and executing the following commands (having only records for calls and 0 for SMS), the results are the following:

For calls or CALLS (works for XML also, only showing json executions here):

curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b9924eaa7e@192.168.1.42:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls

[
  {
    "category": "calls",
    "description": "Total Calls",
    "account_sid": "ACae6e420f425248d6a26948c17a9e2acf",
    "start_date": "2017-10-13",
    "end_date": "2017-10-14",
    "usage": "0",
    "usage_unit": "minutes",
    "count": "3",
    "count_unit": "calls",
    "price": "0.0",
    "price_unit": "USD",
    "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls&StartDate=2017-10-13&EndDate=2017-10-14"
  }
]
curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b924eaa7e@192.168.1.42:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=CALLS
[
  {
    "category": "calls",
    "description": "Total Calls",
    "account_sid": "ACae6e420f425248d6a26948c17a9e2acf",
    "start_date": "2017-10-13",
    "end_date": "2017-10-14",
    "usage": "0",
    "usage_unit": "minutes",
    "count": "3",
    "count_unit": "calls",
    "price": "0.0",
    "price_unit": "USD",
    "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls&StartDate=2017-10-13&EndDate=2017-10-14"
  }
]

For SMS or sms (in this case showing XML, works for json too):

curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b9924eaa7e@192.168.1.42:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily?Category=sms<RestcommResponse>
  <UsageRecords/>
</RestcommResponse>
curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b9924eaa7e@192.168.1.42:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily?Category=SMS
<RestcommResponse>
  <UsageRecords/>
</RestcommResponse>

We still need to enhance the documentation of the API, but strictly for the issue reported by @mithileshkarnati, this simple patch solves it. One warning (and should be part of the documentation, is that as how the Usage Records API is implemented, issuing the command in the following way will not work now (for this we should refurbish the API current implementation quite a bit):

curl -X GET https://AC13b4372c92ed5c07d51cf842e2664ff:7bec2769d3b48e9132e596b60a558d65@cloud.restcomm.com/restcomm/2012-04-24/Accounts/AC13b4372c92ed5c07d951cf842e2664ff/Usage/Records/Daily.json -d "Category=calls"

So, if you agree I can make the pull request for fixing the issue reported here only with previous commit, and proceed to enhance the documentation.

scottbarstow commented 7 years ago

@FerUy Does this change fix the issue with other categories still displaying "Calls" in the response? Seems like it would but just confirming.

And what about the issue with not specifying a category. Do we now get all categories? Not sure what's expected there.

FerUy commented 7 years ago

Hi @scottbarstow

Does this change fix the issue with other categories still displaying "Calls" in the response?

Yes, for example (works identical whichever you use uppercase or lowercase for sms/SMS):

curl -X GET http://ACae6e420f425248d6a26948c17a9e2a1a1cf3b9924eaa7e@192.168.1.49:8080/restcomm/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily?Category=sms
<RestcommResponse>
  <UsageRecords/>
</RestcommResponse>

And what about the issue with not specifying a category. Do we now get all categories?

Good catch, reason for b421efc. Now if you don't specify the category it works identical to what @mithileshkarnati reported in the first place, i.e. "fine". Anyway, again, this simple fix focused on reported concerns between calls/sms category (or none) and gives a quick fix for just that. The Usage Records API, IMHO, should be enhanced/refurbished.

scottbarstow commented 7 years ago

Agree that it needs more work, but if this fixes the immediate stuff.

FerUy commented 7 years ago

Right @scottbarstow, it does. After assessing the current state of the API, I focused in fixing the urgent problem reported initially by @mithileshkarnati, as otherwise it would take more time for both implementing the complete refurbishing as well as posterior peer reviewing.

FerUy commented 7 years ago

Does this change fix the issue with other categories still displaying "Calls" in the response?

@scottbarstow my bad for previous answer, I initially misunderstood your question and tested incorrectly (on the rush). On a further thought and reviewing this, I generated not only calls but messages (SMS) and these are a couple of responses when querying calls and sms (either in lowercase or uppercase, same result)...

1) Retrieving usage records with Category set to calls (same result for no category defined):

curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b9924eaa7e@192.168.1.49:8080/restcomm/2012-04-24Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls
[
  {
    "category": "calls",
    "description": "Total Calls",
    "account_sid": "ACae6e420f425248d6a26948c17a9e2acf",
    "start_date": "2017-10-14",
    "end_date": "2017-10-15",
    "usage": "0",
    "usage_unit": "minutes",
    "count": "4",
    "count_unit": "calls",
    "price": "0.0",
    "price_unit": "USD",
    "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls&StartDate=2017-10-14&EndDate=2017-10-15"
  },
  {
    "category": "calls",
    "description": "Total Calls",
    "account_sid": "ACae6e420f425248d6a26948c17a9e2acf",
    "start_date": "2017-10-15",
    "end_date": "2017-10-16",
    "usage": "0",
    "usage_unit": "minutes",
    "count": "9",
    "count_unit": "calls",
    "price": "0.0",
    "price_unit": "USD",
    "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=calls&StartDate=2017-10-15&EndDate=2017-10-16"
  }
]

Which is consistent with what really happened (I really carried out 13 calls):

image

2) Retrieving usage records with Category set to sms

curl -X GET http://ACae6e420f425248d6a26948c17a9e2acf:f8bc1274677b173d1a1cf3b9924eaa7e@192.168.1.49:8080/restcomm/2012-04-24Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=sms
[
  {
    "category": "calls",
    "description": "Total Calls",
    "account_sid": "ACae6e420f425248d6a26948c17a9e2acf",
    "start_date": "2017-10-15",
    "end_date": "2017-10-16",
    "usage": "2",
    "usage_unit": "minutes",
    "count": "2",
    "count_unit": "calls",
    "price": "0.0",
    "price_unit": "USD",
    "uri": "/2012-04-24/Accounts/ACae6e420f425248d6a26948c17a9e2acf/Usage/Records/Daily.json?Category=sms&StartDate=2017-10-15&EndDate=2017-10-16"
  }
]

Also consistent with the fact that I really generated 2 messages (SMS):

image

But, we are still displaying "calls" in the response for the "category" field when retrieving SMS records.

So, the answer to the question is, not yet. Reported HTTP Status 500 errors are gone, when category is set to a value either in uppercase or lowercase it returns the answer corresponding to that category, but category is always set to calls in the response, even if it corresponds to messages records, which is obviously not correct. Likewise, when not setting the category in the query, it returns the same as if it was set to calls. In other words, we are in better shape than before, we are discriminating the responses accordingly to the category, but still not good enough. I will withdraw pull request #2578 for now.