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

NPE from getDirectDependenciesFromSelf() called on a JavaClass stub #397

Closed
hankem opened this issue Jul 13, 2020 · 3 comments
Closed

NPE from getDirectDependenciesFromSelf() called on a JavaClass stub #397

hankem opened this issue Jul 13, 2020 · 3 comments

Comments

@hankem
Copy link
Member

hankem commented Jul 13, 2020

I've run into an issue with transitive dependencies not being imported completely.

Here's a simple example (which, admittedly, looks strange on its own 😃) to illustrate it:

new ClassFileImporter().importClasses(java.lang.Object.class)
        .get(java.lang.Object.class)
        .getMethod("finalize").getThrowsClause().getTypes().get(0)  // java.lang.Throwable
        .getMethod("getStackTrace").getRawReturnType().getComponentType()  // java.lang.StackTraceElement
        .getDirectDependenciesFromSelf();

Since 85506e2, I'm getting a

java.lang.NullPointerException
	at com.tngtech.archunit.core.domain.JavaClass.getDirectDependenciesFromSelf(JavaClass.java:894)

The line number is for ArchUnit 0.14.1, but getDirectDependenciesFromSelf is currently a one-liner anyways; the issue is that javaClassDependencies is not initialized, i.e. that JavaClass.completeFrom(ImportContext) was not called. But here, I don't know what's expected for stubs.

Should I be able to getDirectDependenciesFromSelf() from a stub JavaClass? (Anyways, the NPE isn't a nice result for sure.)

The hypothesis that the error might be because Throwable.getStackTrace() returns StackTraceElement[] (which seems to be imported correctly), whereas I'm asking for dependencies from StackTraceElement, is supported by the following simpler example:

new ClassFileImporter().importClasses(DependsOnArray.class)
        .get(DependsOnArray.class)
        .getField("array").getRawType().getComponentType()  // Element
        .getDirectDependenciesFromSelf();

with

static class DependsOnArray {
    Element[] array;
}

static class Element {
}

I'm still puzzled though why the same issue does not show up in:

new ClassFileImporter().importClasses(java.lang.Throwable.class)
        .get(java.lang.Throwable.class)
        .getMethod("getStackTrace").getRawReturnType().getComponentType()  // java.lang.StackTraceElement
        .getDirectDependenciesFromSelf();
@codecholeric
Copy link
Collaborator

To answer your first question, no, you should not be able to get dependencies from a stub class 😉 If you want to analyse a class you should import it. Nevertheless I do agree that a NPE is not particularly nice, so I think we should fix this and give stub classes simply empty dependencies. None of the rules API accesses stub classes that way, that's probably why it hasn't shown so far 😉
I might have an explanation for your confusion though 🤔 Throwable has a direct dependency to StackTraceElement. Thus if you resolve missing dependencies from classpath (the default) you'll end up with a correctly imported class StackTraceElement. For Object, Throwable is already a missing dependency and the resolution of missing dependencies does not work transitively. Thus the transitive dependency Throwable -> StackTraceElement is not resolved but replaced by a stub (only direct dependencies are resolved with the configured resolver, after that the chain is cut by introducing stubs). At least that would make sense to me, what do you think?
But yes, a NPE is never correct behavior, so we should fix that 😉

@hankem
Copy link
Member Author

hankem commented Jul 14, 2020

Thanks, @codecholeric!

If you want to analyse a class you should import it.

Fair enough. When looking for transitive dependencies, I'll anyways limit the recursion to the package I've actually imported.
(Playing with the code, I once just gave it a try to grab everything that's there; this is how my recursion ended up in checking transitive dependencies of java.lang.Object... 😃)

we should fix this and give stub classes simply empty dependencies

That would be great! I guess that this will be as simple as providing an API to recognize a stub JavaClass, which does not exist right now, if I'm not mistaken?

@codecholeric
Copy link
Collaborator

One question is what should happen if you call getDirectDependencies{From/To}Self() on a stub class 🤔 If we want to be consistent with other places, it would just return an empty set. It could of course also throw an exception that this class is a stub and you should not do checks like this on it, but I don't know. And kind of orthogonal there is the question if it should be possible to detect / query if a class is a stub. We could add a method like JavaClass.isImported() or similar 🤔

hankem added a commit that referenced this issue Jul 16, 2020
@hankem hankem changed the title Imported dependencies incomplete NPE from getDirectDependenciesFromSelf() called on a JavaClass stub Jul 16, 2020
@hankem hankem self-assigned this Jul 16, 2020
hankem added a commit that referenced this issue Sep 13, 2020
hankem added a commit that referenced this issue Dec 6, 2020
hankem added a commit that referenced this issue Dec 15, 2020
codecholeric pushed a commit that referenced this issue Dec 16, 2020
So far, if a user would call `JavaClass.getDirectDependency{From/To}Self()` on a class that was not directly imported (e.g. a class that was not present in the original import, but accessed by a class from that import) it would cause a NPE. While we do want to cut the class graph at that point (i.e. not further track dependencies for classes that are not part of the import), we do not want this exceptional behavior. This change fixes the behavior and sets empty dependencies for all non-directly imported classes, be it classes that have been resolved from the classpath later on, or stub classes, because further resolution has been turned off.

Resolves: #397

Signed-off-by: Manfred Hanke <[email protected]>
codecholeric pushed a commit that referenced this issue Feb 21, 2021
So far, if a user would call `JavaClass.getDirectDependency{From/To}Self()` on a class that was not directly imported (e.g. a class that was not present in the original import, but accessed by a class from that import) it would cause a NPE. While we do want to cut the class graph at that point (i.e. not further track dependencies for classes that are not part of the import), we do not want this exceptional behavior. This change fixes the behavior and sets empty dependencies for all non-directly imported classes, be it classes that have been resolved from the classpath later on, or stub classes, because further resolution has been turned off.

Resolves: #397

Signed-off-by: Manfred Hanke <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants