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

Wrong paths for cypher queries when the connection to the db instance is behind a proxy #63

Closed
rado-h opened this issue Sep 3, 2012 · 6 comments
Labels

Comments

@rado-h
Copy link
Contributor

rado-h commented Sep 3, 2012

If the neo4j instance is accessed through a proxy the library can't calculate the correct path for cypher command execution.
Example:
a neo4j rest server running on http://127.0.0.1:7474 and a proxy on http://neo4j.vi serving the content of http://127.0.0.1:7474;
$client = new Client("neo4j.vi", 80);
when the library gets the server info all endpoints will be returned with absolute paths like this: "cypher" : "http://127.0.0.1:7474/db/data/cypher", and since the getPaths() method in ExecuteCypherQuery depends on str_replace() using the endpoint (http://neo4j.vi), the returned path will alway be "http://127.0.0.1:7474/db/data/cypher" and so the calculated url for the request will be "http://neo4j.vi:80/db/datahttp://127.0.0.1:7474/db/data/cypher"

@SoundLogic
Copy link

You would probably want to change the client construct, in Client.php, to accept optional proxy parameters for the port and host name where the proxy resides and then setup CURL to use this info.

@jadell - I would be happy to commit some code here, if you would like

I already have a function that I use to build some optional headers:

function buildCurlOptions($useProxy, $proxyIP, $proxyPort, $includeResponseHeaders, $headers, $contentType = NULL, $sslVerifyPeer = FALSE)
{
    $options = array();

    if ($useProxy) {
        if ($proxyIP != NULL && $proxyPort != NULL) {
            $proxySettings = array(
                CURLOPT_PROXY => $proxyIP
            , CURLOPT_PROXYPORT => $proxyPort
            );

            array_push_associative($options, $proxySettings);
        }
    }

    if ($includeResponseHeaders) {
        $responseSettings = array(CURLOPT_HEADER => TRUE);

        array_push_associative($options, $responseSettings);
    }

    if (!$sslVerifyPeer) {
        $sslVerificationSettings = array(CURLOPT_SSL_VERIFYPEER => FALSE);

        array_push_associative($options, $sslVerificationSettings);
    }

    if ($headers != NULL) {
        if ($contentType != NULL) {
            array_push($headers, "Content-Type: " . $contentType);
        }

        $curlHttpHeaders = array(CURLOPT_HTTPHEADER => $headers);

        array_push_associative($options, $curlHttpHeaders);
    }

    return $options;
}

where array_push_associative is:

function array_push_associative(&$arr) {
    $args = func_get_args();
    $ret=0;
    foreach ($args as $arg) {
        if (is_array($arg)) {
            foreach ($arg as $key => $value) {
                $arr[$key] = $value;
                $ret++;
            }
        }else{
            $arr[$arg] = "";
        }
    }
    return $ret;
}

so I would use these functions (or a variation of these functions) and the Client.php construct would then be:

public function __construct($transport = null, $port = 7474, $useProxy = false, $proxyHost = null, $proxyPort = null)
    {
                /* I know this would have to be edited because you already have some things, like content type, in the 
                    Curl class...but not important to illustrate the example */
                $options = buildCurlOptions($useProxy, $proxyHost, $proxyPort, FALSE, $headers, "application/json");
        try {
            if ($transport === null) {
                $transport = new Transport\Curl($options);
            } elseif (is_string($transport)) {
                $transport = new Transport\Curl($options, $transport, $port);
            }
        } catch (Exception $e) {
            if ($transport === null) {
                $transport = new Transport\Stream();
            } elseif (is_string($transport)) {
                $transport = new Transport\Stream($transport, $port);
            }
        }

        $this->setTransport($transport);
        $this->setNodeFactory(function (Client $client, $properties=array()) {
            return new Node($client);
        });
        $this->setRelationshipFactory(function (Client $client, $properties=array()) {
            return new Relationship($client);
        });
    }

and then you would just use array_push_associative to add this into the curl options array, within the Curl class, before making the request.

@jadell
Copy link
Owner

jadell commented Sep 5, 2012

Transport and CURL are not the problem here. The server info that the ExecuteCypherQuery command uses to determine which Cypher path to use shouldn't do a straight string replacement. It should probably just use a regex to find everything after the '/db/data' portion of the URL and use that as the path.

As for CURL options, that should be handled by the Transport, not the client. If CURL-specific options are needed, they should be set on the Transport object, and the Transport should be used to instantiate the client. Similar to pull request #64

@SoundLogic
Copy link

All valid points. I just started working with this project :)

I did observe the similarity to #64 which makes me think a function to build options could still be useful? (and, as you state, the transport would be handed off to the client)

@jadell
Copy link
Owner

jadell commented Sep 5, 2012

Having the Cypher endpoint be correct is a pretty easy fix. I should have it committed tonight.

@SoundLogic If you think there needs to be a way to handle transport options, please make a separate issue for it. Maybe you and @rado-h could team up on figuring out a generic way to do that?

@jadell jadell closed this as completed in c56b123 Sep 5, 2012
@rado-h
Copy link
Contributor Author

rado-h commented Sep 7, 2012

@jadell the db part of the url is configurable and may not be /db/data. Maybe we can solve the problem with all the configurations globally by adding a dependency injector to the library - something like Pimple or Laravel's IoC.

@jadell
Copy link
Owner

jadell commented Sep 7, 2012

@rado-h Good point. Can you please make another issue to make the db path configurable, and then we can figure out how best to handle it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants