neharob / prettyfaces

Automatically exported from code.google.com/p/prettyfaces
0 stars 0 forks source link

SpringBeanNameResolver could be smarter about Spring AOP #119

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
NOTE: For fastest resolution when reporting an issue, please provide attach
example project or file *with sources* that reproduces the problem.

What steps will reproduce the problem?
1. Define a Spring-managed bean with @Named("foo") and @Scope(value=*, 
proxyMode=!NONE), with one or more @URLMappings
2. Watch PrettyFaces ask for a @URLBeanName annotation
3. Be stubborn, get mad ;P

What is the expected output? What do you see instead?
In this case, Spring returns both "foo" and "scopedTarget.foo".  By calling 
ScopedProxyUtils.getTargetBeanName(String) against "foo", 
springBeanNameResolver can be smart enough to discard the scoped proxy, which 
is not for our type of use anyway.

What version of PrettyFaces are you using? On what server and version,
version of JSF, and other relevant technologies?
3.3.1 trunk

Please provide any additional information below.
I consider this an enhancement rather than a defect, but can't see any way to 
report the former.  Git pull request forthcoming.

Original issue reported on code.google.com by gudnabr...@gmail.com on 16 Aug 2011 at 10:45

GoogleCodeExporter commented 9 years ago
You code looks very good! I will take it for a test drive in one of my Spring 
based projects and apply it if everything runs smooth.

Thank you very much! We are always happy to get contributions from the 
community! :)

Original comment by chkalt on 17 Aug 2011 at 5:24

GoogleCodeExporter commented 9 years ago
Pull request:

https://github.com/ocpsoft/prettyfaces/pull/5

Original comment by chkalt on 17 Aug 2011 at 5:24

GoogleCodeExporter commented 9 years ago
Hey there! 

I finally found some time to dig into this issue. You are completely right. 
PrettyFaces currently fails to handle Spring's scoped proxies correctly. I 
didn't noticed that before! Thanks for bringing this up!

I appreciated your work on the patch for the SpringBeanNameResolver. However 
I'm not sure if this is the best solution for the problem.

Is there any reason why you didn't implemented filterProxyNames() in a way so 
that all names starting with "scopedTarget." get ignored? I think this may be a 
even simpler solution than doing more reflection work and it should work 
perfectly, because (as you said) its a bean name we don't want to use as it 
would bypass the proxy.

I took a look at the implementation of ScopedProxyUtils.getTargetBeanName() and 
it looks like this:

public abstract class ScopedProxyUtils {

  private static final String TARGET_NAME_PREFIX = "scopedTarget.";

  /* ... more stuff... */

  public static String getTargetBeanName(String originalBeanName) {
    return TARGET_NAME_PREFIX + originalBeanName;
  }
}

Any thoughts on this? It seems like you already spend some time debugging this, 
so I'm curious to hear your opinion. :)

Original comment by chkalt on 21 Aug 2011 at 3:16

GoogleCodeExporter commented 9 years ago
My approach was based on the idea of not relying on something with no publicly 
available, thus guaranteed, API.

Original comment by gudnabr...@gmail.com on 21 Aug 2011 at 3:25

GoogleCodeExporter commented 9 years ago
Something like this:

https://github.com/chkal/prettyfaces/commit/980537f060caf16f441448e9d0aa4fc52d40
8fb5

Original comment by chkalt on 21 Aug 2011 at 3:52

GoogleCodeExporter commented 9 years ago
I simply feel it is preferable to depend on a published API rather than an 
implementation detail of same.

Original comment by gudnabr...@gmail.com on 21 Aug 2011 at 6:31

GoogleCodeExporter commented 9 years ago
Hmmm... I guess you are right. TARGET_NAME_PREFIX is declared private. Seems 
like I missed that. So I think we should go with your patch... I'll apply it 
later today.. :)

Original comment by chkalt on 22 Aug 2011 at 5:15

GoogleCodeExporter commented 9 years ago
Merged! :)

I just added some more comments and changed the two catch blocks to catch 
individual exceptions instead of java.lang.Exception.

So the latest snapshots of 3.3.1 should fix this issue.

Thanks again for the patch! :)

Christian

Original comment by chkalt on 22 Aug 2011 at 5:54