Extend OAS implementations #440

eokoneyo · 2022-01-23T17:18:02Z

Hi, following our discussion in #426 , I decided to open a different PR to tackle couple of issues I found pertaining to how the definitions from an OAS might be processed.

Here's a couple of issues the PR addresses;

Fixes implementation where the style and explode properties for endpoint parameters didn't adhere to the Open API Spec. Given the partial definition;
```
"/api/path": {
  get: {
   "parameters" : [ {
          "name" : "parameters",
          "in" : "query",
          "required" : true,
          "style" : "form",
          "explode" : true,
          "schema" : {
            "$ref" : "#/components/schemas/SomeSchemaInput"
          }
    } ],
  }
}
```
where SomeSchemaInput has properties of filter and limit for example, I found that the http request made for the generated GraphQL resolver is of the format; http://{baseUrl}/api/path?parameters[limit]=10&parameters[offset]=10 when the http request url should rather be http://{baseUrl}/api/path?limit=10&offset=10 considering the provided values for style and explode, as defined in Open API specification. Relates to Adds query string serialization optimization #312 but also includes support for Arrays and Objects

Also fix issue where an error is thrown when link is created with parameter that's a nested object; for instance given the link definition;

...
        responses: {
          200: {
            links: {
              invitations: {
                operationId: 'someOperationId',
                parameters: {
                  filter: {
                    entityId: '$response.body#/id',
                  },
                },
              },
            },
          },

previously filter would be attempted to be processed as a string, which resulted in errors.

…rated http requests Signed-off-by: Eyo O. Eyo <[email protected]>

Signed-off-by: Eyo O. Eyo <[email protected]>

Alan-Cha · 2022-01-27T07:07:51Z

@eokoneyo Thank you so much for the follow up! I will take a closer look at this later today and try to get it in. This is looking great so far!

Signed-off-by: Alan Cha <[email protected]>

Alan-Cha · 2022-01-28T20:44:41Z

Okay, I have reviewed this and I generally like what I see! I have a few comments, however, which I will try to address. All in all, I think we can merge this in!

Alan-Cha · 2022-01-28T20:46:45Z

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

+  source,
+  args
+}: inferLinkArgumentsParam<TSource, TContext, TArgs>)  {
+  if (Object.prototype.toString.call(value) === '[object Object]') {


I don't really like this pattern. It seems a bit lengthy and we don't use this pattern anywhere else in our code base. I believe that typeof value === 'object' should be sufficient for this use case. Feel free to correct me if I am wrong.

using the prototype method might be overkill, but it does guarantee we are dealing with a real object but I think typeof should suffice in this case.

Signed-off-by: Alan Cha <[email protected]>

Alan-Cha · 2022-01-28T20:52:32Z

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

+  await graphql(createdSchema, query).then((result) => {
+    // target error field because the corresponding server url is not implemented,
+    // also we get the full request url as in failed request errors
+    result.errors.forEach((error) => {


Thank you so much for adding a test case! Adding test cases make my life so much easier.

This is certainly an interesting way to test this functionality, to create an error and examine the request. I think it would be really interesting if we could extract the backend requests that we are making and use them for test cases but I think this pattern isn't the best way to do this.

I am currently working on another issue that I have come across which will also require me to rework this test. We can accept this for now.

Alan-Cha · 2022-01-28T20:54:22Z

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

+ */
+function inferLinkArguments (paramName, value, resolveData, source, args) {
+  if (Object.prototype.toString.call(value) === '[object Object]') {
+    return Object.entries(value).reduce((acc, [key, value]) => {


Thanks for fixing this issue as well! However, I think we are missing a test for this and while I was trying to add it in, I came across some other issues...

Alan-Cha · 2022-01-28T20:57:17Z

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

+    }, {})
+  }
+
+  if (value.search(/{|}/) === -1) {


... namely, we cannot assume that value is a string. There seems to be a mismatch with our type definitions and OAS.

Alan-Cha · 2022-01-28T20:57:57Z

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

@@ -55,7 +55,7 @@ type AuthOptions = {

 type GetResolverParams<TSource, TContext, TArgs> = {
  operation: Operation
-  argsFromLink?: { [key: string]: string }
+  argsFromLink?: { [key: string]: any }


So I had to make a change here.

Source: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.1.0.md#link-object

right... I totally missed this, thanks for spotting this

Alan-Cha · 2022-01-28T20:58:50Z

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

+    }, {})
+  }
+
+  if (typeof value !== 'string') {


As well as a check here. However, I am still working on finishing a test but unfortunately I don't think I will be able to finish this today and I really want to get this checked in. I think for our purposes, this PR is good enough to get merged in!

Alan-Cha · 2022-01-28T21:00:23Z

@eokoneyo It's in! Thanks so much again! I know this fix and the old PR has been getting really stale. This was a really great change! Thank you once again!

eokoneyo mentioned this pull request Jan 23, 2022

Add support for query parameters in generated http request to match OAS query parameters serialization expectation #426

Closed

eokoneyo added 2 commits January 23, 2022 18:46

adds support for OAS query parameter serialization expection for gene…

909f152

…rated http requests Signed-off-by: Eyo O. Eyo <[email protected]>

add implementation to support link parameters with object as properties

f66b523

Signed-off-by: Eyo O. Eyo <[email protected]>

eokoneyo marked this pull request as ready for review January 23, 2022 17:48

Documentation improvements

b0072f0

Signed-off-by: Alan Cha <[email protected]>

Alan-Cha reviewed Jan 28, 2022

View reviewed changes

Refactor code

9bf250c

Signed-off-by: Alan Cha <[email protected]>

Alan-Cha reviewed Jan 28, 2022

View reviewed changes

Alan-Cha merged commit fbefbac into IBM:master Jan 28, 2022

Alan-Cha mentioned this pull request Feb 3, 2022

Non-string and nested link object parameters #444

Merged

eokoneyo deleted the feature/extend-OAS-implementations branch March 14, 2022 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend OAS implementations #440

Extend OAS implementations #440

eokoneyo commented Jan 23, 2022 •

edited

Loading

Alan-Cha commented Jan 27, 2022

Alan-Cha commented Jan 28, 2022 •

edited

Loading

Alan-Cha Jan 28, 2022

eokoneyo Jan 28, 2022

Alan-Cha Jan 28, 2022

Alan-Cha Jan 28, 2022

Alan-Cha Jan 28, 2022

Alan-Cha Jan 28, 2022

eokoneyo Jan 28, 2022

Alan-Cha Jan 28, 2022

Alan-Cha commented Jan 28, 2022

Extend OAS implementations #440

Extend OAS implementations #440

Conversation

eokoneyo commented Jan 23, 2022 • edited Loading

Alan-Cha commented Jan 27, 2022

Alan-Cha commented Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alan-Cha commented Jan 28, 2022

eokoneyo commented Jan 23, 2022 •

edited

Loading

Alan-Cha commented Jan 28, 2022 •

edited

Loading