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.

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

Need to add @Nullable annotation for supporting kotlin 1.9 and above. #3091

Open
Ktaewon opened this issue Jan 14, 2025 · 4 comments
Open

Need to add @Nullable annotation for supporting kotlin 1.9 and above. #3091

Ktaewon opened this issue Jan 14, 2025 · 4 comments
Assignees
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: bug A general bug

Comments

@Ktaewon
Copy link

Ktaewon commented Jan 14, 2025

Problem Description

Hello Spring Data Redis team,

I am using Spring Data Redis in a Kotlin 1.9-based project and have run into an issue when overriding the delete() method from RedisKeyValueAdapter. While the abstract class that RedisKeyValueAdapter implements (or extends) includes a @Nullable annotation on this method, it appears that the actual implementation in RedisKeyValueAdapter does not. As a result, Kotlin 1.9 does not recognize that the method can return null, which leads to a compile-time error when trying to override it with a nullable return type.

Background

In the following excerpt from RedisKeyValueAdapter, the delete() method may return null, as shown in the example logic. However, there is no
@Nullable annotation to indicate this possibility:

@Override
public <T> T delete(Object id, String keyspace, Class<T> type) {
byte[] binId = toBytes(id);
byte[] binKeyspace = toBytes(keyspace);
T value = get(id, keyspace, type);
if (value != null) {
byte[] keyToDelete = createKey(keyspace, toString(id));
redisOps.execute((RedisCallback<Void>) connection -> {
connection.del(keyToDelete);
connection.sRem(binKeyspace, binId);
new IndexWriter(connection, converter).removeKeyFromIndexes(keyspace, binId);
if (RedisKeyValueAdapter.this.keepShadowCopy()) {
RedisPersistentEntity<?> persistentEntity = converter.getMappingContext().getPersistentEntity(type);
if (persistentEntity != null && persistentEntity.isExpiring()) {
byte[] phantomKey = ByteUtils.concat(keyToDelete, BinaryKeyspaceIdentifier.PHANTOM_SUFFIX);
connection.del(phantomKey);
}
}
return null;
});
}
return value;
}

Because there is no @Nullable annotation here, Kotlin sees T as non-null. However, the method may actually return null. When we attempt to override it in Kotlin, such as:

override fun <T> delete(id: Any, keyspace: String, type: Class<T>): T? {
    // ...
}

Kotlin 1.9 raises an error along the lines of:

Return type of 'delete' is not a subtype of the return type 
of the overridden member 'public open fun <T> delete(...) : T'

This stricter nullability check is documented in KT-36770, kotlin docs,where Kotlin 1.9 enforces a stronger rule about Java methods that appear to return non-null. Older Kotlin versions often allowed a nullable return type with just a warning, but 1.9 treats it as a compilation error.

Why @Nullable?

  • The code clearly can return null, so adding @Nullable directly on the delete() method in RedisKeyValueAdapter would help Kotlin accurately recognize this possibility.
  • If @Nullable were present, Kotlin would interpret the method as returning T?, preventing the error when overriding.
  • This aligns with the nullability information already present in the abstract class, which currently is not being picked up due to the missing annotation in the concrete implementation.

Example Change

If the delete() method were annotated like this:

@Override
@Nullable
public <T> T delete(Object id, String keyspace, Class<T> type) {
    T value = get(id, keyspace, type);
    // ...
    return value;
}

Then Kotlin code such as:

override fun <T> delete(id: Any, keyspace: String, type: Class<T>): T? {
    // ...
}

I understand there might be other recommended workarounds or best practices, so please feel free to correct me if I am misunderstanding anything. Any advice on handling this situation would be greatly appreciated. My goal is simply to ensure that Spring Data Redis integrates smoothly with Kotlin 1.9 and higher.

Thank you very much for taking the time to review this issue, and for all your hard work on the Spring Data projects.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 14, 2025
@mp911de
Copy link
Member

mp911de commented Jan 14, 2025

Probably this isn't the only method that would require retrofitting. Do you want to check all the other adapter methods as well? As workaround, have you tried using the KeyValueAdapter interface instead?

@Ktaewon
Copy link
Author

Ktaewon commented Jan 15, 2025

I also considered using the KeyValueAdapter interface. While it may solve the problem, I don’t think it provides a definitive solution. Additionally, there is another workaround: adding a Kotlin compiler option. However, since this option will no longer work with the K2 compiler, I ultimately believe that modifying the code is necessary.

To ensure long-term compatibility and alignment with Kotlin’s null-safety features, retrofitting @Nullable annotations in methods like delete within RedisKeyValueAdapter seems to be the proper solution. This approach would also enhance consistency with other Spring modules and improve the overall developer experience.

If needed, I’m happy to assist in identifying the relevant methods. Thank you for considering this!

@mp911de
Copy link
Member

mp911de commented Jan 15, 2025

Would you like to submit a pull request so that we can include the necessary changes for our service releases shipping on Friday?

@mp911de mp911de added type: bug A general bug status: ideal-for-contribution An issue that a contributor can help us with and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 15, 2025
@mp911de mp911de self-assigned this Jan 15, 2025
@Ktaewon
Copy link
Author

Ktaewon commented Jan 15, 2025

Sure! I'll submit the PR as soon as the work is done, either today or tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ideal-for-contribution An issue that a contributor can help us with type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants