naver/arcus-spring

Setter에 null이 들어가지 않도록 수정

Closed this issue · 7 comments

oliviarla commented

현재 코드 구현 상 아래와 같이 ArcusCache 객체 생성 시 setter로 데이터를 주입해주고 있다.
이 때 사용자가 configuration으로 설정하지 않은 필드는 null값을 가지게 되는데, 이를 그대로 setter에 넣어주어 null 값을 setter에 넣는 형태가 된다.
setter에 null을 입력하는 상황은 혼란을 줄 수 있으므로 setter에 값을 입력하기 전 null인지 검사하고 입력하거나, ArcusCache의 생성자에 ArcusCacheConfiguration 객체를 입력받도록 하는 방안이 괜찮아보인다.

@SuppressWarnings("deprecation")
protected Cache createCache(String name, ArcusCacheConfiguration configuration) {
  ArcusCache cache = new ArcusCache();
  cache.setName(name);
  cache.setServiceId(configuration.getServiceId());
  cache.setPrefix(configuration.getPrefix());
  cache.setArcusClient(client);
  cache.setArcusFrontCache(configuration.getArcusFrontCache());
  cache.setExpireSeconds(configuration.getExpireSeconds());
  cache.setFrontExpireSeconds(configuration.getFrontExpireSeconds());
  cache.setTimeoutMilliSeconds(configuration.getTimeoutMilliSeconds());
  cache.setOperationTranscoder(configuration.getOperationTranscoder());
  cache.setForceFrontCaching(configuration.isForceFrontCaching());
  cache.setWantToGetException(true);

  return cache;
}
uhm0311 commented

과거에는 ArcusCache 객체를 Bean으로 만드는 방식으로 사용하도록 가이드가 되어 있었기 때문에 일반적인 Bean 설정 방식인 Setter를 사용한 객체 생성이 아직까지 유지되고 있는 것으로 보이고, 이에 따라 Setter에 null이 허용되는 것으로 보입니다.
처음 arcus-spring이 릴리즈 되었을 때는 ArcusCacheManager 클래스가 없었기 때문에 Bean으로 등록하여 사용할 수 밖에 없었을 것입니다.

그리고 현재도 ArcusCache 객체를 Bean으로 만들어서 사용하는 방법이 막혀 있지 않아 여전히 사용 가능한 방식입니다.
단, ArcusCache 객체를 Bean으로 등록하여 사용한다면 ArcusCacheManager 객체를 통한 관리가 불가능합니다.
ArcusCache 객체를 Bean으로 등록하여 사용한다면 아래와 같이 사용해야 합니다.

@Autowired
private Cache arcusCache;

public Product getProduct(int id) {
  Product product = arcusCache.get(id, Product.class);

  if (product == null) {
    product = new Product(id);
    arcusCache.put(id, product);
  }
  
  return product;
}

spring-data-redis가 처음 릴리즈 되었을 때를 기준으로 보면 arcus-spring과 달리 RedisCache 클래스와 RedisCacheManager 클래스가 함께 구현되어 있습니다.
이를 보면 arcus-spring이 처음 릴리즈 되었을 때도 ArcusCacheManager 클래스를 구현하는 것 자체는 기술적으로 가능했을 것으로 보입니다.
기술적인 요인 외의 이유(급하게 구현해서 사용해야 했거나 Spring Framework의 캐싱 기술에 익숙하지 않은 사람이 구현했거나 등)로 인해 첫 릴리즈 당시 ArcusCacheManager 클래스가 없었을 것입니다.

따라서 ArcusCache 객체의 생성자의 접근 제한자를 public이 아닌 default로 두어 동일한 패키지 내에서만 객체 생성이 가능하도록 하고, ArcusCacheManager 객체가 ArcusCache 객체 생성이 가능하게끔 하며, ArcusCache 생성자에는 객체 생성에 필요한 정보인 ArcusCacheConfiguration 객체를 넘겨주며, Setter 메소드를 제거하는 것이 향후 발전 방향이라고 생각합니다.

참고로 RedisCache 객체는 생성자의 접근 제한자가 public이 아니고, 객체 생성에 필요한 정보는 생성자를 통해 넘겨주고 있으며, Setter 메소드가 존재하지 않습니다.
이는 spring-data-redis의 첫 릴리즈 당시와 최신 릴리즈 모두 마찬가지입니다.

