requestOptions accepting function so that we can change options depending on the request #381

dinumathai · 2021-03-22T15:01:55Z

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.

Alan-Cha · 2021-03-22T19:35:28Z

@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?

dinumathai · 2021-03-23T08:44:09Z

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".

Alan-Cha · 2021-03-23T14:24:02Z

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?

dinumathai · 2021-03-23T15:14:02Z

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

dinumathai · 2021-03-24T13:40:34Z

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

Feel free to make any changes.

…ding on the request Signed-off-by: Dinu Mathai <Dinu.Mathai1@T-Mobile.com>

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>

Alan-Cha · 2021-03-26T06:34:52Z

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.

Alan-Cha · 2021-03-26T06:36:02Z

packages/openapi-to-graphql/lib/resolver_builder.d.ts

@@ -11,26 +11,31 @@ import { RequestOptions } from './types/options';
 import { GraphQLFieldResolver } from 'graphql';
 import { IncomingHttpHeaders } from 'http';
 export declare const OPENAPI_TO_GRAPHQL = "_openAPIToGraphQL";
-declare type GetResolverParams<TSource, TContext, TArgs> = {
+declare type GetSubscribeParams<TSource, TContext, TArgs> = {


Just cleaning up old code. Just ignore.

Alan-Cha · 2021-03-26T06:36:20Z

packages/openapi-to-graphql/lib/types/options.d.ts

@@ -97,7 +131,7 @@ export declare type InternalOptions<TSource, TContext, TArgs> = {
     * into a GraphQL ID type. To allow for more customzation, this option allows
     * users to specify other formats that should be interpreted as ID types.
     */
-    idFormats?: string[];
+    idFormats: string[];


More clean up.

Alan-Cha · 2021-03-26T06:37:53Z

packages/openapi-to-graphql/src/resolver_builder.ts

-      }
-    }
+    // The options `requestOptions` will finalize the options
+    if (requestOptions) {


requestOptions has been moved to the very bottom, right before the call. This allows requestOptions to have the final say on how the request should be made.

Signed-off-by: Alan Cha <Alan.cha1@ibm.com>

Alan-Cha · 2021-03-26T06:42:31Z

packages/openapi-to-graphql/src/resolver_builder.ts

+              path,
+              title,
+              { source, args, context, info },
+              options


This is where we also provide the generated Request options as you suggested.

Alan-Cha · 2021-03-26T06:43:29Z

packages/openapi-to-graphql/src/resolver_builder.ts

-    } else {
-      const oauthHeader = createOAuthHeader(data, context)
-      Object.assign(options.headers, oauthHeader)
+      options = merge(options, requestOptionsValue)


Notice that we merge the original options with requestOptionsValue. If we were to implement a requestOptionsStrict option like I mentioned previously, if would do something here.

Alan-Cha · 2021-03-26T06:45:52Z

packages/openapi-to-graphql/src/schema_builder.ts

 * provided through the options
 */
-function skipArg<TSource, TContext, TArgs>(
+function shouldSkipArgumentBecauseOptions<TSource, TContext, TArgs>(


This is where the problem I mentioned before comes from.

getResolver in resolver_builder.ts uses requestOptions to change the API call.

shouldSkipArgumentBecauseOptions in schema_builder.ts uses requestOptions to determine if it should add an argument to the field in the GraphQL

They both provide different arguments.

Alan-Cha · 2021-03-26T06:46:29Z

packages/openapi-to-graphql/src/resolver_builder.ts

+       */
+      const requestOptionsValue =
+        typeof requestOptions === 'function'
+          ? requestOptions(


Here we provide method, path, title, resolver arguments, and generated options

Alan-Cha · 2021-03-26T06:46:57Z

packages/openapi-to-graphql/src/schema_builder.ts

+    typeof data.options.requestOptions === 'object'
+      ? data.options.requestOptions
+      : typeof data.options.requestOptions === 'function'
+      ? data.options.requestOptions(


Here we only provide method, path, and title. No resolver arguments or generated request options.

Alan-Cha · 2021-03-26T06:47:31Z

packages/openapi-to-graphql/test/example_api.test.ts

-      headers: (method, path, title) => {
-        if (method === 'get' && path === '/snack') {
-          return {
+    requestOptions: (method, path, title) => {


Example of using requestOptions as a function.

Looks good. Thank you.

dinumathai · 2021-04-04T16:02:27Z

Planned any date for merge ?

Alan-Cha · 2021-04-06T13:07:28Z

@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?

dinumathai · 2021-04-07T12:34:20Z

@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.

dinumathai · 2021-04-21T15:06:48Z

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

ibm-ci-bot · 2021-07-15T17:48:54Z

@dinumathai: PR needs rebase.