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

Named interface detection accidentally picks up nested declarations in nested interfaces scenario #650

Closed
jwedel opened this issue Jun 4, 2024 · 4 comments
Assignees
Labels
in: core Core module meta model type: bug Something isn't working
Milestone

Comments

@jwedel
Copy link

jwedel commented Jun 4, 2024

I get this error:

org.springframework.modulith.core.Violations: - Module 'A' depends on named interface(s) 'B :: NamedInterface' via ABC -> XYZ. Allowed targets: common, B, B :: NamedInterface.

So A is a module. Also B is a module. A needs to access NamedInterface which has a package-info.java inside the package with a NamedInterface of 'NamedInterface'.

ApplicationModule 'A' has the following allowedDependencies:

  • common
  • A
  • B
  • B :: NamedInterface

Still I get the violation error.

When I change the allowedDependencies as follows, it works without an error:

  • common
  • A
  • B
  • B :: *

I'm also using named interfaces in the exact same way in other places and it works perfectly when explicitly declaring it as dependency. I could not find any reason why it does not work, especially as the violoation error message lists exactly that as allowed.

It's really puzzling me.

I'm using 1.2.0 of modulith.

@odrotbohm
Copy link
Member

Thanks for bringing this up. Any chance you can provide a reproducer?

@odrotbohm odrotbohm self-assigned this Jun 6, 2024
@odrotbohm odrotbohm added in: core Core module meta model type: bug Something isn't working labels Jun 6, 2024
@jwedel
Copy link
Author

jwedel commented Jun 10, 2024

@odrotbohm

Took me a bit, was on vacation :)

https://github.com/jwedel/spring-modulith-dependency-error-reproducer

So, maybe this is not 100% the intended use (we are currently in the process of pulling public api to module root level) but this error message is still kind of weird.

So if you run ModulithApplicationTests then you'll get:

org.springframework.modulith.core.Violations: - Module 'domain1' depends on named interface(s) 'domain2 :: persistence.dto' via com.reproducer.modulith.domain1.Domain1Service -> com.reproducer.modulith.domain2.persistence.Domain2Repository. Allowed targets: domain2, domain2 :: persistence.dto.

If you change the dependencies from module 1 as follows it works:

@org.springframework.modulith.ApplicationModule(
      allowedDependencies = {
            "domain2",
            "domain2 :: *",
      }
)
package com.reproducer.modulith.domain1;

@odrotbohm
Copy link
Member

It looks like there's a bug in our named interface detection that currently invalidly assigns Domain2Repository to the persistence.dto interface. That said, once that's corrected, your example still fails as module1 depends on both the presistence interface (via the repository) and the persistence.dto interface (via the DTO). It feels like you're overdoing the technical separation a bit.

@odrotbohm odrotbohm changed the title Violation: Module 'A' depends on named interface(s) 'B :: NamedInterface' Named interface detection accidentally picks up nested declarations in nested interfaces scenario Jun 18, 2024
@odrotbohm odrotbohm added this to the 1.3 M1 milestone Jun 18, 2024
odrotbohm added a commit that referenced this issue Jun 18, 2024
If named interfaces are declared in nested packages, the name detection might accidentally pick up the names declared in child packages. We now deliberately only inspect the base package.
@jwedel
Copy link
Author

jwedel commented Jun 18, 2024

It looks like there's a bug in our named interface detection that currently invalidly assigns Domain2Repository to the persistence.dto interface. That said, once that's corrected, your example still fails as module1 depends on both the presistence interface (via the repository) and the persistence.dto interface (via the DTO). It feels like you're overdoing the technical separation a bit.

On the one hand, as I mentioned before, we are currently on the way of factoring our application. To do this, It would actually help to see some more complex examples either in the documentation or maybe in repository.

Ok the other hand, the separation of public APIs on root level generally sounds like a great idea, but in our case, we have a quite complex application and in this example, the DTO package has around 15 files as our base DTO is quite complex as well. And by the rule of seven, we start introducing sub packages if things grow too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Core module meta model type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants