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

Support for database component type in plantuml #1132

Closed
tfij opened this issue Jul 5, 2023 · 10 comments · Fixed by #1184
Closed

Support for database component type in plantuml #1132

tfij opened this issue Jul 5, 2023 · 10 comments · Fixed by #1184

Comments

@tfij
Copy link
Contributor

tfij commented Jul 5, 2023

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.

tfij added a commit to tfij/ArchUnit that referenced this issue Jul 5, 2023
tfij added a commit to tfij/ArchUnit that referenced this issue Jul 5, 2023
Issue: TNG#1132
Signed-off-by: Tomasz Fijalkowski <[email protected]>
tfij added a commit to tfij/ArchUnit that referenced this issue Jul 6, 2023
Issue: TNG#1132
Signed-off-by: Tomasz Fijalkowski <[email protected]>
@bgalek
Copy link
Contributor

bgalek commented Sep 15, 2023

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 #1133

@codecholeric codecholeric changed the title Suppot for database component type in plantuml Support for database component type in plantuml Oct 8, 2023
@codecholeric
Copy link
Collaborator

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
Copy link
Contributor Author

tfij commented Oct 13, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

tfij commented Nov 1, 2023

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
Copy link
Contributor Author

tfij commented Nov 1, 2023

PR #1184

@tfij
Copy link
Contributor Author

tfij commented Dec 23, 2023

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

@codecholeric
Copy link
Collaborator

@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!

codecholeric pushed a commit to tfij/ArchUnit that referenced this issue Apr 10, 2024
codecholeric pushed a commit to tfij/ArchUnit that referenced this issue Apr 10, 2024
The current implementation prevents using PlantUML diagrams as rules
that are used for wider documentation purposes (e.g. containing a
database component or some other dependency that's irrelevant from the
point of an ArchUnit test).
The consequence of ignoring such unknown components is only to allow
fewer dependencies and fewer classes to be present.
So, at best the test will fail as a false positive, if the components
are declared wrongly by accident.
Thus, there is no major harm in simply ignoring these unknown components
and giving users a little more freedom what elements they want to use
in their diagrams.

Issue: TNG#1132

Signed-off-by: Tomasz Fijalkowski <[email protected]>
codecholeric pushed a commit to tfij/ArchUnit that referenced this issue Apr 10, 2024
The current implementation prevents using PlantUML diagrams as rules
that are used for wider documentation purposes (e.g. containing a
database component or some other dependency that's irrelevant from the
point of an ArchUnit test).
The consequence of ignoring such unknown components is only to allow
fewer dependencies and fewer classes to be present.
So, at best the test will fail as a false positive, if the components
are declared wrongly by accident.
Thus, there is no major harm in simply ignoring these unknown components
and giving users a little more freedom what elements they want to use
in their diagrams.

Issue: TNG#1132

Signed-off-by: Tomasz Fijalkowski <[email protected]>
codecholeric pushed a commit to tfij/ArchUnit that referenced this issue Apr 10, 2024
The current implementation prevents using PlantUML diagrams as rules
that are used for wider documentation purposes (e.g. containing a
database component or some other dependency that's irrelevant from the
point of an ArchUnit test).
The consequence of ignoring such unknown components is only to allow
fewer dependencies and fewer classes to be present.
So, at best the test will fail as a false positive, if the components
are declared wrongly by accident.
Thus, there is no major harm in simply ignoring these unknown components
and giving users a little more freedom what elements they want to use
in their diagrams.

Issue: TNG#1132

Signed-off-by: Tomasz Fijalkowski <[email protected]>
codecholeric added a commit that referenced this issue Apr 10, 2024
The current implementation prevents using PlantUML diagrams as rules
that are used for wider documentation purposes (e.g. containing a
database component or some other dependency that's irrelevant from the
point of an ArchUnit test).
The consequence of ignoring such unknown components is only to allow
fewer dependencies and fewer classes to be present.
So, at best the test will fail as a false positive, if the components
are declared wrongly by accident.
Thus, there is no major harm in simply ignoring these unknown components
and giving users a little more freedom what elements they want to use
in their diagrams.

Resolves: #1132
@uhafner
Copy link

uhafner commented Apr 15, 2024

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
Copy link
Contributor Author

tfij commented Apr 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants