Skip to content

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

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Copy link
Member

@odrotbohm odrotbohm Jun 13, 2018

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 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.

Copy link
Member

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.

Copy link
Contributor Author

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.

public @interface Score {
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
* @author Chris White
* @author Mark Paluch
* @author Ilkang Na
* @author Sascha Woo
*/
public class DefaultResultMapper extends AbstractResultMapper {

Expand All @@ -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;
}
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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);
}
Expand Down Expand Up @@ -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();

Expand All @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ElasticsearchPersistentEntity<?> persistentEntity = mappingContext.getRequiredPersistentEntity(clazz);
ElasticsearchPersistentProperty scoreProperty = persistentEntity.getScoreProperty();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Class<?> type = scoreProperty.getType();
Copy link
Member

Choose a reason for hiding this comment

The 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 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.


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
Expand Up @@ -991,7 +991,8 @@ private SearchRequestBuilder prepareSearch(Query query) {
SearchRequestBuilder searchRequestBuilder = client.prepareSearch(toArray(query.getIndices()))
.setSearchType(query.getSearchType())
.setTypes(toArray(query.getTypes()))
.setVersion(true);
.setVersion(true)
.setTrackScores(query.getTrackScores());

if (query.getSourceFilter() != null) {
SourceFilter sourceFilter = query.getSourceFilter();
Expand Down
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
Expand Up @@ -3,12 +3,14 @@
import org.elasticsearch.search.aggregations.Aggregation;
import org.elasticsearch.search.aggregations.Aggregations;
import org.springframework.data.elasticsearch.core.FacetedPage;
import org.springframework.data.elasticsearch.core.ScoredPage;
import org.springframework.data.elasticsearch.core.ScrolledPage;

/**
* @author Petar Tahchiev
* @author Sascha Woo
*/
public interface AggregatedPage<T> extends FacetedPage<T>, ScrolledPage<T> {
public interface AggregatedPage<T> extends FacetedPage<T>, ScrolledPage<T>, ScoredPage<T> {

boolean hasAggregations();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
*/
package org.springframework.data.elasticsearch.core.aggregation.impl;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

import org.elasticsearch.search.aggregations.Aggregation;
import org.elasticsearch.search.aggregations.Aggregations;
Expand All @@ -29,55 +27,77 @@
* @author Petar Tahchiev
* @author Artur Konczak
* @author Mohsin Husen
* @author Sascha Woo
*/
public class AggregatedPageImpl<T> extends FacetedPageImpl<T> implements AggregatedPage<T> {

private Aggregations aggregations;
private Map<String, Aggregation> mapOfAggregations = new HashMap<>();
private String scrollId;
private String scrollId;
private float maxScore;

public AggregatedPageImpl(List<T> content) {
super(content);
}

public AggregatedPageImpl(List<T> content, float maxScore) {
super(content);
this.maxScore = maxScore;
}

public AggregatedPageImpl(List<T> content, String scrollId) {
super(content);
this.scrollId = scrollId;
}

public AggregatedPageImpl(List<T> content, String scrollId, float maxScore) {
this(content, scrollId);
this.maxScore = maxScore;
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total) {
super(content, pageable, total);
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, float maxScore) {
super(content, pageable, total);
this.maxScore = maxScore;
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, String scrollId) {
super(content, pageable, total);
this.scrollId = scrollId;
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, String scrollId, float maxScore) {
this(content, pageable, total, scrollId);
this.maxScore = maxScore;
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, Aggregations aggregations) {
super(content, pageable, total);
this.aggregations = aggregations;
if (aggregations != null) {
for (Aggregation aggregation : aggregations) {
mapOfAggregations.put(aggregation.getName(), aggregation);
}
}
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, Aggregations aggregations, String scrollId) {
super(content, pageable, total);
this.aggregations = aggregations;
public AggregatedPageImpl(List<T> content, Pageable pageable, long total, Aggregations aggregations, float maxScore) {
this(content, pageable, total, aggregations);
this.maxScore = maxScore;
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, Aggregations aggregations,
String scrollId) {
this(content, pageable, total, aggregations);
this.scrollId = scrollId;
if (aggregations != null) {
for (Aggregation aggregation : aggregations) {
mapOfAggregations.put(aggregation.getName(), aggregation);
}
}
}

public AggregatedPageImpl(List<T> content, Pageable pageable, long total, Aggregations aggregations, String scrollId,
float maxScore) {
this(content, pageable, total, aggregations, scrollId);
this.maxScore = maxScore;
}

@Override
public boolean hasAggregations() {
return aggregations != null && mapOfAggregations.size() > 0;
return aggregations != null;
}

@Override
Expand All @@ -94,4 +114,9 @@ public Aggregation getAggregation(String name) {
public String getScrollId() {
return scrollId;
}

@Override
public float getMaxScore() {
return maxScore;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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> {

Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/
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();
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,24 @@
*
* @author Rizwan Idrees
* @author Mohsin Husen
* @author Sascha Woo
*/

public interface ElasticsearchPersistentProperty extends PersistentProperty<ElasticsearchPersistentProperty> {

String getFieldName();

/**
* Returns whether the current property is a <em>potential</em> score property of the owning
* {@link ElasticsearchPersistentEntity}. This method is mainly used by {@link ElasticsearchPersistentEntity}
* implementation to discover score property candidates on {@link ElasticsearchPersistentEntity} creation you should
* rather call {@link ElasticsearchPersistentEntity#isScoreProperty(PersistentProperty)} to determine whether the
* current property is the version property of that {@link ElasticsearchPersistentEntity} under consideration.
*
* @return
*/
boolean isScoreProperty();

public enum PropertyToFieldNameConverter implements Converter<ElasticsearchPersistentProperty, String> {

INSTANCE;
Expand Down
Loading