Skip to content

Conversation

@rgan
Copy link

@rgan rgan commented Feb 15, 2012

An approach to provide support for uniqueness by creating a Unique annotation for a field in a node entity which will create the node using the UniqueFactory. (There is one extra commit - fix for array index out of bounds - in this pull request that is un-related.)

@jexp
Copy link
Contributor

jexp commented Feb 15, 2012

I already fixed the arrayoutofbounds exception by using a different approach.
Could you please refrain from reformatting? As this adds a lot of diffs that don't have anything to do with the code changes?

I rather thought about supporting multiple unique properties per entity? If I can see it correctly you have just one?

@jexp
Copy link
Contributor

jexp commented Feb 15, 2012

Can you please undo the formatting changes and first pull the latest changes from master before re-issuing the pull request (and remove the arrayoutofboundsexception-commit).

You might create a separate branch which you pull spring-data-neo4j:master into and then apply your commits/changes by cherry picking.

What happens if unique properties are changed? As far as I can see the index is not updated and also there is no check for conflicts? That's why I think an @indexed(type=UNIQUE) would make more sense (then the unique-information would be part of the IndexInfo of PersistentProperty).

Also I think that a separate index name for unique values is not needed (and would also be confusing). Right now only fulltext fields and spatial require a different index name b/c they have a different index-configuration.

@rgan
Copy link
Author

rgan commented Feb 28, 2012

Noticed that you have implemented uniqueness support. Thanks. Your suggestion of using @indexed(indexType= IndexType.UNIQUE) makes more sense - since it does not make sense for full text indexes to be unique? I have been working on this as well and found that putIfAbsent does not seem to work well with full text indexes - it is case-sensitive. Also, supporting multiple unique properties per entity turned out to be quite complicated.
In IndexingPropertyFieldAccessorListener, you remove the entity from the index before trying to index it with putIfAbsent() and the new value. If this fails, then the entity is no longer indexed with the old value. It would be better to try putIfAbsent() first for the updated key/value pair and then remove it from the index only it if succeeds?

@jexp
Copy link
Contributor

jexp commented Feb 28, 2012

@rgan Thanks again for providing the code, I implemented the approach you suggested with a few changes.
Actually I spoke with Mattias and every Index-Provider that supports putIfAbsent should work with the unique approach, even fulltext and spatial indexes. So I added @indexed(unique=true)

I also kept the limitiation of one unique field per entity but added checks when the field is changed.

Good point with the listener, normally the transactiona would be rolled back anyway leaving everything unchanged. It might fail in the REST case though.

@rgan
Copy link
Author

rgan commented Feb 28, 2012

One more question - it seems better to throw an exception when someone tried to create an entity with the same unique key/value pair, rather than simply returning the existing entity. So, this test should actually throw an exception ?
@test
public void shouldOnlyCreateSingleInstanceForUniqueNodeEntity() {
UniqueClub club = new UniqueClub();
club.setName("foo");
uniqueClubRepository.save(club);

 club = new UniqueClub(); club.setName("foo"); uniqueClubRepository.save(club); assertEquals(1, uniqueClubRepository.count()); } 
@jexp
Copy link
Contributor

jexp commented Feb 28, 2012

It is difficult technically as I would like to use the existing UniqueFactory implementations which are rather getOrCreate than fail on duplicate creation.

After all the entity is not created.

And the UniqueFactory is also supported over REST which you didn't tackle in your changes.

@jexp jexp closed this Mar 2, 2012
michael-simons added a commit that referenced this pull request Jul 2, 2020
This adds extensions for `as` and `in` and closes #37.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants