kndndrj / nvim-dbee

Interactive database client for neovim
GNU General Public License v3.0
606 stars 39 forks source link

[Windows]: Create a note creates a set of directories instead of just a single `.sql` file throws error on `mkdir` #106

Open JustBarnt opened 2 months ago

JustBarnt commented 2 months ago

OS: Windows 11

Nvim Version:

NVIM v0.10.0-dev-2765+gb8858dddb
Build type: RelWithDebInfo
LuaJIT 2.1.1710088188

Reproducible on Windows via default nvim-dbee setup.

Creating a global or local note will create the note at the following directory which currently appears to cause two issues.

  1. I think this is what causes nvim-dbee to be unable to find the notes created in a previous session
  2. An error is thrown only when creating a local note stating:
    Error executing vim.schedule lua callback: Vim:E739: Cannot create directory 
    C:\Users\bwiliams\AppData\Local\nvim-data/dbee/notes/file_source_C\\Users: file already exists
    stack traceback:
    [C]: in function 'mkdir'
    ...nvim-dbee/lua/dbee/ui/editor/init/lua:272: in function 'namespace_create_note'
    ...nvim-dbee/lua/dbee/ui/drawer/convert.lua:400: in function 'on_submit'
    ...nui.nvim/lua/nui/input/init.lua:160: in function <...ata/Local/nvim-data/lazy/nui.nvim/lua/nui/input/init.lua:147>

That error I traced back into lua/dbee/ui/drawer/convert.lua where I found a current_connection_id variable that prints out the following value:

It also may be better instead of using the current_connection_id to create the dir for the namespace instead, we use the name key we set when we create the database connection that way your notes folder would line up with the name you give that specific database connection?

It may be possible that resolving this issue fixes #105.

kndndrj commented 2 months ago

hey, the ID with a file path is intended, yes. It also works well on unix, since everything is a valid character for a file name. However, there are a bunch of illegal characters for file names in windows, which is a problem.

One way of addressing these problems is to substitute all of these characters with something else, but I don't really like that approach, because its very error prone.

One idea ghat comes to mind would be to encode the id's as base64 (which contains only legal chars AFAIK). base64 functions are also provided inside nvim api, which is very convenient.

edit: well, it appears that I'm wrong. But maybe still more predictable.

edit: even simpler solution is to only match valid chars (e.g. alphanumerics and a few extras).

JustBarnt commented 2 months ago

Is there any reason in particular as to why current_connection_id is saved as the path to the persistence.json file instead of just a random string of chars at vim.fn.stdpath('state') .. /dbee/notes/?

I'm open to whatever you would prefer to make current_connection_id using the name key was just a suggestion since it was already being saved using a different value unique to that connection.

Another issue that I discovered later last night is that vim.fn.mkdir even with the p flag does not silently fail on Windows and prevents any additional notes from being created because a dir with that name already exists.

So, it may be better instead of just letting vim.fn.mkdir silently fail even on Linux that we just check if the directory exists and just create it if it doesn't.

I also plan on submitting some PR's with these windows related items once we have plan going forward so it's not all on you, I'm more than happy to help solve these issues on Windows.

Thought?

kndndrj commented 2 months ago
  1. no particular reason for that, but we can't just bet on all id's to be valid windows file names
  2. we can just call mkdir with pcall
  3. PR's would be welcome, although, I'm sorry to say that I don't know when I'll be able to merge that.. might be soon and it might take more than a month :(
JustBarnt commented 2 months ago
  1. no particular reason for that, but we can't just bet on all id's to be valid windows file names

Right, which is why I was originally proposing we make it something else, if we use the string util that was added with my other PR that should work because it just a sequence of numbers and letters that are possible correct? Which should always be a viable directory name for Windows & Unix alike.

My current concern for using that entire file path is hitting the Windows character limit. Which is only 260 characters including c:\ and the null character at the end of the path.

Ex: On my computer at work that path total is currently sitting at 140 characters, but depending on how usernames are created for a user I would say it is very plausible that limit could get hit.

  1. we can just call mkdir with pcall
  2. PR's would be welcome, although, I'm sorry to say that I don't know when I'll be able to merge that.. might be soon and it might take more than a month :(

Calling with pcall works as well, that didn't cross my mind, I update my fork and try pcall to see if that works.

As for you schedule that's totally understandable. I would think most of us have fulltime jobs that are not creating Nvim/Vim plugins lol. So if my PR addresses the issues with Windows and still keeps Linux stable, I just stay on my forked branch until you are able to review again!

WillEhrendreich commented 2 months ago

hey, I'm just now getting my feet wet with this plugin, and it seems like it's going to be awesome once it's working. I'm also on windows, and getting this issue.

I agree with @JustBarnt that bloating the length of the path is going to run into issues really quick on windows, and having the mix of path separators is really not great either, but the path thing is fixed quite nicely by making sure to call vim.fs.normalize on everywhere you plan on having paths constructed.

as far as the file name length goes, what if you just took the last generated bit from the id and stuck that as the dir name? like mine has this

"c:.local\state\nvim-data/dbee/notes/file_source_c:/.local/state/nvim-data/dbee/persistence.jsonLwGTlWTFbO/note_hwGawobXkc.sql"

if you ran a replace on it taking out "file_source_c:/.local/state/nvim-data/dbee/persistence.json" I'd end up with this: "c:.local\state\nvim-data/dbee/notes/LwGTlWTFbO/note_hwGawobXkc.sql", which would certainly help quite a bit, and then to run a normalize on it would result with "c:/.local/state/nvim-data/dbee/notes/LwGTlWTFbO/note_hwGawobXkc.sql" which is guaranteed not to flip out Windows.

also checking if a dir exists would make it easier, especially if you could just pass the final filepath wanted in, so, like in this case, if you added these funcs in:

local function dir_exists(path)
  local stat = vim.loop.fs_stat(path)
  local isDir = false
  if stat then
    vim.notify(vim.inspect(stat.type))
    if stat.type == "file" then
      vim.notify(vim.inspect(stat.type))
      isDir = dir_exists(vim.fs.dirname(path))
    end
    if stat.type == "directory" then
        isDir = true
    end
  end
  return isDir
end

local function make_dir(path)
  path = vim.fs.normalize(path)
  local exists = dir_exists(path)
  if not exists then
    vim.uv.fs_mkdir(path, 1, function(err)
      if err then
        vim.notify("Error creating directory: " .. path .. " - " .. err)
      else
        vim.notify("Directory created: " .. path)
      end
    end)
  else
    vim.notify("Directory Already exists: " .. path)
  end
end

you could pass in "c:\.local\state\nvim-data/dbee/notes/LwGTlWTFbO" and get a dir created with a normalized path.

could also get more "fancy" with it and take a dependency on plenary, which has a good path module, and you could take advantage of how it handles creating dirs, which I assume is pretty good.