spring-projects / spring-data-couchbase

Provides support to increase developer productivity in Java when using Couchbase. 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-couchbase
Apache License 2.0
277 stars 190 forks source link

Enum parameter to repository method not handled. #1837

Closed mikereiche closed 1 year ago

mikereiche commented 1 year ago

TL;DR the issue is because the couchbaseCustomConversions created in CouchbaseDataConfiguration does not have the Enum converters.

@Bean(name = BeanNames.COUCHBASE_CUSTOM_CONVERSIONS)
@ConditionalOnMissingBean(name = BeanNames.COUCHBASE_CUSTOM_CONVERSIONS)
CouchbaseCustomConversions couchbaseCustomConversions() {
    return new CouchbaseCustomConversions(Collections.emptyList());
}

From @Pharisaeus comment on #1069 .

@mikereiche I hit the same issue when using a custom @Query and named @Param as in:

@Query("UPDATE #{#n1ql.collection} SET status = $newStatus") Result changeStatus(@Param("newStatus") Status newStatus); gives:

com.couchbase.client.core.error.InvalidArgumentException: Unsupported type for JSON value: class a.b.c.Status at com.couchbase.client.core.error.InvalidArgumentException.fromMessage(InvalidArgumentException.java:28) ~[core-io-2.4.10.jar:na] at com.couchbase.client.java.json.JsonValue.coerce(JsonValue.java:94) ~[java-client-3.4.10.jar:na] at com.couchbase.client.java.json.JsonObject.put(JsonObject.java:222) ~[java-client-3.4.10.jar:na] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.putNamedValue(StringBasedN1qlQueryParser.java:575) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getNamedPlaceholderValues(StringBasedN1qlQueryParser.java:517) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.StringBasedN1qlQueryParser.getPlaceholderValues(StringBasedN1qlQueryParser.java:543) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.core.query.StringQuery.toN1qlSelectString(StringQuery.java:83) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.core.ReactiveFindByQueryOperationSupport$ReactiveFindByQuerySupport.assembleEntityQuery(ReactiveFindByQueryOperationSupport.java:281) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.core.ReactiveFindByQueryOperationSupport$ReactiveFindByQuerySupport.all(ReactiveFindByQueryOperationSupport.java:182) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.core.ExecutableFindByQueryOperationSupport$ExecutableFindByQuerySupport.all(ExecutableFindByQueryOperationSupport.java:95) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQuery.lambda$getExecutionToWrap$1(AbstractCouchbaseQuery.java:124) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.CouchbaseQueryExecution$ResultProcessingExecution.execute(CouchbaseQueryExecution.java:84) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQuery.doExecute(AbstractCouchbaseQuery.java:93) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQueryBase.execute(AbstractCouchbaseQueryBase.java:132) ~[spring-data-couchbase-5.1.4.jar:5.1.4] at org.springframework.data.couchbase.repository.query.AbstractCouchbaseQueryBase.execute(AbstractCouchbaseQueryBase.java:113) ~[spring-data-couchbase-5.1.4.jar:5.1.4] I'm guessing it goes through a different code path in this scenario and the previous fix is not enough.

mikereiche commented 1 year ago

@Pharisaeus I can't reproduce this. At line 508, the parameter is converted to a json type. Can you provide the definition of your Status?
https://github.com/spring-projects/spring-data-couchbase/blob/247164c6c2e13beb4423a9898bb28a6678dd6d79/src/main/java/org/springframework/data/couchbase/repository/query/StringBasedN1qlQueryParser.java#L507C1-L516C53

I tried:

List<User> findByFirstname(@Param("firstName")FirstName firstName );

List<User> findByFirstnameIn(@Param("firstNames")FirstName[] firstNames );

@Query("#{#n1ql.selectEntity} where #{#n1ql.filter} and (firstname = $firstName)")
List<User> queryByFirstnameNamedParameter(@Param("firstName")FirstName firstName );

@Query("#{#n1ql.selectEntity} where #{#n1ql.filter} and (firstname = $1)")
List<User> queryByFirstnamePositionalParameter(@Param("firstName")FirstName firstName );

enum FirstName {
    Dave,
    William
}

@Query("#{#n1ql.selectEntity} where #{#n1ql.filter} and (jsonNode.myNumber = $myNumber)")
List<User> queryByIntegerEnumNamed(@Param("myNumber")IntEnum myNumber );

@Query("#{#n1ql.selectEntity} where #{#n1ql.filter} and (jsonNode.myNumber = $1)")
List<User> queryByIntegerEnumPositional(@Param("myNumber")IntEnum myNumber );

