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

Why this "The body, json and form options are mutually exclusive" error? #1165

Closed
1 task done
PopGoesTheWza opened this issue Apr 20, 2020 · 19 comments
Closed
1 task done
Labels
bug Something does not work as it should

Comments

@PopGoesTheWza
Copy link
Contributor

What would you like to discuss?

I am upgrading my (functional) code from the latest got@10.x.X to got@11.0.1.

Went through the release notes, updated GotError and .pagination... Typescript compiles ok but code breaks.

Not really sure why but I failed at catching any exception occurring within got so I resorted to vscode debugger.

The first got call fails with this sequence sequence:

  1. call got to query for some CRM data as json
  • GET method
  • a json payload defining the filter
  • allowGetBody: true
  • an afterResponse hook to acquire/refresh oauth token (using got with a POST method with a json payload)
  1. since there is no initial oauth token, the afterResponse hook is executed:
    afterResponse: [
      async (response, retryWithMergedOptions) => {
        // Unauthorized
        if (response.statusCode === 401) {
          const updatedOptions: ExtendOptions = { headers: { 'oauth-token': await getNewAuthToken() } };

          // Save for further requests
          authenticatedInstance.defaults.options = got.mergeOptions(
            authenticatedInstance.defaults.options,
            updatedOptions
          );

          return retryWithMergedOptions(updatedOptions);
        }

        // No changes otherwise
        return response;
      },
    ],

The oauth token is correctly acquired/renewed, object updatedOptions is correct but retryWithMergedOptions(updatedOptions) fails with throw new TypeError('The body, jsonandform options are mutually exclusive')
At this point of _finalizeBody(), this.option holds my json filter in both body and json property.

I am unsure if I originally made a mistake somewhere in the past and must feel lucky my code works in got@10.x.x or if I happen to hit e regression.

Please advise.

Checklist

  • I have read the documentation.
@szmarczak szmarczak added the unconfirmed bug The issue is possibly a bug - needs more info label Apr 20, 2020
@PopGoesTheWza
Copy link
Contributor Author

@szmarczak FYI I am trying to reproduce & isolate my case. Sharing current code is not a practical options for it would imply divulging some business information beyond my scope of decision ;)

So far it seems that:

  • If I first get a valid oauth token and provide it to the got instance before querying, this partly solves the issue (by obviously avoiding going through the .afterResponse () hook)
  • If I change my queries from got(url, {json: awesome}) to got(url, {body: JSON.stringify(awesome)}) it seems to fix the issue.
  • There are still some of the above TypeError occurring, when using the '.paginate()' method, but I am still investigating this.

@PopGoesTheWza
Copy link
Contributor Author

PopGoesTheWza commented Apr 21, 2020

@szmarczak Please give a look at this test case:

test('async afterResponse allows to retry with allowGetBody and json payload', withServer, async (t, server, got) => {
	server.get('/', (request, response) => {
		if (request.headers.token !== 'unicorn') {
			response.statusCode = 401;
		}

		response.end();
	});

	const {statusCode} = await got({
		allowGetBody: true,
		json: {hello: 'world'},
		hooks: {
			afterResponse: [
				async (response, retryWithMergedOptions) => {
					if (response.statusCode === 401) {
						return retryWithMergedOptions({headers: {token: 'unicorn'}});
					}

					return response;
				}
			]
		}
	});
	t.is(statusCode, 200);
});

It fails unless:

  • you drop the payload json: {hello: 'world'}
  • you make the afterResponse no longer async

Both of those options being a NO-GO for my use since I need a json payload to query the api, and renewing the token implies an await got call.

This may not be the exact problem I am encountering but is like some part of it.

Note that the same code was working in got@10.x.x so it is possibly a regression.

@szmarczak
Copy link
Collaborator

Thanks for the reproducible code, I'll look into this ASAP

@szmarczak szmarczak added bug Something does not work as it should and removed unconfirmed bug The issue is possibly a bug - needs more info labels Apr 21, 2020
@szmarczak
Copy link
Collaborator

On the master branch it give this:

  ✖ Timed out while running tests

  1 tests were pending in test/hooks.ts

  ◌ async afterResponse allows to retry with allowGetBody and json payload

which shouldn't happen at all.

@PopGoesTheWza
Copy link
Contributor Author

