Skip to content

Fix local class scanning #111

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.

Sign up for GitHub

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

Open
wheelerlaw wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix local class scanning #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ceb3ae
Maven disables https now by default
wheelerlaw Oct 28, 2021
d11990d
We don't want to use Reflections because it only selects direct subty…
wheelerlaw Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ target
*.pyc
build.log
**/*.versionsBackup
.DS_Store
.DS_Store
logdir*
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) a
## Unreleased

* Fixed a bug for Java 11 compatibility
* Changed Jenkins Maven repo URL from http:// to https://
* The LocalProjectPipelineExtensionDetectorSpec#getClassesOfTypeInPackage method now finds classes that are
descendants of `java.lang.Object` but aren't direct subtypes.

## 2.1.5

Expand Down
4 changes: 2 additions & 2 deletions examples/helper-script/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
<repository>
<id>jenkins-releases</id>
<name>Jenkins Releases</name>
<url>http://repo.jenkins-ci.org/releases</url>
<url>https://repo.jenkins-ci.org/releases</url>
</repository>
<repository>
<id>jenkins-public</id>
<name>Jenkins Public</name>
<url>http://repo.jenkins-ci.org/public</url>
<url>https://repo.jenkins-ci.org/public</url>
</repository>
</repositories>

Expand Down
4 changes: 2 additions & 2 deletions examples/shared-library/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
<repository>
<id>jenkins-releases</id>
<name>Jenkins Releases</name>
<url>http://repo.jenkins-ci.org/releases</url>
<url>https://repo.jenkins-ci.org/releases</url>
</repository>
<repository>
<id>jenkins-public</id>
<name>Jenkins Public</name>
<url>http://repo.jenkins-ci.org/public</url>
<url>https://repo.jenkins-ci.org/public</url>
</repository>
</repositories>

Expand Down
4 changes: 2 additions & 2 deletions examples/whole-pipeline/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
<repository>
<id>jenkins-releases</id>
<name>Jenkins Releases</name>
<url>http://repo.jenkins-ci.org/releases</url>
<url>https://repo.jenkins-ci.org/releases</url>
</repository>
<repository>
<id>jenkins-public</id>
<name>Jenkins Public</name>
<url>http://repo.jenkins-ci.org/public</url>
<url>https://repo.jenkins-ci.org/public</url>
</repository>
</repositories>

Expand Down
20 changes: 9 additions & 11 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@
<repository>
<id>jenkins-releases</id>
<name>Jenkins Releases</name>
<url>http://repo.jenkins-ci.org/releases</url>
<url>https://repo.jenkins-ci.org/releases</url>
</repository>
<repository>
<id>repo.jenkins-ci.org</id>
<name>Jenkins Repo</name>
<url>https://repo.jenkins-ci.org/public</url>
</repository>
</repositories>

Expand Down Expand Up @@ -271,12 +276,6 @@
<version>${objenesis.version}</version>
</dependency>

<dependency>
<groupId>org.reflections</groupId>
<artifactId>reflections</artifactId>
<version>0.9.12</version>
</dependency>
Comment on lines -274 to -278
Copy link
Copy Markdown
Contributor

@awittha awittha Nov 29, 2021

Choose a reason for hiding this comment

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

The motivation behind using Reflections AND GlassGraph, was that Reflections would only see files in the local project; nothing on the extended classpath from Maven dependencies would be included.

How does one configure the current ClassGraph to only see files that are physically present on disk in the current project?

Or, to ask another way:

With this code change, if there's a dependency of the project that includes a file like the ScriptToTest.groovy that's added here, will these changes detect that script, or not?

They should not.

Is that done with the .disableJarScanning() here: https://github.com/ExpediaGroup/jenkins-spock/pull/111/files#diff-7cf77f094673fdcead882d1c0b654a186428288727ba01a59de640f2a9f80d7fR57 ?


<dependency>
<groupId>io.github.classgraph</groupId>
<artifactId>classgraph</artifactId>
Expand Down Expand Up @@ -419,10 +418,10 @@

<!-- 'parallel' step comes from workflow-cps -->

<!--
<!--
for mocking the Jenkins singleton outside Spock
Only done to verify that IF that is done properly, THEN
jenkins-spock can use that object
jenkins-spock can use that object
-->
<dependency>
<groupId>org.mockito</groupId>
Expand All @@ -436,7 +435,6 @@
<build>
<pluginManagement>
<plugins>

<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-assembly-plugin</artifactId>
Expand Down Expand Up @@ -477,7 +475,7 @@
<artifactId>maven-dependency-plugin</artifactId>
<configuration>
<usedDependencies>
<!-- these will be used by Groovy code, when the included test utilities
<!-- these will be used by Groovy code, when the included test utilities
are spun up during a test run -->
<usedDependency>cglib:cglib-nodep</usedDependency>
<usedDependency>junit:junit</usedDependency>
Expand Down
77 changes: 34 additions & 43 deletions ...ain/java/com/homeaway/devtools/jenkins/testing/LocalProjectPipelineExtensionDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,17 @@
package com.homeaway.devtools.jenkins.testing;

import java.lang.annotation.Annotation;
import java.util.HashMap;
import java.util.HashSet;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.Set;

import org.reflections.Reflections;
import org.reflections.ReflectionsException;
import org.reflections.scanners.SubTypesScanner;
import org.reflections.util.ClasspathHelper;
import org.reflections.util.ConfigurationBuilder;
import groovy.lang.Closure;
import io.github.classgraph.ClassGraph;
import io.github.classgraph.ScanResult;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -51,46 +50,33 @@ public class LocalProjectPipelineExtensionDetector extends APipelineExtensionDet
@Override
public Set<Class<?>> getClassesOfTypeInPackage(Class<?> _supertype, Optional<String> _package) {

Set<Class<?>> classes = new HashSet<>();

Map<String, Throwable> failures = new HashMap<>();

Reflections reflector = new Reflections(
new ConfigurationBuilder()
.setScanners(new SubTypesScanner(false))
.setUrls(ClasspathHelper.forPackage(_package.orElse(""))));

Set<String> all_types = new HashSet<>();

try {
all_types = reflector.getAllTypes();
} catch( ReflectionsException re ) {
if( re.getMessage().contains( "Couldn't find subtypes of Object." ) ) {
// this can happen if there are no classes local to the project.
// the full error message is
// Couldn't find subtypes of Object.
// Make sure SubTypesScanner initialized to include Object class - new SubTypesScanner(false)
// which we have done above, so that is NOT the actual cause.
// If there are no classes local to the project, that's OK!
// Just use an empty set instead of throwing an exception.
LOG.info( "Looks like there aren't any classes compiled by this project." );
} else {
// We still do want to error with "real" exceptions, though.
throw re;
}
List<String> classnames;
try (
ScanResult scanResult = new ClassGraph()
.acceptPackages(_package.orElse(""))
.disableJarScanning()
.scan()
) {
classnames = scanResult
.getAllClasses()
.filter(ci -> !ci.extendsSuperclass(Closure.class.getName()))
.filter(ci -> !ci.extendsSuperclass("spock.lang.Specification"))
.filter(ci -> !(ci.extendsSuperclass(InvalidlyNamedScriptWrapper.class.getName()) || ci.getName().equals(InvalidlyNamedScriptWrapper.class.getName())))
.filter(ci -> ci.extendsSuperclass(_supertype.getName()))
.getNames();
}

for(String classname : all_types ) {

Class<?> clazz = null;
Set<Class<?>> classes = new HashSet<>();
Map<String, Throwable> failures = new HashMap<>();
for(String classname : classnames ) {
Class<?> clazz;

try {
clazz = Class.forName( classname );
} catch( ClassNotFoundException | NoClassDefFoundError e ) {
failures.put( classname, e );
continue;
}

classes.add( clazz );
}

Expand All @@ -114,12 +100,17 @@ public Set<Class<?>> getClassesOfTypeInPackage(Class<?> _supertype, Optional<Str
@Override
public Set<Class<?>> getClassesWithAnnotationOfTypeInPackage( Class<? extends Annotation> _annotation, Class<?> _supertype, Optional<String> _package) {

Set<Class<?>> annotated_classes = new HashSet<>();

for( Class<?> annotated_class : new Reflections( _package.orElse("") ).getTypesAnnotatedWith( _annotation ) ) {
if( _supertype.isAssignableFrom( annotated_class ) ) {
annotated_classes.add( annotated_class );
}
Set<Class<?>> annotated_classes;
try (
ScanResult scanResult = new ClassGraph()
Copy link
Copy Markdown
Contributor

@awittha awittha Nov 29, 2021

Choose a reason for hiding this comment

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

Should this also .disableJarScanning()?

.acceptPackages(_package.orElse(""))
.enableAnnotationInfo()
.scan()
) {
annotated_classes = new HashSet<>(scanResult
.getClassesWithAnnotation(_annotation.getName())
.filter(ci -> ci.extendsSuperclass(_supertype.getName()))
.loadClasses(true));
}

return annotated_classes;
Expand Down
44 changes: 28 additions & 16 deletions ...n/java/com/homeaway/devtools/jenkins/testing/WholeClasspathPipelineExtensionDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@
import java.util.Optional;
import java.util.Set;

import io.github.classgraph.ClassGraph;
import io.github.classgraph.ScanResult;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import io.github.classgraph.ClassGraph;

/**
* Search through classes in the entire classpath for Jenkins Pipeline extensions.
Expand All @@ -44,14 +45,19 @@ public class WholeClasspathPipelineExtensionDetector extends APipelineExtensionD
@Override
public Set<Class<?>> getClassesOfTypeInPackage(Class<?> _supertype, Optional<String> _package) {

Set<Class<?>> classes = new HashSet<>();

List<String> classnames = new ClassGraph()
.enableAnnotationInfo()
.enableClassInfo()
.acceptPackages(_package.orElse(""))
.scan().getAllStandardClasses().getNames();
List<String> classnames;
try (
ScanResult scanResult = new ClassGraph()
.acceptPackages(_package.orElse(""))
.scan()
) {
classnames = scanResult
.getAllClasses()
.filter(ci -> ci.extendsSuperclass(_supertype.getName()))
.getNames();
}

Set<Class<?>> classes = new HashSet<>();
HashMap<String, Throwable> failures = new HashMap<>();

for(String classname: classnames) {
Expand All @@ -60,7 +66,7 @@ public Set<Class<?>> getClassesOfTypeInPackage(Class<?> _supertype, Optional<Str

try {
clazz = Class.forName( classname );
} catch( ClassNotFoundException e ) {
} catch( ClassNotFoundException | NoClassDefFoundError e ) {
failures.put( classname, e );
continue;
} catch( Throwable t ) {
Expand Down Expand Up @@ -94,16 +100,22 @@ public Set<Class<?>> getClassesOfTypeInPackage(Class<?> _supertype, Optional<Str
@Override
public Set<Class<?>> getClassesWithAnnotationOfTypeInPackage( Class<? extends Annotation> _annotation, Class<?> _supertype, Optional<String> _package) {

Set<Class<?>> annotated_classes = new HashSet<>();
List<String> annotated_classnames;
try (
ScanResult scanResult = new ClassGraph()
.acceptPackages(_package.orElse(""))
.enableAnnotationInfo()
.scan();
) {
annotated_classnames = scanResult
.getClassesWithAnnotation(_annotation.getName())
.filter(ci -> ci.extendsSuperclass(_supertype.getName()))
.getNames();
}

Set<Class<?>> annotated_classes = new HashSet<>();
HashMap<String, Throwable> failures = new HashMap<>();

List<String> annotated_classnames = new ClassGraph()
.enableAnnotationInfo()
.enableClassInfo()
.acceptPackages(_package.orElse(""))
.scan().getClassesWithAnnotation( _annotation.getName() ).getNames();

for(String classname: annotated_classnames) {

Class<?> clazz = null;
Expand Down
18 changes: 18 additions & 0 deletions ...vy/com/homeaway/devtools/jenkins/testing/LocalProjectPipelineExtensionDetectorSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.homeaway.devtools.jenkins.testing

import com.homeaway.devtools.jenkins.testing.functions.ScriptToTest

class LocalProjectPipelineExtensionDetectorSpec extends JenkinsPipelineSpecification {

def "LocalProjectPipelineExtensionDetectorSpec should detect classes that aren't direct subtypes of java-lang-Object" () {
Copy link
Copy Markdown
Contributor

@awittha awittha Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
def "LocalProjectPipelineExtensionDetectorSpec should detect classes that aren't direct subtypes of java-lang-Object" () {
def "LocalProjectPipelineExtensionDetector should detect classes that aren't direct subtypes of java-lang-Object" () {

It's really the LocalProjectPipelineExtensionDetector that you're testing, right?

If you were testing the LoalProjectPipelineExtensionDetectorSpec, you'd be checking the DEFAULT_TEST_CLASSES ( https://github.com/ExpediaGroup/jenkins-spock/blob/master/src/main/groovy/com/homeaway/devtools/jenkins/testing/JenkinsPipelineSpecification.groovy#L1137 ) variable's contents, right?

I think this test can actually just extend Specification, since it's testing a standalone, non-Jenkins-specific class (LocalProjectPipelineExtensionDetector), right?

And if that's the case... since the class-under-test is in Java, can the test itself be written in ("matching") JUnit Java, too?

(These are actual questions, not change requests phrased as questions, by the way.)

when:
Set<Class<?>> classes = new LocalProjectPipelineExtensionDetector()
.getClassesOfTypeInPackage(
Object.class,
Optional.<String>empty()
)

then:
classes.contains(ScriptToTest)
}
}
5 changes: 5 additions & 0 deletions src/test/groovy/com/homeaway/devtools/jenkins/testing/functions/ScriptToTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.homeaway.devtools.jenkins.testing.functions

def helloWorld(String argument) {
echo argument
}