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

On afterResponse token renewal, chained .text()|.json()|.buffer() method returns the wrong body #1120

Closed
2 tasks done
PopGoesTheWza opened this issue Mar 11, 2020 · 4 comments
Labels
bug Something does not work as it should ✭ help wanted ✭

Comments

@PopGoesTheWza
Copy link
Contributor

PopGoesTheWza commented Mar 11, 2020

Describe the bug

  • Node.js version: 12.16.1
  • OS & version: MacOS Catalina

Given a client Got instance with a afterResponse hook to set/renew OAuth2 token upon 401 response.statusCode

const unchained = await client(url, options)
// unchained.body holds the correct (as a string) json result (i.e. after token properly set)

const chained = await client(url, options).json()
// chained is the json result of the 401 response (i.e. before token is properly set)

Actual behavior

When .json() is chained with a got instance with an afterResponse hook, it returns the json result of the first request and ignores (or overwrites?) the result of the second (and valid) request.

Expected behavior

When .json() is chained with a got instance with an afterResponse hook, it should return the json result of the last request.

Code to reproduce

Add the following to test\hooks.ts and run npx tsc && npx ava test/hooks.ts

test('afterResponse with retry as correct .json()', withServer, async (t, server, got) => {
	server.get('/', (request, response) => {
		if (request.headers.token !== 'unicorn') {
			response.statusCode = 401;
			response.end(JSON.stringify({hello: 'nasty'}));
		} else {
			response.end(JSON.stringify({hello: 'world'}));
		}
	});

	const body = await got({
		hooks: {
			afterResponse: [
				(response, retryWithMergedOptions) => {
					if (response.statusCode === 401) {
						return retryWithMergedOptions({
							headers: {
								token: 'unicorn'
							}
						});
					}

					return response;
				}
			]
		}
	}).json() as any;
	t.is(body.hello, 'world');
});

Checklist

  • I have read the documentation.
  • I have tried my code with the latest version of Node.js and Got.
@PopGoesTheWza
Copy link
Contributor Author

I opened a PR with the (currently failing) test to reproduce this issue.

@szmarczak
Copy link
Collaborator

Thank you for the report, I'll get back to this ASAP.

@PopGoesTheWza PopGoesTheWza changed the title On afterResponse token renewal, chained .json() returns the wrong body On afterResponse token renewal, chained .text()|.json()|.buffer() method returns the wrong body Mar 12, 2020
@PopGoesTheWza
Copy link
Contributor Author

I added test cases for .text() and .buffer() methods for they are also affected.

@szmarczak szmarczak added bug Something does not work as it should ✭ help wanted ✭ labels Mar 12, 2020
@szmarczak
Copy link
Collaborator

Fixed in 99d70df

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as it should ✭ help wanted ✭
Projects
None yet
Development

No branches or pull requests

2 participants