Split out VisibleStore and ReferrersStore from Store

[Ericson2314]

Motivation

More progress on issue #5729

queryAllValidPaths and queryReferrers were two methods that failing unsupported default implementation. This is not type safe. Now, those methods are in their own classes, and there is no more unsupported default instances of them.

computeFSClosure with flipDirection = true also exposes the concept of referrers (and its default implementation uses queryReferrers) so I moved it to ReferrersStore. That is; computeFSClosure now no longer takes the flipDirection argument, and ReferrersStore has a computeFSCoClosure method.

Context

(I thought we could get away with one class for both, but sadly that is not quite the case: LocalBinaryCacheStore and S3BinaryCacheStore implement the former but the latter.)

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

• agreed on idea
• agreed on implementation strategy
• tests, as appropriate
  • functional tests - tests/**.sh
  • unit tests - src/*/tests
  • integration tests - tests/nixos/*
• documentation in the manual
• documentation in the internal API docs
• code and comments are self-explanatory
• commit message explains why the change was made
• new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

[edolstra]

I'm not sure if moving this stuff to the type level really makes life easier. It feels over-engineered, especially because it requires multiple inheritance which often introduces weird problems. Much easier for unsupported methods to either throw an error or return std::nullopt or whatever.

[Ericson2314]

Multiple inheritance is perfectly fine for method-only "interface" types (with a unique override implementation); It is analogous to multiple inheritance of Rust traits.

We did this same thing for LogStore and GcStore about a year ago, and we've had no issue with that to my knowledge. It's a good pattern and I would like to continue with it.

[Ericson2314]

Much easier for unsupported methods to either throw an error or return std::nullopt or whatever.

That is only good when you can then pass around the result of that method call. E.g. getFSAccessor can return an abstract ref<FSAcessor> and that can be passed around.

But you want to only have this fallibility, whether it is down-casting or calling method that might fail, at the boundary of the system (CLI or entry points). Internally everything should be infallible so the types actually mean something.

[Ericson2314]

Finally, I think it is very advantageous to split these out of the main Store interface because of the capability / ambient authority aspects. A highly desired feature for Nix is secret management, and table to stakes to that is not exposing all store objects.

For LocalFSStore, I agree mandatory access control via ACLs as your gist described is a natural way of doing things since file permission bits / closed file permissions were designed around that model. And then sure, these methods could be implemented in that way by just filtering their output. But for other less Unix-y stores, discretionary access control is more natural, and indeed we already have it (for free) for HttpBinaryCacheStore and other stores that simply never implemented these methods.

I think that's a fruitful direction to explore, and splitting out an interface like this is a very simple and easy thing that allows doing so in the future. (To the extent that Nix is "functionality" capability-based security is the natural way of doing things too.)

[Ericson2314]

The second commit cleans up the use of require, and makes ReferrersStore` more interesting.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-05-05-nix-team-meeting-minutes-52/27893/1

[fricklerhandwerk]

Discussed in Nix team meeting 2023-07-07:

• Tracking this kind of things statically might not be enough since some of them may be dynamic

  • For instance, ACLs could cause a local store where queryAllValidPaths wouldn't work
  • Also, this doesn't play well with the C++ type system as it's abusing the class system as a capability system
    • Causes issues when you need to mix two capabilities as there's no easy and elegant way of doing it

• @edolstra: What's the value in it?

• @thufschmitt: Could we instead make the store compose these capabilities as std::optional fields?

• Four big paths we can take:

  • Keep the single monolithic class, maybe improving the “not implemented” thing (by making these methods std::optional for instance so that they can be queried)
  • Push this splitting forward, knowing that it'll be an approximation because some capabilities are dynamic
  • Only split things that we know are static as separate classes
  • Implement the capabilities as optional fields in the class

No decision taken

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-07-07-nix-team-meeting-minutes-69/30470/1