#704: added onlyBeCalledByClassesThat conjuntion - initial part #810

JKLedzion · 2022-02-24T13:55:15Z

Initial part of work for issue #704. Work is in progress but it would be great to receive some feedback if it goes in right direction.

I’ve started from adding method onlyBeCalledByClassesThat(DescribedPredicate predicate) to CodeUnitsShould.
Now I’m working on possibility to have a rule like:
should().onlyBeCalled().{byClassesThat()/methodsThat()/constructorsThat()}.

Could you let me know if my understanding of the issue is correct for below points?

I’ve implemented condition “code unit should only be called by”, but few possibilities came to my mind, not sure if I’ve decided on the correct one:
- code unit should be called only by classes that,
- code unit should be called by at least one class that,
- code unit should be called by all classes that (but can be called also by other)
- code unit should be called by all classes that (and can’t be called by any other).
should().onlyBeCalled().byMethodsThat() - here I’m checking if methods calling selected code unit meet provided criteria but still this code unit can be called by constructors not meeting criteria. Is it correct?

I would appreciate any feedback.
Justyna

Signed-off-by: Justyna Kubica-Ledzion justyna.kubica-ledzion@digitalnewagency.com

codecholeric · 2022-02-25T07:26:12Z

Thanks a lot for tackling this, we really appreciate it 😄 Also good idea to reach out early so we can discuss unclear points 👍

To answer your questions:

