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

Improve documentation for Cacheable#result (re: Optional wrapper) [SPR-14587] #19156

Closed
spring-projects-issues opened this issue Aug 15, 2016 · 5 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 15, 2016

Karol Krawczyk opened SPR-14587 and commented

After migrating from spring-context 4.2.7 to spring-context 4.3.2 I noticed a change in a way you handle the caching of optionals.
If you look at the code below:

class UserService {
    @Cacheable(cacheNames = "userCache", unless = "!#result.isPresent()")
    public Optional<User> getUser(Integer id) {
        //return user
    }
}

it throws:
EL1004E:(pos 1): Method call: Method isPresent() cannot be found on com.test.User type
It's because #result expression is now a concrete class and not Optional like before.

I am not sure if this is an intended behaviour or a bug. Though for me it is a bit misleading to be able to use Optionals for caching and at the same time to operate on unwrapped object for #result expression.


Affects: 4.3.2

1 votes, 2 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2016

Stéphane Nicoll commented

Karol Krawczyk it looks like you tried to implement a support for Optional in your own code. Spring Framework 4.3 has an explicit support for Optional now and we will unwrap the target object so that it's cached (rather than caching the Optional instance as you were probably doing yourself). The side effect of that is that User is now the result object (as it should since we have official support for Optional).

If you drop the unless condition, you should be just fine and User will be cached rather than Optional<User>. Is there anything wrong with that? If so, please provide more details that explains why you think it is a bug.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2016

Karol Krawczyk commented

@snicoll I understand that you don’t really cache Optional, but rather the target object. But this is an implementation detail.
My point is that you handle caching with optionals(which is great :)). So you return wrapped object or Optional.empty to services that use cached method. And at the same time #result is an unwrapped object. I just find it a bit inconsistent and misleading.
So if you decide to stick to the current state, it would be cool if you could put some explanation/remark in the documentation.
And btw, I didn’t implement support for Optionals in my code. I believe that in version 4.2.7, that I was using, caching optionals was already supported by spring.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2016

Stéphane Nicoll commented

Thanks for the fast reply.

And at the same time #result is an unwrapped object. I just find it a bit inconsistent and misleading.

I think I have to disagree. Considering that we now support optional, we make sure that result is consistent (i.e. that it defines the actual object being cached). Prior to 4.3 we didn't know about Optional at all, which is why result was handled "as-is". In 5.0 we'll add support for Mono and other reactive return types and the result will be treated consistently there as well (i.e. always what ends up in the cache, that is the entity to cache and not some infrastructure wrapper).

If you had to truly do something on the result object, Optional wouldn't be super useful. Same thing for Mono.

And btw, I didn’t implement support for Optionals in my code. I believe that in version 4.2.7, that I was using, caching optionals was already supported by spring.

Actually, you did (without noticing I suppose).. #18804 was implemented only as of 4.3.RC2 so you were previously storing the Optional object rater than User (oops). Good thing that your cache was not configured to serialize its entries then ;-)

Having said that, we should be much more explicit with our meaning of result there. I'll update the doc.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2016

Karol Krawczyk commented

Ok, thank you for explanations!

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 16, 2016

Stéphane Nicoll commented

The documentation has been improved in 0d59a1. Thank you for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants