jadell / neo4jphp

PHP wrapper of the Neo4j REST interface
Other
532 stars 137 forks source link

Add support for unique indexes #66

Closed sgarner closed 11 years ago

sgarner commented 12 years ago

I have implemented support for unique indexes (issue #34) according to the Neo4j REST API: http://docs.neo4j.org/chunked/stable/rest-api-unique-indexes.html

It is simply an optional argument passed when adding a key:value to an index. If the key:value already exists then Neo4j will not add it to the index.

codingjoe commented 12 years ago

You might want to add unit-tests.

jadell commented 12 years ago

I don't know if this should be merged in yet. The way that unique indexing is handled will be changing soon in the REST API to handle different situations (get_or_create and create_or_fail). http://docs.neo4j.org/chunked/snapshot/rest-api-unique-indexes.html

Any ideas how to handle this for clients talking to a servers that could be using either protocol?

ikwattro commented 12 years ago

Well I think this can be merged after unit tests have been added to provide feature compatibility with current versions of Neo4j, this can be easily managed with tags/branches.

sgarner commented 12 years ago

I will have a crack at writing a unit test for this.

@jadell Ah, I wasn't aware of those upcoming changes to Neo4j. They look like they will make unique indexes a lot, lot more useful!

I guess there are two options for providing feature compatibility with different versions of Neo4j:

  1. As @kwattro suggested, create different tagged releases of neo4jphp tied to particular versions of Neo4j.
  2. We could do a request to the service root at startup and detect the server version, then conditionally provide functionality based on that.
jadell commented 12 years ago

I use the service root version trick for figuring out if Cypher is a plugin or built-in, so using the same trick to determine which version of "unique" to use has some precedence. I more in favor of that over keeping separate versions tied to different server versions.

jadell commented 12 years ago

Also, please revert the commit to composer.json. If you want to make changes to your pull request, make a branch locally, use that for development, then merge to the branch you want pulled.

sgarner commented 12 years ago

Sorry, that commit is not meant to be here!

sgarner commented 12 years ago

As per discussion at https://github.com/lphuberdeau/Neo4j-PHP-OGM/pull/30#issuecomment-8708635 I have added a check to test whether the unique "constraint" was violated or not.

This should probably throw a different class of Exception, or use a different error code, so that it can be caught - but I'm not sure how you prefer to implement that.

jadell commented 12 years ago

So here's what I'm thinking:

We keep the $unique flag, but make it mean different things based on the server version. For clients talking to servers pre-1.8, the $unique flag can be any truthy value (except the string "create_or_fail", more on that later). If it is a truthy value, the implicit meaning is "get_or_create" which is what it means to the server. The "?unique" parameter is passed along in the URL.

For clients talking to server versions 1.8+, the value can be truthy (including the string "get_or_create"). If it's truthy, the parameter "?unique=get_or_create" is passed to the server. If it's truthy and it is the string "create_or_fail" then "?unique=create_or_fail" is passed in the URL.

Now comes the tricky part. If the client is talking to a pre-1.8 server, and the value of $unique is "create_or_fail", this throws an exception, since the client is trying to do something that the server cannot accomplish.

Thoughts?

sgarner commented 12 years ago

Sounds good to me!

Just a minor correction: from my reading of the docs, the create_or_fail mode is only present in 1.9-snapshot, it does not exist in 1.8.

jadell commented 12 years ago

You're right. Pretend that I said 1.9

sgarner commented 12 years ago

I have now implemented this as per previous discussion.

Unfortunately the current v1.9 snapshot releases of Neo4j report a server version number like "1.8.M07-188-ged08ddd", which looks just like a v1.8 version number, so version detection is kind of broken. For now, I've set it to use the new uniqueness functionality for any v1.8 server. I'm not sure what this will do on a real v1.8 server that doesn't support create_or_fail (I imagine it will just ignore it and add to the index with no unique constraints).

Note that I have implemented this according to the latest snapshot docs, which have changed the parameter name in the REST API from ?unique=create_or_fail to ?uniqueness=create_or_fail.

sgarner commented 12 years ago

Example usage:

$crewIndex = new Everyman\Neo4j\Index\NodeIndex($client, 'crew');

// Start a transaction, this will ensure that a unique index violation prevents node creation
$client->startBatch();

// Create a node
$zaphod = $client->makeNode()
    ->setProperty('name', 'Zaphod Beeblebrox')
    ->save();

// Index the node on one of its properties, with unique constraint set to fail if violated
$crewIndex->add($zaphod, 'name', $zaphod->getProperty('name'), Everyman\Neo4j\Index::CreateOrFail);

// Commit transaction, this should be successful the first time
$client->endBatch();

// Do it again
$client->startBatch();

// Create a duplicate node and index it the same way
$zaphod2 = $client->makeNode()
    ->setProperty('name', 'Zaphod Beeblebrox')
    ->save();

$crewIndex->add($zaphod, 'name', $zaphod->getProperty('name'), Everyman\Neo4j\Index::CreateOrFail);

// Commit transaction, this should fail
$client->endBatch();

This of course requires a v1.9 server with ?unique=create_or_fail support.

jadell commented 12 years ago

Sorry I haven't responded to this. I'm out of town until next weekend.

The code looks alright. Can you please add unit test for the expected behavior? Make sure to cover different code paths for different server versions. Also, I thought an exception would be thrown for CreateOrFail on a server version < 1.8. Is there a reason to not do that?

bazo commented 11 years ago

any progress on this?

jadell commented 11 years ago

Can you please add some unit tests, as I asked in my previous comment. Also, I thought an exception would be thrown for CreateOrFail on a server version < 1.8. Is there a reason to not do that?

bazo commented 11 years ago

i guess sgarner is no longer interested.

jadell commented 11 years ago

If I have time this weekend, I will merge and test this. If it's not this weekend, it won't be until after new year.

sgarner commented 11 years ago

Sorry chaps, I am interested but don't have time to work on this at the moment. :(

wwwdata commented 11 years ago

hi, what is the status of this? I currently need this feature, but for the stable version 1.8.1 I'll check the code and maybe can help out if nobody else has time at the moment.

jadell commented 11 years ago

The status of this is that I haven;t found time to write tests yet. If you'd like to take a stab at writing some unit tests to assert the expected behavior, that would be appreciated. We can talk about any changes to the behavior as it exists.

codewode commented 11 years ago

I get error. i have the latest version of neo4jphp.

Fatal error: Undefined class constant 'CreateOrFail' in /var/www/neo4jphp-master/examples/indexing.php on line 23

<?php use Everyman\Neo4j\Client, Everyman\Neo4j\Index\NodeIndex, Everyman\Neo4j\Index\RelationshipIndex, Everyman\Neo4j\Index\NodeFulltextIndex, Everyman\Neo4j\Node, Everyman\Neo4j\Batch;

require_once 'example_bootstrap.php';

$client = new Client(); $crewIndex = new Everyman\Neo4j\Index\NodeIndex($client, 'crew');

// Start a transaction, this will ensure that a unique index violation prevents node creation $client->startBatch();

// Create a node $zaphod = $client->makeNode() ->setProperty('name', 'Zaphod Beeblebrox') ->save();

// Index the node on one of its properties, with unique constraint set to fail if violated $crewIndex->add($zaphod, 'name', $zaphod->getProperty('name'), Everyman\Neo4j\Index::CreateOrFail);

// Commit transaction, this should be successful the first time $client->endBatch();

// Do it again $client->startBatch();

// Create a duplicate node and index it the same way $zaphod2 = $client->makeNode() ->setProperty('name', 'Zaphod Beeblebrox') ->save();

$crewIndex->add($zaphod, 'name', $zaphod->getProperty('name'), Everyman\Neo4j\Index::CreateOrFail);

// Commit transaction, this should fail $client->endBatch();

jadell commented 11 years ago

I'm sorry to say that this feature will not be implemented.

I've been spending the last couple of days at GraphConnect San Francisco with the Neo4j folks and a large number of the driver authors for other languages. Everyone pretty much agrees that the "create unique" REST endpoint was poorly designed and not very efficient or intuitive. Plus, most of the new development work is going towards Cypher. To that end, it is vastly preferable to use the Cypher "CREATE UNIQUE" syntax (http://docs.neo4j.org/chunked/1.8.3/query-create-unique.html and http://docs.neo4j.org/chunked/1.9.4/query-create-unique.html)

Neo4j version 2.0 is getting a unique constraint feature, as well as better unique support in Cypher, and full transaction support over the REST interface. The "?uniqueness" construct is obsolete (it's deprecated as of 2.0, and will be going away soon).

The suggestion is to use Cypher for creating unique nodes, which you can start to use if you are running a Neo4j version 1.8+