[2.0] Allow dynamic configuration of data sources

[martijnwalraven]

This PR adds callbacks to RESTDataSource that allow for asynchronous and dynamic configuration of data sources. It addresses #1181 and #1238.

It allows a data source to specify baseURL, defaults and defaultParams either as simple properties, properties that return a Promise, or as a function that takes RequestOptions and returns either a value or a Promise.

So you could implement the example from #1181 (comment) as follows:

class FooAPI extends RESTDataSource {
  baseURL = (options: RequestOptions) => {
    return new Promise<string>((resolve, reject) => {
      dns.resolveSrv(options.path.split("/")[1] + ".service.consul", (err, addresses) => {
        err ? reject(err) : resolve("http://" + addresses[0].name + ":" + addresses[0].port);
      });
    });
  }

  getFoo() {
    return this.get('foo');
  }
}

And you can specify default params as discussed in #1238:

class FooAPI extends RESTDataSource {
  baseURL = 'https://api.example.com';

  defaultParams = () => ({
    api_key: this.context.token,
  });

  getFoo(a) {
    return this.get('foo', { a });
  }
}

So getFoo(1) would result in a request to 'https://api.example.com/foo?api_key=secret&a=1'.

I also added a defaults callback, which allows you to specify default fetch options:

class FooAPI extends RESTDataSource {
  baseURL = 'https://api.example.com';
  defaults = { credentials: 'include' };
}

One worry is that there is overlap between defaults and willSendRequest. You could use both to set headers for example:

class FooAPI extends RESTDataSource {
  baseURL = 'https://api.example.com';
  defaults = () => ({
    headers: { 'Authorization': this.context.token }
  });
}

vs:

class FooAPI extends RESTDataSource {
  baseURL = 'https://api.example.com';

  willSendRequest(request: Request) {
    request.headers.set('Authorization', this.context.token);
  }

  getFoo() {
    return this.get('foo');
  }
}

It seems this could get confusing.

Also, I think ideally we could get rid of defaultParams and rely on willSendRequest for this as well, but unfortunately request.url is a read-only string, not a URL. So you can't do:

class FooAPI extends RESTDataSource {
  baseURL = 'https://api.example.com';

  willSendRequest(request: Request) {
    if (request.method === 'GET') {
      request.url.searchParams.set('api_key', this.context.token);
    } else {
      request.headers.set('Authorization', this.context.token);
    }
  }

  getFoo() {
    return this.get('foo');
  }
}

One option would be to pass in RequestInit and URL to willSendRequest instead of Request, but that also feels awkward.

[danilobuerger]

I personally would get rid of defaults and defaultParams in favor of willSendRequest and accept the awkwardness vs having to explain to users that there are different ways to do the same thing and which one is better and so on.

[martijnwalraven]

@danilobuerger I agree with your feedback and simplified the API. I removed defaults and defaultParams, and changed willSendRequest to take RequestOptions instead of Request.

willSendRequest is now called before baseURL and has access to path and params. I also changed RequestOptions to include params as a URLSearchParams and headers as a Headers instance.

That means you can now do:

  willSendRequest(request: RequestOptions) {
    if (request.method === 'GET') {
      request.params.set('api_key', this.context.token);
    } else {
      request.headers.set('Authorization', this.context.token);
    }
  }

What do you think?

I like allowing baseURL to return a Promise to allow for async resolving, but I'm wondering if we really need the complexity of allowing baseURL to be a function that takes method, path and other RequestOptions. Do you have a real world use case for this in mind?

[danilobuerger]

You are probably right, I can't think of a good one. For me I would just use a datasource per microservice thus not needing to access the path. Promise might be enough!

[martijnwalraven]

@danilobuerger I'm still bike shedding over this. It seems allowing baseURL to return a Promise may get a little awkward. There is no such thing as an async getter in JavaScript, so if you want to await other calls you'll have to wrap it in an immediately invoking async function. Moreover, by default it would resolve the base URL for every request, and you'd have to cache it manually to avoid this.

I was thinking a more straightforward solution might be to introduce a resolveURL method that takes a RequestOptions. That method would then be called after willSendRequest (with the same options object), and has the opportunity to asynchronously resolve the URL in any way that is needed.

