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

Add "onion architecture" builder #174

Merged
merged 8 commits into from Jul 19, 2019

Conversation

@spanierm
Copy link
Contributor

commented May 5, 2019

As discussed in #89, we added the OnionArchitecture class. It is based on the LayeredArchitecture class and dynamically creates the corresponding layers so that the user only needs to define the layers but not take care of the wiring. A proposed example usage looks as follows (copied from 1aaab34#diff-55f996d59802ba953c99c1e0199b6a85R158).

OnionArchitecture architecture = onionArchitecture()
        .domainModel("com.tngtech.archunit.library.testclasses.onionarchitecture.domain.model")
        .domainService("com.tngtech.archunit.library.testclasses.onionarchitecture.domain.service")
        .application("com.tngtech.archunit.library.testclasses.onionarchitecture.application")
        .adapter("cli", "com.tngtech.archunit.library.testclasses.onionarchitecture.adapter.cli")
        .adapter("persistence", "com.tngtech.archunit.library.testclasses.onionarchitecture.adapter.persistence")
        .adapter("rest", "com.tngtech.archunit.library.testclasses.onionarchitecture.adapter.rest");

In particular, the usage of multiple .adapter calls allows to define the adapters that should be kept independent from each other in a very generic way. One could, for instance, decide that no adapter is allowed to access another one (see the above example), or that all adapters can share code (by using only a single .adapter call).

Let us know if the builder looks as expected. If yes, we will add the missing documentation.

@deepcode-ci-bot

This comment has been minimized.

Copy link

commented May 5, 2019

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

@cyriux

This comment has been minimized.

Copy link

commented May 6, 2019

Sounds like a good addition to ArchUnit to me; Hexagonal lovers can write in the simplest case:

OnionArchitecture simpleHexagonalArchitecture = onionArchitecture()
        .domainModel("com.acme.domain")
        .adapter("infrastructure", "com.acme.infra");

And leave out the code doing all the instantiations (aka Component Root) outside of these rules so that it can know every package (but no package should reference the Component Root).

Also, from a documentation perspective, (since such tests are documentation), making primary vs secondary adapters explicit would be a plus, even if it doesn't make a difference in the dependency rules.

@ask4gilles

This comment has been minimized.

Copy link

commented May 6, 2019

@spanierm Thanks, this perfectly meets my need expressed in #89
Can someone also update the doc?
(@cyriux are you 18 on your profile’s picture? 😂😉)

@fdw

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Awesome, thanks. The API is perfect :)

However, I have one or two ideas about the code - is it okay if I add them (even if I'm not a contributor)? Also, I think you need to sign your commits accourding to the DCO

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@fdw Feel free to do any changes. I invited you to become a contributor in my fork :)

@fdw

This comment has been minimized.

Copy link
Member

commented May 10, 2019

Thanks, @spanierm, but I only wanted to leave some ideas in the code, not actually change it ;)
But I'll take that as a "yes" :)

private static final String APPLICATION_LAYER = "application";
private static final String ADAPTER_LAYER = "adapter";

private final Set<String> domainModelPackageIdentifiers = new LinkedHashSet<>();

This comment has been minimized.

Copy link
@fdw

fdw May 10, 2019

Member

As they're only used as varargs, I think you could also store them in an array. I don't think using a HashSet of any kind provides a benefit.

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 10, 2019

Author Contributor

done :)

}

private void updateLayersDelegate() {
layeredArchitectureDelegate = layeredArchitecture()

This comment has been minimized.

Copy link
@fdw

fdw May 10, 2019

Member

Do you think this must be done after every change, or would once in the check method suffice?

Alternatively, you could initialize the layeredArchitectureDelegate in the constructor and hold the LayerDefinitions as fields. Then, in domainModel(), you'd only have to call domainModelLayer.definedBy(packageIdentifiers).

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 10, 2019

Author Contributor

done :)

@ask4gilles

This comment has been minimized.

Copy link

commented May 22, 2019

