Implementing NFS API rename


#1

Here I go again :wink:

I’m about to implement file rename file in an NFS container.

As you can’t change the key which holds the name (and possibly path) of the file, the process will be:

  • access the file’s immutable data reference
  • insert it into a new or existing entry corresponding to the new name
  • delete the old entry

My question is: what’s the best way to get hold of or create the File object which you then insert (or update) into the entry for the new name?

Can I just use the File object returned by fetch() and pass that to insert() or update()?

I ask because that seems unlikely given that insert() and update() are normally passed a slightly different File object which looks like a wrapper, that hold a reference to the one obtained via fetch()


#2

Yes.

fetch() returns same file context as open().
This is good stuff. Let me know what gave you the impression that fetch() returns an object that can’t be passed to insert() and update(), so I can make sure it’s clarified in docs/code/comment.


#3

Cool that’s good news. Thanks Hunter.

Because when using open( await fetch (),...) the returned File is not the same object as that returned by the fetch(). I think the docs actually say that, but regardless I believe that because:

  • I think that I was initially passing the wrong one to insert() or update() and that it didn’t work until I switched. It is possible though that the actual reason was something else.
  • inspecting them in the debugger, the second appears to be a different object, and holds a ref to the first.

Thanks for the link to the implementation which I see confirms the second bullet and helps me understand what is in the File object, and the role of that mysterious Ctx member!

Thanks again Hunter. :slight_smile:


#4

Right, I was making an assumption in my statement. The JS File object returned from fetch() is identical in format to the one returned by open(), with the assumption that it’s been written to and closed.

Each should be expected to successfully be passed to insert() or update().


#5

@hunterlester, a question related to ‘rename’

To copy/move files within a container I’m using File objects and inserting the exising file into a new or existing entry (so I avoid the need to read the content of the source file and saving that to a new File object).

If I do this as follows, the new file will inherit the NFS and user metadata of the old file:

let file = await nfs.fetch(sourcePath)
nfs.insert(newPath, file)

If I want the new entry to be created with its own NFS metadata (e.g. created data), what’s a good way to do this? I can imagine a few options, but am not sure which is the most sensible.

Thanks.


#6

I’m going to run a few tests myself but initially it looks like the best option is to add a third argument to nfs.insert to write new user metadata.


#7

Sorry, I wasn’t clear - I’m also referring to the NFS system metadata (ie created, updated etc). I could manually set them but it seems brittle. I think with userMetadata it is straightforward just to include or drop it.


#8

Oh I see what you are asking now.
Something like:
Although I may be creating a file context that is referring to the same XOR data map, it’s still a newly created file context and should not inherit created and modified fields.
I agree.

With the current API, the only option is to fetch the file contents and create a new file context with those contents.
I’m glad you posted your thoughts about changing NFS because this problem ties into that as well.

I’m wondering if the emergence of a strong RDF API will make NFS redundant and surpass it’s usage?


#9

OK, then this should be feature request for whatever the final API is: a way to initialise a new file with the ImmD of another without having to read the file content itself.

A good question. I’m looking forward to hearing the team’s plans in this area as you know. I suspect that while there are different ways of doing this, part of the RDF API will need to support something very like the existing NFS API, whatever is going on underneath. As it stands, I think the approach Josh was considering was hiding RDF from the API level as far as possible, but having it underneath as a base layer that can be leveraged when needed. That’s a bit like having the existing NFS API and hiding the RDF underneath. I favour having options alongside each other, and making each API style as easy to employ as possible rather than simplifying things in ways which might make it harder to make use of the power of underlying RDF, but it is still very early days and I’m open to all approaches.


#10

Hunter, I encountered an unexpected side effect of using an existing File object to create or update an NFS entry. I’m not sure if this should be a bug, or documented or what but it caught me out.

When you use a File object from one entry to insert/update a different entry, it takes on the version of the latter.

This meant that I was using the wrong version afterwards when I tried to delete the first entry (implementing file rename / move).

So if you use a File object on a different entry, you should then discard it and fetch() it anew.


#11

I’m trying to reproduce this right now.
I created a new file context with create() then inserted it as 4 entries with different file names, file1, file2, file3, file4.
I then fetch() file1 and observe that the version is still 0.

Let me know which steps you are taking so that I can reproduce.


#12

Hmm, maybe it’s not as I understood. I’ve deleted the code that was doing this so will have to backtrack and see what happens. I’ll get back to you.

Until I do, I think one difference is that I was using the File returned by fetch() then insert /update to another entry, then delete the original entry. But the File has the version of the other entry.

