Skip to content

Added handling of graphql body mode in all codegens#120

Merged
umeshp7 merged 16 commits intodevelopfrom
feature/graphql-support
Nov 15, 2019
Merged

Added handling of graphql body mode in all codegens#120
umeshp7 merged 16 commits intodevelopfrom
feature/graphql-support

Conversation

@shreys7
Copy link
Member

@shreys7 shreys7 commented Nov 7, 2019

In the request body, the graphql object is comprised of two strings, namely query and variables.. We will manually stringify it as one JSON object and then send it as raw body.
JSON.stringify({ query: query, variables: variables})

Need to discuss how this can be tested. Do we write unit tests in all the codegens stating whether the correct part of code is added in the generated snippet, or add a newman test in the basic collection? If we decide to go with the newman test, what graphql server will be used for the sample request?

@shreys7 shreys7 requested a review from umeshp7 November 7, 2019 06:46
Comment on lines +52 to +57
try {
graphqlVariables = JSON.parse(requestBody.graphql.variables);
}
catch (e) {
graphqlVariables = {};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this required?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To catch the error from JSON parsing. Just sending an empty object for variables. Not to send any undefined value or something.

snippet += 'client.FollowRedirects = false;\n';
}
snippet += `var request = new RestRequest(${isUnSupportedMethod ? '' : ('Method.' + request.method)});\n`;
if (request.body && request.body.mode === 'graphql' && !request.headers.has('Content-Type')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is request.headers case-insensitive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. content-type or Content-Type, both the cases it returns the same

},
"response": []
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also add a unit test for the codegens that do not run newman tests.
@shreys7

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added unit tests @umeshp7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants