Includes wrong diagram

[Kira-Cesonia]

I was asked to post the following issue in this project instead of asciidoctor-intellij-plugin:

The issue is highly counter-intuitive logic when including diagrams such as from PlantUML in the IntelliJ plugin. I was told by a contributor that this is caused by the underlying Asciidoctor Diagram logic

I am using the following statement to include a Plant UML diagram on a page:

["plantuml","containers"]
----
include::../diagrams/containers.puml[]
----

However, instead of including ../diagrams/containers.puml as intended, this includes a different diagram from the seemingly unrelated resource of ../external-references/diagrams/external-components.puml, and then scales it up to the size of ../diagrams/containers.puml`.

For some reason, this only happens for this one includes statement. The following inclusion of a different PUML diagram, for example, works no matter where in the AsciDoc I put it:

["plantuml","system-context"]
----
include::../diagrams/system-context.puml[]
----

Now, as mentioned before, I managed to get it to work by changing the whole statement to the following (removing ",containers" in the header):

["plantuml"]
----
include::../diagrams/containers.puml[]
----

By now I understand that the second parameter in the block attributes sets a global filename that is stored in "Mystic Space" and can cause issues even in smaller projects with only a few dozen documentation pages and diagrams (this happened within a week of us setting up our first documentation). What is the benefit of this parameter anyway, since you still need to pass the exact path and filename in the include statement afterwards? According to my experimentation, omitting it does not cause any issues. I intuitively expected block to work on its own without needing an extra import statement below if you've declared and named it once, but alas, that does not seem to be the case.

Aka, this works:

["plantuml","system-context"]
----
include::../diagrams/system-context.puml[]
----

This also works:

["plantuml"]
----
include::../diagrams/system-context.puml[]
----

However, I intuitively expected this to work, but it does not (expected it to display the same diagram twice, but it only displays it once):

["plantuml","system-context"]
----
include::../diagrams/system-context.puml[]
----
["plantuml","system-context"]

Instead, this is required to display the same diagram twice:

["plantuml","system-context"]
----
include::../diagrams/system-context.puml[]
----
["plantuml","system-context"]
----
include::../diagrams/system-context.puml[]
----

But that could also be shortened to:

["plantuml"]
----
include::../diagrams/system-context.puml[]
----
["plantuml"]
----
include::../diagrams/system-context.puml[]
----

So, what is the advantage of including the filename in the block? As I see it, it only serves as a source of Type 3 refactoring errors.

Quote ahus1:

As https://asciidoctor.org/docs/asciidoctor-diagram/ points out, not specifying a target name will lead to a file name derived from the diagram's content. If I understand it correctly the generated file name will change once the contents of the file change, and the same content will lead to the same file name (if used in multiple places).

This doesn't suite everyone, especially if one wants to reference and image by (file) name from a different source.

Logging a warning message when two diagrams use the same specified target name (and maybe file format) sounds like a good idea to me.

As this is not related to the IntelliJ plugin but to Asciidoctor Diagram, I suggest you open an issue in their GitHub repo: https://github.com/asciidoctor/asciidoctor-diagram
A solution within Asciidoctor Diagram would fix it not only for the IntelliJ plugin, but also in situation where it is run from the command line.

Also see the original issue in the asciidoctor-intellij-plugin issue tracker: asciidoctor/asciidoctor-intellij-plugin#388

[pepijnve]

Scratching my head on this one. I'll try to reproduce the problems your describing and let you know what I find.

Regarding

["plantuml","containers"]
----
include::../diagrams/containers.puml[]
----

that's a curious way to do this. With this syntax, asciidoctor core is processing the include:: bit; asciidoctor-diagram never even sees it. Not sure why it would resolve to the wrong file.

You can use the block macro to achieve the same result with fewer moving parts:

plantuml::../diagrams/containers.puml[]

Specifying the target image name on a diagram block is something you typically do when the diagram source code is specified inline and you want to have control over the name of the generated diagram.

[mojavelinux]

My guess is that an earlier run resulted in a file being placed into the cache with the contents of a different diagram. @Kira-Cesonia, are you aware of how to discard the cache? Does it still happen then?

I agree that the plantuml block macro is more suited for this task.

[Kira-Cesonia]

Interesting, however, I note that while this solution prevents the wrong diagram to be included on all occasions...:

["plantuml"]
----
include::../diagrams/containers.puml[]
----

...this one, while working, still includes the wrong diagram at times:

plantuml::../diagrams/containers.puml[]

The reason for this is an import in another file that was phrased as this:

["plantuml","containers"]
----
include::../../diagrams/external-components.puml[]
----

So, what happened?
Apparently at some point during the project, containers.puml was renamed to external-components.puml, and a new file named containers.puml created. However, the refactoring missed the "components" in the ["plantuml","containers"] block, causing this Type 3 error.

Everything summed up again:
One file, hidden somewhere in the project, has a Type 3 refactoring error, where the wrong name is declared in the header block for a PlantUML diagram:

["plantuml","containers"]
----
include::../../diagrams/external-components.puml[]
----

In another file, somewhere else in the project, coincidentally, the same header is used, but for the "correct" file.

["plantuml","containers"]
----
include::../../diagrams/containers.puml[]
----

What happens now?
The diagram external-components.puml[] is displayed in this second place, but it is scaled to the size of containers.puml[]. Now, I don't know what the intended use case here is, but there is no way in Dragon that this is intended behaviour.
Interestingly, this same behaviour also occurs if only that second part is written in the short form suggested above:

plantuml::../diagrams/containers.puml[]

However, writing it like this causes the intended behaviour of containers.puml[] being displayed, and at the correct scale too:

["plantuml"]
----
include::../../diagrams/containers.puml[]
----

[pepijnve]

I'll have a think about how we can improve this, but fundamentally what you're asking the system to do is very poorly defined.

When you write plantuml::../diagrams/containers.puml[] the system will generate containers.png. The image file name is derived from the source file name.
When you then add

["plantuml","containers"]
----
include::../../diagrams/external-components.puml[]
----

that "containers" target attribute is also explicitly telling the system to generate containers.png instead of using some auto-generated name.

Since the system is writing to the same destination file in both cases, you're going to end up having one of the two images in the final results; last processed diagram wins.

I don't think it would be a good idea to second guess the user of the system here and start generating different filenames, but I think it should be feasible to generate a warning.

[mojavelinux]

I like the idea of generating a warning if the same filename is used twice in the same conversion. The only case that would be valid is if the same diagram is being shown in two different places in the document...but even then I would recommend two different filenames.

[Kira-Cesonia]

I think the point would be to throw a warning, or maybe even an error if a directive would cause the system to try and create a file with a filename that already exists. Any other system I can think of will either throw an error or a warning in that case.

If you try to create a file with a name that already exists in pretty much any operating system, you will get a warning and a question of whether you want to overwrite the file.

Meanwhile, most programming languages will not even allow you to declare the same variable twice. If you declare a variable somewhere, and then declare a variable of the same name within the same scope, you will get a compiler error.

Considering that Asciidoc is closer to programming than to GUI experiences, I would really suggest making this a case for a compiler error following the above logic. The error message should be something along the lines of:

A diagram with the name "MyDiagramName" is already being created in [Filename:Line]

However, something to consider here is that at least in the project that I have inherited, the following structure was consistently used throughout the code:

["plantuml","containers"]
----
include::../diagrams/containers.puml[]
----

That is, we have multiple such "declaration blocks". It may not be the "as intended" way to do it, but I think it is a valid use case since it does work. Naturally, we don't want to throw errors or even warnings if a documentation uses the same such block in multiple places as long as the header and the contents of the block are identical.

Summed up, it is probably going to be tricky finding a perfect solution for this, but the bottom line is that we need a feedback mechanism that notifies users of why the wrong diagram is displayed if such a case happens. The current system is not very intuitive in that way.

Reasoning why it is not intuitive:
If a user specifies a filename in the code, such as in

plantuml::../diagrams/containers.puml[]

or

["plantuml":"containers"]
----
include::../diagrams/containers.puml[]
----

...then it is intuitive that the diagram that should be displayed at this location should be the diagram specified by filename.
However, in both cases, there are possible configurations that will cause AsciiDoc to display a diagram different from the one that is being explicitly specified, and at distorted dimensions too.
This is counter-intuitive.

[mojavelinux]

I would really suggest making this a case for a compiler error following the above logic

Asciidoctor doesn't work that way. We log messages, then you can decide which level of message should produce a program failure. The reason for this is that many users of Asciidoctor are not programmers and an aborted program is unfriendly to that audience and is perceived as a crash (not to mention disruptive to writing). We try to be informative to help the writer course correct in the next round of edits. Asciidoctor is not a compiler. It's a writing tool.

[mojavelinux]

This is counter-intuitive.

To you. But as Pepijn points out, what you've done is actually valid operation. It just isn't desirable. The program cannot read your mind. But as we've discussed, it's reasonable to add an assumption that no two diagram blocks should have the same name (which is a new assumption), which would help avoid this type of user error and thus be more friendly.

[Kira-Cesonia]

Okay, let me ask it like this:
Can you think of any Use Case where someone explicitly defines the name of a diagram to be used at a position, but does not actually want that diagram to be used at that position?

[mojavelinux]

You're looking the problem only through the lens of your situation. How is Asciidoctor Diagram going to make any sense of a name mixup? You named one diagram something in English, another diagram something in English, and to you, the names are wrong. To Asciidoctor Diagram, it sees a string of characters. For all it knows, that's what you meant. You are the one that mixed up the names, not Asciidoctor Diagram. (To help clarify, Asciidoctor Diagram never sees the name of the include file. That information is simply not visible to the extension. It only sees the filename if you use the block macro).

Having said that, we've already established that if the same diagram name is appearing twice in the same document (which Asciidoctor Diagram could confirm is or is not the same diagram content), that it's an indication something is wrong and a warning would help the user find the mistake and fix it.

[ahus1]

@pepijnve - As a proof of concept I've added a monkey-patch based on 1.5.18 version of the asciidoctor-diagram that is currently used in the IntelliJ plugin.

My ruby skills are limited, feel free to criticize, improve or discard it. I'm looking forward to throw away this piece of code as soon as possible 🙄

The method check_duplicate_target() adds a warning to the two conflicting diagrams with the same name referencing each other with file and line number. It uses a dummy attribute per target file name in the document as "memory" (as a quick-and-dirty solution to keep something with the scope of the document).

[pepijnve]

@ahus1 thanks for getting the ball rolling on this one. @mojavelinux any suggestions as to what the most appropriate way to do this is?

[slonopotamus]

Is asciidoctor-diagram the only extension that generates images during conversion process? Can't it be that there is a collision between multiple extensions?

[Kira-Cesonia]

Good question, how can I check that?