Predict specific object type in EhCacheFactoryBean

[sslavic]

Prior to this change, before bean gets created by EhCacheFactoryBean,
its getObjectType would return only Ehcache interface. This caused
unwanted wiring issues like one described in related JIRA issue.
This fix makes use of EhCacheFactoryBean configuration to determine
object type even before it's created, so that container is provided
with as specific as possible object type when resolving dependencies.
Nevertheless, users are advised to code to Ehcache interface.

Issue: SPR-7843

I have signed and agree to the terms of SpringSource Individual Contributor License Agreement.

[cbeams]

Hi Stevo, thanks for the submission (and thanks for all the others, too!). We've just worked up a contributor guidelines doc. Please read this over and touch up these pull requests accordingly. This should streamline things going forward. Cheers!

[sslavic]

Amended and force pushed changed commit for this pull request - fixed broken test, formatted commit message, added note on ICLA.

[cbeams]

This is looking great, Stevo. Thanks for taking the time to polish this up.

Just a couple additional things to change.

Regarding the subject line:

Instead of

EhCacheFactoryBean.getObjectType specific before bean is created

Consider the following

Predict specific object type in EhCacheFactoryBean

From the "Format commit messages" section of the guidelines:

• It is an imperative statement (1.)
• It begins with a capitalized verb (2.)
• It comes in at 50 characters or less (4.)
• It concisely explains what this change does

The body of the commit comment looks good, though one line comes in at 74 characters instead of 72 (5.), and recognition of the CLA should actually go in the pull request comments, not the commit comment (4. from "Submit your pull request").

Note that I haven't reviewed deeply the actual substance of the commit (though at a glance it looks good). I just wanted to get these corrections in here as quickly as possible as I know you have other pull requests that you may be polishing up in the meantime.

Thanks again!

[sslavic]

Chris,

I've amended 3 out of 4 of my current pull requests - hopefully I got them OK this time. Anyway, thanks for your patience and support.

Remaining one (36) is javadoc related. Among other things, have to check javadoc line lengths there. While working on the issue I've noticed several approaches being used in javadoc for linking to annotations. Here are examples:

@{@link foo.bar.Inject}

import foo.bar.Inject;
@{@link Inject}

{@link foo.bar.Inject @Inject}

import foo.bar.Inject;
{@link Inject @Inject}

Maybe it would be wise to use one only, consistently. Is there agreement among Spring developers, which approach is preferred? Or is mixup of different approaches OK?

Personally, I prefer ones (2, 4) with import statements (even when annotation is not actually used in the type) as they make javadoc compact, and refactoring packages will update references (so "typesafe"). Difference between the two is whether @ is or isn't part of the link (so different font colour for @ and annotation type in the link), and compactness. In pull request 36 (which I'll amend) I used more 4 and 3, but thinking now, 2 seems best of all.

[cbeams]

Stevo,

I've committed the pull request, with a few changes:

1. Rebased your branch locally against latest springsource/master (nothing you could reasonably do about this, since you initiated the request days ago); we do this so that the merge history is as tight and near-linear as possible.
2. Touched-up grammar in the commit comment (not a big deal)
3. Updated the Apache license headers to reflect that the sources have now been edited in 2012 (previous dates were 2011 and 2009). This is an easy one to forget, but should always be double-checked and done.
4. Removed trailing whitespace in the original commit. I recommend switching on "Show Whitespace Characters" if you're using Eclipse, or whatever its equivalent is in IDEA. Eclipse is particularly bad about leaving trailing whitespace around after newlines.
5. Removed those newlines entirely, following suit with the formatting in other parts of the class, e.g. #decorateCache has logic very similar to what is now in #getObjectType, but does not have newlines between conditional blocks.
6. Added Javadoc explaining the behavior of #getObjectType, as it is now non-trivial.
7. Added assertions where necessary in EhCacheSupportTests to properly cover the new logic in #getObjectType. Prior to this, coverage was near-zero. You did add a meaningful assertion in your original commit (thanks), but please be sure to provide reasonable coverage for all non-trivial changes.

With that said, the implementation itself looks really good, and Author: attribution is retained as the substance of your changes are intact.

Congrats on your first commit, and thanks so much for taking the time with this and all the other issues lately!

[cbeams]

Regarding {@link ...} tags in Javadoc, it's a good question. This is one area of the framework where there are definitely different styles as you point out.

2 and 4 require importing the type in order to save space in the {@link ...} tag. There is nothing fundamentally 'wrong' with doing this, but we don't do it in the framework for a couple of reasons: First and most importantly, the set of imports in a given file has real significance to us as maintainers. It's a quick way of reasoning about this class and its relationships without a full static analysis. When documentation-only imports creep in, it makes this less meaningful. Second, it is not a complete solution. There are many cases where we refer in {@link} and {@see} tags to classes that cannot be imported due to being in higher-level packages. Because of the way we generate our Javadoc, this is not a problem for the rolled up public API documentation (i.e. the links all work there), but it would be impossible to import these classes at the source level.

So for the reasons above, the best approach is to go with 2 if the type has already been imported, i.e. is already actually used in the class at the source level:

import foo.bar.Inject;
@{@link Inject}

the benefit here is that this requires as little duplication as possible, and is quite refactoring-friendly.

if the type in question is not used at the source level, then go with 3:

{@link foo.bar.Inject @Inject}

this avoids the need to import and renders the link as a short name instead of fully-qualified.

As you mention above, this does create a slight inconsistency in the rendered Javadoc, because in 3 the ampersand is not part of the hyperlink, whereas in 4 it is. This is basically a compromise worth making because of the brevity and refactoring friendliness of 2 and the fact that when using 3, the fully-qualified names can get quite long, meaning that due to line breaks, the {@link} can end up being the first character of that line of javadoc, and it is illegal to lead with a '@' character, unless you're dealing with a legal javadoc param like 'see', 'author', 'param', etc.

It's not perfect, but that's more due to idiosyncrasies of Javadoc than anything else.

Thanks for asking!