-
Notifications
You must be signed in to change notification settings - Fork 1.3k
DATAES-462 - Add score mapping support to DefaultResultMapper #207
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package org.springframework.data.elasticsearch.annotations; | ||
|
||
import java.lang.annotation.Documented; | ||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Inherited; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
import org.springframework.data.annotation.ReadOnlyProperty; | ||
|
||
/** | ||
* Specifies that this field is used for storing the document score. | ||
* | ||
* @author Sascha Woo | ||
*/ | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Target(ElementType.FIELD) | ||
@Documented | ||
@Inherited | ||
@ReadOnlyProperty | ||
public @interface Score { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,7 @@ | |
* @author Chris White | ||
* @author Mark Paluch | ||
* @author Ilkang Na | ||
* @author Sascha Woo | ||
*/ | ||
public class DefaultResultMapper extends AbstractResultMapper { | ||
|
||
|
@@ -62,7 +63,8 @@ public DefaultResultMapper() { | |
super(new DefaultEntityMapper()); | ||
} | ||
|
||
public DefaultResultMapper(MappingContext<? extends ElasticsearchPersistentEntity<?>, ElasticsearchPersistentProperty> mappingContext) { | ||
public DefaultResultMapper( | ||
MappingContext<? extends ElasticsearchPersistentEntity<?>, ElasticsearchPersistentProperty> mappingContext) { | ||
super(new DefaultEntityMapper()); | ||
this.mappingContext = mappingContext; | ||
} | ||
|
@@ -81,6 +83,8 @@ public DefaultResultMapper( | |
@Override | ||
public <T> AggregatedPage<T> mapResults(SearchResponse response, Class<T> clazz, Pageable pageable) { | ||
long totalHits = response.getHits().getTotalHits(); | ||
float maxScore = response.getHits().getMaxScore(); | ||
|
||
List<T> results = new ArrayList<>(); | ||
for (SearchHit hit : response.getHits()) { | ||
if (hit != null) { | ||
|
@@ -90,14 +94,17 @@ public <T> AggregatedPage<T> mapResults(SearchResponse response, Class<T> clazz, | |
} else { | ||
result = mapEntity(hit.getFields().values(), clazz); | ||
} | ||
|
||
setPersistentEntityId(result, hit.getId(), clazz); | ||
setPersistentEntityVersion(result, hit.getVersion(), clazz); | ||
setPersistentEntityScore(result, hit.getScore(), clazz); | ||
populateScriptFields(result, hit); | ||
results.add(result); | ||
} | ||
} | ||
|
||
return new AggregatedPageImpl<T>(results, pageable, totalHits, response.getAggregations(), response.getScrollId()); | ||
return new AggregatedPageImpl<T>(results, pageable, totalHits, response.getAggregations(), response.getScrollId(), | ||
maxScore); | ||
} | ||
|
||
private <T> void populateScriptFields(T result, SearchHit hit) { | ||
|
@@ -112,8 +119,8 @@ private <T> void populateScriptFields(T result, SearchHit hit) { | |
try { | ||
field.set(result, searchHitField.getValue()); | ||
} catch (IllegalArgumentException e) { | ||
throw new ElasticsearchException("failed to set scripted field: " + name + " with value: " | ||
+ searchHitField.getValue(), e); | ||
throw new ElasticsearchException( | ||
"failed to set scripted field: " + name + " with value: " + searchHitField.getValue(), e); | ||
} catch (IllegalAccessException e) { | ||
throw new ElasticsearchException("failed to access scripted field: " + name, e); | ||
} | ||
|
@@ -177,23 +184,19 @@ public <T> LinkedList<T> mapResults(MultiGetResponse responses, Class<T> clazz) | |
} | ||
|
||
private <T> void setPersistentEntityId(T result, String id, Class<T> clazz) { | ||
|
||
if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) { | ||
|
||
ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz); | ||
ElasticsearchPersistentProperty idProperty = persistentEntity.getIdProperty(); | ||
|
||
// Only deal with String because ES generated Ids are strings ! | ||
if (idProperty != null && idProperty.getType().isAssignableFrom(String.class)) { | ||
persistentEntity.getPropertyAccessor(result).setProperty(idProperty, id); | ||
} | ||
|
||
} | ||
} | ||
|
||
private <T> void setPersistentEntityVersion(T result, long version, Class<T> clazz) { | ||
if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) { | ||
|
||
ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getPersistentEntity(clazz); | ||
ElasticsearchPersistentProperty versionProperty = persistentEntity.getVersionProperty(); | ||
|
||
|
@@ -206,4 +209,16 @@ private <T> void setPersistentEntityVersion(T result, long version, Class<T> cla | |
} | ||
} | ||
} | ||
|
||
private <T> void setPersistentEntityScore(T result, float score, Class<T> clazz) { | ||
if (mappingContext != null && clazz.isAnnotationPresent(Document.class)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need that guard? Shouldn't the constructor of |
||
ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz); | ||
ElasticsearchPersistentProperty scoreProperty = persistentEntity.getScoreProperty(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shall we use |
||
Class<?> type = scoreProperty.getType(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to already reject score properties with other types than |
||
|
||
if (scoreProperty != null && (type == Float.class || type == Float.TYPE)) { | ||
persistentEntity.getPropertyAccessor(result).setProperty(scoreProperty, score); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
|
||
package org.springframework.data.elasticsearch.core; | ||
|
||
import org.springframework.data.domain.Page; | ||
|
||
/** | ||
* A score-aware page gaining information about max score. | ||
* | ||
* @param <T> | ||
* @author Sascha Woo | ||
*/ | ||
public interface ScoredPage<T> extends Page<T> { | ||
|
||
float getMaxScore(); | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,13 +16,15 @@ | |
package org.springframework.data.elasticsearch.core.mapping; | ||
|
||
import org.springframework.data.mapping.PersistentEntity; | ||
import org.springframework.lang.Nullable; | ||
|
||
/** | ||
* ElasticsearchPersistentEntity | ||
* | ||
* @author Rizwan Idrees | ||
* @author Mohsin Husen | ||
* @author Mark Paluch | ||
* @author Sascha Woo | ||
*/ | ||
public interface ElasticsearchPersistentEntity<T> extends PersistentEntity<T, ElasticsearchPersistentProperty> { | ||
|
||
|
@@ -49,4 +51,22 @@ public interface ElasticsearchPersistentEntity<T> extends PersistentEntity<T, El | |
String settingPath(); | ||
|
||
boolean isCreateIndexAndMapping(); | ||
|
||
/** | ||
* 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add |
||
*/ | ||
boolean hasScoreProperty(); | ||
|
||
/** | ||
* Returns the score property of the {@link ElasticsearchPersistentEntity}. Can be {@literal null} in case no score | ||
* property is available on the entity. | ||
* | ||
* @return the score {@link ElasticsearchPersistentProperty} of the {@link PersistentEntity} or {@literal null} if not | ||
* defined. | ||
*/ | ||
@Nullable | ||
ElasticsearchPersistentProperty getScoreProperty(); | ||
} |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thePersistentEntity
metamodel to drop the properties that Jackson has discovered in case aPersistentProperty
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, will rebase the PR tomorrow and address the other stuff as well.