Any movement on this?

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Yes, I am on it but did not find time. Hope I can finalize it soon.

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@cyriux I decided to not include distinction between primary/secondary ports/adapters. I think it can be added easily once the initial setup is in place.

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

@codecholeric I am more or less done with the changes and updates of the docs. Can you please take a look? In particular, the docs did not render correctly using docker-compose (the user guide always showed the initial version and never the updated ones). But in IntelliJ, the preview looks as expected.
Looking forward to your feedback.

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Thanks a lot for the initiative 😄 Looks like a cool addition.
Looks good so far!! I have a couple of remarks (if you're short on time, let me know, since I'm finally on holiday 😉)

  • Regarding docker-compose, unfortunately you have to run ./gradlew within the docs folder once to recreate the user guide. This is due to the blacklisting of the Jekyll asciidoc plugin by GitHub ☹️ You also have to check in the adapted HTML
  • It would be cool to add an example like for com.tngtech.archunit.exampletest.LayeredArchitectureTest and an integration test, see com.tngtech.archunit.integration.ExamplesIntegrationTest#LayeredArchitectureTest
  • I'm not sure if the description is optimal yet? As far as I could see, you pass on the description of the layered architecture delegate, but I think the rule description should be exactly like the code, i.e. not talk about layers but 'domain model' and 'domain services', etc.
private String[] applicationPackageIdentifiers = new String[0];
private Map<String, String[]> adapterPackageIdentifiers = new LinkedHashMap<>();

private LayeredArchitecture layeredArchitectureDelegate = layeredArchitecture();

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jun 15, 2019

Contributor

I don't think I would even keep a field here. Make updateLayersDelegate() a layeredArchitectureDelegate() returning a new layeredArchitecture() with the current configuration every time. Yes, you might create it more often than necessary, but I'm fairly sure that's not where the performance problems are 😉
Especially because you want to be able to query the getDescription() at every point in time, and now it only works after evaluate has been called. I think the code is easier without the field and keeping anything in sync, simply create the layered architecture whenever needed. Similar if you use the as(..) method.
Maybe a unit test for these trivial things like as(..) and because(..) wouldn't be so bad 😉
You might have to keep a separate field description though, if you want a description independent of LayeredArchitecture's description.

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 20, 2019

Author Contributor

Thanks for the hint. Sounds very reasonable and I changed the code accordingly. In particular, I now understand as and because 😉

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 20, 2019

Author Contributor

I also changed the description accordingly.

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 20, 2019

Author Contributor

Unfortunately, I still cannot build the docs. Am I blind or what is wrong with my setup? I already tried building the docs folder with ./gradlew build but always get the following error:

Starting a Gradle Daemon (subsequent builds will be faster)

FAILURE: Build failed with an exception.

* What went wrong:
Project directory '<SOME_LOCAL_PATH>/TNG/ArchUnit/docs' is not part of the build defined by settings file '<SOME_LOCAL_PATH>/TNG/ArchUnit/settings.gradle'. If this is an unrelated build, it must have its own settings file.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

BUILD FAILED in 2s

EvaluationResult result = architecture.evaluate(classes);

