-
Notifications
You must be signed in to change notification settings - Fork 132
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
Store documents in a memdb-backed table #771
Conversation
filesystem
into memdb tablefilesystem
into documents
memdb table
efb972f
to
2d40ec6
Compare
2d40ec6
to
64ddf97
Compare
filesystem
into documents
memdb table…mentStore Previously filesystem package had two major use cases, to offer a unified io/fs.FS interface for e.g. parsing *.tf or *.tfvars, which was implemented mostly via external library (spf13/afero). Secondly it also provided a direct full access to the "in-memory layer" of the filesystem for RPC handlers (e.g. didOpen, didChange, didClose, ...). These use cases rarely overlap throughout the codebase and so this lead to unnecessary imports of the `filesystem` package in places where we only needed either the OS-level FS or in-mem FS, but almost never both. This decoupling allows us to import `filesystem` or `state.DocumentStore` separately. Also, as we no longer need the in-mem backend of afero, it makes more sense to just reimplement the small part of the 3rd party library instead.
64ddf97
to
215ebe5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work and presentation! Thank you for taking the time to make the review as easy as possible.
I was able to follow through with all of your changes, and the language server still works ;)
I've added a few questions to understand memdb handling transactions better.
ccf8946
to
3a4d871
Compare
3a4d871
to
cce44ec
Compare
cce44ec
to
5af49be
Compare
The refactored I also moved some of the existing logic we had in place for case-insensitive comparison of URIs there and added some comments after researching the problem. There is a few problems which this PR does not address, but we have issues to track them separately:
@dbanck Do you mind reviewing the newly added commits? I intentionally left the old ones intact, so it should be easier to filter out. I re-tested the PR in a Windows VM on VMware Fusion but I'd also appreciate any additional testing on any other Windows machine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for answering all my questions and adding more tests!
Your refactorings around the handle/URI logic look solid, and the comments are helpful. I would never have guessed that VS Code is making paths so difficult.
Next, I'll test the changes on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🎉
Everything works fine for me on Windows, too.
This functionality has been released in v0.26.0 of the language server. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
This is a first part of refactoring per #719
Why
Per LSP contract the language server is basically expected to maintain the up-to-date copy of the document sent by client via text synchronization methods (e.g.
didOpen
ordidChange
). Previously we did this as part of thefilesystem
package where we kept it within a map:terraform-ls/internal/filesystem/filesystem.go
Lines 20 to 21 in b13641c
The
filesystem
package also served two use cases, which ended up not being as related to each as we originally thought:spf13/afero
While this was a reasonable implementation originally, I believe we have slowly outgrown it with our needs.
Much of the important state which LS maintains is held in memdb and so it makes sense to move this data to memdb as well. We can benefit from the "data locality" as we don't need to be importing another package like
filesystem
if we only need access to documents (which is majority of the RPC layer). Secondly we can query and update data across memdb tables more easily. Lastly it makes the architecture a bit easier to understand when data/state is kept in one place, stored the same way.New
document
packageA new package is dedicated to:
Document
type describing the document itself (exactly how it is stored in memdb)lsp
,filesystem
packages and elsewhere. This makes it a single place which deals with conversions between file, document and directory URIs as needed in various places.Having a dedicated (smaller) package makes it easier to import throughout the codebase without running into import cycles.
New
documents
memdb tablePreviously the
filesystem
package maintained aisOpen
flag next to a document indicating whether the document is open but that flag was never used because we only ever tracked open documents and removed them when they get closed - seeterraform-ls/internal/filesystem/filesystem.go
Lines 171 to 187 in b13641c
It therefore made sense to just avoid that flag.
Removal of
spf13/afero
After decoupling documents into memdb and re-plumbing DocumentStore methods back to the
filesystem
FS interface I realized that there's actually relatively little benefit we would now get from the external library and decided to just reimplement the small part that we actually need - which is translation betweendocument.Document
andfs.FileInfo
orfs.DirEntry
which ended ininternal/filesystem/document.go
,internal/filesystem/inmem.go
andinternal/filesystem/os_fs.go
- altogether relatively few LOC.Unfortunately we still depend on afero indirectly through
mockery
which we use in tests:but this is "dev" dependency only.