jj icon indicating copy to clipboard operation
jj copied to clipboard

Prototype for `jj api`

Open matts1 opened this issue 1 year ago • 16 comments

This is a prototype of jj api. It doesn't have anything like tests or documentation, but it works and I'd like to get approval on the general code structure before continuing.

Checklist

If applicable:

  • [ ] I have updated CHANGELOG.md
  • [ ] I have updated the documentation (README.md, docs/, demos/)
  • [ ] I have updated the config schema (cli/src/config-schema.json)
  • [ ] I have added tests to cover my changes

matts1 avatar Apr 30 '24 06:04 matts1

some interesting stuff here. you've noted that the idea is that eventually the jj CLI would be reimplemented on top of this API... do you have any thoughts on how that would interact with the server component and its lifetime management? is the "servicer" supposed to be a lower level piece, used by both server and CLI?

gulbanana avatar Apr 30 '24 13:04 gulbanana

more specifically, do you think it would be feasible for the lower-level apis to be hosted in a long-running server that isn't request-response like gRPC? jj web, for example, might want to use a websocket connection with server->client push.

gulbanana avatar Apr 30 '24 13:04 gulbanana

The intention was that all communication would be done via the Servicer class, which just takes FooRequest protos and returns FooResponse protos.

This would mean that jj-cli would completely bypass the gRPC layer, and simply write:

let servicer = jj_lib::api::Servicer::new(Some(command.workspace_loader()?));
let response = servicer.list_workspaces(ListWorkspacesRequest{
});
// print based on response

It would also mean that we could implement:

  • jj api grpc (grpc sockets)
  • jj api grpc --web (grpc over web)
  • jj api web (websocket connection)
  • jj api json (stdin / stdout json communication)
  • jj api json --pipe=/path/to/pipe

My was to ensure that we didn't lock ourselves into gRPC here, and so I made that intermediary layer Servicer with the intention that we can easily just add other layers on top of it (eg. your jj web suggestion).

matts1 avatar May 01 '24 00:05 matts1

Would this mean that each CLI command becomes a method on Servicer, implementing the semantics and options of the former command? That’s a very high level interface. It would imply that alternate clients also each need their own set of Servicer methods with the appropriate semantics. For example, jj web would want to be able to control when snapshots occur, rather than having them implicitly take place at the start of commands as the CLI does.

gulbanana avatar May 01 '24 02:05 gulbanana

No, it would mean that each CLI command can be constructed out of some combination of servicer methods. For example, jj commit might involve:

