internal types can be used inside methods - verify still succeeds

[patrickuhlmann]

trafficstars

Given the modules "order" and "inventory". The module order contains a package "internal" with a OrderRepository and has an interface OrderApi. The inventory module contains a service "OrderService" with a method "hasOrder".

Also we have a ModularityTest that calls verify on ApplicationModules.

We observed that the call to verify fails when we are returning the OrderRepository in the method getOrder. So far so good. However if we are just utilizing the OrderRepository inside the method but in the end return a boolean, the test does not fail.

For example:

@Autowired
private OrderApi orderApi;

public boolean hasOrder(UUID id) {
    var repository = orderApi.getInternalRepository();
    return repository.exists(id);
}

Thus according to our observation the verify test does not fully enforce that internal types cannot be used at all in another module. It only enforces that internal types cannot be used as return value (or parameters) in methods in another module.

I am aware that such a case might be a bit "searched for" (probably nobody would implement a getInternalRepository method ;D) but still think that the library might benefit from an improved validation (check that classes from other modules are not allowed at all to reference any internal class).

[odrotbohm]

We rely on ArchUnit to detect the dependencies between the different modules, so I would have to resort to @codecholeric to comment on why that dependency is not detected.

Independent of that, we could check, that API types do not expose internal types in the signatures of public methods. That said, I think we can only make this opt-in, as it might still be a valid declaration as internal types might refer to those methods and might have to be able to invoke them. I can't imagine a case in which this couldn't be fixed by rearranging the types, but I guess it indeed could happen.

[codecholeric]

Sorry, I somehow missed the notification on this :see_no_evil: I assume that the exists(..) method you call comes from an interface and not just the implementation? Or is this a method that is purely existing on the implementation? Because in the latter case I would still guess a violation would be found and would be puzzled if not :thinking: To answer this more thoroughly I would need a little code sample that shows how exactly your stuff is implemented. Then I could also tell you why in this case it's not detected. Judging just the little snippet I see I could imagine the following: OrderRepository implements OrderApi, OrderApi declares exists(id) as you call it and var repository is just translated by the compiler to the most generic interface that still satisfies the requirements, i.e. the type in the bytecode then is OrderApi :man_shrugging: In other words, I wouldn't be surprised if you could switch the return type of getInternalRepository() to OrderApi and everything else would just compile fine? In this case, while this is not exactly clean, it will also only let the most easy to fix violations slip through, i.e. they can't really contribute to a hard to fix entangled mess... To check if my assumption here is valid you could adjust your code to OrderRepository repository = orderApi.getInternalRepository() and see if this causes a violation (given that exists(..) is really directly implemented on OrderRepository and doesn't come from an allowed supertype of some sort). Or you could create a specific method OrderRepository.testMe() (not present on any interface) and call that from there and see if this changes things. In any case, if you would really want to prevent something like this you would have to enforce that internal types may not be exposed by any public modifier. Because, you could also declare something like OrderApi api = orderApi.getInternalRepository() and even though the return type would be an internal type, it's not used in that nature on the callsite. And by that the only violation that could ever be found for such a code is that the public method exposes the internal type. But that's a different thing as "depends" for ArchUnit, because if I declare OrderApi api = ... the code technically doesn't "depend" on the internal type, even though it's exposed.