swagger-api / swagger-core

Examples and server integrations for generating the Swagger API Specification, which enables easy access to your REST API
http://swagger.io
Apache License 2.0
7.39k stars 2.18k forks source link

Resource packages config does not restrict what packages and classes are scanned #2631

Open tsiq-karold opened 6 years ago

tsiq-karold commented 6 years ago

Specifying resource packages on BeanConfig currently only filters the results of the scan, it does not restrict the packages and classes that the scan is performed over. While this does not seem to cause incorrect behaviour, it can result in arguably unnecessary warnings in logs together with huge stack traces.

I discovered this problem while running a Dropwizard service with an uber/fat jar with dependencies that themselves declare optional dependencies. My specific example is that a dependency of ours depends on logback which itself has an optional Maven dependency on Groovy which we do not explicitly specify and therefore do not pull in. With an uber/far jar, the scanning in BeanConfig.classes() as determined by ClasspathHelper.forPackage(pkg) will scan everything in the jar and return only classes in specified resource packages. The scanning performed by Reflections looks at super classes. In this case, it finds a specific Logback class whose super type is groovy.lang.GroovyObject and logs an exception because that does not exist in the jar (by virtue of being an optional dependency).

The start of the resulting error is as follows:

WARN  [2018-01-18 09:02:59,996] org.reflections.Reflections: could not get type for name groovy.lang.GroovyObject from any class loader
 ! java.lang.ClassNotFoundException: groovy.lang.GroovyObject
 ! at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
 ! at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
 ! at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:335)
 ! at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
 ! at org.reflections.ReflectionUtils.forName(ReflectionUtils.java:388)
 ! ... 68 common frames omitted

I have omitted the rest of the stack trace because it's the best part of 100 frames long in addition to the omitted ones. I get this for a sufficient number of classes that it totals the best part of 2K lines of logs that are not at all useful to me. If those classes were in my packages or packages I want to include in the swagger spec then this would indeed be useful so just silencing this log line is not an option.

Reflections offers the option of specifying an input filter which gets invoked with both the location being scanned (path in jar or class file path, etc.) and also the fully qualified name of the class being scanned.

Would it make sense to move the filtration logic based on resource packages from filtering the output of Reflections into an input filter implementation? This would prevent non-matching classes from being scanned in the first place. It would silence the warnings I mention above and would have a (admittedly almost certainly insignificant) performance boost in that classes which will never be returned from BeanConfig.classes() won't even be scanned in the first place.

I've tried this locally and it works well in my case with uber/fat jars, I've also tried it with class dirs. Can anyone see any cases where this may not work? If no issues, I'm happy to submit a PR for this change.

One potential issue I can think of is if, for example, a resource in a matching package extends from a resource in a non-matching package. However, the behaviour in such a scenario is already inconsistent. With an uber/jar operations from such a parent resource do get included as the parent class is in the jar. With individual jars or class dirs, if the parent is in a different dir or jar those operations won't be included. With that in mind, my proposed change would actually make the approaches consistent (with the exception of them being in non-sub package names but in the same module and hence class dir).

tsiq-karold commented 6 years ago

Does anyone have any thoughts on this? Or should I submit a PR before discussing this further?

JayeshS commented 6 years ago

@tsiq-karold, I have the same problem and your proposal makes sense to me.

Evanthx commented 6 years ago

@tsiq-karold Just a +1 to having your PR!

jmilkiewicz commented 6 years ago

@tsiq-karold When can we expect your PR ?