하지만 ArcusCache 클래스를 RedisCache 클래스와 같은 방향으로 수정하게 되면 ArcusCache 객체를 Bean으로 등록하여 사용하는 방법은 불가능(생성자의 접근 제한자가 public이 아니게 되는 경우)하게 되거나 사용법이 달라(Setter 메소드를 제거하는 경우)져 하위 호환성이 깨질 수 있습니다.
하위 호환성을 깨도 되는지 논의가 필요하며, 깨지 않는 것으로 결론이 나면 spring-data-arcus에 적용해야 할 것 같습니다.

@jhpark816 @brido4125
ArcusCache 클래스 생성 방식에 대한 하위 호환성을 깨는 것이 좋을지 아니면 유지하는 것이 좋을지 검토 부탁드립니다.

brido4125 commented

제 의견은 다음과 같습니다.

정리해보면 현재 구현은 사용자에게 두 가지 방안은 제공합니다.

  1. ArcusCache를 빈으로 등록하는 방법
  2. ArcusCacheManager를 통해서 등록하는 방법

장기적으로 보면 고객의 사용성에서는 단일화된 방안을 제공하는게 좋다고 생각합니다.
그래서 Redis와 동일하게 적용시키는게 좋지 않을까 생각합니다.

아니면, spring-data-arcus에 arcus-spring과 별도로
해당 기능을 CacheManager를 사용해서 구현하는것도 하나의 방안입니다.

jhpark816 commented

@oliviarla 검토 바랍니다.

oliviarla commented

제 생각에는 ArcusCache의 setter(prefix, keyProvider, transcoder, arcusFrontCache 설정을 위한)에 null을 입력하지 못하도록 NonNull 어노테이션을 달든, Assert.notNull()을 사용하든 조치를 취하는 것이 좋아보입니다.

하위호환성으로 인해 위 방식 적용이 어렵다면, 기존 코드는 유지한 채 새로운 ArcusCache 생성자를 protected로 만들고 객체 생성에 필요한 정보인 ArcusCacheConfiguration 객체를 넘겨주도록만 변경하고 기존 public 생성자는 Deprecate시키는게 어떤가요?

protected ArcusCache(String name, RedisCacheConfiguration cacheConfiguration) {
  Assert.notNull(name, "Name must not be null");
  Assert.notNull(cacheConfiguration, "CacheConfiguration must not be null");
  
  this.name = name;
  if(cacheConfiguration.getPrefix() != null) { this.prefix = cacheConfiguration.getPrefix(); }
  // ...
}
uhm0311 commented

하위 호환성을 지킨다고 하면 @oliviarla 님 의견대로 기존의 public 생성자와 setter 메소드에 @Deprecated를 붙이고 ArcusCacheConfiguration 객체를 인자로 받는 default 접근 제한자 생성자를 추가해서 내부적으로 사용하는 것이 좋겠네요.

jhpark816 commented

@oliviarla @uhm0311
아래와 같이 처리하시죠.

  • ArcusCacheConfiguration 객체를 인자로 받는 default 접근 제한자의 생성자를 추가하고 내부적으로 사용
  • ArcusCache 객체에 대한 setter 메소드에서 @Nullable 제거
  • ArcusCacheConfiguration 객체에 대한 setter 메소드에서 @Nullable 제거

ArcusCache에 대한 기존 public 생성자와 setter 메소드는 그대로 유지하고,
deprecate 여부는 나중에 결정하여 진행하죠.

  • 👀1
uhm0311 commented

ArcusCacheConfiguration 객체에 대한 setter 메소드에서 @Nullable 제거

ArcusCache의 setter 메소드는 공식 사용법이 아니므로 제거해도 됩니다.
그러나 ArcusCacheConfiguration의 settter 메소드는 공식적으로 Bean을 생성할 때 사용하는 메소드입니다.
ArcusCacheConfiguration의 setter 메소드에 @Nullable을 제거하는 경우, 코틀린 환경에서 어떤 필드 값을 지정했다가 다시 제거할 방법이 없습니다.
또한 코틀린 환경에서 경우에 따라 null이 될 수도 있는 변수를 setter에 넣으려면 반드시 if문이 필요합니다.

@Nullable String prefix = null;
ArcusCacheConfiguration config = new ArcusCacheConfiguration();

if ("true".equals(System.getProperty("isProduction"))) {
  prefix = "prod";
}

...

// Setter 메소드에 @Nullable이 붙은 경우
config.setPrefix(prefix); 


// Setter 메소드에 @Nullable이 붙지 않은 경우
if (prefix != null) {
  config.setPrefix(prefix);
}

다른 Spring Framework 라이브러리에서 Bean으로 생성하는 클래스의 setter 메소드 중 null일 수 있는 필드의 setter 메소드에는 @Nullable이 붙어 있습니다.