opensearch-project / opensearch-php

Official PHP Client for OpenSearch
Other
109 stars 58 forks source link

Add includePortInHostHeader in fromConfig #118

Closed joanna-bak-sourceability closed 1 year ago

joanna-bak-sourceability commented 1 year ago

Description

This Pull Request adds a new optional configuration includePortInHostHeader to the \OpenSearch\ClientBuilder::fromConfig(). By default, it is not set.

Passing includePortInHostHeader into \OpenSearch\ClientBuilder::fromConfig() forces the Opensearch Client Url Port to be used from the configured client url.

Issues Resolved

Closes Don't modify the client port use

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock commented 1 year ago

Thanks. Just adding a note here to acknowledge that this is a copy from https://github.com/elastic/elasticsearch-php/pull/1181/files which is under the MIT license.

joanna-bak-sourceability commented 1 year ago

@paulborgermans could you take a look on this Pull request? It is super short.

dblock commented 1 year ago
joanna-bak-sourceability commented 1 year ago

@dblock I added user guide her and changes to https://github.com/opensearch-project/opensearch-php/blob/main/CHANGELOG.md: https://github.com/opensearch-project/opensearch-php/pull/120

dblock commented 1 year ago

@dblock I added user guide her and changes to https://github.com/opensearch-project/opensearch-php/blob/main/CHANGELOG.md: #120

It should be part of this PR, please.

joanna-bak-sourceability commented 1 year ago

I think I don't understand this change anymore. In which case would you not want to use the port from your hosts config? Why wouldn't this be the default behavior all the time? What is the reason to make it optional, and what tests fail if you don't introduce this as an optionm, and just make the change to use the port?

@dblock This Pull request is about forcing the port usage, that we setup in our URL. I am not sure, what is the case scenario, not to want to use that config port.

We use yaml configuration to set up the client. Without additional option includePortInHostHeader = true in that configuration that we pass, our port is removed from the request.

We are passing in our configuration: hosts: 'localhost:9200' Here is the request from response object if we do not use includePortInHostHeader = true (port is set to null):

"error" => GuzzleHttp\Exception\ConnectException^ {#8206
    #message: "cURL error 7: Failed to connect to opensearch port 80: Connection refused (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for http://opensearch/parts_test"
    #code: 0
    #file: "./vendor/guzzlehttp/guzzle/src/Handler/CurlFactory.php"
    #line: 210
    -request: GuzzleHttp\Psr7\Request^ {#13306
      -method: "HEAD"
      -requestTarget: null
      -uri: GuzzleHttp\Psr7\Uri^ {#13314
        -scheme: "http"
        -userInfo: ""
        -host: "opensearch"
        -port: null
        -path: "/parts_test"
        -query: ""
        -fragment: ""
        -composedComponents: "http://opensearch/parts_test"
      }
      -headers: array:6 [
        "Authorization" => array:1 [
          0 => "Basic ZWxhc3RpYzpjaGFuZ2VtZQ=="
        ]
        "Host" => array:1 [
          0 => "opensearch"
        ]
        "Content-Type" => array:1 [
          0 => "application/json"
        ]
        "Accept" => array:1 [
          0 => "application/json"
        ]
        "User-Agent" => array:1 [
          0 => "opensearch-php/2.0.0 (Linux 5.15.49-linuxkit; PHP 8.1.8)"
        ]
        "Accept-Encoding" => array:1 [
          0 => "gzip"
        ]
      ]
shyim commented 1 year ago

I would add it also into the main branch. For my case it works when you fully type the url like http://localhost:9200. When you are using the default port, you should be even able to just type localhost

dblock commented 1 year ago

@joanna-bak-sourceability do you have time to look into https://github.com/opensearch-project/opensearch-php/issues/122? If we remove that option altogether then we won't need this PR and it will have less impact on users.

joanna-bak-sourceability commented 1 year ago

@joanna-bak-sourceability do you have time to look into https://github.com/opensearch-project/opensearch-php/issues/122? If we remove that option altogether then we won't need this PR and it will have less impact on users.

@dblock I can't promise that I will be able to check it soon. But I will come back to it.