DATAGRAPH-1428 - Implement deleteAllById(Iterable<ID> ids). #554

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.

Sign up for GitHub

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

Jump to bottom

Closed

schauder wants to merge 4 commits into master from issue/DATAGRAPH-1428

Contributor

schauder commented Nov 13, 2020

Merge right after DATACMNS-800


          DATAGRAPH-1428 - Prepare branch

9ca86ee

michael-simons requested changes

View reviewed changes

Collaborator

michael-simons left a comment

Thanks for adding this from VMWare's side, much appreciate it.

Please be so kind and remove the unrelated chances and have a look at the imperative test. While the reactive makes complete sense, I don't understand the unused additional person instance.

Thanks.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

-              import org.springframework.data.neo4j.integration.shared.common.ThingWithAssignedId;
-              import org.springframework.data.neo4j.integration.shared.common.ThingWithGeneratedId;
-              import org.springframework.data.neo4j.integration.shared.common.WorksInClubRelationship;
+              import org.springframework.data.neo4j.integration.shared.common.*;

Collaborator

michael-simons Nov 16, 2020

Can we keep the explicit imports, please?

Member

mp911de Nov 16, 2020

Is there an import style config that we can configure on our side when touching Spring Data Neo4j? In other Spring Data modules, we apply https://github.com/spring-projects/spring-data-build/tree/master/etc/ide#intellij-setup.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

+              import java.util.stream.IntStream;
+              import java.util.stream.StreamSupport;
+              import static org.assertj.core.api.Assertions.*;

Collaborator

michael-simons Nov 16, 2020

Why the order change?

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

               			assertThat(optionalPerson).isPresent().contains(person1);
               		}
-              		@Test // GH-112
+              		@Test

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

               			assertThat(persons).containsExactlyInAnyOrder(person1, person2);
               		}
-              		@Test // DATAGRAPH-1374

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

               			assertThat(slice.hasNext()).isFalse();
               		}
-              		@Test // DATAGRAPH-1412

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

@@ @@ -2610,7 +2598,8 @@ void count(@Autowired PersonRepository repository) { @@
               			assertThat(repository.count()).isEqualTo(2);
               		}
-              		@Test // GH-112

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

-              	interface SimilarThingRepository extends Neo4jRepository<SimilarThing, Long> {}
+              	interface SimilarThingRepository extends Neo4jRepository<SimilarThing, Long> {
+              	}
-              	interface BaseClassRepository extends Neo4jRepository<Inheritance.BaseClass, Long> {}
+              	interface BaseClassRepository extends Neo4jRepository<Inheritance.BaseClass, Long> {
+              	}

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

Comment on lines 3237 to 3240

-              	interface BaseClassWithLabelsRepository extends Neo4jRepository<Inheritance.BaseClassWithLabels, Long> {}
+              	interface BaseClassWithLabelsRepository extends Neo4jRepository<Inheritance.BaseClassWithLabels, Long> {
+              	}
               	interface EntityWithConvertedIdRepository
-              			extends Neo4jRepository<EntityWithConvertedId, EntityWithConvertedId.IdentifyingEnum> {}
+              			extends Neo4jRepository<EntityWithConvertedId, EntityWithConvertedId.IdentifyingEnum> {
+              	}

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

Comment on lines 3248 to 3249

-              	interface FriendRepository extends Neo4jRepository<Friend, Long> {}
+              	interface FriendRepository extends Neo4jRepository<Friend, Long> {
+              	}

Collaborator

michael-simons Nov 16, 2020

Please undo.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

Comment on lines 3282 to 3286

-              		@Autowired private Driver driver;
+              		@Autowired
+              		private Driver driver;

Collaborator

michael-simons Nov 16, 2020

Please undo.

mp911de reviewed

View reviewed changes

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

-              import org.springframework.data.neo4j.integration.shared.common.ThingWithAssignedId;
-              import org.springframework.data.neo4j.integration.shared.common.ThingWithGeneratedId;
-              import org.springframework.data.neo4j.integration.shared.common.WorksInClubRelationship;
+              import org.springframework.data.neo4j.integration.shared.common.*;

Member

mp911de Nov 16, 2020

Is there an import style config that we can configure on our side when touching Spring Data Neo4j? In other Spring Data modules, we apply https://github.com/spring-projects/spring-data-build/tree/master/etc/ide#intellij-setup.