enum IntEnum {
    One(1),
    Two(2),
    OneThousand(1000);
    Integer value;
    IntEnum(Integer i){
        value = i;
    }
    @JsonValue
    public Integer getValue(){
        return value;
    }
}
mikereiche commented 1 year ago

Please reopen if you have additional information.

Pharisaeus commented 1 year ago

@mikereiche see: https://github.com/Pharisaeus/couchbase-enum it's as much bare-bones as I could make it. There are 2 tests at https://github.com/Pharisaeus/couchbase-enum/blob/master/src/test/java/org/example/test/SomeTest.java#L20 and both crash with the enum issue. The tests are running via testcontainers so you should be able to simply clone the repo and run tests as long as you have docker daemon running.

mikereiche commented 1 year ago

The issue is that the 'couchbaseConverter' does not have the. EnumToObject converter factory, so it just passes through the enum instead of converting it to a json-friendly object.

Object value = couchbaseConverter.convertForWriteIfNeeded(rawValue);

The EnumToObject converter is added in AbstractCouchbaseConfiguration.customConversions() and is usually obtained with a "config" class that extends it. But you don't have such a class. I need to figure out where the configuration comes from in your test.

public CustomConversions customConversions(CryptoManager cryptoManager, ObjectMapper objectMapper) {
    List<Object> newConverters = new ArrayList();
    // The following
    newConverters.add(new OtherConverters.EnumToObject(getObjectMapper()));
    newConverters.add(new IntegerToEnumConverterFactory(getObjectMapper()));
    newConverters.add(new StringToEnumConverterFactory(getObjectMapper()));
    newConverters.add(new BooleanToEnumConverterFactory(getObjectMapper()));
Pharisaeus commented 1 year ago

I only do https://github.com/Pharisaeus/couchbase-enum/blob/master/src/main/java/org/example/configuration/BeansConfiguration.java#L8

@EnableCouchbaseRepositories(basePackageClasses = {MyCouchbaseRepository.class})

and nothing more, so the configuration has to be some default/coming from springboot

mikereiche commented 1 year ago

Yes - I see where it comes from. I have to move the addition of converters into CouchbaseCustomConversions constructor.

Edit: This won't work -> For a work-around you can add a class that extends AbstractCouchbaseConfiguration.

mikereiche commented 1 year ago

You'll need back-ticks around value because it is a reserved word.

@Query("UPDATE #{#n1ql.collection} SET status = $newStatus WHERE #{#n1ql.filter} AND `value` in $values #{#n1ql.returning}")

And make your BeansConfiguration.java like this:

 package org.example.configuration;

import com.couchbase.client.core.encryption.CryptoManager;
import com.couchbase.client.java.encryption.annotation.Encrypted;
import com.couchbase.client.java.encryption.databind.jackson.EncryptionModule;
import com.couchbase.client.java.json.JsonValueModule;
import com.fasterxml.jackson.annotation.JsonValue;
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.example.repository.MyCouchbaseRepository;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.convert.CustomConversions;
import org.springframework.data.convert.PropertyValueConverterRegistrar;
import org.springframework.data.convert.SimplePropertyValueConversions;
import org.springframework.data.couchbase.config.BeanNames;
import org.springframework.data.couchbase.core.convert.BooleanToEnumConverterFactory;
import org.springframework.data.couchbase.core.convert.CouchbaseCustomConversions;
import org.springframework.data.couchbase.core.convert.CouchbasePropertyValueConverterFactory;
import org.springframework.data.couchbase.core.convert.CryptoConverter;
import org.springframework.data.couchbase.core.convert.IntegerToEnumConverterFactory;
import org.springframework.data.couchbase.core.convert.JsonValueConverter;
import org.springframework.data.couchbase.core.convert.OtherConverters;
import org.springframework.data.couchbase.core.convert.StringToEnumConverterFactory;
import org.springframework.data.couchbase.core.mapping.CouchbaseMappingContext;
import org.springframework.data.couchbase.repository.config.EnableCouchbaseRepositories;

import java.lang.annotation.Annotation;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

@Configuration
@EnableCouchbaseRepositories(basePackageClasses = {MyCouchbaseRepository.class})
public class BeansConfiguration {

  volatile ObjectMapper objectMapper;
  volatile CryptoManager cryptoManager = null;

  @Bean(name = BeanNames.COUCHBASE_CUSTOM_CONVERSIONS)
  public CustomConversions customConversions() {
    return customConversions(getCryptoManager(), getObjectMapper());
  }

  /**
   * Register custom Converters in a {@link CustomConversions} object if required. These {@link CustomConversions} will
   * be registered with the {@link #mappingCouchbaseConverter(CouchbaseMappingContext, CouchbaseCustomConversions)} )}
   * and {@link #couchbaseMappingContext(CustomConversions)}.
   *
   * @param cryptoManager
   * @return must not be {@literal null}.
   */
  public CustomConversions customConversions(CryptoManager cryptoManager, ObjectMapper objectMapper) {
    List<Object> newConverters = new ArrayList();
    // The following
    newConverters.add(new OtherConverters.EnumToObject(getObjectMapper()));
    newConverters.add(new IntegerToEnumConverterFactory(getObjectMapper()));
    newConverters.add(new StringToEnumConverterFactory(getObjectMapper()));
    newConverters.add(new BooleanToEnumConverterFactory(getObjectMapper()));
    CustomConversions customConversions = CouchbaseCustomConversions.create(configurationAdapter -> {
      SimplePropertyValueConversions valueConversions = new SimplePropertyValueConversions();
      valueConversions.setConverterFactory(
          new CouchbasePropertyValueConverterFactory(cryptoManager, annotationToConverterMap(), objectMapper));
      valueConversions.setValueConverterRegistry(new PropertyValueConverterRegistrar().buildRegistry());
      valueConversions.afterPropertiesSet(); // wraps the CouchbasePropertyValueConverterFactory with CachingPVCFactory
      configurationAdapter.setPropertyValueConversions(valueConversions);
      configurationAdapter.registerConverters(newConverters);
    });
    return customConversions;
  }

  Map<Class<? extends Annotation>, Class<?>> annotationToConverterMap() {
    Map<Class<? extends Annotation>, Class<?>> map = new HashMap();
    map.put(Encrypted.class, CryptoConverter.class);
    map.put(JsonValue.class, JsonValueConverter.class);
    return map;
  }

  final public ObjectMapper getObjectMapper() {
    if (objectMapper == null) {
      synchronized (this) {
        if (objectMapper == null) {
          objectMapper = couchbaseObjectMapper();
        }
      }
    }
    return objectMapper;
  }

  /**
   * Creates a {@link ObjectMapper} for the jsonSerializer of the ClusterEnvironment and spring-data-couchbase
   * jacksonTranslationService and also some converters (EnumToObject, StringToEnum, IntegerToEnum)
   *
   * @return ObjectMapper
   */
  protected ObjectMapper couchbaseObjectMapper() {
    ObjectMapper om = new ObjectMapper();
    om.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
    om.registerModule(new JsonValueModule());
    if (getCryptoManager() != null) {
      om.registerModule(new EncryptionModule(getCryptoManager()));
    }
    return om;
  }
  /**
   * cryptoManager can be null, so it cannot be a bean and then used as an arg for bean methods
   */
  private CryptoManager getCryptoManager() {
    if (cryptoManager == null) {
      synchronized (this) {
        if (cryptoManager == null) {
          cryptoManager = cryptoManager();
        }
      }
    }
    return cryptoManager;
  }

  protected CryptoManager cryptoManager() {
    return null;
  }

public static Map<Class<? extends Annotation>, Class<?>> annotationToConverterMap() {
    Map<Class<? extends Annotation>, Class<?>> map = new HashMap();
    map.put(Encrypted.class, CryptoConverter.class);
    map.put(JsonValue.class, JsonValueConverter.class);
    return map;
  }
}
mikereiche commented 1 year ago

I do plan on fixing this, but that should get you going for now.

mikereiche commented 1 year ago

This is a little tidier

package org.example.configuration;

import org.example.repository.MyCouchbaseRepository;
import org.springframework.boot.autoconfigure.couchbase.CouchbaseProperties;
import org.springframework.boot.autoconfigure.data.couchbase.CouchbaseDataProperties;
import org.springframework.context.annotation.Configuration;
import org.springframework.data.couchbase.config.AbstractCouchbaseConfiguration;
import org.springframework.data.couchbase.repository.config.EnableCouchbaseRepositories;

@Configuration
@EnableCouchbaseRepositories(basePackageClasses = { MyCouchbaseRepository.class })
public class BeansConfiguration extends AbstractCouchbaseConfiguration {

    private final CouchbaseProperties properties;
    private final CouchbaseDataProperties dataProperties;

    BeansConfiguration(CouchbaseProperties properties, CouchbaseDataProperties dataProperties) {
        this.properties = properties;
        this.dataProperties = dataProperties;
    }

    @Override
    public String getConnectionString() {
        return properties.getConnectionString();
    }

    @Override
    public String getUserName() {
        return properties.getUsername();
    }

    @Override
    public String getPassword() {
        return properties.getPassword();
    }

    @Override
    public String getBucketName() {
        return dataProperties.getBucketName();
    }

}