Path resolution incorrect if no output directory specified

[danyill]

I'm looking at updating the vscode extension to the latest version of asciidoctor-kroki as part of https://github.com/asciidoctor/asciidoctor-vscode/issues/301

My working document is (say) here:

/media/mulhollandd/KINGSTON/operating_information/Operating_Information/architecture/Operating_Information_Architecture.adoc

and the document sets:

:imagesdir: media
:kroki-server-url: https://kroki.io
:kroki-fetch-diagram:

The extension does set the base_dir API option but not imagesoutdir, outdir, to_dir or base_dir document attributes as appear to be expected in fetch.js:

https://github.com/Mogztter/asciidoctor-kroki/blob/f9b9abfb64be2339bde78bb77c41339e88dfcbfd/src/fetch.js#L5-L9

Under these circumstances, my images turn up in /home/mulhollandd/media/ which surprised me no end and it was only looking through the code which made me look :wink:

The cause is fs.writeFileSync in:

https://github.com/Mogztter/asciidoctor-kroki/blob/f9b9abfb64be2339bde78bb77c41339e88dfcbfd/src/node-fs.js#L7-L12

where filePath might be something like media/ba38556d595f9b57735747d9de9ee77e7335cdb2.svg.

The node fs module which uses the fs.openSync on this relative path and somewhat deep in node internals it appears that if there is only a relative path then perhaps it uses the user's home directory (or perhaps this is what vs code is using as the cwd, although I doubt it). The node filesystem docs don't help me very much.

I think the problem is that the asciidoctor-kroki extension is incorrectly handling base_dir which is an API option, not a document attribute. Perhaps we should be using instead the document attribute docdir?

https://asciidoctor.org/docs/user-manual/#piping-content-through-the-cli

Looking at the (Ruby) code:

https://github.com/asciidoctor/asciidoctor/blob/8f838709af08cdd5c3f1c91e4a6bba54e87cca54/lib/asciidoctor/convert.rb#L12-L20

It would seem that if docdir is used then it is set base on base_dir if it is specified.

https://github.com/asciidoctor/asciidoctor/blob/88bc891d6a0143f3a2142f41d29cabe3cb21fe3f/lib/asciidoctor/document.rb#L380-L390

So I think the correct fix would be to replace baseDir with docDir along the lines of:

  const imagesOutputDir = doc.getAttribute('imagesoutdir')
  const outDir = doc.getAttribute('outdir')
  const toDir = doc.getAttribute('to_dir')
  const docDir = doc.getAttribute('docdir') || ''
  const imagesDir = doc.getAttribute('imagesdir') || ''
  let dirPath
  if (imagesOutputDir) {
    dirPath = imagesOutputDir
  } else if (outDir) {
    dirPath = path.join(outDir, imagesDir)
  } else if (toDir) {
    dirPath = path.join(toDir, imagesDir)
  } else {
    dirPath = path.join(docDir, imagesDir)
  }

but I haven't entirely convinced myself so I haven't submitted a PR.

Thoughts?

[ggrossetie]

The current path resolution is incorrect but it's complex! I did have a look at Asciidoctor diagram and here's an highly simplified sample:

def normalize_system_path(target, start = nil)
  if start
    start = ::File.join(doc.base_dir, start) unless start.start_with? '/' # absolute path
  else
    start = doc.base_dir
  end
  ::File.expand_path(target, start)
end

document = parent.document
images_dir = parent.attr('imagesoutdir', nil, true)
if images_dir
  base_dir = nil
else
  base_dir = parent.attr('outdir', nil, true) || document.options[:to_dir]
  images_dir = parent.attr('imagesdir', nil, true)
end

image_name = 'image.svg'
image_dir = normalize_system_path(images_dir, base_dir)
image_file = normalize_system_path(image_name, image_dir)
p image_file

In summary:

• the document base directory is always used to expand the path
• it seems that the attribute docdir is not used

The precedence order seems to be correct:

1. imagesoutdir
2. attr.outdir/imagesdir
3. options.to_dir/imagesdir

If attr.outdir and options.to_dir are both nil then I think the value is doc.base_dir/imagesdir. We should probably use document.getBaseDir() and not doc.getAttribute('base_dir').

I will try to add some tests to make sure that the path resolution is correct.

[danyill]

@Mogztter thanks. Can I do something to help? I've updated the PR to use getBaseDir and removed the previous commit with a force push FWIW.

[ggrossetie]

@danyill I think we want to add a unit test to reproduce this exact use case then we can be sure that this fix is actually working. So we will need to create a file in test/fixtures/subdir/doc.adoc with the following content:

:imagesdir: media
:kroki-server-url: https://kroki.io
:kroki-fetch-diagram:

[plantuml,alice-bob,svg]
....
alice -> bob
....

Convert it and check that the image is generated in the right path. We should first run the test on master branch to make sure that we reproduce the behavior you've described, then apply the fix and test again to see if this issue is fixed.

[danyill]

@danyill I think we want to add a unit test to reproduce this exact use case then we can be sure that this fix is actually working.

Bonjour! I have rebased onto master and added this test (and force pushed, was that a sin?). Although my minutes per line of code would make the brave cry. C'est la vie.

I also discovered that one reason why 'docdir is not a good choice is because depending on the level of safe mode this information is not available (see here).

I shall be happy to receive further review :smile: