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

Remove false positives from Transactional Proxy Rule #1253

Open
fletchgqc opened this issue Feb 20, 2024 · 2 comments
Open

Remove false positives from Transactional Proxy Rule #1253

fletchgqc opened this issue Feb 20, 2024 · 2 comments

Comments

@fletchgqc
Copy link

fletchgqc commented Feb 20, 2024

I think that the ProxyRule

no_classes_should_directly_call_other_methods_declared_in_the_same_class_that_are_annotated_with(Transactional)

creates false positives. We are checking that no method calls Transactional methods in the same class. However, if the calling method is also Transactional, there is probably no problem. The Transaction is started by the calling method and includes the called method. Thefore, the rule should really be something like:

no_methods_not_annotated_with_should_directly_call_other_methods_declared_in_the_same_class_that_are_annotated_with

Do you agree with my logic?

@fletchgqc fletchgqc changed the title no_classes_should_directly_call_other_methods_declared_in_the_same_class_that_are_annotated_with Remove false positives from Transactional Proxy Rule Feb 20, 2024
@fletchgqc
Copy link
Author

fletchgqc commented Feb 20, 2024

OK, I worked out the solution. The methods in ProxyRules.java should return ArchCondition<JavaCodeUnit>, for example:

  public static ArchCondition<JavaCodeUnit> directly_call_other_methods_declared_in_the_same_class_that_are_annotated_with(Class<? extends Annotation> annotationType) {
        return directly_call_other_methods_declared_in_the_same_class_that(are(annotatedWith(annotationType)));
    }

This would allow us to apply these methods with methods(). Like this:

  public static final ArchRule no_transaction_bypassing_calls = ArchRuleDefinition.noMethods().that().areNotAnnotatedWith(Transactional.class).should(directly_call_other_methods_declared_in_the_same_class_that_are_annotated_with(Transactional.class));

I made the changes locally and it worked for me.

@rweisleder
Copy link
Contributor

What about this class?

public class MyService {

	@Transactional
	public void a() [
		b();
	}

	private void b() [
		c();
	}

	@Transactional
	public void c() [
		// ...
	}
}

Should the method call in b() be a violation? Currently, it will only be invoked inside a transaction started by a().

Another example from Spring:

class MyRepo {

	@Cacheable("x")
	public String x() [
		y();
		// ...
	}

	@Cacheable("y")
	public int y() [
		// ...
	}
}

In this case, the method call in x() should be considered as violation, because the two @Cacheable are independent.

I'm not really happy with ProxyRules and not sure if ArchUnit is the right place for that. Clearly, the example in the documentation is not the best.

That's why I started archunit-spring and support for @Transactional is on my todo list: rweisleder/archunit-spring#2
(In case you are working with Jakarta EE instead, I plan to create dedicated rules for that as well.)

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

No branches or pull requests

2 participants