The default implementation simply creates a new URL from path and baseURL, so you could still invoke that as part of your implementation.

I think that would turn your original example into:

async resolveURL(request: RequestOptions) {
  if (!this.baseURL) {
    const addresses = await resolveSrv(request.path.split("/")[1] + ".service.consul");
    this.baseURL = addresses[0];
  }
  return super.resolveURL(request);
}

But you could also do something more complicated and circumvent the superclass implementation completely. Not sure if this makes sense, but you could store the complete list ofaddresses for example, and implement failover in that way.

Maybe I'm overcomplicating this. What do you think?

[Saeris]

I think that change to willSendRequest will work just fine. I agree with you that it is a bit clunkier, but ultimately I think this makes the method more powerful overall. Particularly in that you can use it to debug exactly what will be sent at a more granular and controllable level.

I don't see an issue with baseURL returning a promise. If a niche use case arises, this will give people more power than what a simple string value provides. One possible scenario could be that someone might want to use a single DataSource class to handle all of their REST calls, determining a baseURL dynamically based on the endpoint being called.

Edit: Going back to willSendRequest, with the example I gave above, you could then also apply different default params based on the endpoint being called (say one baseURL requires an api key as a param, another as a header). Sounds contrived, but totally a possibility.

Also read the new replies, and I think I understand the concerns you have about baseURL being a promise. Computing the URL on every request does seem excessive, especially if it involves another tick of the event loop. I don't know what the best solution for this would be.

[martijnwalraven]

@Saeris The issue I saw with allowing baseURL to return a promise is that there isn't a way to define an async getter, and calling other async functions may get awkward.

Setting baseURL to a function could be an alternative, but on second thought it seemed cleaner to extract this into a separate resolveURL method. That also give you control over caching of base URL resolving.

So you would do:

async resolveURL(request: RequestOptions) {
  if (!this.baseURL) {
    const addresses = await resolveSrv(request.path.split("/")[1] + ".service.consul");
    this.baseURL = addresses[0];
  }
  return super.resolveURL(request);
}

Instead of:

baseURL = async (request: RequestOptions) => {
  const addresses = await resolveSrv(request.path.split("/")[1] + ".service.consul");
  return addresses[0];
}

What do you think?

[Saeris]

resolveURL would probably be fine. I didn't have a need for a dynamic baseURL personally, so I'll defer to your judgement on that one. Perhaps the only concern is we're again heading back towards a solution that adds a new method and adds an extra step to the process which may confuse users.

[danilobuerger]

A bit late now, but why is willSendRequest even necessary now with having resolveURL ?

[martijnwalraven]

@danilobuerger Although technically you could do everything in resolveURL, I think it still makes sense to have separate callbacks for clarity.

I see resolveURL as something most people won't need to override, except if they need dynamic async resolving.

Implementing willSendRequest is pretty common on the other hand, because that's where you add headers or parameters. And I want to make that as easy as possible (so no need to call super for instance).

[danilobuerger]

Ok, sure I can see that.

[martijnwalraven]

@danilobuerger An alternative might be to allow willSendRequest to return a URL (or a complete Request). Not sure if that makes things clearer though.

[danilobuerger]

Hmm, all speculation at that point. Maybe lets just leave it as is now and see how other people feel about it when they start using it?

[martijnwalraven]

Yeah, you're right, I know I'm prone to bikeshedding :)

[godspeedelbow]

Hey @martijnwalraven looks great! Thanks for picking up on this and extending the Datasources.

Just out of bike-shedding interest, would there be a way (in theory) to have a method that doesn't require calling super? I find methods that require super calls unintuitive as they tend to lead to hard to catch bugs, because you cannot derive it from looking at the code, you have to know it (aka have read the docs).

I am not sure if this would be a good idea but if the base class's resolveURL always needs to be called (it looks like that's the case as there's some hygiene stuff happening there), maybe you should rework that into a sanitizeURL method (or whatever) and then here call both, with resolveURL only being called if it exists.

let url = this.resolveUrl ? await this.resolveURL(options) : undefined;
url = await this.sanitizeURL(options);

I know this ^ won't work but you get what I mean I guess :)

[danilobuerger]

You don't have to call super in resolveURL if you override it. Actually, if you override resolveURL you will most likely not want to call super.