Prototype for jj api
#3601
Prototype for jj api
#3601
Conversation
This crate contains only the minimal set of code for an external library to start using it.
To try this out, run: cargo run api grpc --port 8888 Then run cargo run api_client
|
|
||
| message ListWorkspacesRequest { | ||
| jj_api.objects.RepoOptions repo = 1; | ||
| google.protobuf.FieldMask field_mask = 2; |
There was a problem hiding this comment.
I think that field masks would be really handy because sometimes you only want change IDs, while sometimes you want full changes. Unfortunately, although the type exists, there's currently no way to actually do anything based on these field masks in rust.
| @@ -0,0 +1,25 @@ | |||
| use std::path::{Path, PathBuf}; | |||
There was a problem hiding this comment.
Serialization and deserialization are a pain here. I wrote a bunch of wrappers because it gets to be a massive pain in the ass since protos are represented as a string, but in reality the type we usually want is Option<String>, Option<Path>, Option<ChangeID>, etc.
I'd like to see if we can think of a better way to do it, but I'm not hopeful.
Maybe we need to have every proto message have its own rust type, and implement
impl From<Change> for ChangeProto { ... }
impl TryFrom<ChangeProto> for Change {
type Error = tonic::Status,
....
}
| } | ||
|
|
||
| #[tonic::async_trait] | ||
| impl JjService for GrpcServicer { |
There was a problem hiding this comment.
This is mostly just to get around the fact this requires async functions, while I intend for the jj CLI to call the sync functions directly.
| } | ||
|
|
||
| #[tokio::main(flavor = "current_thread")] | ||
| pub async fn start_api( |
There was a problem hiding this comment.
I don't really like the fact that the GRPC server is started in the jj_lib crate rather than the jj_cli crate. However, I anticipate that:
- The jj daemon will likely need to start this server
- My proposal for generalized hook support (FR: Generalized hook support #3577) will require jj_lib to ensure that an API server is running.
Happy to move this to jj_cli if we don't think that's the case.
| opts: &Option<jj_api::objects::RepoOptions>, | ||
| ) -> Result<WorkspaceLoader, Status> { | ||
| opts.as_ref() | ||
| .map(|opts| from_proto::path(&opts.repo_path)) |
There was a problem hiding this comment.
This is a lot of work to just get one field, which is why I'm considering a mechanism where we just have:
grpc_servicer.rs would write:
// servicer.rs
fn list_workspaces(TypeSafeListWorkspacesRequest) -> Result<TypeSafeListWorkspacesResponse, Status> {
...
}
// grpc_servicer.rs
async fn list_workspaces(
&self,
request: Request<ListWorkspacesRequest>,
) -> Result<Response<ListWorkspacesResponse>, Status> {
self.servicer
.list_workspaces(request.get_ref().try_into()?)
.to_proto()
.map(Response::new)
}
| }) | ||
| } | ||
|
|
||
| fn repo( |
There was a problem hiding this comment.
I suspect that we may be able to take advantage of caching to improve performance of functions like this. Seems like work for later though
|
some interesting stuff here. you've noted that the idea is that eventually the |
|
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? |
|
The intention was that all communication would be done via the This would mean that It would also mean that we could implement:
My was to ensure that we didn't lock ourselves into gRPC here, and so I made that intermediary layer |
|
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. |
|
No, it would mean that each CLI command can be constructed out of some combination of servicer methods. For example, 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}})
} |
|
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.
|
+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 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:
Correct. I think it's the right approach, though. I don't think there's anything inherently wrong with it being vast. |
|
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.) |
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. |
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 @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 |
|
I think that would be a great way to learn what shape the design would have to take. |
|
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
Could we please not go in that direction, imo |
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
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.
It feels to me like we might have a different understanding of "start a server". I agree that all
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 |
|
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. |
I added a bunch of questions and suggestions, to provide some feedback. |
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:
CHANGELOG.md