Vinelab / NeoEloquent

The Neo4j OGM for Laravel
MIT License
643 stars 200 forks source link

`create` not included in transactions #138

Closed bramdevries closed 4 years ago

bramdevries commented 8 years ago

I'm using Laravel 5.1 and in our tests we make use of database transactions to make sure all tests start with a clean slate.

The application has 2 databases: Mysql and neo4j. In one of our tests I'm using Label::create() to create a node.

We've setup our setUp() like this:

$db = $this->app->make('db');
$db->beginTransaction();
$db->connection('neo4j')->beginTransaction();

$this->beforeApplicationDestroyed(function () use ($db) {
     $db->connection('neo4j')->beginTransaction();
     $db->rollback();
});

However, after the tests are finished the nodes still exist in the database.

I think the reason for this is that the create call doesn't use the transaction when the model uses incrementing ids. In that case it simply does a node->save() instead of passing through the connection:

insertGetId vs insert.

Is there a specific reason the insertGetId does not use the connection instance? And if there isn't, is it possible to make use of it?

I've also tried to "cheat" by using a createWith() instead, but then I suffer from another bug:

Unexpected end of input: expected whitespace or org$neo4j$cypher$internal$frontend$v2_3$parser$Clauses$$ReturnItem (line 1, column 169 (offset: 168))
"CREATE (project:`Project` { reference_id: 'acac5bd7-a6b7-43c7-b946-9ccb88c59232', updated_at: '2015-12-15 16:19:34', created_at: '2015-12-15 16:19:34'}) RETURN project,"

