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

Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation) #1631

Open
1 task done
mellster2012 opened this issue Apr 26, 2024 · 6 comments
Labels
enhancement New feature or request help wanted Extra attention is needed openapi-fetch Relevant to the openapi-fetch library

Comments

@mellster2012
Copy link

mellster2012 commented Apr 26, 2024

Description
v0.9.3, 0.9.x
Need a way to disable certification validation via a client option, as the fetch function needs to be replaced in order to make setGlobalDispatcher work, and the fetch signature used by openapi differs, passing a single request object.

Proposal
line 39 src/index.js

let {
    baseUrl = "",
    dispatcher = undefined,
    fetch: baseFetch = globalThis.fetch,
    [...]

then check in line 94 in src/index.js for a dispatcher object in the client options and pass it through if present, e.g.
let response = await fetch(request, dispatcher ? { dispatcher } : undefined);
Checklist

@mellster2012 mellster2012 added enhancement New feature or request help wanted Extra attention is needed openapi-fetch Relevant to the openapi-fetch library labels Apr 26, 2024
@mellster2012 mellster2012 changed the title Send init parameter(s) into fetch requests which is currently unused (= undefined) Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation) Apr 26, 2024
mellster2012 added a commit to mellster2012/openapi-typescript that referenced this issue Apr 27, 2024
mellster2012 added a commit to mellster2012/openapi-typescript that referenced this issue May 4, 2024
mellster2012 added a commit to mellster2012/openapi-typescript that referenced this issue May 4, 2024
@drwpow
Copy link
Owner

drwpow commented May 16, 2024

Thanks for opening, but can you describe what a “dispatcher” is? Is this a concept from another library? Why won’t the custom fetch or middleware work? What prior art are you referencing?

I’m not opposed to adding this, but I want to be 100% sure before we commit to adding breaking changes to the API there’s not a simpler solution to solve the problem (and I do not understand the problem)

@mellster2012
Copy link
Author

Thanks for the comments!

  • Previously you could provide an Agent to override default configurations, such as not failing on self-signed certificates. Node's global fetch does not use the http/https stack, but an internally wired undici client and you cannot replace the global agent easily, see here: https://stackoverflow.com/questions/73817412/why-is-the-agent-option-not-available-in-node-native-fetch
  • Replacing fetch with a custom implementation is a much larger change than providing a custom dispatcher - which is an undici specific Agent different from Node's http(s) Agent. Also we've ran into signature issues/incompatibilities when swapping out fetch. So this seems like the simplest solution (as suggested in the linked SO thread above) to pass custom configurations.
  • We already forked openapi-fetch for this, there must be many others with the same need. AFAIK adding an optional dispatcher to the configuration option should not be breaking changes to the (public facing) API, but I could be wrong.

mellster2012 added a commit to mellster2012/openapi-typescript that referenced this issue May 16, 2024
mellster2012 added a commit to mellster2012/openapi-typescript that referenced this issue May 16, 2024
…o disable certificate validation) drwpow#1631

rebase from upstream/main
@mellster2012
Copy link
Author

mellster2012 commented May 16, 2024

There actually seems to be an option to modify the globalFetch Dispatcher:
globalThis[Symbol.for('undici.globalDispatcher.1')] = yourDispatcher;
Will verify this. You can just reject the reworked (according to your comments) PR at (#1636) if you think it's not useful enough, but the above is certainly a bit of a hack and I would prefer the option to inject a dispatcher per client.

@mellster2012
Copy link
Author

The above doesn't work, neither does setGlobalDispatcher, so we will still rely on our fork in the meantime. Note that you can also pass a dispatcher as init parameter to fetch, however openapi-fetch does discard this additional parameter here:
let response = await fetch(request); with

ObjectDefineProperty(globalThis, 'fetch', {
    __proto__: null,
    configurable: true,
    enumerable: true,
    set,
    get() {
      function fetch(input, init = undefined) {
        // Loading undici alone lead to promises which breaks lots of tests so we
        // have to load it really lazily for now.
        const { fetch: impl } = require('internal/deps/undici/undici');
        return impl(input, init);
      }
      set(fetch);
      return fetch;
    },
  });

where init can take a dispatcher object.

@mellster2012
Copy link
Author

Specifying a custom fetch is tricky as the globalFetch does not use undici's exposed fetch method, so you have to do what node does and require the internal dependencies which necessitates the --expose-internals flag (or write it from scratch):

function fetch(input: any, init = { dispatcher }) {
  const { fetch: impl } = require('internal/deps/undici/undici');
  return impl(input, init);
}

@mellster2012
Copy link
Author

Ok so this does work, still not really clean, but using this override plus ts-expect-error providing a custom fetch is sufficient.

function myFetch(input: RequestInfo) {
  // @ts-expect-error 'dispatcher does not exist in type RequestInit'
  return globalThis.fetch(input, { dispatcher });
}

Feel free to close this and the related PR at (#1636), or modify the proposed changes as you see fit. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed openapi-fetch Relevant to the openapi-fetch library
Projects
None yet
Development

No branches or pull requests

2 participants