fn commit_command(args: Args, servicer: Servicer) {
  let commit = servicer.get_commit(GetCommitRequest{
      CommitRef{revset: "@"}
  });
  let current_commit = CommitRef{change_id: commit.change_id};

  let description = args.description.unwrap_or_else(|| editor(commit.description));
  servicer.update_commit(CommitUpdateRequest{
    commit: current_commit,
    desc: description,
  });
  let new_commit = servicer.create_commit(CreateCommitRequest{
    parents: vec![current_commit],
  }
  servicer.edit_commit(EditCommitRequest{commit: CommitRef{id: new_commit.id}})
}

matts1 avatar May 01 '24 02:05 matts1

That seems workable, although figuring out exactly where to draw the API line will be a long process. I'm a bit concerned that it means each change to jj will have duplicated work - updating the internal data model, and updating the way it's exposed in the API.

Servicer could also become rather vast, since it'd be a flattened Facade (in the design patterns sense) for the entire codebase.

gulbanana avatar May 01 '24 06:05 gulbanana

figuring out exactly where to draw the API line will be a long and complex process

+1 to that. It seems like a massive pain in the ass that will inevitably involve some very long discussions, but I think it's one that needs to be done.

I'm a bit concerned that it means each change to jj will have duplicated work.

I do agree here, but I think that's inherent in any mechanism through which we expose jj internals to the outside world. The objective of jj-lib structures is to be performant, while the objective of any structures you expose is to:

  • Easily serialized and deserialized
  • Support field masking (eg. I want to be able to get just a change ID, or get a whole change, at will)
  • Stable
    • If we want to change the type of something internally to optimize performance, that shouldn't result in a change to the API
    • If we want to change the type of something externally, proto supports reserved felds, so you can go through the deprecation process:
      1. Update the proto to add the new field, fill in both fields, mark the old field as deprecated
      2. Give users some time to migrate
      3. Delete the old field, mark it as reserved. When users depending on the old proto serialize it, they will just find the field empty, so can hopefully fail gracefully.

Servicer could also become rather vast

Correct. I think it's the right approach, though. I don't think there's anything inherently wrong with it being vast.

matts1 avatar May 01 '24 06:05 matts1

Should we create a discussion/issue separate from #3219 to collect the needs of both the cli and gg (and potentially other tooling being developed right now like vim/vscode extensions) so we know more clearly what APIs will need to be exposed?

Afaict #3219 is more concerned with an API command existing, not which APIs the servicer facade will provide over all it's potential channels (directly calling it in the library, gRPC, etc.)

noahmayr avatar May 01 '24 09:05 noahmayr

I'm a bit concerned that it means each change to jj will have duplicated work - updating the internal data model, and updating the way it's exposed in the API.

This sounds wrong to me. None of the internal data model should be exposed; otherwise it's not internal. Only some of the data would be transferred back and forth, as in REST or GraphQL.

joyously avatar May 01 '24 12:05 joyously

Should we create a discussion/issue separate from #3219 to collect the needs of both the cli and gg (and potentially other tooling being developed right now like vim/vscode extensions) so we know more clearly what APIs will need to be exposed?

Sounds like a good idea to me. I assume it's not all of the current Rust API, but it's unclear to me which parts of it would be available in the RPC API. I suppose everything used by commands will need to be available, but probably not everything in cli_util.rs because we should hide more of that in the library crate. OTOH, maybe those APIs should still be available outside of the crate even there are higher-level APIs available?

@matts1, if you agree with creating a separate issue (or design doc, perhaps) for documenting the API requirements, what do you think about picking an arbitrary command and showing how the rewritten version of that would look like? For example, what would jj parallelize look like after rewriting to use the RPC API?

martinvonz avatar May 01 '24 13:05 martinvonz

I think that would be a great way to learn what shape the design would have to take.

gulbanana avatar May 01 '24 15:05 gulbanana

I am happy to see that you're so eager to see it implemented, but I would need at least two design docs before even starting this huge project. As defining the jj-lib API in terms of jj api is already a huge change and then jj api itself. See my concerns in the issue here: https://github.com/martinvonz/jj/issues/3034#issuecomment-2030300465. We've also have nothing to gain for the promised stability, as we're not nearing a 1.0 yet.

It would also mean that we could implement:

  • jj api grpc (grpc sockets)
  • jj api grpc --web (grpc over web)
  • jj api web (websocket connection)
  • jj api json (stdin / stdout json communication)
  • jj api json --pipe=/path/to/pipe

Could we please not go in that direction, imo jj api should only start a server, not define how it behaves. As this is once again duplicating work which could be covered by the templating language (#3262).

PhilipMetzger avatar May 01 '24 17:05 PhilipMetzger

I would need at least two design docs before even starting this huge project.

I agree, I implemented this to see if it was viable, try things out, see what did and didn't work, and get some feedback

As defining the jj-lib API in terms of jj api is already a huge change and then jj api itself. See my concerns in the issue here: #3034 (comment). We've also have nothing to gain for the promised stability, as we're not nearing a 1.0 yet.

You're right that there's not a large benefit to this right now. I think the main reason to do this is simply because it's going to get harder to do this as we add more things to jj.

Imo jj api should only start a server, not define how it behaves.

It feels to me like we might have a different understanding of "start a server". I agree that all jj api should do is to start a server. However, the way I see it, different clients might want to communicate with the server in different ways. For example:

  • One client might expect to communicate over gRPC
  • Another client might be working from javascript, so would prefer gRPC-web
  • Another client might not have proto/grpc support, and so would prefer to communicate over HTTP with REST/json
  • Another client might not want to have to deal with the network, because that introduces a new class of errors, so maybe they want to communicate over a linux pipe instead

How would you deal with all these situations? You say that all it should do is "bring up a server", but what kind of server? If it's gRPC, that would certainly meet my needs, but why specifically gRPC? Why not a HTTP server which communicates over json, for example? Whether they're represented as json, serialized proto, textproto, or anything else, is irrelevant to the actual API.

If I were to write let response = grpcClient.UpdateCommit(UpdateCommitRequest{change_id: "abc"}), why is that any different, fundamentally, to sending that same serialized proto over a named pipe? Or sending a json-encoded variant of that proto over a named pipe? And why should we disallow it?

matts1 avatar May 02 '24 02:05 matts1

I've started work on the design doc. For now, it just has a bunch of design questions we need answered before starting on the project, and my personal thoughts on how they stack up to each other.

Any feedback would be appreciated. I expect there's things I've missed in the comparison, and maybe there's more options I haven't considered.

matts1 avatar May 14 '24 04:05 matts1

I've started work on the design doc. For now, it just has a bunch of design questions we need answered before starting on the project, and my personal thoughts on how they stack up to each other.

Any feedback would be appreciated.

I added a bunch of questions and suggestions, to provide some feedback.

PhilipMetzger avatar May 15 '24 20:05 PhilipMetzger

I've created a prototype schema in #3869 (no functionality, just a schema).

I've added Martin and Phillip as reviewers, as they seemed the most invested in it in the design doc I wrote, but I'd welcome feedback from anyone (it's very much still a WIP though, I'm treating it more as a design doc than as a PR).

matts1 avatar Jun 13 '24 03:06 matts1