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

Improve BufferOptions (body, json_body) #680

Closed
malud opened this issue Jan 30, 2023 · 3 comments · Fixed by #749
Closed

Improve BufferOptions (body, json_body) #680

malud opened this issue Jan 30, 2023 · 3 comments · Fixed by #749
Assignees
Labels
refactoring E.g. optimizing an already existing feature
Milestone

Comments

@malud
Copy link
Collaborator

malud commented Jan 30, 2023

To further optimize the memory and cpu consumption we should determine an explicit usage of body or json_body content for its request context. We should skip parsing and converting to cty.Object if just the .body or .json_body gets passed without accessing any properties. Also the sequence usage of the mentioned root variables could just get piped to the next item.

@malud malud added the refactoring E.g. optimizing an already existing feature label Jan 30, 2023
@johakoch
Copy link
Collaborator

johakoch commented Feb 3, 2023

We could create a new BufferOption for JSON-parsing apart from BufferResponse (and BufferRequest?) and set this option in eval.MustBuffer().
The new option would be read in newBerespValues() and could be passed to parseRespBody() to help decide whether or not to parseJSONBytes().

However, looking at Test_StoreInvalidBackendResponse with its configuration testdata/integration/error_handler/06_couper.hcl:

server {
  api {
    endpoint "/anything" {
      proxy {
        backend {
          origin = "${env.COUPER_TEST_BACKEND_ADDR}"

          openapi {
            file = "02_schema.yaml"
          }
        }
      }
    }

    error_handler "backend_openapi_validation" {
      response {
        status = 418
          json_body = {
            req_path = backend_requests.default.path
            resp_status = backend_responses.default.status
            resp_json_body_query = backend_responses.default.json_body.Query
            resp_ct = backend_responses.default.headers.content-type
          }
      }
    }
  }
}

, the error handler gets its own BufferOpts, so the backend_responses.default.json_body.Query in the error_handler block does not affect the endpoint's BufferOpts to cause the response body to be JSON-parsed.

This test works, because both BufferOpts have BufferResponse set, the endpoint's because of the openapi block, the error_handler's because of a reference to json_body.

If we consider another configuration

server {
  api {
    endpoint "/anything" {
      request "r" {
        url = "https://blackhole.webpagetest.org/"
        backend {
          timeout = "1s"
        }
      }
      proxy {
        backend {
          origin = "https://httpbin.org"
        }
      }
    }

    error_handler "backend_timeout" {
      response {
        status = 418
        json_body = {
          resp_json_body_headers = backend_responses.default.json_body.headers
        }
      }
    }
  }
}

, the error handler's BufferOpts are BufferResponse (because of the reference to json_body), whereas the endpoint's are BufferNone. So the proxy response's body is not buffered, and resp_json_body_headers is null.

@johakoch
Copy link
Collaborator

johakoch commented Feb 6, 2023

blocked by #706

@johakoch
Copy link
Collaborator

johakoch commented Feb 14, 2023

We should skip parsing and converting to cty.Object if just the .body or .json_body gets passed without accessing any properties.

I agree on skipping the JSON-parsing with .body.
But .json_body is expected to be a JSON "thing". So we need to JSON-parse it.

json_body = {
  a = backend_responses.a.json_body
}

@johakoch johakoch linked a pull request Mar 28, 2023 that will close this issue
@johakoch johakoch added this to the 1.13 milestone Mar 31, 2023
@johakoch johakoch self-assigned this Mar 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring E.g. optimizing an already existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants