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

Using @Resource to resolve the fileExtensionWhitelist dependency in ProcompressedArtifactFilter #1815

Conversation

danielcolgrove
Copy link
Contributor

Copy link
Contributor

@phillipuniverse phillipuniverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielcolgrove this does not work and is unsafe to merge in. If you use @Resource here in a filter, starting up with Spring Boot will fail since filters are registered directly with the servlet container, and the servlet container (like Tomcat) will try to inject into the @Resource by doing a JNDI lookup. See #1771 and BroadleafCommerce/BroadleafCommercePrivate#958.

@Jerry77oz
Copy link
Member

@phillipuniverse we saw the issue where the change (and reason) why it got changed to @Autowired but the issue we were struggling with was, why was it failing when you were testing but it was working for Casey (Icon).

Casey was booting up with @Resource (on 5.2.3-M6) without exceptions and now that he upgraded to 5.2.3-GA he can't start up because the @Autowired is failing to resolve the List<String>.

@phillipuniverse
Copy link
Contributor

phillipuniverse commented Mar 2, 2018

@danielcolgrove did a bit of research here and it seems like this is a common problem with primitive types like this. Random person on the internet from https://tamasgyorfi.net/2014/04/02/spring-injection-with-collections/ suggests to change it to this:

@Value("#{blPrecompressedArtifactFileExtensionWhitelist}")
List<String> fileExtensionWhitelist;

The #{} expression in SpEL means look up a bean named whatever is inside.

The issue seems to be that when you @Autowire a collection, Spring treats that as trying to inject every bean of that type in the generic. So Spring looks for every String that it manages and then tries to filter that list down to the specific name in the @Qualifier. I guess since we don't have any String beans, it fails that it cannot find it. The mismatch is that we have a List<String> bean and it actually looks for a String bean.

Likely that we need to spot-check the other changes that I made in the referenced issues.

@phillipuniverse
Copy link
Contributor

…red bean. As stated in the PR comments, this is a suggested approach for resolving generic collection types.
@Jerry77oz
Copy link
Member

Juergen has spoken!

This was the only case of List<String> in the PR, all the others were using specific classes with the @Autowired.

@danielcolgrove
Copy link
Contributor Author

@Jerry77oz These changes worked for Casey. We should be able to merge.

@Jerry77oz Jerry77oz dismissed phillipuniverse’s stale review March 27, 2018 20:44

The changes Phillip suggested have been implemented

@Jerry77oz Jerry77oz merged commit e9bc15f into develop-5.2.x Mar 27, 2018
@Jerry77oz Jerry77oz deleted the qa-3237-reverted-resource-resolver-precompressedArtifactFilter branch March 27, 2018 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants