Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/axios nodejs #232

Merged
merged 14 commits into from
May 25, 2020
Merged

Feat/axios nodejs #232

merged 14 commits into from
May 25, 2020

Conversation

r3dsm0k3
Copy link
Contributor

Changes

Tests ✅

Fixes postmanlabs/postman-app-support#3822

Currently there is no async await style code generation. However, adding it is very trivial.

I've tested for node-axios. Since axios also works on browser, it can also be used on the browser with minimal changes (I guess formdata file reading might need to change, but cant be very certain since I didnt test it myself)

I could not update the author in the package.json since there is a test to make sure it is Postman labs.

@fdterr
Copy link

fdterr commented Mar 30, 2020

Oh I'm excited about this

@fjufju
Copy link

fjufju commented Apr 30, 2020

any updates? I want this so muh :)
Can we ask someone from the team for a review?

@shreys7
Copy link
Member

shreys7 commented May 5, 2020

@r3dsm0k3 can you rebase your branch with the latest develop from postman-code-generators?

@shreys7 shreys7 self-requested a review May 5, 2020 09:04
@shreys7
Copy link
Member

shreys7 commented May 5, 2020

Also, can you post some example snippets here? for GET, POST, etc.

@r3dsm0k3
Copy link
Contributor Author

r3dsm0k3 commented May 6, 2020

Sure, based on the tests;

✓ should generate snippet for basicCollection request: Request Headers

const axios = require('axios');