To be consistent with the rest "should only be called by classes that" should follow your first point. I.e. every class that calls the respective code unit should match the predicate. Otherwise we would e.g. use "any class that" or sth. like that. But we should also add Javadoc in the end to explain this precisely 🙂
If you say "only be called by methods that" I would forbid constructor calls. You are right that both is valid (again, that's why we should definitely add Javadoc), you could as well ignore constructor calls and only assert method calls. But we will also have "only be called by code units that", and that one will match methods and constructors, so given that I think it makes sense to forbid constructors when we talk about "only...by methods". Also, if we ponder how you would write the condition if you really want to forbid constructors. Then you would have to write something like "only be called by methods that ... are not constructors" and that just sounds weird to me 😉

codecholeric · 2022-02-25T07:30:45Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaCodeUnit.java

@@ -171,6 +171,12 @@ public JavaClass getRawReturnType() {
        return fieldAccesses;
    }

+    @PublicAPI(usage = ACCESS)
+    public abstract Set<? extends JavaAccess<?>> getCallsOfSelf();


Couldn't the bound here be JavaCall instead of JavaAccess?

codecholeric · 2022-02-25T07:31:44Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaCodeUnit.java

+             * @see #getCallsOfSelf()
+             */
+            @PublicAPI(usage = ACCESS)
+            public static final ChainableFunction<JavaCodeUnit, Set<? extends JavaAccess<?>>> GET_CALLS_TO_SELF =


By convention the constant name should be the exact method name in upper case. I.e. if the getter is getCallsOfSelf then the function should be GET_CALLS_OF_SELF

codecholeric · 2022-02-25T07:35:41Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaConstructor.java

    @PublicAPI(usage = ACCESS)
    public Set<JavaConstructorCall> getCallsOfSelf() {
        return getReverseDependencies().getCallsTo(this);
    }

+    @PublicAPI(usage = ACCESS)
+    public Set<JavaConstructorCall> getMethodCallsOfSelf() {


This method is confusing to me because "get method calls..." sounds to me as if it would return JavaMethodCall. But instead it's returning JavaConstructorCall which is no "method call". I know where you're coming from, but I think we need to find a different naming here 🤔 Maybe it would be something like getCallsOfSelfByMethods or ...FromMethods? 🤔 Or if we only need it in one place we don't add a new API method and simply filter in the one place 🤷‍♂️ We should ponder carefully if this is a common use case that should go in the API or not...

Method was removed as all calls should be analyzed not only calls done by methods.

codecholeric · 2022-02-25T07:37:12Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaConstructor.java

+    @Override
+    @PublicAPI(usage = ACCESS)
+    public boolean isMethod() {
+        return false;


You don't need to override this because the default is false anyway, no?

Yes, I've missed that.

codecholeric · 2022-02-25T07:38:00Z

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaMethod.java

+    @Override
+    @PublicAPI(usage = ACCESS)
+    public boolean isConstructor() {
+        return false;


Same here, the default is false anyway. As long as we go with a default (which you could question 🤷‍♂️) we should use it then I guess...

JKLedzion · 2022-02-28T13:01:43Z

Thanks @codecholeric for quick review and feedback :).

I’ve made corrections and added methods onlyBeCalled().byConstructorsThat(DescribedPredicate predicate) / onlyBeCalled().byCodeUnitsThat(DescribedPredicate predicate).

Ad. 2. I’ve implemented logic as follows:
.should().onlyBeCalled().byMethodsThat - calls done by methods are assessed against provided predicate, calls done by constructors are treated as violation of the rule. Method getMethodCallsOfSelf() was removed as all calls should be analyzed.

Currently below listed posibilities are implemented but I’m not sure if all of them are needed.
Maybe only following pattern should be left:
{constructors()/methods()/codeUnits()}.should().onlyBeCalled().by…
WDYT?

constructors()/methods()/codeUnits().should().onlyBeCalledByClassesThat(DescribedPredicate predicate)
constructors()/methods()/codeUnits().should().onlyBeCalledByClassesThat()
constructors()/methods()/codeUnits().should(onlyBeCalledByClassesThat(DescribedPredicate predicate)
constructors()/methods()/codeUnits().should().onlyBeCalled().byClassesThat(DescribedPredicate predicate)
constructors()/methods()/codeUnits().should().onlyBeCalled().byClassesThat()
constructors()/methods()/codeUnits().should().onlyBeCalled().byMethodsThat(DescribedPredicate predicate)
constructors()/methods()/codeUnits().should().onlyBeCalled().byConstructorsThat(DescribedPredicate predicate)
constructors()/methods()/codeUnits().should().onlyBeCalled().byCodeUnitsThat(DescribedPredicate predicate)

codecholeric · 2022-03-02T06:08:32Z

Thanks a lot for all your hard work 🙏 😄 Yes, I think there are already enough separate ways to express the same thing, we should probably not introduce both versions onlyBeCalledByClassesThat and onlyBeCalled().byClassesThat 👍 I think we should only go with the second version as it's more flexible and encapsulates this part of the syntax in a separate interface.

JKLedzion · 2022-03-02T21:11:24Z

Thanks for confirmation.
I’ve removed onlyBeCalledByClassesThat from CodeUnitsShould leaving only onlyBeCalled().byClassesThat.
You mentioned earlier adding precise JavaDoc for this new methods. I put them in OnlyBeCalledSpecification interface but I’m not sure if I was precisely enough 😄 WDYT?

codecholeric · 2022-03-03T05:45:50Z

archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/CodeUnitsShould.java

+     * <br><br>
+     * E.g.
+     * <pre><code>
+     * {@link ArchRuleDefinition#codeUnits() codeUnits()}.{@link GivenCodeUnits#should() should()}.{@link CodeUnitsShould#onlyBeCalled()} onlyBeCalled().{@link OnlyBeCalledSpecification#byClassesThat(DescribedPredicate)}  byClassesThat(equivalentTo(SomeClass.class))}


Instead of the version with predicate byClassesThat(equivalentTo(SomeClass.class)) maybe we can give an example with the fluent API, e.g. byClassesThat().areAnnotatedWith(SomeAnnotation.class) or something like that? 🤔

I've used example from tests.

codecholeric

Thanks a lot for all this hard word, very nice 😍 To me your Javadoc looks good, I added some minor suggestions to adjust it, but in general it's exactly how I imagined it 😃 Doesn't need to be a novel, but the important point (e.g. that byConstructors excludes methods) should be prominent 👍
I added some comments about minor things I noticed that could maybe be improved if you're still motivated?
In a future PR we can also fix the generics on MembersThat/CodeUnitsThat/... so that it can be offered as a fluent API completion of byCodeUnitsThat() 🙂 (But I consider that out of scope for this PR)

archunit/src/main/java/com/tngtech/archunit/core/domain/JavaCodeUnit.java

archunit/src/main/java/com/tngtech/archunit/lang/conditions/CodeUnitCallsCondition.java

archunit/src/main/java/com/tngtech/archunit/lang/conditions/ArchConditions.java

archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/OnlyBeCalledSpecification.java

archunit/src/main/java/com/tngtech/archunit/lang/syntax/elements/CodeUnitsShould.java

...main/java/com/tngtech/archunit/lang/syntax/OnlyBeCalledConstructorSpecificationInternal.java

archunit/src/test/java/com/tngtech/archunit/lang/syntax/elements/CodeUnitsShouldTest.java

JKLedzion · 2022-03-04T13:56:50Z

Yes, still motivated😃 but I will be able to work on changes from Wednesday next week.

JKLedzion · 2022-03-10T10:06:28Z

Hi, I've added modification for almost all comments, hope my understanding was correct 😄

There is one which I'm not sure about:
But if we unify CodeUnitCallsCondition it would likely just take description and predicate into the constructor separately anyway 🤷‍♂️ (at least that's how the respective Classes... does it)

There are 4 methods in ArchConditions using the same class CodeUnitOnlyCallsCondition, each with different description (i.e. "only be called by classes that “, “only be called by methods that ")
that is why I'm passing description as a parameter to CodeUnitOnlyCallsCondition constructor.
Also I’m passing extended predicate (i.e. origin.is(method().and(predicate)) so extracting predicate description inside CodeUnitOnlyCallsCondition constructor would end up with description: “… should only be called by methods that is method and custom predicate”.

In a future PR we can also fix the generics on MembersThat/CodeUnitsThat/... so that it can be offered as a fluent API completion of byCodeUnitsThat() 🙂 (But I consider that out of scope for this PR)

I think I can work on that PR also.

... which allows restricting callers of code units via * `...onlyBeCalled().byClassesThat(..)` * `...onlyBeCalled().byMethodsThat(..)` * `...onlyBeCalled().byConstructorsThat(..)` * `...onlyBeCalled().byCodeUnitsThat(..)` Signed-off-by: Justyna Kubica-Ledzion <justyna.kubica-ledzion@digitalnewagency.com>

codecholeric

Thanks for addressing all the comments and all your hard work!! I've squashed it now and adjusted the message a little bit to make it ready to be merged 🙂
Gonna merge as soon as CI is green 🚀

codecholeric · 2022-03-12T09:11:57Z

BTW: I've already fixed up the generics on MembersThat and others to make it reusable -> #832. I hope by that change it would now be easy to add this later on, i.e. that it's possible to add onlyBeCalled().byMethodsThat() in an easy way.

codecholeric reviewed Feb 25, 2022

View reviewed changes

codecholeric reviewed Mar 3, 2022

View reviewed changes

codecholeric requested changes Mar 4, 2022

View reviewed changes

JKLedzion force-pushed the feature/704 branch from dcd28a9 to 707d57e Compare March 9, 2022 17:34

JKLedzion requested a review from codecholeric March 10, 2022 10:17

codecholeric force-pushed the feature/704 branch from 33f382b to e4bf93f Compare March 12, 2022 08:49

codecholeric force-pushed the feature/704 branch from e4bf93f to 0bc8cef Compare March 12, 2022 08:54

codecholeric approved these changes Mar 12, 2022

View reviewed changes

codecholeric merged commit 348819d into TNG:main Mar 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#704: added onlyBeCalledByClassesThat conjuntion - initial part #810

#704: added onlyBeCalledByClassesThat conjuntion - initial part #810

JKLedzion commented Feb 24, 2022

codecholeric commented Feb 25, 2022

codecholeric Feb 25, 2022

JKLedzion Feb 28, 2022

codecholeric Feb 25, 2022

JKLedzion Feb 28, 2022

codecholeric Feb 25, 2022 •

edited

Loading

JKLedzion Feb 28, 2022

codecholeric Feb 25, 2022

JKLedzion Feb 28, 2022

codecholeric Feb 25, 2022

JKLedzion Feb 28, 2022

JKLedzion commented Feb 28, 2022 •

edited

Loading

codecholeric commented Mar 2, 2022

JKLedzion commented Mar 2, 2022

codecholeric Mar 3, 2022

JKLedzion Mar 10, 2022

codecholeric left a comment

JKLedzion commented Mar 4, 2022

JKLedzion commented Mar 10, 2022 •

edited

Loading

codecholeric left a comment

codecholeric commented Mar 12, 2022

#704: added onlyBeCalledByClassesThat conjuntion - initial part #810

#704: added onlyBeCalledByClassesThat conjuntion - initial part #810

Conversation

JKLedzion commented Feb 24, 2022

codecholeric commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecholeric Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JKLedzion commented Feb 28, 2022 • edited Loading

codecholeric commented Mar 2, 2022

JKLedzion commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecholeric left a comment

Choose a reason for hiding this comment

JKLedzion commented Mar 4, 2022

JKLedzion commented Mar 10, 2022 • edited Loading

codecholeric left a comment

Choose a reason for hiding this comment

codecholeric commented Mar 12, 2022

codecholeric Feb 25, 2022 •

edited

Loading

JKLedzion commented Feb 28, 2022 •

edited

Loading

JKLedzion commented Mar 10, 2022 •

edited

Loading