DATAES-462 - Add score mapping support to DefaultResultMapper #207

xhaggi · 2018-06-13T10:10:53Z

See https://jira.spring.io/browse/DATAES-462

You have read the Spring Data contribution guidelines.
There is a ticket in the bug tracker for the project in our JIRA.
You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.
You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

odrotbohm · 2018-06-13T10:20:32Z

src/main/java/org/springframework/data/elasticsearch/core/ScoredEntity.java

+
+	float getScore();
+
+	void setScore(float score);


Is that an interface that's supposed to be implemented by user code? If so, does it makes sense that the score can be modified? That feels quite invasive. Should we provide an annotation based approach instead, so that users can simply annotate a field in their domain class and we invoke this reflectively (via PersistentEntity.getPropertyAccessor(…).setProperty(scoreProperty, value); in DefaultResultMapper? I guess, that would need ElasticsearchPersistentEntity` to expose a score property.

After talking to @christophstrobl, it looks like the Solr module has an @Score annotation that's a @ReadOnlyProperty. Would be cool if you could follow that design. A good starting point for the integration into the metamodel is probably SolrPersistentEntity.

Thought exactly the same before I saw this comment ;) Will address your comments

Feel free to ping me in Gitter if you need help regarding the meta-model and property access.

odrotbohm · 2018-06-13T10:26:05Z

src/main/java/org/springframework/data/elasticsearch/core/ScoredPage.java

+/**
+ * @author Sascha Woo
+ */
+public interface ScoredPage<T> extends Page<T> {


A bit of Javadoc highly appreciated :).

odrotbohm · 2018-06-13T10:28:05Z

src/main/java/org/springframework/data/elasticsearch/core/query/AbstractQuery.java

+		return trackScores;
+	}
+
+	public void setTrackScores(boolean trackScores) {


At a quick glance, this setter doesn't seem to be used anywhere, right?

Oh, it is. From NativeSearchQueryBuilder. However, it should be in package scope then to limit the API surface.

This setter needs to be public for cases where NativeSearchQueryBuilder is not used.

I am probably too unfamiliar with the ES specifics here. I am merely looking at this from an "How much API do we expose?" point of view and API that assumes or exposes mutability is usually candidate for extra scrutiny.

Can you elaborate on the use case? I guess you're referring to the usage via CriteriaQuery, StringQuery and the likes?

There are cases in which the query is created using the constructors of NativeSearchQuery and then additional setters are called. If we want to enforce the use of the NativeSearchQueryBuilder, all constructors and setters should be package scoped. But we should not do that, as it would break existing code. So in my opinion, this setter should be public analog to the other setters. In our internal projects, we implemented a less complex search query builder implementation which needs to have access to the setTrackScores method.

Understood, thanks. Looks like Sascha opened up the visibility to public again.

Yeah we discussed it before.

xhaggi · 2018-06-13T11:26:21Z

@olivergierke pushed some fixups to address your comments and drop the interface ScoredEntity in favor of @Score annotation. Mind taking another look? I'll squash everything together if you are happy ;)

odrotbohm · 2018-06-13T12:12:59Z

src/main/java/org/springframework/data/elasticsearch/annotations/Score.java

+@Target(ElementType.FIELD)
+@Documented
+@Inherited
+public @interface Score {


Let's meta-annotate this with @ReadOnlyProperty so that @Transient is not needed in the domain model as we always consider a score property metadata, right?

Ah thx, don't know that this is possible ;)

odrotbohm · 2018-06-13T12:14:45Z

Let me take over quickly. I'll add a commit that takes care of proper integration with the MappingContext.

xhaggi · 2018-06-13T12:24:39Z

Let me take over quickly. I'll add a commit that takes care of proper integration with the MappingContext.

I'm on it. I need some time to push another fixup that address it.

odrotbohm · 2018-06-13T12:26:14Z

Okay, ping me if I should take over again. I was planning to literally copy the code of Spring Data Solr in …PersistentEntity / …PersistentProperty and move over the code in the mapper to work with the entity, use the PropertyAccessor etc.

xhaggi · 2018-06-13T13:42:18Z

@olivergierke force-pushed with everything addressed. Now it use the mapping context + meta model to determine the score property etc.

odrotbohm · 2018-06-13T13:46:43Z

src/main/java/org/springframework/data/elasticsearch/core/DefaultResultMapper.java

@@ -206,4 +209,16 @@ private String buildJSONFromFields(Collection<DocumentField> values) {
 			}
 		}
 	}
+
+	private <T> void setPersistentEntityScore(T result, float score, Class<T> clazz) {
+		if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) {


Do we need that guard? Shouldn't the constructor of DefaultResultMapper already make sure that it's never null? What's the second guard forI'd vote to avoid any Java-native reflection in the context of actually working with the metamodel.

odrotbohm · 2018-06-13T13:47:24Z

src/main/java/org/springframework/data/elasticsearch/core/DefaultResultMapper.java

+	private <T> void setPersistentEntityScore(T result, float score, Class<T> clazz) {
+		if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) {
+			ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz);
+			ElasticsearchPersistentProperty scoreProperty = persistentEntity.getScoreProperty();


Shall we use ….hasScoreProperty() here to avoid the downstream null check?

odrotbohm · 2018-06-13T13:48:49Z

src/main/java/org/springframework/data/elasticsearch/core/DefaultResultMapper.java

+		if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) {
+			ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz);
+			ElasticsearchPersistentProperty scoreProperty = persistentEntity.getScoreProperty();
+			Class<?> type = scoreProperty.getType();