EDIT: My editor had the deleted code in its undo cache :slight_smile: You could run this again if you want to try it out (using SAFE Drive), by adding it back in place of copyFile() in safentworkjs/src/nfs-files.js), but maybe it is just enough for it to confirm what I said immediately above. It just shows that I’m using a File obtained from fetch() to call insert() or update() and subsequently use it again for delete() that’s the point where the version is wrong.

  async copyFile_TEST (sourcePath, destinationPath, copyMetadata) {
    debug('%s.MODIFIED copyFile(\'%s\', \'%s\')', this.constructor.name, sourcePath, destinationPath, copyMetadata)
    try {
      let srcFileState = await this.getOrFetchFileState(sourcePath)
      if (!srcFileState) throw new Error('copyFile error - source file not found:', sourcePath)

      if (!copyMetadata) {
        // TODO need to clear and reset metadata in srcFileState._fileFetched before
        // Have asked for advice on how to do this: https://forum.safedev.org/t/implementing-nfs-api-rename/2109/5?u=happybeing
      }
      let destFileState = await this.getOrFetchFileState(destinationPath)
      if (!destFileState) {
        await this.nfs().insert(destinationPath, srcFileState._fileFetched)
      } else {
        await this.nfs().update(destinationPath, srcFileState._fileFetched, destFileState._fileFetched.version + 1)
      }
      await this.nfs().delete(sourcePath, srcFileState._fileFetched.version + 1)
    } catch (e) {
      debug(e)
      throw e
    }
  }

If you look at the now working code in copyFile() (this is now all pushed to master BTW), you can see I got it to work by calling _purgeFileState(srcFileState) which ensures that when I try to delete (which is in moveFile()) the File handle used by copyFile() has been discarded from my cache and will be fetched anew.

async copyFile (sourcePath, destinationPath, copyMetadata) {
    debug('%s.copyFile(\'%s\', \'%s\')', this.constructor.name, sourcePath, destinationPath, copyMetadata)
    try {
      let srcFileState = await this.getOrFetchFileState(sourcePath)
      if (!srcFileState) throw new Error('copyFile error - source file not found:', sourcePath)

      if (!copyMetadata) {
        // TODO need to clear and reset metadata in srcFileState._fileFetched before
        // Have asked for advice on how to do this: https://forum.safedev.org/t/implementing-nfs-api-rename/2109/5?u=happybeing
      }
      let perms // If auth needed, request default permissions
      let destFileState = await this.getOrFetchFileState(destinationPath)
      if (!destFileState) {
        // Destination is a new file, so insert
        await this._safeJs.nfsMutate(this.nfs(), perms, 'insert', destinationPath, srcFileState._fileFetched)
      } else {
        // Destination exists, so update
        await this._safeJs.nfsMutate(this.nfs(), perms, 'update', destinationPath, srcFileState._fileFetched, destFileState.version() + 1)
        await this._purgeFileState(destFileState) // New file so purge the cache
      }
      // After using the fetched file to update another entry, it takes on the version of the other, so needs refreshing
      await this._purgeFileState(srcFileState)
    } catch (e) {
      debug(e)
      throw e
    }
  }

It may be a bit confusing to follow the above fragments because in copyFile_TEST() I’m calling the NFS API directly to help debugging, whereas in copyFile() I’m calling those same functions, but via other APIs such as _safeJs.nfsMutate() (which will do an auth request if needed).

Hope that helps!


#13

I’m rooting around looking to find where a file is stored once it’s been created. Could you update that value with the returned value from this update() operation?
If so then you should always have the correct File context version number.

Another thing I’m looking at, but not your issue:
I’m able to always pass 0 as the version when making an update without error any number of times. Is this a bug or is it expected? @marcin


#14

I’m keeping File objects around in _AllNfsFiles here for the duration of an app-wide fd/file descriptor (eg during POSIX/FUSE open, write, write… close).

In addition, the NfsContainer object which manages a SAFE NFS container has its own cache of FileState, so a File object can be retrieved from one, the other, or if not available in either it is obtained by fetch() and will be put into the second (path based) cache until purged.

I’m not sure if you realise I have now solved this? I just had to make sure the File object is re-fetched after the update.

I think if I keep the object returned by update() I’ll still have an object that refers to the updated entry, rather than the source entry which I then want to delete(). But as I say, it works so long as I re-fetch() the entry I start with.

That repeated update() with version 0 has to be a bug. Very odd! So good catch :slight_smile:


#15

It’s technically not a bug :slight_smile:

If version is 0, the correct version is obtained automatically.

However, this does seem potentially confusing and misleading now that I look at it. Maybe better would be to have a GET_NEXT_VERSION constant, and encourage people to use that instead of hard-coding 0. What do you think @hunterlester @happybeing?


#16

Yes, sounds good. Would that mean an extra get, or is there just one get in both cases?


#17

@happybeing it is an extra GET as it has to retrieve the current version. I think we should have GET_... in the name of the constant to reflect that.


#18

Oh nice! I should have known to look there for the answer. Thank you.
Yeah, we’ll add a constant for people to use in safe_app_nodejs and update docs.


#19

No problem, thanks for tagging me. :slight_smile: We’ll be adding a constant on the Rust side as well.