ImmutableSet<String> expectedRegexes = ImmutableSet.of(

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jun 15, 2019

Contributor

The naming seems pretty driven by layeredArchitecture() 😉 (since they are all called ..LayerClass)
I would try to keep the language completely independent of layers, because

  • in an onion architecture shouldn't it be called "ring"? 😉
  • layeredArchitecture() is implementation detail from my point of view
  • the whole reason for the onionArchitecture() was to better express the concepts, even though you could write the rules with layeredArchitecture() (as you've demonstrated within the implementation 😉). So I would ban all "layer" talk from the API, description and tests and just talk about "domain model", "application" and so on.

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 21, 2019

Author Contributor

True, I would love to use an appropriate name as you suggest. But I always find layer in any article about onion architectures. So I assume that it is the most appropriate name. It might be confusing if you compare the concept with LayeredArchitecture. But on the other hand, layer seems to be the standard name. So I prefer to leave it that way.

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 23, 2019

Author Contributor

@codecholeric I added some integration tests as well. Thanks for the hint that those exist in the project since they revealed a minor flaw in the layer definitions.
Unfortunately, It was not easily possible to add the onion architecture integration test to the example project. I therefore put it into a separate package to avoid breaking/needing to modify the existing tests. Is that how you expect it? I think there should not be a single project that deals with all test cases as it will get hard to add new features. Moreover, some architectures might contradict each other (e.g. 3-tier vs. onion architecture).

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jun 29, 2019

Contributor

Yeah, I guess this project has just grown and it would be better to split parts of the existing example as well. Maybe I'll do that at some point, since the "shopping" example is kind of odd in there, too.
I would probably choose a naming like "example.subcase" instead of "subcase.example" if I had the chance, but for now I think it's best how you added it 👍

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jun 29, 2019

Contributor

What I wonder is, if the unit test and the integration test really test anything different? I.e. is it necessary to assert all the violation messages here, if we have the same classes again in the integration test and assert the messages there?

This comment has been minimized.

Copy link
@spanierm

spanierm Jun 29, 2019

Author Contributor

Actually, we could also add a new "more-or-less real world example" instead of those dummy classes that shows the architecture in a better way. But then the tests might get really odd as there are so many error cases to check for if you do not want to miss any layer interaction. But we can have a chat on that. In particular, whether it is worth the effort building a nice example.
Looking at the time, I assume you are heading to Austria right now 😉 and I will head over there tomorrow morning. So maybe we find some time there.

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jun 29, 2019

Contributor

I'm actually on holiday 😉 (as you can clearly see as I have time to work on ArchUnit 😛)
What I meant is, you can maybe drop the unit test for the specific violation patterns and keep the example. It is true though, that the examples are usually a little closer to "real world", i.e. you would maybe not violate all constraints in every class.
So I would either make the example closer to the "real world" (even though bodies of methods are just comment stubs) or drop the unit test. (the example + integration test I would keep in any case in some way, just a redundant assertion of the same case is superfluous in my opinion)

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jul 5, 2019

Contributor

As a follow up to this: I would add a more realistic example, like the shopping example with an onion architecture. It does not need to have every possible violation, just a couple, but those in a somewhat reasonable way (not just writing down all possible accesses in all classes). Then this would still make sense as a unit test, testing all accesses in detail and there would be a more realistic example for users to see, that can also serve as an integration test with lesser details.
For example there could be domain service having slipped into domain, a domain service using an application service or a repository, etc. Just a hand full of violations should be fine...
Do you want to add such an example (instead of the current one), or should I do that?

This comment has been minimized.

Copy link
@spanierm

spanierm Jul 6, 2019

Author Contributor

I like this idea and prefer that you add the example. I think it is better to have an external view on the topic and provide an example that is different from my first suggestions. This should also speed up the PR as I really want to use the feature in production :)
Concerning the documentation: Is it sufficient? Can you do a short review on it as well? I think the real world example might also be handy for the documentation. As stated above, I fail building the documentation locally. Any hints for that?

This comment has been minimized.

Copy link
@codecholeric

codecholeric Jul 6, 2019

Contributor

Okay, I'll try to add an example and let you review it 😃
About the docs, wasn't your problem that you haven't executed ./gradlew within the docs folder to regenerate the user guide? You need to check in the generated html. This would be a lot easier if GitHub Pages would whitelist the Jekyll Asciidoc plugin ☹️

@deepcode-ci-bot

This comment has been minimized.

Copy link

commented Jun 23, 2019

DeepCode analyzed this pull request.
There are 2 new info reports.

Click to see more details.

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

@spanierm I've replaced the example by some simple shopping example with a couple of common mistakes and adjusted the integration test.
I've also restructured the whole example to better accompany future additions (you were definitely right that it has grown a little unstructured and now adjusting the example for one use case breaks another one, so it should be easier in the future).
Furthermore I've updated the user guide and tested it locally, looks good! I've adjusted the diagram a little to group domain and adapters so it's easier to see which concepts belong together. You can check it out, if that is okay for you.
Can you check my commits? If it looks okay to you, we're good to go 😉
One thing I've noticed though is, that the original hexagonal architecture AFAIK didn't really speak about these types of rings/layers. So I'm not sure if there might be a difference in perception, if you talk about the original hexagonal architecture or the onion architecture. But in the end I think the concept should be expressible by the current API (maybe a little reduced if you don't want the additional rings and just the hexagon plus adapters).

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Hmm, there is something really weird going on with the Maven Surefire plugin 😦
You can still do the review, I have no idea what this is. For some reason Surefire reruns every test in CyclicDependencyRulesTest (and only there) twice reporting failure on the first run and success on the two consecutive runs. The text report says 9 tests were run but the JUnit xml report contains every test 3 times 😕
Couldn't find any reasonable info in the debug logs either so far, rerunFailingTestsCount is 0, but the behavior for only that test is like it was 2. I'll try to figure out why this happens since I've restructured the example project. This is one of those twilight things for me 😞

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Nevermind, I found the problem, it's the aggregated tests that include different examples. After restructuring some now need to import layers and some cycles ☹️
Gonna fix it...

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

