[#IDEADEV-22977] Generate an error for custom scoped beans with missing <aop:scoped-proxy/> sub-element and provide a quickfix

IDEA: Development

Generate an error for custom scoped beans with missing <aop:scoped-proxy/> sub-element and provide a quickfix

Created: 01 Nov 07 21:47 Updated: 02 Nov 07 19:37

Component/s:

Editor. Error Highlighting, Editor. Intention Actions, J2EE.Spring

Fix Version/s:

None

Original Estimate:

Unknown

Remaining Estimate:

Unknown

Time Spent:

Unknown

Build:	7,523
Severity:	Low

Description

« Hide

When a developer enters

<bean id="someCustomScopedBean" scope="mycustomscope">
   </bean>

IDEA should detect that <aop:scoped-proxy/> was forgotten, report an error and provide a quickfix to add it so that the same bean looks like

<bean id="someCustomScopedBean" scope="mycustomscope">
        <aop:scoped-proxy/>
   </bean>

The error should be generated on the scope attribute of the scoped bean so that the user get an hint that the error is scope related

All

Comments

Work Log

Change History

Sort Order:

[ Permlink | « Hide ]

Dmitry Avdeev - 02 Nov 07 13:05

Should the problem be reported if only the bean is involved in some AOP processing? How can we detect it?

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 13:47 - edited

You don't need <aop:scoped-proxy/> only for the built-in core bean factory scopes: singleton, prototype. It is actually an error that the BeanFactory will raise so maybe this intention should report it too.
Any other scope (even the web spring built-in scopes like session, globalSession, request) MUST be used in conjunction with <aop:scoped-proxy/>
Therefore you can assume it is always an unintentional user error.

Add this reference in the intention documentation:
2.5.x reference: http://static.springframework.org/spring/docs/2.5.x/reference/beans.html#beans-factory-scopes-other-injection
2.0.x reference: http://static.springframework.org/spring/docs/2.0.x/reference/beans.html#beans-factory-scopes-other-injection

IMHO by default this intention should be an error.

PS: Obviously one can only use the aop namespace if they are using the xsd style configuration and not the dtd's
Do you have an intention to convert from dtd style to xsd that could be chained with this issue intention?

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 13:54 - edited

So to summarize:

scope	requirement	error location	quickfix
singleton	scoped bean must not have a `<aop:scoped-proxy/>` sub-element	`<aop:scoped-proxy/>` sub-element	Remove `<aop:scoped-proxy/>`
request, session, globalSession	scoped bean must have a `<aop:scoped-proxy/>` sub-element	scope attribute	Add `<aop:scoped-proxy/>` sub-element to bean

Note: I was incorrect in that the 'prototype' unlike 'singleton' has no restriction.

[ Permlink | « Hide ]

Dmitry Avdeev - 02 Nov 07 14:02

As it follows from the docs, the problem should be detected if only the bean is injected into another one as a dependency.

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 14:19

Strictly speaking you would be right.
However I haven't found a base spring usage of custom scope with aop proxying. Without proxying the scoping just does not work.
Having said that, you could have custom integration that are not compatible and/or provide a custom proxying mechanism like seam. IMHO this falls in the marginal use case and users of such cases can always disable this intention.

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 14:20

PS: Somebody got tired of me editing my own comment

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 14:22

In my previous post I meant: "I haven't found a base spring usage of custom scope without aop proxying"

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 14:25

Related issue IDEA-16161 to deal with this problem proactively on scope completion

[ Permlink | « Hide ]

Taras Tielkes - 02 Nov 07 14:48

Various comments/questions:

1) <aop:scoped-proxy/> is not related to AOP functionality at all.
While the decorator is from the "aop" namespace, it's not related to regular advice/pointcuts/advisors/etc.

2) About question that Dmitry asked: imagine we have a dependency graph like this:
A (singleton) -> B (session) -> C (session)

Should only B be decorated with <aop:scoped-proxy/>, or C as well?

My experience with custom scopes is limited, time for some code exploration and experimentation. (also note that quite a number of scope related bugs were fixed recently in Spring, however mostly related to obscure edge cases)

3) Should warning be shown for custom scoped beans that are not referenced anywhere? I guess it won't hurt to do it..

4) Perhaps it would make sense to do the warning highlighting on the "scope" attribute value.

That way the fix could be invoked directly after specifying a custom scope. This would solve Jacques request IDEA-16161 without doing non-intuitive things like magically inserting a <aop:scoped-proxy/> child element when a custom scope attribute has been added.

[ Permlink | « Hide ]

Jacques Morel - 02 Nov 07 16:56

2) A (singleton) -> B (session) -> C (session)
Yes you would be right again that in that case you don't need proxying. However this is VERY error prone and to me to be discouraged. This is one of the very few aspects of spring that is rather ugly (a complete break of encapsulation IMHO): the proxying is not an attribute of a scope and has to be declared on the bean definition completely in isolation to how it is used. You could have other beans that are not session scope that depends on C for example.
This is tantamount to premature optimization but I guess you could need it if you were very sensitive to the aop dispatch overhead involved.
I would not make the intention behave to let this happen. Either have an option (disabled by default) to let it or let people suppress the intention on a case to case basis when they really know what they are doing.

3) The only hurt is additional processing. I would have an option to disable it if this isn't just a lookup in a reference table and guarantied to be a very low cost even in projects of very large size (thousands of beans, 10Ks classes)

4) Great idea!

See my response to your comment against completion in IDEA-16161

[ Permlink | « Hide ]

Taras Tielkes - 02 Nov 07 16:57

Some additional questions:
5) In above table "prototype" is not listed. I assume it doesn't make sense to use <aop:scoped-proxy/> with prototype bean. Or does it?

6) What should happen in the scenario where the current file does not yet import the "aop" namespace? Should it automatically be imported?
If so, a schemaLocation needs to be chosen as well.
In spring 2.0.x there are two options: "spring-aop-2.0.xsd" (versioned) and "spring-aop.xsd" (unversioned, "sliding window" schema).
In spring 2.5.x there are three options: above two, and "spring-aop-2.5.xsd" as well.

[ Permlink | « Hide ]

Taras Tielkes - 02 Nov 07 19:37

One additional issue

7) When <aop:scoped-proxy/> decorator is used with proxy-target-class="false", the type of the bean is no longer the same as specified in "class" attribute. Instead, a proxy implementing all of the beans interfaces is created.

This means properties of other beans (referring to the custom scoped bean) using the beans class type are in error.

This is a relatively rare case, as the default of <aop:scoped-proxy/> is proxy-target-class="true". However, at the moment the Spring facet cannot represent such bean types AFAIK. Similar problem occurs for ProxyFactoryBean etc proxying multiple interfaces. There a TODO in the code for that.