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 demonstration #19

Closed
wants to merge 3 commits into from
Closed

NPE demonstration #19

wants to merge 3 commits into from

Conversation

koppor
Copy link

@koppor koppor commented Jul 16, 2017

I wanted to check the behavior of `noClasses: All classes having an annotation should be the same as no classes with not that annotation. But that test case simply throws an NPE. I don't know the reason for it. Maybe @codecholeric sees it directly.

java.lang.NullPointerException
	at java.util.regex.Matcher.getTextLength(Matcher.java:1283)
	at java.util.regex.Matcher.reset(Matcher.java:309)
	at java.util.regex.Matcher.<init>(Matcher.java:229)
	at java.util.regex.Pattern.matcher(Pattern.java:1093)
	at java.util.Formatter.parse(Formatter.java:2547)
	at java.util.Formatter.format(Formatter.java:2501)
	at java.util.Formatter.format(Formatter.java:2455)
	at java.lang.String.format(String.java:2940)
	at com.tngtech.archunit.lang.ArchCondition.as(ArchCondition.java:65)
	at com.tngtech.archunit.lang.syntax.ArchRuleDefinition$1.apply(ArchRuleDefinition.java:119)
	at com.tngtech.archunit.lang.syntax.ArchRuleDefinition$1.apply(ArchRuleDefinition.java:116)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal$FinishedRule.createCondition(ObjectsShouldInternal.java:100)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal$FinishedRule.get(ObjectsShouldInternal.java:96)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal$FinishedRule.get(ObjectsShouldInternal.java:93)
	at com.google.common.base.Suppliers$MemoizingSupplier.get(Suppliers.java:131)
	at com.tngtech.archunit.lang.syntax.ObjectsShouldInternal.check(ObjectsShouldInternal.java:75)
	at com.tngtech.archunit.lang.syntax.elements.GivenClassesThatTest$Evaluator.on(GivenClassesThatTest.java:481)
	at com.tngtech.archunit.lang.syntax.elements.GivenClassesThatTest.areAnnotatedWith_type_inverse(GivenClassesThatTest.java:225)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.mockito.internal.junit.JUnitRule$1.evaluate(JUnitRule.java:16)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)

@codecholeric
Copy link
Collaborator

Maybe not 'directly' 😉 , but actually the NPE is from the test setup. mock(ArchCondition.class) returns null as description, which would normally never happen.
Also noClasses() does behave a little different from what you seem to expect. It does not invert the filtering of the classes, but it inverts the condition. Thus, if you say

noClasses().that().areNotAnnotatedWith(Foo.class)

the set of classes to analyze is still all classes not annotated with @Foo, just like

classes().that().areNotAnnotatedWith(Foo.class)

The difference applies, when the condition on those classes is evaluated. noClasses() will invert the condition. Thus in your test example, both times SimpleClass will be analyzed, but classes() will accept when the condition is satisfied, while noClasses() will fail.

The test you were patching, is simply supposed to check this filtering, thus it's not the right place to look for differences between classes() and noClasses(). (Nevertheless, I'll improve this test a little bit, so it doesn't use mock objects returning unrealistic values)

Maybe you can elaborate, what you're trying to do in your real life architecture test, to see why it behaves unexpected in your case?

…an be ignored. Remove @Allow from Okay.java and the violation will be reported (likewise, add it to Bad.java and the test will pass). Note that annotations may not be @retention(SOURCE) to be picked up.
@codecholeric
Copy link
Collaborator

From what I read on the linked issue, you're trying to ignore some known violations by annotating classes that may violate the rule? I've added a simple demo here:
7a7e3d5
Also ArchUnit itself uses a similar pattern ( @MayResolveTypesViaReflection ), in case you want to check that out.
At least with the simple example, I had no problems though... Let me know, if the problem persists!!

@koppor
Copy link
Author

koppor commented Jul 16, 2017

Thank you for 7a7e3d5. It shows my usecase.

However, when executing it in my case, I get

java.lang.AssertionError: Architecture Violation [Priority: MEDIUM] - Rule 'no classes that are not annotated with @ApacheCommonsLang3Allowed should access classes that reside in a package 'org.apache.commons.lang3'' was violated:
Method <org.jabref.logic.formatter.bibtexfields.HtmlToUnicodeFormatter.format(java.lang.String)> calls method <org.apache.commons.lang3.StringEscapeUtils.unescapeHtml4(java.lang.String)> in (HtmlToUnicodeFormatter.java:36)

The class HtmlToUnicodeFormatter is annotated with the annotation allowing to access the evil class:

JabRef/jabref@cd5a87c#diff-65cb57dfbc237c0545ddfe947ed5a12c

I have no clue, why the minimal example works, but in a larger setting, it does not.

21:03:31.692 [main] INFO com.tngtech.archunit.core.importer.JavaClassProcessor - Analysing class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter' 21:03:31.692 [main] INFO com.tngtech.archunit.core.importer.JavaClassProcessor - Analysing class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter' 21:03:31.692 [main] DEBUG com.tngtech.archunit.core.importer.JavaClassProcessor - Found interfaces [org.jabref.logic.layout.LayoutFormatter, org.jabref.model.cleanup.Formatter] on class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter' 21:03:31.692 [main] DEBUG com.tngtech.archunit.core.importer.JavaClassProcessor - Found interfaces [org.jabref.logic.layout.LayoutFormatter, org.jabref.model.cleanup.Formatter] on class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter' 21:03:31.692 [main] DEBUG com.tngtech.archunit.core.importer.JavaClassProcessor - Found superclass com.tngtech.archunit.base.Optional$Present@3f6979b2 on class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter' 21:03:31.692 [main] DEBUG com.tngtech.archunit.core.importer.JavaClassProcessor - Found superclass com.tngtech.archunit.base.Optional$Present@3f6979b2 on class 'org/jabref/logic/formatter/bibtexfields/HtmlToUnicodeFormatter'

I added some debug output concerning the annotations at 6ca5a8a, but currently not used in my project.

@codecholeric
Copy link
Collaborator

I think it's what I tried to point out in the last part of the commit message of my simple POC. ArchUnit analyses bytecode, but your annotation has @Retention(SOURCE)

@Retention(RetentionPolicy.SOURCE)
public @interface ApacheCommonsLang3Allowed { ... }

You need at least @Retention(CLASS) to enable ArchUnit to pick up the annotation, otherwise the information is lost, by the time the class files are imported.
Can you check if that is the problem?

@koppor
Copy link
Author

koppor commented Jul 18, 2017

I totally overlooked that. 😞 Thank you for your patience and time 👍 Now everything works.

@koppor koppor closed this Jul 18, 2017
@codecholeric
Copy link
Collaborator

No problem, glad you got it working 😄 Let me know if you run into any further problems!!

@koppor koppor deleted the NPE-at-test branch August 4, 2017 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants