openapi-to-graphql requestOptions accepting function so that we can change options depending on the request

What is this change ? Added urlResolver on Resolver options, so that we can change the URL for api call depending on the request context.

Why Is this change ? To pull data from multiple api which is having the similar signature, depending on a value from the request header.

Mar 22 '21 15:03 dinumathai

@dinumathai Thank you for this contribution! I generally like the idea of this PR. Giving full control over the requests that the resolvers make will make OtG more usable and gives more options to users.

However, I wonder if we should in a different way. I will give some background. In the beginning, there was only the headers and qs options for controlling requests. Later on, we added requestOptions. requestOptions provides more functionality than headers and qs so are thinking of deprecating headers and qs.

Another user also requested the programmatically control request headers so we modified requestOptions so it can accept a function for the headers field.

I do not want to add new request options (I would like to deprecate headers and qs) but after seeing your PR, I think it makes more sense that we allow the entire requestOptions to be a function, instead of just headers, and this way users can also specify the url.

Before:

export type RequestHeadersFunction<TSource, TContext, TArgs> = (
  method: string,
  path: string,
  title: string,
  resolverParams?: {
    source: TSource
    args: TArgs
    context: TContext
    info: GraphQLResolveInfo
  }
) => Headers

export declare type RequestURLFunction<TSource, TContext, TArgs> = (
  method: string, 
  path: string, 
  title: string, 
  resolverParams?: {
    source: TSource;
    args: TArgs;
    context: TContext;
    info: GraphQLResolveInfo;
}) => string;

export type RequestOptions<TSource, TContext, TArgs> = Omit<
  NodeRequest.OptionsWithUrl,
  'headers'
> & {
  headers?: Headers | RequestHeadersFunction<TSource, TContext, TArgs>
}

// Has `headers`, `qs`, and `urlResolver`
// Has awkward `requestOptions` with modified `headers` field
export type InternalOptions<TSource, TContext, TArgs> = {
  ...
  headers?: Headers | RequestHeadersFunction<TSource, TContext, TArgs>
  qs?: { [key: string]: string }
  urlResolver?: RequestURLFunction<TSource, TContext, TArgs>
  requestOptions?: Partial<RequestOptions<TSource, TContext, TArgs>>
  ...
}

After:

export type RequestOptionsFunction<TSource, TContext, TArgs> = (
  method: string,
  path: string,
  title: string,
  resolverParams?: {
    source: TSource
    args: TArgs
    context: TContext
    info: GraphQLResolveInfo
  }
) => Partial<NodeRequest.OptionsWithUrl>

// Deprecated `headers` and `qs`
// requestOptions is an object or a function
// if `url` or `headers` is changed, then request will reflect those changes
export type InternalOptions<TSource, TContext, TArgs> = {
  ...
  requestOptions: Partial<NodeRequest.OptionsWithUrl> | RequestOptionsFunction<TSource, TContext, TArgs>
  ...
}

Does this make sense? What do you think?

Mar 22 '21 19:03 Alan-Cha

That is a better solution. But do we need to keep qs and headers for backward compatibility ?

Also we need to think of replacing the deprecated package "request".

Mar 23 '21 08:03 dinumathai

I am fine with making breaking changes as long as they are released in a major version and it is properly documented. However, I do have a few concerns about this change.

Currently, we perform an Object.assign() to the requestOptions (see here). If we want to give total control to the user, then we need to give a higher priority to requestOptions and even add the option to prevent OtG from changing it/for OtG to use the raw requestOptions.

Perhaps we should also consider adding a new option, like requestOptionsStrict that is a boolean that controls this. If it is set to false, we stick to the original behavior, Object.assign(), and this is useful if a user just wants to use a new authentication header but does not want to change anything else. If it is set to true, then we use the raw requestOptions (the object or the output of the function), and this will give more confidence to users who want to have total control over everything.

I'm concerned that there may be a third case. The url is also a field under requestOptionsStrict. I wonder if it may need special treatment. For example, there may be occasions where we want to strictly apply all of requestOptions but not url as well as occasions where we want to strictly apply all of requestOptions. Maybe we can make it so that if the user wants to strictly apply requestOptions and url is not provided, then we default to whatever OtG comes up with (as url is a required field).

I also wonder if we should add a new option like requestOptionsStrict or make requestOptions an object that also contains a strict field. I feel like the latter might be more intrusive to use.

Currently, we are using the request module to perform our API calls. However, the module has been long deprecated and we need to use a new library. There is a PR for it #315 (as well as related #268).

Until we find a solution for this, I hesitate to make very drastic changes. Therefore, I do not think we should remove headers and qs just yet. I still think that making requestOptions is still a useful feature until we finally switch libraries.

What do you think?

Mar 23 '21 14:03 Alan-Cha

Just wanted to put one more option for consideration.

Shall we pass "options" as a parameter to the RequestOptionsFunction. This way developer will have more control. But we will need to validate for url and method in options.

export type RequestOptionsFunction<TSource, TContext, TArgs> = (
  options: Partial<NodeRequest.OptionsWithUrl>,
  method: string,
  path: string,
  title: string,
  resolverParams?: {
    source: TSource
    args: TArgs
    context: TContext
    info: GraphQLResolveInfo
  }
) => void

Mar 23 '21 15:03 dinumathai

@Alan-Cha, Added RequestOptionsFunction. Please review, and let me know the changes needed.

Feel free to make any changes.

Mar 24 '21 13:03 dinumathai

Shall we pass "options" as a parameter to the RequestOptionsFunction.

Yes, this is a great idea. I was also thinking of this.

@dinumathai I made a large number of changes, including restructuring the resolver_builder.ts to make it clearer and cleaner.

There is a complication that has come up that I wasn't aware of. This is a note that I have added to the README.md regarding the new requestOptions.

If used as a function, this will be used to modify resolvers AND the schema. For example, if a foo header is set, then the resolver call will append the foo header, and the foo parameter will be removed from the GraphQL schema. In resolver_builder.ts, the function will be called with all arguments, but in schema_builder.ts, only the method, path, and title will be provided (because resolverParams and generatedRequestOptions are only available during execution). Therefore, for query, path, header, and cookie parameters, a value must be provided without resolverParam and generatedRequestOptions or else the schema will not have the right parameters.

First, let me know if this makes any sense to you. Secondly, I just want to point out that this has always been a problem, for example with using the headers option as a function. It was just never documented properly. Thirdly, I'm not very happy with this solution. This seems to force the user to do some unnatural things if he/she wants to use resolverParams and generatedRequestOptions.

Maybe there should be two different functions, one that is used for resolver_builder.ts and get all of the parameters and produces the request options, and one used for schema_builder.ts that gets only method, path, and title and returns true/false if certain arguments should be skipped in the schema.

I hope this all makes sense. If it's not intuitive, that is exactly why this is a big issue.

Mar 26 '21 06:03 Alan-Cha

Planned any date for merge ?

Apr 04 '21 16:04 dinumathai

@dinumathai I have a lot of concerns about whether this is the right design. As I mentioned previously, requestOptions is used by both the resolver_builder.ts and schema_builder.ts and they provide different arguments. Do you have any opinions on this?

Apr 06 '21 13:04 Alan-Cha

@dinumathai I have a lot of concerns about whether this is the right design. As I mentioned previously, requestOptions is used by both the resolver_builder.ts and schema_builder.ts and they provide different arguments. Do you have any opinions on this?

Only solution I can think of is - Update README - explaining about this.

Apr 07 '21 12:04 dinumathai

Or shall we move back to initial option of adding new parameter ?

Apr 21 '21 15:04 dinumathai

@dinumathai: PR needs rebase.