The above test doesn’t fully reproduce my original issue (TypeError instead of a timeout)
As I was putting up a test case of my own code I ended being stuck by that timeout. My real case is slightly more advanced/complex. I will complete it when there is no longer a timeout (which may share the same root cause as my original issue.)

@szmarczak
Copy link
Collaborator

Ok. I'm going to fix this tomorrow, then release 11.0.2 :)

@szmarczak
Copy link
Collaborator

Indeed this promise throws

RequestError: The body, json and form options are mutually exclusive.

but for some reason the main promise doesn't throw... It's a double bug.

@PopGoesTheWza
Copy link
Contributor Author

@szmarczak So this is indeed the original problem I identified using the vscode debugger, and it confirms I was unable to catch any thrown error/exception...

@szmarczak
Copy link
Collaborator

It doesn't throw because the error handler is executed only once (as expected). It should raise an uncaught exception error, but it doesn't because there's another error handler attached to the PromisableRequest instance. I don't know why there's a second handler, but I'm looking at it rn.

@szmarczak
Copy link
Collaborator

I think it's a get-stream bug. It uses pump, which uses end-of-stream, which attaches this handler

https://github.com/mafintosh/end-of-stream/blob/e104395e50015a6436d9747b4b1c2a617b267769/index.js#L43-L45

It is not cleaned up for some reason. But it may be a bug in Got as well, dunno.

@szmarczak
Copy link
Collaborator

Indeed, it doesn't throw because of the get-stream bug. The RequestError itself is a Got bug.

sindresorhus/get-stream#36

@szmarczak
Copy link
Collaborator

It's a Node.js bug too https://runkit.com/szmarczak/5e9ff8b7d36ff4001b085053

@PopGoesTheWza
Copy link
Contributor Author

So I luckily found a whole lotta bugs? ;)

@szmarczak
Copy link
Collaborator

That's right :P

@szmarczak
Copy link
Collaborator

So I got it throwing 8501c69

@PopGoesTheWza
Copy link
Contributor Author

@szmarczak Thanks for releasing got@11.0.2

I am now able to continue the transition from got@10.x.x to version 11...

And I now have your name all over my screen ;)

ℹ [CRM-eLease-bridge] says hi…
✖ [ERROR] RequestError: socket hang up
    at ClientRequest.<anonymous> (/Users/guillaumec/rc-dev/rcsf/node_modules/got/dist/source/core/index.js:759:25)
    at Object.onceWrapper (events.js:418:26)
    at ClientRequest.emit (events.js:323:22)
    at ClientRequest.origin.emit (/Users/guillaumec/rc-dev/rcsf/node_modules/@szmarczak/http-timer/dist/source/index.js:39:20)
    at TLSSocket.socketCloseListener (_http_client.js:400:11)
    at TLSSocket.emit (events.js:323:22)
    at net.js:668:12
    at connResetException (internal/errors.js:604:14)
    at TLSSocket.socketCloseListener (_http_client.js:400:25)
    at TLSSocket.emit (events.js:323:22)
    at net.js:668:12
    at TCP.done (_tls_wrap.js:556:7) {
  name: 'RequestError',
  code: 'ECONNRESET',
  timings: {
    start: 1587548453305,
    socket: 1587548453305,
    lookup: undefined,
    connect: undefined,
    secureConnect: undefined,
    upload: undefined,
    response: undefined,
    end: undefined,
    error: 1587548453306,
    abort: 1587548453307,
    phases: {
      wait: 0,
      dns: undefined,
      tcp: undefined,
      tls: undefined,
      request: undefined,
      firstByte: undefined,
      download: undefined,
      total: 2
    }
  }
}
ℹ [CRM-eLease-bridge] waves bye-bye…

Any idea what could be causing this and how I could investigate further? Would you prefer me to open an new issue (maybe in a different repos?)

Thanks in advance.

@szmarczak
Copy link
Collaborator

It's quite similar to #1062 but I'd rather prefer if that was a separate issue, since it's socket hang up and not read ECONNRESET.

@szmarczak
Copy link
Collaborator

It looks like you called request.abort() instead of promise.cancel(), but I'm not sure. A reproducible example would be best.

@PopGoesTheWza
Copy link
Contributor Author

I did neither of the above. I opened a new issue and try to be as descriptive as can be.
I’ll try to put up a reproducible example when done with home office and daycare duties… Happy Lockdown to all of us.

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
Projects
None yet
Development

No branches or pull requests

2 participants