close
The Wayback Machine - https://web.archive.org/web/20200905040151/https://github.com/elastic/elasticsearch-js/issues/1291
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

RequestOptions type says body can't be null but it can #1291

Open
villasv opened this issue Aug 26, 2020 · 1 comment · May be fixed by #1300
Open

RequestOptions type says body can't be null but it can #1291

villasv opened this issue Aug 26, 2020 · 1 comment · May be fixed by #1300

Comments

@villasv
Copy link
Contributor

@villasv villasv commented Aug 26, 2020

🐛 Bug Report

This is the definition on Connection.d.ts:

interface RequestOptions extends http.ClientRequestArgs {
  asStream?: boolean;
  body?: string | Buffer | ReadableStream;
  querystring?: string;
}

To Reproduce

I've been using this class to provide authenticated connection with AWS Elasticsearch:

import { Readable } from 'stream';
import { Connection as UnsignedConnection } from '@elastic/elasticsearch';
import * as AWS from 'aws-sdk';
import RequestSigner from 'aws-sdk/lib/signers/v4';
import { ClientRequest, IncomingMessage } from 'http';

class AwsElasticsearchError extends Error {}
type RequestOptions = Parameters<UnsignedConnection['request']>[0];

class AwsSignedConnection extends UnsignedConnection {
  public request(
    params: RequestOptions,
    callback: (err: Error | null, response: IncomingMessage | null) => void,
  ): ClientRequest {
    const signedParams = this.signParams(params);
    return super.request(signedParams, callback);
  }

  private signParams(params: RequestOptions): RequestOptions {
    const region = AWS.config.region || process.env.AWS_DEFAULT_REGION;
    if (!region) throw new AwsElasticsearchError('missing region configuration');
    if (!params.method) throw new AwsElasticsearchError('missing request method');
    if (!params.path) throw new AwsElasticsearchError('missing request path');
    if (!params.headers) throw new AwsElasticsearchError('missing request headers');

    const endpoint = new AWS.Endpoint(this.url.href);
    const request = new AWS.HttpRequest(endpoint, region);

    request.method = params.method;
    request.path = params.querystring
      ? `${params.path}/?${params.querystring}`
      : params.path;
    if (typeof params.body === 'string') request.body = params.body;
    else if (params.body instanceof Buffer) request.body = params.body.toString();
    else if (params.body instanceof Readable) throw new AwsElasticsearchError('unsupported');
    else if (params.body !== undefined) {
      throw new AwsElasticsearchError('body type unhandled');
    }

    Object.entries(params.headers).forEach(([header, value]) => {
      if (typeof value === 'string') request.headers[header] = value;
      else if (typeof value === 'number') request.headers[header] = `${value}`;
      else if (Array.isArray(value)) request.headers[header] = value.join('; ');
      else if (value !== undefined) throw new AwsElasticsearchError('header type unhandled');
    });
    request.headers.Host = endpoint.host;

    const signer = new RequestSigner(request, 'es');
    signer.addAuthorization(AWS.config.credentials, new Date());
    return request;
  }
}

When using the connection, it throws with body type unhandled because params.body is null.

Expected behavior

The typing should say it can be null, or it should never be null.

Your Environment

  • node version: 10.14.1
  • @elastic/elasticsearch version: >=7.9.0
  • os: Linux
@delvedor
Copy link
Member

@delvedor delvedor commented Sep 1, 2020

Good catch! Would you like to send a pull request? :)

villasv added a commit to villasv/elasticsearch-js that referenced this issue Sep 4, 2020
@villasv villasv linked a pull request that will close this issue Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.