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

[APM] Ensure correct encoding of body in callApi #51600

Closed
wants to merge 3 commits into from

Conversation

dgieselaar
Copy link
Member

Fixes issue with the ML and Watcher integrations where the body ended up being encoded to JSON twice. Introduced in 06bee60.

@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Nov 25, 2019
@dgieselaar dgieselaar requested a review from a team as a code owner November 25, 2019 13:24
// Need an empty body to pass route validation
const body = isGet
const bodyObject = isGet
Copy link
Member

Choose a reason for hiding this comment

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

Why should bodies in GET requests not be stringified?

btw. What do you think about a small rename:

Suggested change
const bodyObject = isGet
const bodyObject = isGetRequest

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIR, body is not allowed in GET requests per NP requirements. All of this is to work around schema limitations, so hopefully we can get rid of it when #50179 is addressed.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIR, body is not allowed in GET requests per NP requirements

In that case we'll never send body with GET requests, right? So having this check won't matter afaict.

Copy link
Member

Choose a reason for hiding this comment

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

Eg. we could always just JSON.stringify(body) afaict - or will that cause validation issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

I might not be understanding what you mean. This code adds { body: { ... } } to all non-GET requests (either empty or whatever it was given). We need at least an empty body value to pass the current NP request validation. Do you see a way we can do this without checking the request method?

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking something like this. So not depending in the http verb but only whether there is a body or not:

  return {
    ...rest,
    ...(fetchOptions.body ? JSON.stringify(body) : {}),
    query: {
      ...fetchOptions.query,
      ...(debugEnabled ? { _debug: true } : {})
    }

But if NP requires a body for all requests then that's not possible (although that sounds like an odd requirement)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

dgieselaar and others added 3 commits November 25, 2019 16:37
Fixes issue with the ML and Watcher integrations where the body ended up being encoded to JSON twice. Introduced in 06bee60.
Co-Authored-By: Søren Louv-Jansen <sorenlouv@gmail.com>
@dgieselaar dgieselaar force-pushed the apm-integrations-broken branch from dcb2fc0 to d2f6c34 Compare November 25, 2019 15:38
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@dgieselaar
Copy link
Member Author

Going to close this in favor of an incoming PR that addresses some other issues as well.

@dgieselaar dgieselaar closed this Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants