deepgram / deepgram-js-sdk

Official JavaScript SDK for Deepgram's automated speech recognition APIs.
https://developers.deepgram.com
MIT License
127 stars 45 forks source link

Allow non-encrypted requests #220

Closed andreroggeri closed 5 months ago

andreroggeri commented 6 months ago

Proposed changes

The v2 allows unencrypted requests by setting a parameter on the constructor and the v3 removed that capability.

Context

I have an on-prem instance running on a local network that doesn't have SSL certificates.

Possible Implementation

Accept a boolean in the createClient function that allows using http/https/ws/wss based on that flag.

const deepgram = createClient('my-key', { global: { useSsl: false, url: 'my-host.com' } })
lukeocodes commented 6 months ago

Have you tried using a http URL in the app config?

andreroggeri commented 6 months ago

Yes, but the AbstractClient enforces that (And some other places as well)

lukeocodes commented 6 months ago

When you put in your on-prem host into the new config, can you tell me what error you get?

andreroggeri commented 6 months ago
DeepgramUnknownError: fetch failed
    at PrerecordedClient.<anonymous> (/Users/me/my-script/node_modules/@deepgram/sdk/src/packages/AbstractRestfulClient.ts:41:14)
    at Generator.next (<anonymous>)
    at fulfilled (/Users/me/my-script/node_modules/@deepgram/sdk/dist/main/packages/AbstractRestfulClient.js:5:58)
    at processTicksAndRejections (node:internal/process/task_queues:95:5) {
  __dgError: true,
  originalError: TypeError: fetch failed
      at Object.fetch (node:internal/deps/undici/undici:11576:11)
      at processTicksAndRejections (node:internal/process/task_queues:95:5) {
    cause: [Error: C09E48E601000000:error:0A00010B:SSL routines:ssl3_get_record:wrong version number:../deps/openssl/openssl/ssl/record/ssl3_record.c:355:
    ] {
      library: 'SSL routines',
      reason: 'wrong version number',
      code: 'ERR_SSL_WRONG_VERSION_NUMBER'
    }
  }
}
lukeocodes commented 6 months ago

Thanks, I'll take a look at this ASAP

lukeocodes commented 6 months ago

I'm struggling to reproduce this. I can make both client and server requests to a http server (assuming the client is also http). I can also make https requests to a https address.

I am using a mock server while I wait for an on-prem instance. But I can't imagine why a request to one http server would struggle and not another.

Can you give me more information about the type of app you're building?

andreroggeri commented 6 months ago

Oh, so if you provide the URL with the protocol (http/https), the current implementation will honor that. But if you only provide the host, it will assume that it should use SSL.

import { createClient } from "@deepgram/sdk";

const deepgramHost = "8.8.8.8"; // Put http:// in front to fix the ssl error
const deepgramApiKey = "fake-key";

const deepgram = createClient(deepgramApiKey, {
  global: { url: deepgramHost },
});

(async () => {
  const result = await deepgram.listen.prerecorded.transcribeUrl({
    url: "http://google.com",
  });
  console.log(result);
})();

The previous version would work just fine with the host. That's why I was confused when upgrading 😅

lukeocodes commented 6 months ago

Ahh yes, this is true. This is a side effect of the URL being stored as a JS URL object. It always defaults to https: for the protocol (as it should).

I think it is right to require the entire hostname, as it is actually clearer than additional options that modify it. Our docs also show the usage of the entire hostname.

We wouldn't downgrade the protocol from one provided.

This is just (IMO) a better way to handle custom domains.

andreroggeri commented 6 months ago

Are you proposing to make the protocol (HTTP/HTTPS) required for the url parameter? If that's the case, I think it's great because the behavior will be explicit.

lukeocodes commented 6 months ago

Are you proposing to make the protocol (HTTP/HTTPS) required for the url parameter? If that's the case, I think it's great because the behavior will be explicit.

Yes! That is how we have built it. The URL class is internal to JS, and will add a protocol if one isn't provided. It might be that we need to call this out more explicitly in the docs