src/test/java/org/springframework/data/neo4j/integration/imperative/RepositoryIT.java Outdated

               		}
               	}
-              	interface BidirectionalStartRepository extends Neo4jRepository<BidirectionalStart, Long> {}
+              	interface BidirectionalStartRepository extends Neo4jRepository<BidirectionalStart, Long> {

Member

mp911de Nov 16, 2020

Our formatter config is at https://github.com/spring-projects/spring-data-build/blob/master/etc/ide/eclipse-formatting.xml. Let me know whether the formatting changes here stem from our config so we can fix that on our side.

Collaborator

michael-simons Nov 16, 2020

I thought we had this applied. My bad if that isn't the case.

Regarding the star imports: Isn't the common agreement that they should be avoided? At least that was my impression when I did this: 7804bdc

Member

mp911de Nov 16, 2020

I think your code is fine, I assume some IDE settings or an outdated config on our side.

RE star imports: Each Spring project has its own approach to code style. Following the Boot style is fine, it would be good to document that somewhere as the Spring Data import style looks different.

Collaborator

michael-simons Nov 16, 2020

I am a bit surprised that Jenkins is happy… I just tracked the branch and a mvn checkstyle:check@validate fails as expected. Checkstyle runs in verify phase, after all tracks (as we discussed a while back in d824e5d).

Member

mp911de Nov 16, 2020

Actually, the associated Job fails (https://jenkins.spring.io/view/SpringData/job/spring-data-neo4j/job/issue%252FDATAGRAPH-1428/4/). It's green because we suppress upstream trigger causes. For some reason, the suppression still creates a no-op job that appears to be green.

Contributor Author

schauder Nov 24, 2020 •

edited

Loading

I didn't have the proper code formatting applied. I fixed that but still have some changes left. I'll undo them manually.

The points where the formatting of Neo4J differs from Spring Data seems to be:

Spring Data Formatting prescribes star imports for static imports and for 10 or more imports.
The import order seams to be subtly different, the ordering of subpackages is different.

mp911de mentioned this pull request

DATACMNS-800 - Adds deleteAllById to (Reactive)CrudRepository. spring-projects/spring-data-commons#476

Closed

mp911de approved these changes

View reviewed changes

michael-simons and others added 3 commits

November 24, 2020 14:36


          DATAGRAPH-1428 - Allow branch-snapshots of Data-Commons in StartupLog…

02b2b65

…gerTest, too.

This allows for development against changes in commons which aren't on master yet.


          DATAGRAPH-1428 - Implement deleteAllById(Iterable<ID> ids).

e533a75


          DATAGRAPH-1428 - Save additional person for test.

d7aef08

The additional person allows to actually delete multiple persons, without deleting all of them.

schauder force-pushed the issue/DATAGRAPH-1428 branch from 19e4089 to d7aef08 Compare

November 24, 2020 14:09

Contributor Author

schauder commented Nov 24, 2020

have a look at the imperative test. While the reactive makes complete sense, I don't understand the unused additional person instance.

The third person now gets saved, allowing us to delete more than one person without deleting all.

mp911de pushed a commit that referenced this pull request


          DATAGRAPH-1428 - Implement CrudRepository and ReactiveCrudRepository.…

bcddf8e

…deleteAllById(Iterable<ID> ids).

Original pull request: #554.

mp911de pushed a commit that referenced this pull request


          DATAGRAPH-1428 - Allow branch-snapshots of Data-Commons in StartupLog…

04e095d

…gerTest, too.

This allows for development against changes in commons which aren't on master yet.

Original pull request: #554.

mp911de added a commit that referenced this pull request


          DATAGRAPH-1428 - Polishing.

3786ecd

Reorder methods. Fix import ordering.

Original pull request: #554.

Member

mp911de commented Nov 25, 2020

That's merged and polished now.

mp911de closed this

mp911de deleted the issue/DATAGRAPH-1428 branch

November 25, 2020 13:36

This was referenced Dec 31, 2020

Implement CrudRepository.delete(Iterable<ID> ids). [DATAGRAPH-1428] #1989

Closed

Build is failing and PR says "All checks have passed." [DATAGRAPH-1432] #1994

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet