spring-projects / spring-data-mongodb

Provides support to increase developer productivity in Java when using MongoDB. Uses familiar Spring concepts such as a template classes for core API usage and lightweight repository style data access.
https://spring.io/projects/spring-data-mongodb/
Apache License 2.0
1.62k stars 1.08k forks source link

Seperate `QueryMapper.getMappedObject()` recusive logic. #3824

Open RyanGibb opened 3 years ago

RyanGibb commented 3 years ago

Summary

This issue is proposing separating the recursive logic of QueryMappergetMappedObject() from the the public method it is invoked by. This allows to handle the outer Document query and the nested levels of Document query separately.

Detail

We (Cisco Defense Orchestrator) are overriding MongoTemplate to provide some restrictions on database access. Specifically, injecting a Criteria to all queries, setting a field on all objects stored in the database, and preventing that field being updated.

To inject the Criteria we’ve set MongoTemplate.queryMapper() to an overridden implementation of QueryMapper through reflection that modifies the Document query parameter of QueryMapper.getMappedObject().

An issue we’ve encountered is that QueryMapper.getMappedObject() is called recursively inside QueryMapper, so this modification is done at every level.

For example, if we had the query:

{ $text: { $search: "java coffee shop" } }

We would want:

{ $text: { $search: "java coffee shop" }, <new criteria> }

But we would get:

{ $text: { $search: "java coffee shop", <new criteria> }, <new criteria> }

Which fails with UncategorizedMongoDbException: Query failed with error code 2 and error message 'extra fields in $text'.

We solved this by setting adding an isNested parameter:

public class OverriddenQueryMapper extends QueryMapper {

  private ThreadLocal<Boolean> isNested = ThreadLocal.withInitial(() -> Boolean.FALSE);

  @SneakyThrows
  @Override
  public Document getMappedObject(Bson query, MongoPersistentEntity<?> entity) {
    if (isNested.get()) {
      return super.getMappedObject(query, entity);
    }
    <inject criteria here>
    isNested.set(Boolean.TRUE);
    Document mappedQuery = super.getMappedObject(query, entity);
    isNested.set(Boolean.FALSE);
    return mappedQuery;
  }

  @Override
  public Document getMappedFields(Document fieldsObject, MongoPersistentEntity<?> entity) {
    isNested.set(Boolean.TRUE);
    Document mappedFields = super.getMappedFields(fieldsObject, entity);
    isNested.set(Boolean.FALSE);
    return mappedFields;
  }
}

The ThreadLocal is for thread safety.

A nicer solution would be provided by introducing a separate protected recurse method called internally in QueryMapper, so that the public getMappedObject could be overridden without effecting recursive calls. See the proposed change here: https://github.com/RyanGibb/spring-data-mongodb/commit/19a1f759344715d4b0c6abea59b073cc5319f664

Any thoughts or feedback would be much appreciated.

christophstrobl commented 3 years ago

Thanks for bringing this up.

RyanGibb commented 3 years ago

You're very welcome.