Translate projected properties of the fluent query API into a fetchgraph. #2345

schauder · 2021-10-26T14:37:34Z

When a property path based projection is specified we still return the root entity.
But we do provide a fetchgraph.
The JPA implementation will (should) load only the specified attributes eagerly.
It most likely will also load all other attributes from all selected tables.

Once we have infrastructure in place for for multilevel projections the same approach can and should be used for those.
Currently this is not the case.

Closes #2329
See spring-projects/spring-data-commons#2420

Comments and formatting.

gregturn

Impeccable work!

My comments were simple format opinions.

Question: Are we forced to the use Query DSL's impl class of AbstractJPAQuery so that we can get ahold of their setHint method?

gregturn · 2021-10-26T15:32:40Z

src/test/java/org/springframework/data/jpa/repository/support/DefaultProjectorUnitTests.java

+
+	@BeforeEach
+	void beforeEach() {
+		entityGraph = mock(EntityGraph.class, RETURNS_DEEP_STUBS);


Nit: Do we need a blank link here?

gregturn · 2021-10-26T15:35:35Z

...a/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java

+		em.clear();
+
+		assertThat(users).extracting(User::getFirstname) //
+				.containsExactlyInAnyOrder( //


Personally: I prefer the contains on the previous line instead of its own line.

gregturn · 2021-10-26T15:36:31Z

...a/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java

+		em.clear();
+
+		assertThat(users).extracting(User::getFirstname) //
+				.containsExactlyInAnyOrder( //


See earlier.

mp911de · 2021-10-27T06:52:41Z

src/main/java/org/springframework/data/jpa/repository/support/DefaultProjector.java

+
+			for (String property : properties) {
+
+				final String[] split = property.split("\\.");


You should be able to convert properties into PropertyPath.

mp911de · 2021-10-27T06:53:19Z

src/main/java/org/springframework/data/jpa/repository/support/Projector.java

+ */
+public interface Projector {
+
+	AbstractJPAQuery<?, ?> apply(Class<?> domainType, AbstractJPAQuery<?, ?> query, @Nullable Set<String> properties);


@Nullable doesn't work well with collections. We rather should have an empty collection.

mp911de · 2021-10-27T06:54:48Z

...a/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java

+				.isThrownBy( //
+						() -> users.forEach( //
+								u -> u.getRoles().forEach(r -> { //
+									System.out.println(r.getName() //


mp911de · 2021-10-27T06:55:01Z

...a/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutorUnitTests.java

+						() -> users.forEach( //
+								u -> u.getRoles().forEach(r -> { //
+									System.out.println(r.getName() //
+									); //


These are a lot of //…

If I let Eclipse formatter have its way, I can't read the code anymore :-/

Simplified the test so we need less // to make it readable.

gregturn · 2021-10-27T14:09:50Z

...in/java/org/springframework/data/jpa/repository/support/FetchableFluentQueryByPredicate.java

+	private AbstractJPAQuery<?, ?> createSortedAndProjectedQuery() {
+
+		final AbstractJPAQuery<?, ?> query = this.finder.apply(this.sort);
+		projector.apply(entityType, query, properties);


Can we be consistent on using this?

We should be consistent on not using this which is what we do in the rest of the code base, except for constructor and setters where parameters have the same names as fields.

mp911de · 2021-10-29T07:29:42Z

src/main/java/org/springframework/data/jpa/repository/support/Projector.java

+		this.entityManager = entityManager;
+	}
+
+	public void apply(Class<?> domainType, Q query, Set<String> properties) {


I think it would be neat to turn this method into createEntityGraph and inverse the responsibilities. Instead of apply(…) here and the abstract method, it would be good to have more encapsulation and not carry the query type around.

Since this is all internal API, we're free to change it, but it would help to understand this processing better.

…aph. When a property path based projection is specified we still return the root entity. But we do provide a fetchgraph. The JPA implementation will (should) load only the specified attributes eagerly. It most likely will also load all other attributes from all selected tables. Once we have infrastructure in place for for multilevel projections the same approach can and should be used for those. Currently this is not the case. Closes #2329 Original pull request: #2345.

Rename Projector to EntityGraphFactory. Remove QuerydslProjector and TypedQueryProjector. Remove MappingContext creation. See #2329 Original pull request: #2345.

…aph. When a property path based projection is specified we still return the root entity. But we do provide a fetchgraph. The JPA implementation will (should) load only the specified attributes eagerly. It most likely will also load all other attributes from all selected tables. Once we have infrastructure in place for for multilevel projections the same approach can and should be used for those. Currently this is not the case. Closes #2329 Original pull request: #2345.

Rename Projector to EntityGraphFactory. Remove QuerydslProjector and TypedQueryProjector. Remove MappingContext creation. See #2329 Original pull request: #2345.

For some reason, EclipseLinkNamespaceUserRepositoryTests freezes. Can't figure out the source, so we are disabling it for now. See #2329 Original pull request: #2345.

schauder added 2 commits October 22, 2021 13:28

2329-consider-property-paths - Prepare branch

878f2e2

Polishing.

02a9c63

Comments and formatting.

schauder requested review from gregturn and mp911de October 26, 2021 14:37

gregturn reviewed Oct 26, 2021

View reviewed changes

mp911de reviewed Oct 27, 2021

View reviewed changes

schauder force-pushed the issue/2329-consider-property-paths branch 2 times, most recently from 82d845b to 1196a89 Compare October 27, 2021 13:56

gregturn reviewed Oct 27, 2021

View reviewed changes

mp911de reviewed Oct 29, 2021

View reviewed changes

mp911de self-assigned this Oct 29, 2021

mp911de added this to the 2.6 GA (2021.1.0) milestone Oct 29, 2021

schauder and others added 2 commits October 29, 2021 12:02

Polishing.

811d2f1

Rename Projector to EntityGraphFactory. Remove QuerydslProjector and TypedQueryProjector. Remove MappingContext creation. See #2329 Original pull request: #2345.

mp911de force-pushed the issue/2329-consider-property-paths branch from 1b41ffb to 811d2f1 Compare October 29, 2021 10:06

gregturn pushed a commit that referenced this pull request Nov 4, 2021

Polishing.

d637ce3

Rename Projector to EntityGraphFactory. Remove QuerydslProjector and TypedQueryProjector. Remove MappingContext creation. See #2329 Original pull request: #2345.

gregturn closed this Nov 4, 2021

gregturn deleted the issue/2329-consider-property-paths branch November 4, 2021 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate projected properties of the fluent query API into a fetchgraph. #2345

Translate projected properties of the fluent query API into a fetchgraph. #2345

schauder commented Oct 26, 2021 •

edited

Loading

gregturn left a comment •

edited

Loading

gregturn Oct 26, 2021

gregturn Oct 26, 2021

gregturn Oct 26, 2021

mp911de Oct 27, 2021

mp911de Oct 27, 2021

mp911de Oct 27, 2021

mp911de Oct 27, 2021

schauder Oct 27, 2021

schauder Oct 27, 2021

gregturn Oct 27, 2021

schauder Oct 28, 2021

mp911de Oct 29, 2021


		for (String property : properties) {

		final String[] split = property.split("\\.");

Translate projected properties of the fluent query API into a fetchgraph. #2345

Translate projected properties of the fluent query API into a fetchgraph. #2345

Conversation

schauder commented Oct 26, 2021 • edited Loading

gregturn left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schauder commented Oct 26, 2021 •

edited

Loading

gregturn left a comment •

edited

Loading