const config = {
  'method': 'get',
  'url': 'https://postman-echo.com/headers',
  'headers': {         
    'my-sample-header': 'Lorem ipsum dolor sit amet',
    'not-disabled-header': 'ENABLED'
  }
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

✓ should generate snippet for basicCollection request: POST json with raw

const axios = require('axios');
const data = JSON.stringify({
  query: "{\n    body{\n        graphql\n    }\n}",
  variables: {"variable_key":"variable_value"}
})
const config = {
  'method': 'post',
  'url': 'http://postman-echo.com/post',
  'headers': {         
    'Content-Type': 'application/json'
  },
  data : data
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

✓ should generate snippet for basicCollection request: DELETE Request

const axios = require('axios');
const FormData = require('form-data')
const data = new FormData();
data.append("pl", "\'a\'");
data.append("qu", "\"b\"");
data.append("sa", "d");
data.append("Special", "!@#$%&*()^_+=`~");
data.append("more", ",./\';[]}{\":?><|\\\\");

const config = {
  'method': 'post',
  'url': 'https://postman-echo.com/post',
  'headers': {
    ...data.getHeaders()
  },
  data : data
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

✓ should generate snippet for formdataCollection request: POST multipart/form-data with text parameters

const axios = require('axios');
const FormData = require('form-data')
const data = new FormData();

const config = {
  'method': 'post',
  'url': 'https://postman-echo.com/post',
  'headers': {
    ...data.getHeaders()
  },
  data : data
}
axios(config)
.then(function (response) {
  console.log(JSON.stringify(response.data));
})
.catch(function (error) {
  console.log(error);
});

@shreys7
Copy link
Member

shreys7 commented May 7, 2020

Tests failing

@r3dsm0k3
Copy link
Contributor Author

r3dsm0k3 commented May 7, 2020

@shreys7 I see that all of the axios related tests are passing it is failing at completely unrelated places, although there's no change in those related libraries (same as upstream). Is there anything I'm missing here?

@someshkoli
Copy link
Contributor

@shreys7 these blocks are due to #246.

@someshkoli
Copy link
Contributor

someshkoli commented May 13, 2020

@shreys7 I've tested this version locally and generated snippets are perfect. Tests are failing due to #246 else everything seems alright. Here's a ss of failed request. All other requests are failing due to same parameter. The the data parameter is unique for delete request.
image

I think the x-amzn... parameter has to be changed from all the variants in order to tests to get passed.

@shreys7
Copy link
Member

shreys7 commented May 13, 2020

I did check for snippet generation before. By review, I meant to review the code used to generate the snippets. The code should be in alignment with other languages' code, proper linting, correct code generated for options, etc.
One obvious mistake that I can see is the option ES6_enabled is not taken into consideration while generating code, even though it is exposed in the list of options. Can you do a thorough review of code once and point out such errors? @someshkoli

@someshkoli
Copy link
Contributor

Okay sure got it ✌️

@someshkoli
Copy link
Contributor

someshkoli commented May 14, 2020

@shreys7 reviewed code thoroughly. All requests are generating according to the options provided. Also I've suggested some changes below.

* `requestTimeout` : Integer denoting time after which the request will bail out in milli-seconds
* `trimRequestBody` : Trim request body fields
* `followRedirect` : Boolean denoting whether to redirect a request
* `AsyncAwait_enabled` : Boolean denoting whether to generate snippet with async await coding style (WIP)
Copy link
Contributor

@someshkoli someshkoli May 15, 2020

Choose a reason for hiding this comment

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

You can remove async await option from the readme as its not supported by the code generator

options = {
indentType: 'Space',
indentCount: 2,
ES6_enabled: true
Copy link
Contributor

@someshkoli someshkoli May 15, 2020

Choose a reason for hiding this comment

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

Same for es_6 enable option

Copy link
Member

Choose a reason for hiding this comment

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

rather than removing the option, it would be great to have that as an option and add support for it in the code.

snippet += indentString + 'console.log(error);\n';
snippet += '});\n';

// if(options.AsyncAwait_enabled === true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these unwanted commented code

default: false,
description: 'Remove white space and additional lines that may affect the server\'s response'
}
// ,
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unwanted commented code

* @param {String} options.indentCount - number of spaces or tabs for indentation.
* @param {Boolean} options.followRedirect - whether to enable followredirect
* @param {Boolean} options.trimRequestBody - whether to trim fields in request body or not
* @param {Boolean} options.AsyncAwait_enabled : whether to enable async await pattern
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove AsyncAwait_enabled option as it is not there in the code generator


runNewmanTest(convert, options, testConfig);

describe('Convert for request incorporating ES6 features', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since your variant does not have this option you can remove this test

Copy link
Member

Choose a reason for hiding this comment

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

Same comment, add the support for ES6 syntax

@shreys7
Copy link
Member

shreys7 commented May 18, 2020

@r3dsm0k3 Can you address the changes as suggested by @someshkoli ?

@r3dsm0k3
Copy link
Contributor Author

Yeah sure.

@shreys7
Copy link
Member

shreys7 commented May 20, 2020

Any updates @r3dsm0k3 ?

@r3dsm0k3
Copy link
Contributor Author

Hey guys, I'm quite occupied these days. unfortunately, I'll only be able to spend time on the weekend. Feel free to take a stab at it if you feel like. Cheers.

@someshkoli
Copy link
Contributor

@shreys7 shall I take over this ?

@r3dsm0k3
Copy link
Contributor Author

@someshkoli I've given you access to my repo (check your invitation). If you push to the same branch, it should update the PR automatically. Thanks for the effort 👍

@someshkoli
Copy link
Contributor

someshkoli commented May 20, 2020

@someshkoli I've given you access to my repo (check your invitation). If you push to the same branch, it should update the PR automatically. Thanks for the effort 👍

Okay cool ✌️

@shreys7
Copy link
Member

shreys7 commented May 21, 2020

sure @someshkoli. Feel free to make the changes. Also, I have fixed the CI, changes up on latest develop. You can rebase with the latest develop, and the CI should pass.

@someshkoli
Copy link
Contributor

Great. Will make these changes and puch today ✌️✌️

@someshkoli
Copy link
Contributor

@shreys7 can you please check why Single/multiple file upload via form-data request is failing. I'm unable to figure out if its due to ci or due to codegenerator. Rest its done

@shreys7
Copy link
Member

shreys7 commented May 22, 2020

Can you add transfer-encoding to the list of headers to delete in newmanTestUtil file? This should solve the CI failure if the snippet generated is correct
https://github.com/postmanlabs/postman-code-generators/blob/develop/test/codegen/newman/newmanTestUtil.js

@someshkoli
Copy link
Contributor

someshkoli commented May 22, 2020

again failed due to csharp @shreys7

Copy link
Member

@shreys7 shreys7 left a comment

Choose a reason for hiding this comment

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

I have added some review comments. The comments are mostly about adding proper indentations, semicolons, sanitization of strings, etc. Tell me if you face any queries regarding the comments 🙂

}
snippet += varDeclare + ' config = {\n';
snippet += configArray.join(',\n') + '\n';
snippet += '}\n';
Copy link
Member

Choose a reason for hiding this comment

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

need a semicolon and a new line after the closing bracket.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

options.ES6_enabled);
snippet += dataSnippet+'\n';

configArray.push(indentString + `'method': '${request.method.toLowerCase()}'`);
Copy link
Member

Choose a reason for hiding this comment

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

no need of adding single quotes around the method, url and headers key. just add

{
  method: 'post',
  url: 'example.com',
  headers: {...}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

done

return '';
}
(trim) && (inputString = inputString.trim());
return inputString.replace(/\\/g, '\\\\').replace(/'/g, '\\\'').replace(/\n/g, '\\n').replace(/"/g, '\\"');
Copy link
Member

Choose a reason for hiding this comment

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

there's no need to sanitize both single as well as double quotes. The need for sanitization is that when you receive a single quote inside a single quoted string, it needs to be escaped. For example
'ab\'cd'. But if we have a single quoted string, we can place double quotes inside it, e.g. 'ab"cd'. Thus, we only need to escape a single one from both.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

*/
function parseFormData (body, trim, ES6_enabled) {
var varDeclare = ES6_enabled ? 'const' : 'var',
bodySnippet = varDeclare + ' FormData = require(\'form-data\')\n';
Copy link
Member

Choose a reason for hiding this comment

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

need a semicolon at the end of the require('form-data') statement

Copy link
Contributor

Choose a reason for hiding this comment

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

done

// check if there's file
const fileArray = body.filter(function (item) { return !item.disabled && item.type === 'file'; });
if (fileArray.length > 0) {
bodySnippet += varDeclare + ' fs = require(\'fs\')\n';
Copy link
Member

Choose a reason for hiding this comment

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

need a semicolon at the end of the require('fs') statement

Copy link
Contributor

Choose a reason for hiding this comment

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

done

// https://github.com/axios/axios/issues/789#issuecomment-577177492
if (!_.isEmpty(body) && body.formdata ) {
// we can assume that data object is filled up
headers.push(`\n${indentString}...data.getHeaders()`);
Copy link
Member

Choose a reason for hiding this comment

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

We don't really need to add a \n before the ...data.getHeaders(). I guess headers.join(,\n) will take care of it 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This is the snippet generated when request has some headers, and this formdata header is also added. The indentation looks weird, can you check on it once?

'headers': { 
      'my-sample-header': 'Lorem ipsum dolor sit amet', 
      'TEST': '@#$%^&*()', 
      'more': ',./\';[]}{\":?><|\\\\', 
  
  ...data.getHeaders()
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

done


body = request.body && request.body.toJSON();

dataSnippet = parseRequest.parseBody(body,
Copy link
Member

Choose a reason for hiding this comment

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

only add the body if it is non empty object, and the mode that it states has non empty data

Copy link
Contributor

Choose a reason for hiding this comment

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

done

*/
function parseURLEncodedBody (body, trim, ES6_enabled) {
var varDeclare = ES6_enabled ? 'const' : 'var',
bodySnippet = varDeclare + ' qs = require(\'qs\')\n',
Copy link
Member

Choose a reason for hiding this comment

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

need a semicolon after the require('qs') statement

Copy link
Contributor

Choose a reason for hiding this comment

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

done

}
bodySnippet = varDeclare + ' data = JSON.stringify({\n';
bodySnippet += `${indentString}query: "${sanitize(query, trim)}",\n`;
bodySnippet += `${indentString}variables: ${JSON.stringify(graphqlVariables)}\n})`;
Copy link
Member

Choose a reason for hiding this comment

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

need a semicolon at the end of line

Copy link
Contributor

Choose a reason for hiding this comment

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

done

 - adds ; where necessary
 - removes \n where not required
 - removes double quote sanitization
 - adds \n where necessary
bodySnippet += `JSON.stringify(${JSON.stringify(jsonBody)});\n`;
}
catch (error) {
bodySnippet += `"${sanitize(body.toString(), trim)}";\n`;
Copy link
Member

Choose a reason for hiding this comment

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

In the sanitize function, we escape single quotes only. If the body here contains a double quote, it won't get escaped, resulting in a wrong snippet generated. e.g. "this is a sample "body preview". Look at the sanitize functions of other snippets to see how we handle the sanitization of quotes.

@shreys7
Copy link
Member

shreys7 commented May 25, 2020

Merging this PR. Thanks a lot for the contribution @r3dsm0k3 @someshkoli 🙏😄

@shreys7 shreys7 merged commit 31e5c35 into postmanlabs:develop May 25, 2020
@fjufju
Copy link

fjufju commented May 26, 2020

@shreys7 this now available when making docs in postman? Thank you!

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.

Feature request: Support generating code snippets for Axios
5 participants