(note the extra , at the end of the query, because I'm passing no relationships).

Mulkave commented 8 years ago

@bramdevries is this on v2-alpha or v1.2?

bramdevries commented 8 years ago

v1.2

Mulkave commented 8 years ago

it is true createWith doesn't work with no relations. Thanks for the report will check it out.

bramdevries commented 8 years ago

@Mulkave That's another bug though :p I more curious about the transactions.

Mulkave commented 8 years ago

haha yep was talking about the original one not the createWith :+1:

Mulkave commented 8 years ago

Just taken a loot at it, the reason why this is not going through the connection is that it's using the driver's Client "shortcuts" to create a node and get its ID. This has been changed as you're suggesting in v2-alpha https://github.com/Vinelab/NeoEloquent/blob/2.0.0-alpha/src/Vinelab/NeoEloquent/Query/Builder.php#L110

Can you please give it a shot? I know it's still in alpha but basic functionality (including full backward compatibility) is working.

bramdevries commented 8 years ago

Would like to use it, but running into some install issues:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - neoxygen/neoclient 2.2.8 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.7 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.6 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.5 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.4 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.3 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.2 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2.1 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.2 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.9 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.8 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.7 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.6 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.5 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.4 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.3 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.20 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.2 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.19 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.18 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.17 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.16 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.15 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.14 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.13 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.12 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.11 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.10 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.1.x-dev requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.1 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - neoxygen/neoclient 2.1.0 requires guzzlehttp/guzzle 4.* -> no matching package found.
    - Installation request for vinelab/neoeloquent dev-2.0.0-alpha -> satisfiable by vinelab/neoeloquent[dev-2.0.0-alpha].
    - vinelab/neoeloquent dev-2.0.0-alpha requires neoxygen/neoclient ~2.1 -> satisfiable by neoxygen/neoclient[2.1.0, 2.1.1, 2.1.1.x-dev, 2.1.10, 2.1.11, 2.1.12, 2.1.13, 2.1.14, 2.1.15, 2.1.16, 2.1.17, 2.1.18, 2.1.19, 2.1.2, 2.1.20, 2.1.3, 2.1.4, 2.1.5, 2.1.6, 2.1.7, 2.1.8, 2.1.9, 2.2, 2.2.1, 2.2.10, 2.2.2, 2.2.3, 2.2.4, 2.2.5, 2.2.6, 2.2.7, 2.2.8, 2.2.9].
    - neoxygen/neoclient 2.2.10 requires guzzlehttp/guzzle ^5.0 -> satisfiable by guzzlehttp/guzzle[5.3.x-dev].
    - neoxygen/neoclient 2.2.9 requires guzzlehttp/guzzle ^5.0 -> satisfiable by guzzlehttp/guzzle[5.3.x-dev].
    - Conclusion: don't install guzzlehttp/guzzle 5.3.x-dev

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://groups.google.com/d/topic/composer-dev/_g3ASeIFlrc/discussion> for more details.

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

These are the other dependencies:

"minimum-stability": "dev",
    "require": {
        "php": ">=5.5.9",
        "anahkiasen/arrounded": "^0.8.0",
        "anahkiasen/former": "^4.0",
        "aws/aws-sdk-php": "^3",
        "barryvdh/laravel-cors": "0.7.x",
        "bugsnag/bugsnag-laravel": "^1.4",
        "codesleeve/laravel-stapler": "^1.0",
        "dingo/api": "1.0.x@dev",
        "guzzlehttp/guzzle": "^6.1",
        "laravel/framework": "^5.1",
        "laravelcollective/html": "^5.1",
        "league/tactician": "^0.6.1",
        "ramsey/uuid": "^3.0",
        "rcrowe/twigbridge": "^0.9.0",
        "robclancy/presenter": "^1.3",
        "vinelab/neoeloquent": "dev-2.0.0-alpha"
    },
    "require-dev": {
        "barryvdh/laravel-debugbar": "^2.0",
        "barryvdh/laravel-ide-helper": "^2.0",
        "codeception/codeception": "^2.0",
        "codeception/mockery-module": "^0.2.2",
        "fabpot/php-cs-fixer": "^1.7",
        "fzaninotto/faker": "^1.4",
        "laracasts/generators": "^1.1",
        "laravel/homestead": "^2.1",
        "mockery/mockery": "^0.9",
        "phpunit/phpunit": "^4.0",
        "pda/pheanstalk": "^3.1"
    },
Mulkave commented 8 years ago

hmm, probably you need to have you stability as dev since it's not stable yet?!

bramdevries commented 8 years ago

It is set to dev, tried lowering the requirement of guzzlehttp/guzzle to ^5.3 but now getting the following:

  Problem 1
    - Installation request for vinelab/neoeloquent dev-2.0.0-alpha -> satisfiable by vinelab/neoeloquent[dev-2.0.0-alpha].
    - Conclusion: remove laravel/framework v5.1.24
    - Conclusion: don't install laravel/framework v5.1.24
    - vinelab/neoeloquent dev-2.0.0-alpha requires illuminate/database 5.1.16 -> satisfiable by laravel/framework[v5.1.16], illuminate/database[v5.1.16].
    - Can only install one of: laravel/framework[v5.1.16, v5.1.24].
    - don't install illuminate/database v5.1.16|don't install laravel/framework v5.1.24
    - Installation request for laravel/framework == 5.1.24.0 -> satisfiable by laravel/framework[v5.1.24].
Mulkave commented 8 years ago

yeah, that is due to an issue with a future version of Laravel 5.1 so had to fix it to that. I've tried transactions on my side with 2-alpha and it's working properly, i.e.

DB::transaction(function () {
// do stuff here
});

is that what you're trying to achieve?

bramdevries commented 8 years ago

No I'm doing:

$db->connection('neo4j')->beginTransaction();

$this->beforeApplicationDestroyed(function () use ($db) {
     $db->connection('neo4j')->rollback();
});

So every query run during a test is in a transaction

RomaGilyov commented 8 years ago

Have the same issue with 1.3.1, laravel version is 5.2

Is any solutions?

kcalliauw commented 8 years ago

@bramdevries How did you handle this in the end? Wrapping it in a DB::beginTransaction closure doesn't look very clean I think, your method looks preferable, but doesn't seem to work as you reported.

bramdevries commented 8 years ago

I didn't get it to work, I was waiting on version 2.

kcalliauw commented 8 years ago

OK, thanks

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.