I finally found some time to review the new example:

  • I think the ShoppingService interface should be removed. There is really no need for an interface here as there is no dependency inversion needed.
  • Other than that, I like the example and the new structure of the examples.

I did not take a closer look at all the other changes/restructuring of the repository. Is that sufficient for you? Should I review all stuff?

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Should be fine then, I'll remove the interface, rebase the branch and then merge it.

spanierm and others added 8 commits May 26, 2019
Add support for onion architecture #89
Signed-off-by: Markus Spanier <markus.spanier@tngtech.com>
Add support for onion architecture #89
Signed-off-by: Markus Spanier <markus.spanier@tngtech.com>
Add support for onion architecture #89
* add proper implementations of 'as' and 'because'
* always return a new delegate so that all methods work without the need to call 'evaluate'

Signed-off-by: Markus Spanier <markus.spanier@tngtech.com>
Add support for onion architecture #89
* add integration test
* only show layers in description that are actually used

Signed-off-by: Markus Spanier <markus.spanier@tngtech.com>
Review: Restructure example project -> aspects should be separated. G…
…rouped most of the original project into subproject "layers", even if it contains some classes like ClassViolatingCodingRules. Extracted cycles and plantuml as separate aspects and integrated onion architecture.

Issue: #174
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Review: Add more realistic shopping example application for onion arc…
…hitecture. Adjust integration test to match example.

Issue: #174
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Review: Regenerate User Guide with added Onion Architecture (note tha…
…t that part about FreezingArchRule was outdated as well). Adjusted the diagram a little, because I think it is easier if domain and adapters are grouped.

Issue: #174
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>
Review: Removed some obsolete code from OnionArchitecture, refactored…
… and improved test (3 pattern assertions were actually wrong, but matched "in (ApplicationLayerClass.java:xx)" by accident).

Issue: #174
Signed-off-by: Peter Gafert <peter.gafert@tngtech.com>

@codecholeric codecholeric merged commit 89e6004 into TNG:master Jul 19, 2019

1 of 2 checks passed

DeepCode DeepCode is checking for issues.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Bamm, it's merged, thank you so much for the initiative!! 😄

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2019

Thanks a lot. Really looking forward to the next release to use the feature for my customer ;)

By the way: When do you plan to release the next version?

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2019

I have one more feature I'm pretty far with that I would like to add to the new release. But I'll see how things look next weekend, if I'm too far away from completion I'll release the current state. In any case I plan to release within the next 2 or 3 weeks at most (I want to use FreezingArchRule in my project 😉)

@spanierm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Hmmm, seems like we need to take a look at the release process ;) Releases should be that easy that you do not think about them and never postpone them. Is there a reason for you to wait, e.g. missing automation? If yes, I volunteer to help as I really like such stories. "If it hurts, do it more often" ;)

@codecholeric

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

Yes, I'm open to go over the release process 😉
A lot of things are automated, but there are still some small things (like running the tests one last time on Windows since I still don't have an environment for that ☹️)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.