Spec gotcha: directory entry must end with /

[jastice]

SourceItem specifies that a directory entry must end with /. This is a detail that is likely to be missed by implementors of the spec, both client and server and requires looking at the string to determine if something is a directory. I suggest making it an explicit field in SourceItem (and probably DependencySourceItem as well.

trait SourceItem {
  /** Either a text document or a directory. A directory entry must end with a forward
    * slash "/" and a directory entry implies that every nested text document within the 
    * directory belongs to this source item.
    */
  def uri: Uri

  /** Indicates if this source is automatically generated by the build and is not
    * intended to be manually edited by the user. */
  def generated: Boolean
}

[olafurpg]

The "directory ends with slash" convention was borrowed from nio.file.Path but it's quite universal I think. I like it because by construction avoids stuff like

uri: "file:///Users/foo.scala"
directory: true

I don't feel strongly about this however.

[jastice]

any opinion @jvican ?

[jvican]

I like this convention and I think there's no benefit in splitting up the entities to represent files and directories. You make a good point that we should improve this invariant in the specification so that implementors and clients know about it. I would get the contents of that comment and put it in an independent paragraph.

[jastice]

Okay with me. My concern is that ideally the spec wouldn't need to be referred to much while implementing. But this may not be a major issue here.

[jastice]

Just ran into this actually. Java File and Path don't differentiate between directory and file, and serialize the path to the same string without trailing / either way. I suspect other file system libraries do the same. I think implementors are likely to run into this.

[jastice]

even the bsp API has this issue:

> Uri("file:///here/or/there/").toPath
res0: java.nio.file.Path = /here/or/there

So the only way to check if an uri is intended as directory is to check the string of the uri. I think this is unelegant and error-prone

[jvican]

Yes that’s expected, you need to add the trailing slash yourself, we do that in bloop

[jastice]

You expect it, but I expect this to cost most people implementing this a few hours of debugging :(

[olafurpg]

@jastice can you give an example where this situation causes issues? I haven't had issues with this so far in Metals, I work with the plain bsp4j data structures I parse URIs into paths with Paths.get(URI.create(item.getUri)). Would it help if we add a SourceItem.isDirectory: Boolean method in bsp4j?

[jastice]

The issue is, on client side I need to know if a path is intended to be a directory or a regular file, especially if this path doesn't exist (yet). With the way this information is encoded, I think it is likely that server implementors will just forget to do that, because the trailing / convention is not supported by file system APIs.

The most straightforward approach:

val dir = new File("/some/directory/")
new SourceItem(dir.toURI.toString, false)

will lead to the wrong encoding:

dir: java.io.File = /some/directory
item: ch.epfl.scala.bsp4j.SourceItem =
SourceItem [
  uri = "file:/some/directory"
  generated = false
]

Which in itself is easy enough (albeit a bit ugly) to fix once you know the problem, but it's kind of subtle to figure out what's wrong in the first place when items aren't imported into the client correctly. I wouldn't rely solely on documentation to guide correct implementation.

Therefore I suggest creating a SourceItem should make it explicit in some way if it's a directory or file, either by a dedicated field like directory: Boolean, or a separate type SourceDirectory -- or some other way of ensuring the intended is explicitly encoded.

[olafurpg]

I agree this is a likely pitfall for server implementations (but less so for clients). How about we introduce an an optional kind field with enum type that is DIRECTORY by default?

[jastice]

I did have the same issue implementing it as client, then forgot about it when implementing the mock server and had the same issue again :)

I think the optional kind field would be sufficient and keep compatibility with the current version of the spec, while allowing the library to enforce the choice in constructors.

[olafurpg]

Sounds good, if you open a PR I'm happy to LGTM 👍

[jvican]

I think the optional kind field would be sufficient and keep compatibility with the current version of the spec, while allowing the library to enforce the choice in constructors.

This would be a breaking change for existing servers that don't set the directory field in servers. Everyone would need to migrate to the new API.

It's a shame that the Java NIO APIs strip the trailing path separator by default for directories and that, when the file doesn't exist, clients need to check with .endsWith(java.io.File.pathSeparator) to distinguish non-existing directories from non-existing files.

I see why @jastice would want that and if this is really an imperative for him we can do it. But I would strongly suggest having a consistent API that relies on URIs instead of replacing every instance of a path with our own abstraction.

The more BSP looks like LSP, the better IMO.

[jastice]

This would be a breaking change for existing servers that don't set the directory field in servers. Everyone would need to migrate to the new API.

In the case that a server uses URIs to encode plain files, yes. Are there servers besides bloop?

clients need to check with .endsWith(java.io.File.pathSeparator)

Yet another pitfall: In the URI it's always /, while pathSeparator is system-dependent :)

I see why @jastice would want that and if this is really an imperative for him we can do it. But I would strongly suggest having a consistent API that relies on URIs instead of replacing every instance of a path with our own abstraction.

Plain URIs would be more elegant in some way, but relying on an implicit string encoding of an important property is just too error prone.

The more BSP looks like LSP, the better IMO.

Agree, but as far as I see, LSP doesn't use URIs in a way where it's ambiguous whether they are a file or diectory

[jvican]

Agree, but as far as I see, LSP doesn't use URIs in a way where it's ambiguous whether they are a file or diectory

That's a good point! It looks like the only place where they use URI to mean a directory is in the rootUri of the initialize params request. It's not clear to me that directories and files can be passed in together as inputs of the file watching request but assuming they cannot then your point is solid.

OK, I'm good with this change 👍

[olafurpg]

Yet another pitfall: In the URI it's always /, while pathSeparator is system-dependent

URIs are system-independent, they use / on all operating systems. The output from Path.toString and File.toString should not be used to construct URIs, Path.toUri and File.toURI should be used instead. The biggest pitfall IMO is the fact that Path.toUri doesn't include a trailing / when the path doesn't exist and I think that's a good enough reason to go ahead with this change.

[jastice]

That's a good point! It looks like the only place where they use URI to mean a directory is in the rootUri of the initialize params request. It's not clear to me that directories and files can be passed in together as inputs of the file watching request but assuming they cannot then your point is solid.

For file watching I don't think it matters: You watch what's there, whether it's a file or directory..

But I've looked into the spec more closely and there's more cases, only they call directories folders.
There's file operations that are supposed to work on both files and folders:

File resource changes allow servers to create, rename and delete files and folders via the client. Note that the names talk about files but the operations are supposed to work on files and folders. This is in line with other naming in the Language Server Protocol (see file watchers which can watch files and folders).

There is a CreateFile operation, and it doesn't specify anywhere how it is supposed to denote whether it's a directory or actual file. I'll try to figure this out.

[jvican]

Good.

For file watching I don't think it matters: You watch what's there, whether it's a file or directory..

Note that it does matter because you don't file watch a directory in the same way you would file watch a file. If you have a non-existing URI and you start to file watch it as a file, the file watcher could fail whenever it is created afterwards as a directory.

[jastice]

So it seems on LSP side they simply haven't thought much about file/directory. See eg microsoft/language-server-protocol#272 (comment)

I'll go ahead with the change then.

[PavelSosin]

URI assumes that some protocol portion is expected. I think that building remote workspace is fantasy

[PavelSosin]

Indeed, Opened workspace can be represented by URI which is mapped to some place in the storage by workspace run-time LSP servers and builders are parts of workspace runtime. Every resource inside workspace can be addressed relatively to workspace URI. Relative path which contains protocol is meaningless.

[PavelSosin]

Regardless of my previous comment, I think that designation of folder resource by trailing / is consistent
BSP doesn't create resources by itself but delegates this job to Workspace. Workspace has its own protocol like in eclipse/che. Get WorkspaceURI/relative_path_to resource, PUT WorkspaceURI/relative_path_to resource and POST WorkspaceURI/relative_path_to resource. Trailing / designates folder.
I also expect that Workspace protocol defined in such way that
+/folder1/
+subfolder12/
+subfolder123/
and + folder1/subfolder12/subfolder/123/file1
are equivalent. Leading space designates Workspace root as usual. But this is requirement for Workspace protocol and BSP should not extend or correct this protocol. These protocols are unrelated.

[jastice]

Solved via #83