Skip to content
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.

Sign up for GitHub

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

Inconsistent key names in COPY / RENAME / RENAMEX / PUBLISH command documentation and other fixes. #2196

Open
wants to merge 3 commits into
base: 2.7.x
Choose a base branch
from
+32 −31
Open

Inconsistent key names in COPY / RENAME / RENAMEX / PUBLISH command documentation and other fixes. #2196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc6ea07
Small documentation typo
toddmerrill Nov 2, 2021
0487202
Merge remote-tracking branch 'upstream/main' into main
toddmerrill Nov 8, 2021
3953719
Inconsistent key names in COPY / RENAME / RENAMEX / PUBLISH command d…
toddmerrill Nov 8, 2021
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
46 changes: 23 additions & 23 deletions src/main/java/org/springframework/data/redis/core/RedisOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,17 +160,17 @@ <T> T execute(RedisScript<T> script, RedisSerializer<?> argsSerializer, RedisSer
// -------------------------------------------------------------------------

/**
* Copy given {@code sourceKey} to {@code targetKey}.
* Copies the value stored at the {@code source} to the {@code destination}.
*
* @param sourceKey must not be {@literal null}.
* @param targetKey must not be {@literal null}.
* @param source must not be {@literal null}.
* @param destination must not be {@literal null}.
Comment on lines +165 to +166
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, neither source nor destination describe what they are. For instance, are they keys, are they values, what?

"must not be {@literal null}" is not informative and arguably no longer needed given the package-level @NonNullApi requires non-null values when method parameters are not explicitly annotated with @Nullable.

I am fine with shorter names as long as the Javadoc makes it apparent what things are and what they are used for.

* @param replace whether the key was copied. {@literal null} when used in pipeline / transaction.
* @return
* @see <a href="https://redis.io/commands/copy">Redis Documentation: COPY</a>
* @since 2.6
*/
@Nullable
Boolean copy(K sourceKey, K targetKey, boolean replace);
Boolean copy(K source, K destination, boolean replace);

/**
* Determine if given {@code key} exists.
Expand Down Expand Up @@ -269,24 +269,24 @@ <T> T execute(RedisScript<T> script, RedisSerializer<?> argsSerializer, RedisSer
K randomKey();

/**
* Rename key {@code oldKey} to {@code newKey}.
* Rename key {@code key} to {@code newKey}.
*
* @param oldKey must not be {@literal null}.
* @param key must not be {@literal null}.
* @param newKey must not be {@literal null}.
* @see <a href="https://redis.io/commands/rename">Redis Documentation: RENAME</a>
*/
void rename(K oldKey, K newKey);
void rename(K key, K newKey);

/**
* Rename key {@code oldKey} to {@code newKey} only if {@code newKey} does not exist.
* Rename {@code key} to {@code newKey} only if {@code newKey} does not exist.
*
* @param oldKey must not be {@literal null}.
* @param key must not be {@literal null}.
* @param newKey must not be {@literal null}.
* @return {@literal null} when used in pipeline / transaction.
* @see <a href="https://redis.io/commands/renamenx">Redis Documentation: RENAMENX</a>
*/
@Nullable
Boolean renameIfAbsent(K oldKey, K newKey);
Boolean renameIfAbsent(K key, K newKey);
Copy link
Contributor

@jxblum jxblum Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the renames are fine here.

But, I think the Javadoc needs serious work. For example, what does the oldKey/key refer to? Is a key to a data structure in Redis? Is it a key in a hash data structure mapping. I know I was confused by this when I first started learning about Redis.

Copy link
Member

Choose a reason for hiding this comment

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

It is not our responsibility to educate folks about the concepts of their data store.


/**
* Set time to live for given {@code key}.
Expand Down Expand Up @@ -355,7 +355,7 @@ default Boolean expireAt(K key, Instant expireAt) {
Boolean persist(K key);

/**
* Move given {@code key} to database with {@code index}.
* Move given {@code key} to database with {@code dbIndex}.
*
* @param key must not be {@literal null}.
* @param dbIndex
Expand All @@ -376,7 +376,7 @@ default Boolean expireAt(K key, Instant expireAt) {
byte[] dump(K key);

/**
* Create {@code key} using the {@code serializedValue}, previously obtained using {@link #dump(Object)}.
* Create {@code key} using the {@code value}, previously obtained using {@link #dump(Object)}.
*
* @param key must not be {@literal null}.
* @param value must not be {@literal null}.
Expand All @@ -389,7 +389,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
}

/**
* Create {@code key} using the {@code serializedValue}, previously obtained using {@link #dump(Object)}.
* Create {@code key} using the {@code value}, previously obtained using {@link #dump(Object)}.
*
* @param key must not be {@literal null}.
* @param value must not be {@literal null}.
Expand Down Expand Up @@ -576,13 +576,13 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
void slaveOfNoOne();

/**
* Publishes the given message to the given channel.
* Publishes the given message to the given {@code channel}.
*
* @param destination the channel to publish to, must not be {@literal null}.
* @param channel channel to publish to, must not be {@literal null}.
* @param message message to publish
* @see <a href="https://redis.io/commands/publish">Redis Documentation: PUBLISH</a>
*/
void convertAndSend(String destination, Object message);
void convertAndSend(String channel, Object message);

// -------------------------------------------------------------------------
// Methods to obtain specific operations interface objects.
Expand All @@ -607,7 +607,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
GeoOperations<K, V> opsForGeo();

/**
* Returns geospatial specific operations interface bound to the given key.
* Returns geospatial specific operations interface bound to the given {@code key}.
*
* @param key must not be {@literal null}.
* @return never {@literal null}.
Expand All @@ -625,7 +625,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
<HK, HV> HashOperations<K, HK, HV> opsForHash();

/**
* Returns the operations performed on hash values bound to the given key.
* Returns the operations performed on hash values bound to the given {@code key}.
*
* @param <HK> hash key (or field) type
* @param <HV> hash value type
Expand All @@ -648,7 +648,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
ListOperations<K, V> opsForList();

/**
* Returns the operations performed on list values bound to the given key.
* Returns the operations performed on list values bound to the given {@code key}.
*
* @param key Redis key
* @return list operations bound to the given key
Expand All @@ -663,7 +663,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
SetOperations<K, V> opsForSet();

/**
* Returns the operations performed on set values bound to the given key.
* Returns the operations performed on set values bound to the given {@code key}.
*
* @param key Redis key
* @return set operations bound to the given key
Expand All @@ -688,7 +688,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
<HK, HV> StreamOperations<K, HK, HV> opsForStream(HashMapper<? super K, ? super HK, ? super HV> hashMapper);

/**
* Returns the operations performed on Streams bound to the given key.
* Returns the operations performed on Streams bound to the given {@code key}.
*
* @return stream operations.
* @since 2.2
Expand All @@ -703,7 +703,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
ValueOperations<K, V> opsForValue();

/**
* Returns the operations performed on simple values (or Strings in Redis terminology) bound to the given key.
* Returns the operations performed on simple values (or Strings in Redis terminology) bound to the given {@code key}.
*
* @param key Redis key
* @return value operations bound to the given key
Expand All @@ -718,7 +718,7 @@ default void restore(K key, byte[] value, long timeToLive, TimeUnit unit) {
ZSetOperations<K, V> opsForZSet();

/**
* Returns the operations performed on zset values (also known as sorted sets) bound to the given key.
* Returns the operations performed on zset values (also known as sorted sets) bound to the given {@code key}.
*
* @param key Redis key
* @return zset operations bound to the given key.
Expand Down
17 changes: 9 additions & 8 deletions src/main/java/org/springframework/data/redis/core/RedisTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
* @author Mark Paluch
* @author Denis Zavedeev
* @author ihaohong
* @author Todd Merrill
* @param <K> the Redis key type against which the template works (usually a String)
* @param <V> the Redis value type against which the template works
* @see StringRedisTemplate
Expand Down Expand Up @@ -699,15 +700,15 @@ protected List<Object> execRaw() {

/*
* (non-Javadoc)
* @see org.springframework.data.redis.core.RedisOperations#delete(java.lang.Object, java.lang.Object, boolean)
* @see org.springframework.data.redis.core.RedisOperations#copy(java.lang.Object, java.lang.Object, boolean)
*/
@Override
public Boolean copy(K source, K target, boolean replace) {
public Boolean copy(K source, K destination, boolean replace) {

byte[] sourceKey = rawKey(source);
byte[] targetKey = rawKey(target);
byte[] destinationKey = rawKey(destination);

return execute(connection -> connection.copy(sourceKey, targetKey, replace), true);
return execute(connection -> connection.copy(sourceKey, destinationKey, replace), true);
}

/*
Expand Down Expand Up @@ -935,9 +936,9 @@ public K randomKey() {
* @see org.springframework.data.redis.core.RedisOperations#rename(java.lang.Object, java.lang.Object)
*/
@Override
public void rename(K oldKey, K newKey) {
public void rename(K key, K newKey) {

byte[] rawOldKey = rawKey(oldKey);
byte[] rawOldKey = rawKey(key);
byte[] rawNewKey = rawKey(newKey);

execute(connection -> {
Expand All @@ -951,9 +952,9 @@ public void rename(K oldKey, K newKey) {
* @see org.springframework.data.redis.core.RedisOperations#renameIfAbsent(java.lang.Object, java.lang.Object)
*/
@Override
public Boolean renameIfAbsent(K oldKey, K newKey) {
public Boolean renameIfAbsent(K key, K newKey) {

byte[] rawOldKey = rawKey(oldKey);
byte[] rawOldKey = rawKey(key);
byte[] rawNewKey = rawKey(newKey);
return execute(connection -> connection.renameNX(rawOldKey, rawNewKey), true);
}
Expand Down