Does it make sense to already reject score properties with other types than float/Float when the ElasticsearchPersistentProperty is created? That would fail much faster and not let the application bootstrap and the score not being mapped without any indication that the type doesn't match.

odrotbohm · 2018-06-13T13:49:44Z

.../java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java

+	 * Returns whether the {@link ElasticsearchPersistentEntity} has an score property. If this call returns
+	 * {@literal true}, {@link #getScoreProperty()} will return a non-{@literal null} value.
+	 *
+	 * @return false when {@link ElasticsearchPersistentEntity} does not define a score property.


Let's add @since 3.1 for newly introduced API. Probably also for the method below, and ScoredPage etc.

odrotbohm · 2018-06-13T13:51:00Z

...org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java

+								+ "as version. Check your mapping configuration!", property.getField(), scoreProperty.getField()));
+			}
+
+			Assert.isTrue(property.getType() == Float.class || property.getType() == Float.TYPE, "Score property must be of type float!");


I'd move that check into the creation of the ElasticsearchPersistentProperty.

odrotbohm · 2018-06-13T13:54:26Z

src/main/java/org/springframework/data/elasticsearch/annotations/Score.java

+@Target(ElementType.FIELD)
+@Documented
+@Inherited
+@ReadOnlyProperty


I've just checked the EntityWriter implementation and it looks like it's solely based on Jackson. This unfortunately makes it impossible that our mapping annotations are considered, as we'd effectively have to translate @ReadOnlyProperty into @JsonIgnore.

This seems to be a bigger issue, and ideally I'd like to move off Jackson at some point entirely (as Jackson annotations might be used to customize the web representation of domain types and thus couple the store representation to the external one). However, do you think we can register a Jackson module that registers a BeanSerializerModifier and in ….changeProperties(…) it uses the PersistentEntity metamodel to drop the properties that Jackson has discovered in case a PersistentProperty is annotated with @ReadOnlyProperty?

Feel free to create a separate ticket for that and assign to me. I guess it's quite a bit involved additional code that's needed. However, without that, what we have is not going to work properly as an @Transient on the property will make the property disappear from the mapping context, i.e. the code that's trying to populate the property is never going to find a score property.

I've just filed DATAES-464 and am going to push a fix for that. With that in place, the read-only properties shouldn't be written to Elasticsearch.

Nice, will rebase the PR tomorrow and address the other stuff as well.

odrotbohm · 2018-06-13T16:26:32Z

I'll take it from here.

odrotbohm · 2018-06-13T17:01:34Z

I'm pretty much done with polish and merge.

Original pull request: #207.

@SInCE

SimpleElasticsearchPersistentProperty now already checks for the correct type of score properties. Added unit tests for that. Also added unit tests for SimpleElasticsearchPersistentEntity rejecting more than one score property being present. Additional non-null assertions on components that are required so that we can remove superfluous null checks. A bit o formatting, Javadoc, missing @SInCE tags and license headers. Original pull request: #207.

xhaggi · 2018-06-13T17:14:43Z

Thanks for taking over.

@SInCE

SimpleElasticsearchPersistentProperty now already checks for the correct type of score properties. Added unit tests for that. Also added unit tests for SimpleElasticsearchPersistentEntity rejecting more than one score property being present. Additional non-null assertions on components that are required so that we can remove superfluous null checks. A bit o formatting, Javadoc, missing @SInCE tags and license headers. Original pull request: spring-projects#207.

xhaggi requested a review from akonczak June 13, 2018 10:11

odrotbohm requested changes Jun 13, 2018

View reviewed changes

odrotbohm self-assigned this Jun 13, 2018

odrotbohm requested changes Jun 13, 2018

View reviewed changes

odrotbohm assigned xhaggi and unassigned odrotbohm Jun 13, 2018

xhaggi added 2 commits June 13, 2018 15:37

DATAES-462 - add query builder option to track scores

69a15c7

DATAES-462 - add support for mapping max score and document scores

b3185ef

xhaggi force-pushed the DATAES-462 branch from 96fffe7 to b3185ef Compare June 13, 2018 13:40

odrotbohm reviewed Jun 13, 2018

View reviewed changes

odrotbohm assigned odrotbohm and unassigned xhaggi Jun 13, 2018

odrotbohm pushed a commit that referenced this pull request Jun 13, 2018

DATAES-462 - Add query builder option to track scores.

1126002

Original pull request: #207.

odrotbohm pushed a commit that referenced this pull request Jun 13, 2018

DATAES-462 - Add support for mapping max score and document scores.

d996406

Original pull request: #207.

odrotbohm approved these changes Jun 13, 2018

View reviewed changes

odrotbohm closed this Jun 13, 2018

xhaggi deleted the DATAES-462 branch June 14, 2018 04:41

spring-projects-issues mentioned this pull request Dec 31, 2020

Add support for mapping document scores to entities [DATAES-462] #1040

Closed

DATAES-462 - Add score mapping support to DefaultResultMapper #207

DATAES-462 - Add score mapping support to DefaultResultMapper #207

Uh oh!

Conversation

xhaggi commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xhaggi commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm commented Jun 13, 2018

Uh oh!

xhaggi commented Jun 13, 2018

Uh oh!

odrotbohm commented Jun 13, 2018

Uh oh!

xhaggi commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

odrotbohm commented Jun 13, 2018

Uh oh!

odrotbohm commented Jun 13, 2018

Uh oh!

xhaggi commented Jun 13, 2018

Uh oh!

Uh oh!

odrotbohm Jun 13, 2018 •

edited

Loading

odrotbohm Jun 13, 2018 •

edited

Loading