TNG / ArchUnit

A Java architecture test library, to specify and assert architecture rules in plain Java
http://archunit.org
Apache License 2.0
3.24k stars 297 forks source link

Support for database component type in plantuml #1132

Closed tfij closed 7 months ago

tfij commented 1 year ago

It will be great if Archunit could support such diagrams:

@startuml

database "DB"
component [Comp1] <<..c1..>>
component [Comp 2] as Comp2 <<..c2..>>

Comp2 <-- [Comp1]
Comp2 -> DB

@enduml

www.plantuml.com/plantuml/png/SoWkIImgAStDuU9AIIn9J4eiJbLGSd5IuahEpot8pqlDAr68TWOo3MCLR6pqz98DzVJixD0b5OnY5HAB5K1C8uWo8x0oBgY8hYxC4AY8hfs2YnCNbqDgNWhGQW00

In this case, the database component and the dependency on the database component could be just ignored.

bgalek commented 1 year ago

Hi! I came upon the same problem.

I can't use PlantUML Component Diagrams as rules with a PlantUML diagram that features database component. ArchUnit is unaware of any database connections and fails with an error.

ArchUnit can remain agnostic to database connections and should not raise errors when it encounters them during analysis - instead, it could ignore these components to maintain its focus on architectural concerns (or maybe this option should be configurable).

Database connections are typically not considered architectural concerns at a high level. Yet including them in a resulting diagram for the sake of documentation - would be a cool feature.

@hankem WDYT? :)

I believe @tfij has already done some work to make it work in https://github.com/TNG/ArchUnit/pull/1133

codecholeric commented 1 year ago

Big apologies for the slow follow up! I was unfortunately deeply overloaded over the last months 🙈 @tfij Thanks for the initiative, but to be honest I'd like to discuss this a little before. Because I don't see a use case per se for ArchUnit to understand databases as there can be no checks done on this. I understand that you simply want to extend your diagram with extra context information that you want ignored by ArchUnit, right? Thinking about this I would be more leaning towards some generic solution that allows to ignore certain lines, either programmatically with some predicate approach or by some additional syntax in the PlantUML file, e.g. some control sequence within a comment to signal "ignore the next line for ArchUnit". Because I want to avoid increasing the code complexity substantially (e.g. adding the additional "database component") without any real gain from the test perspective. And maybe in two weeks somebody wants to add a queue and we're back to extending the parsing 🤔 What do you guys think?

tfij commented 1 year ago

Hi @codecholeric

This change is not about understanding database components but does not crash on parsing valid plantUML diagrams. I agree that we still don't support full plantUML, but step by step we can achieve it.

A solution with extra comments is a workaround, ArchUnit will still be unable to parse valid diagrams.

In my opinion, the most convenient solution should allow you to take an existing diagram and run it as a test. Then you can have one diagram for documentation and testing. In other words, tests confirm that the documentation is up to date.

The commented solution will allow you to use the same diagram, but it is not user-friendly. The user not only has to add additional code to the diagram. The user must know that such a code needs to be added. So the documentation needs to be expanded and the user needs to find this piece of documentation. Still, valid diagrams will not be parsed correctly and changes will need to be made to them to be handled by ArchUnit. That is why I say this is a workaround.

codecholeric commented 1 year ago

Just that from my point of view the idea never was that ArchUnit can understand all PlantUML diagrams. It was always meant to just take a well-defined subset (that keeps complexity low) and use that as a test input for convenience. To have something visual as a test input. We already only support a very special way of writing component diagrams. I.e. you have to write the components in square brackets (instead of using the keyword component) and you have to specify a package identifier as stereotype, which is also something not common in a general PlantUML component diagram. I'm just afraid if we strive for "step by step we can achieve it" we will end up with a massively more complex implementation that in the end can take an arbitrary complex diagram of which only a tiny part is then really evaluated as a rule 🤔 I'm still not convinced this is a good idea vs. just keeping the syntax a well-defined subset, consciously only supporting what makes sense for an architecture test and at most just adding a generic way for users to tweak this if they want to use more complex diagrams 🤔

Or, as another option I could imagine we could just ignore "invalid" dependencies altogether instead of throwing an exception (which would similar to other behavior, because I think it already ignores unparsable lines atm, if they don't match any known pattern). Because in the end this doesn't increase danger of writing "false green" tests, since it would just consider less dependencies as allowed than the user intended. Downside of this is of course that it could create a hassle for users to locate why their test behaves weirdly if they specified an intended dependency in an invalid way 🤷‍♂️

tfij commented 1 year ago

The second approach which you've suggested should be OK. Just to ignore mismatched dependencies instead of throwing an exception.

However, I'd like to add component keyword support. The keyword is optional so it is about ignore it. It should be a simple change in regexp.

I can prepare a PR if you are ok with it.

tfij commented 1 year ago

PR https://github.com/TNG/ArchUnit/pull/1184

tfij commented 10 months ago

@codecholeric Have you had a chance to check out this PR?

codecholeric commented 10 months ago

@tfij sorry that I was a little slow on this! I looked into the PR, thanks a lot! I think it's a good compromise after our discussions 🙂 Please let me know what you think about my review comments in the PR!

uhafner commented 7 months ago

Is there a specific reason that you are using only components in these diagrams? Would it be ok if I provide a PR that also allows to use packages? Actually we are enforcing package rules and not component rules. So I would prefer to use a package UML diagram (and not a component diagram).

tfij commented 7 months ago

Your proposal sounds good. I think you can open a new issue and describe a sample plantUML that you'd like to cover.