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

Support .dependOnClassesThat() for Methods #1060

Open
u3r opened this issue Feb 15, 2023 · 6 comments · May be fixed by #1135
Open

Support .dependOnClassesThat() for Methods #1060

u3r opened this issue Feb 15, 2023 · 6 comments · May be fixed by #1135

Comments

@u3r
Copy link
Contributor

u3r commented Feb 15, 2023

I'd like to write a test for
<no non-private Methods should "depend" on a specific (dangerous) class>

After writing lots of DescribedPredicates I noticed that noClasses().should().dependOnClassesThat(). ... already does NEARLY what I need but is too all-encompassing.

Internally everything is there Dependency.tryCreateFromReturnType and friends are all there - just not public api.

Therefore I'd like to propose exposing something like

   noMethods().should().dependOnArgumentsThat(). ... // just the arguments
              .orShould().dependOnReturnTypesThat()...  // just the return type
              .orShould().accessClassesThat() // just access within the method
              .orShould().dependOnClassesThat() // arguments + return type + access within the method
@codecholeric
Copy link
Collaborator

I'm open for the suggestion, but I'd also be interested what "too all-encompassing" means in your context 🤔

@u3r
Copy link
Contributor Author

u3r commented Feb 27, 2023

Thanks @codecholeric ,

let me try to lay this out then:

I have some difficult-to-work-with 3rd party class, I want to contain the use of. So I have 2 classes that can work correctly with that class but should only use that class internally.
So what I want to say is essentially

ArchRule noReturnOfRepresentation = noMethods()
         .that( not( HasModifiers.Predicates.modifier( JavaModifier.PRIVATE ) ) )
         .and().areNotDeclaredIn( AbstractWhitelistedClass.class )
         
         .dependOnArgumentsThat(JavaClass.Predicates.assignableTo(BadClass.class)) // <-- from proposal
         .orShould().dependOnReturnTypesThat(JavaClass.Predicates.assignableTo(BadClass.class))  // <-- from proposal

         .because( "Interfacing with BadClass requires to know the secret internal naming scheme "
               + "and this should be restricted to WhitelistedClass1 + 2 and their parents." ); 

If I'm in the same codebase, I can write

@ArchTest
   ArchRule noUsageOfRoleRepresentation = noClasses()
         .that(
               not( JavaClass.Predicates.assignableTo( WhitelistedClass1.class ) )
                     .and( not( JavaClass.Predicates.assignableTo( WhitelistedClass2.class ) ) )
                     .and( not( JavaClass.Predicates.assignableTo( AbstractWhitelistedClass.class ) ) )
         )
         .should().dependOnClassesThat().areAssignableTo( BadClass.class )

         .because( "Interfacing with BadClass requires to know the secret internal naming scheme "
               + "and this should be restricted to WhitelistedClass1 + 2 and their parents." );

But if I want to verify a public API for some library, I can not use that snipped, as any public method in WhitelistedClass1 can still reference the bad type as long as it is not used anywhere in that library.

Sidenote: Anything that could be done to JavaType to make working with generic types (read: create predicates or conditions for them) easier would also be much appreciated. But I think these methods would cover > 80% of usecases.

Addendum: by "too all encompassing" I mean:
Using noClasses().should().dependOnClassesThat(). i need to make an exception for the whitelisted classes - thereby NOT catching public API provided by them.

@codecholeric
Copy link
Collaborator

Ah, I think I understand. But even if WhiteListedClass1 would expose BadClass as public API no other class could use it or it would be caught by the rule again, no? Because using that return type or method parameter would then cause a violation of that other class depending on it?

@u3r
Copy link
Contributor Author

u3r commented Feb 28, 2023

That is technically correct but
a) I'd like to check that my LIBRARY does not expose this class in it's api (not requiring the caller to write/import archunit tests)
And YES I am aware that the caller STILL could violate this due to transitive dependency import.... ;-)
-- I just like MY code to be clean.

b) let me extend the feature request to also support predicates of the form noMethods().that(JavaMethods.Predicates.HasArgumentsThat(<subPredicate using generic types>)

Or should b) be a separate feature?

@leonardhusmann
Copy link
Contributor

Hi, i would like to work on this issue 😄

Let me rephrase how I understood this issue to make sure that we're on the same page:

a) You want to check for certain methods (matched by your predicate) that these don't expose or interact with a specific class (e.g. BadClass) via their 1) arguments (dependOnArgumentsThat()), 2) return types (dependOnReturnTypesThat()), 3) access to the BadClass in its method body (accessClassesThat()) and 4) the combination of all of them (dependOnClassesThat()).
This would allow for more fine granular rules that only check the defined constraints (e.g. only check for return types) and for rules on method level instead of class level. Correct?

b) I'm not sure if I understood this correctly. Is this covered by noMethods().that(HasReturnType.Predicates.rawReturnTypes(...)? e.g.

@ArchRule
static final ArchRule methodsShouldNotExposeBadParameters = noMethods()
    .that(HasParameterTypes.Predicates.rawParameterTypes(BadClass.class))
    .should().beFinal();

Or what would you expect HasArgumentsThat to do differently? Is it that your proposed predicate matches if any of the included arguments is of the specified type?

leonardhusmann added a commit to leonardhusmann/ArchUnit that referenced this issue Jul 8, 2023
Issue: TNG#1060
Signed-off-by: Leonard Husmann <leonard.husmann@tum.de>
@leonardhusmann leonardhusmann linked a pull request Jul 10, 2023 that will close this issue
@u3r
Copy link
Contributor Author

u3r commented Nov 7, 2023

Having looked at the Dependency type a little more, I wonder, if an API like

noMethods().should().dependOnArgumentsThat(DescribedPredicate<Dependency>...). ... // just the arguments
              .orShould().dependOnReturnTypesThat(DescribedPredicate<Dependency>...)...  // just the return type
              .orShould().accessClassesThat(DescribedPredicate<Dependency>...) // just access within the method
              .orShould().dependOnClassesThat(DescribedPredicate<Dependency>...) // arguments + return type + access within the method

because this way the predicate has origin and target available, which is often needed to say something:

"no methods should depend on Classes that are named "foo" and not part of its own class hierarchy"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants