watson-developer-cloud / node-sdk

:comet: Node.js library to access IBM Watson services.
https://www.npmjs.com/package/ibm-watson
Apache License 2.0
1.48k stars 670 forks source link

[speech-to-text] Asynchronous callback API is not exposed in Node.js SDK #402

Closed l2fprod closed 7 years ago

l2fprod commented 7 years ago

The API to send asynchronous STT jobs does not exist in the Node.js SDK: https://www.ibm.com/watson/developercloud/speech-to-text/api/v1/?node#async_methods

It does exist in the Java SDK.

This API is very useful to not have to wait for a job to complete or even to poll the status. Using the callback approach makes this API very useful in a serverless scenario where you will want to be notified by STT of the end of the processing.

adrien2p commented 7 years ago

@nfriedly i could see if i can add a method to add callback url registering according to the documentation. i can see that tomorrow night if you want.

germanattanasio commented 7 years ago

If you are going to work on a PR make sure you link to this issue. Actually, create the pull request and I will mark it as in progress so that we don't work on this while you are.

adrien2p commented 7 years ago

ok i'll create PR to night, and i'll start to work on this tomorrow 👍

l2fprod commented 7 years ago

@germanattanasio @adrien2p

I don't see any API to unregister a callback in https://www.ibm.com/watson/developercloud/doc/speech-to-text/async.shtml#usage. Is there one?

While implementing my first callback I found it useful to be able to register/unregister. Use case: I implemented one that works. Now I change the code, I want to verify the registration still works but for this I need to pick a new URL as I have no way to remove the existing callback or to force the registration again.

jeffpk62 commented 7 years ago

The service interface doesn't have an unregister call. I"m copying @daniel-bolanos to see whether he has some means of achieving this.

daniel-bolanos commented 7 years ago

Hi @l2fprod, yeah, I see what you mean. For the first version of STT Async we decided not to provide such feature since it was not critical. But I think we can add it to the roadmap, deleting callback urls is sometimes convenient.

@jsstylos you ok with that right?

jsstylos commented 7 years ago

@l2fprod For clarification, is it necessary to invalidate the old callback urls, or is it enough to enable the validation of new callback urls? Because these are not long-lived webhook-like listeners, the registration step is used for security purposes, and so there's nothing preventing multiple urls to be registered and used as desired.

daniel-bolanos commented 7 years ago

yeah, what @jsstylos said is correct.

l2fprod commented 7 years ago

@daniel-bolanos @jsstylos

for on of my usecase, it is about invalidating old URLs so that they can be validated again - specially when you iterate on the implementation of the callback as I was doing yesterday. My callback was implemented in a Cloud Foundry nodejs app at first so I ended up using different routes for every try I made. The other use case was testing registration. And if you share the STT instance you want to clean up after your test is complete so an "unregister" would make sense.

BTW I found an issue with the Accept header sent by register_callback when it calls the callback url. STT sends a request with Accept: application/json however for the callback to be registered it needs to return the challenge string as the body, i.e not a JSON object but the plain string. Thus the callback url ends up returning a content that is not application/json. Either STT should send an Accept: text/plain or / or it should just validate that the challenge string appears somewhere in the body, including as a field of a proper JSON object (so that I can return Content-Type: application/json to match the Accept).

daniel-bolanos commented 7 years ago

Hi @l2fprod,

Yeah, we will add a new method to unregister a callback_url, we do not think this is high priority, but a good to have definitely.

Regarding the second issue, yes, that is a bug, we will make it send the right accept header text/plain

adrien2p commented 7 years ago

Hi @daniel-bolanos have you an idea when this feature will be available ? I implementing at this time the asynchronous http interface on the sdk

daniel-bolanos commented 7 years ago

@adrien2p I already added the functionality. We will try to push to prod on the next release within the next few weeks.