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

Much simpler way to implement required UUIDs. #478

Merged
merged 10 commits into from
Sep 21, 2014
Merged

Conversation

cheerfulstoic
Copy link
Contributor

Just remove the methods/properties/constants every time it's called again. Thanks @subvertallchris !

…/properties/constants every time it's called again. Thanks @subvertallchris !
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 1c37d85 on require_uuid into cdd0b69 on master.

@subvertallchris
Copy link
Contributor

Simpler AND it can be merged automatically! 👏

Though I still think merge (or at least a release with this) should be accompanied be a rake task to add IDs. I can give that a shot tomorrow, if you want.

@subvertallchris
Copy link
Contributor

Boom! Automating the process of adding IDs to existing nodes was trickier than I imagined. I wrote a bit about it at http://stackoverflow.com/questions/25952884/neo4j-assign-unique-values-to-all-nodes-matching-query/25954136#25954136.

@andreasronge
Copy link
Member

@subvertallchris really cool

…n nodes for a particular label, adding a constraint takes 141 seconds and it seems to be asynchronous for some reason...
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling fa71907 on require_uuid into cdd0b69 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 99105d9 on require_uuid into a00d124 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 99105d9 on require_uuid into a00d124 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling de559ba on require_uuid into a00d124 on master.

@subvertallchris
Copy link
Contributor

Just so I'm getting it right, it's dropping the constraint at load if one
is already defined, right? And that would happen if it's changed or if it's
inherited and then the subclass has its id property changed?

On Sunday, September 21, 2014, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/1236692

Coverage decreased (-0.15%) when pulling de559ba
de559ba
on require_uuid
into a00d124
a00d124
on master
.


Reply to this email directly or view it on GitHub
#478 (comment).

@cheerfulstoic
Copy link
Contributor Author

I'm pretty sure it's dropping the constraint just for the class/label concerned. Inheriting and calling id_property again should just act like more calls to id_property (which at each point drop the old constraint and undefined the old property)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling 43c5f0b on require_uuid into a00d124 on master.

@cheerfulstoic
Copy link
Contributor Author

If these tests pass (which I believe they will) I'm going to merge it!

@subvertallchris
Copy link
Contributor

But when does it drop it? It looks like every time the model is loaded if there was already an id property defined, right? Does that force a wipe and rebuild of all indexes, too, since indexes follow labels?

EDIT: Wait, that's a little drunk logic talking. I think it might have to reindex the ID properties of all nodes with that label, though.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling cb10870 on require_uuid into a00d124 on master.

@cheerfulstoic
Copy link
Contributor Author

Yeah, that's a good point ;) I'll look at that now. I would think part of the problem is that there's no way to query for constraints (except, maybe, a separate endpoint), but maybe it's possible to make a single query which doesn't change anything if the constraint is already there.

@subvertallchris
Copy link
Contributor

If you look in at data/graph.db/messages.log, it shows when it's dropping and rebuilding indexes.

If dropping the old indexes is just to try keeping the database a little cleaner, maybe that should be left alone for now and the user can maintain that on their own?

@cheerfulstoic
Copy link
Contributor Author

Ok, just made a change so that it shouldn't remove the constraint if it already exists. Merging!

cheerfulstoic added a commit that referenced this pull request Sep 21, 2014
Much simpler way to implement required UUIDs.
@cheerfulstoic cheerfulstoic merged commit 5392cf0 into master Sep 21, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling cb6695e on require_uuid into a00d124 on master.

@subvertallchris
Copy link
Contributor

That is brilliant! I didn't think you could get constraint info from
Cypher. Great work!

On Sunday, September 21, 2014, Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/1236772

Coverage decreased (-0.05%) when pulling cb6695e
cb6695e
on require_uuid
into a00d124
a00d124
on master
.


Reply to this email directly or view it on GitHub
#478 (comment).

@subvertallchris subvertallchris deleted the require_uuid branch November 15, 2014 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants