refactor(api): aligning our API with spec #1734

Arqu · 2021-04-01T20:40:32Z

Aiming to get us closer to the API spec contained in #1731, I've refactored some of the code to move to POST only, cleaned up the route definitions and rearranged most of the routes in the same order as in our tables. The main reason for rearranging is to make the follow up steps more explicit:

Which handler methods need to move into which handler
Highlight unfinished or missing routes
Highlight routes that are no longer needed or otherwise misplaced

I'll run a round of self commenting on the PR just to add some color to it. Looking for some input before diving into the code move.

api/api.go

Arqu · 2021-04-01T21:16:33Z

api/api.go

 	}

-	dsh := NewDatasetHandlers(s.Instance, cfg.API.ReadOnly)
+	// aggregate endpoints
 	m.Handle(lib.AEList.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "dataset.list"))).Methods(http.MethodPost, http.MethodGet)
 	m.Handle(lib.AEListRaw.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "dataset.listrawrefs"))).Methods(http.MethodPost, http.MethodGet)
 	m.Handle(lib.AEPeerList.String(), s.Middleware(dsh.PeerListHandler(lib.AEPeerList.String())))


This should also merge into AEList

Isn't this "/peer/list"? It's still there in #1697.

So this particular endpoint is not the list of peers, but instead the list of datasets you have from a specific peer. Which I believe would become /list w/ the filter=ramfox in the params

Yes what @ramfox said.

/peer/list lists your peers

/list/{peer} lists datasets of a given peer and should be moved into list with the new filtering approach

Arqu · 2021-04-01T21:17:48Z

api/api.go

+
+	// dataset endpoints
+	routeParams = newrefRouteParams(lib.AEWhatChanged, false, false, http.MethodGet, http.MethodPost)
+	handleRefRoute(m, routeParams, s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "fsi.whatchanged")))


All the handleRefRoute mean that these routes include all the sugar which we should drop and lock to POST only. Only /ds/get should be an exception for now.

Correct, yes.

api/api.go

Arqu · 2021-04-01T21:20:15Z

api/api.go

+	handleRefRoute(m, routeParams, s.Middleware(dsh.SaveHandler(lib.AESave.String())))
+	routeParams = newrefRouteParams(lib.AEPull, false, false, http.MethodPost, http.MethodPut)
+	handleRefRoute(m, routeParams, s.Middleware(dsh.PullHandler(lib.AEPull.NoTrailingSlash())))
+	routeParams = newrefRouteParams(lib.AEPush, false, false, http.MethodGet, http.MethodPost, http.MethodDelete)


Push should also move into dataset?

Yes, I believe that is correct.

Arqu · 2021-04-01T21:21:04Z

api/api.go

+
+	// dataset endpoints
+	routeParams = newrefRouteParams(lib.AEWhatChanged, false, false, http.MethodGet, http.MethodPost)
+	handleRefRoute(m, routeParams, s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "fsi.whatchanged")))
 	routeParams = newrefRouteParams(lib.AEGet, false, true, http.MethodGet, http.MethodPost)
 	handleRefRoute(m, routeParams, s.Middleware(dsh.GetHandler(lib.AEGet.String())))


/ds/log is still TODO

Arqu · 2021-04-01T21:22:23Z

api/api.go

-	m.Handle(lib.AEHistory.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "log.history"))).Methods(http.MethodPost)
+	// TODO(aqru): clear up these endpoints up to spec
+
+	m.Handle(lib.AELog.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "log.history"))).Methods(http.MethodPost)


We've had a discussion around these, but I don't have all the details to spell them all out on how we will merge these/how they should behave/which we will just drop from the HTTP API.

According to #1730, rawlogbook and logbookssummary should be dropped from the API, because they're "hard-mode" only. Not totally sure about the other 2.

I believe history will be the /log endpoint with filter=history (or selector=history, whatever language we choose)

api/api_test.go

lib/fsi.go

dustmop

Comments so far

lib/datasets.go

dustmop · 2021-04-02T00:32:09Z

api/api.go

 	}

-	dsh := NewDatasetHandlers(s.Instance, cfg.API.ReadOnly)
+	// aggregate endpoints
 	m.Handle(lib.AEList.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "dataset.list"))).Methods(http.MethodPost, http.MethodGet)
 	m.Handle(lib.AEListRaw.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "dataset.listrawrefs"))).Methods(http.MethodPost, http.MethodGet)
 	m.Handle(lib.AEPeerList.String(), s.Middleware(dsh.PeerListHandler(lib.AEPeerList.String())))


Isn't this "/peer/list"? It's still there in #1697.

api/api.go

dustmop · 2021-04-02T00:34:24Z

api/api.go

+
+	// dataset endpoints
+	routeParams = newrefRouteParams(lib.AEWhatChanged, false, false, http.MethodGet, http.MethodPost)
+	handleRefRoute(m, routeParams, s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "fsi.whatchanged")))


Correct, yes.

dustmop · 2021-04-02T00:38:15Z

api/api.go

-	m.Handle(lib.AEHistory.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "log.history"))).Methods(http.MethodPost)
+	// TODO(aqru): clear up these endpoints up to spec
+
+	m.Handle(lib.AELog.String(), s.Middleware(lib.NewHTTPRequestHandler(s.Instance, "log.history"))).Methods(http.MethodPost)


According to #1730, rawlogbook and logbookssummary should be dropped from the API, because they're "hard-mode" only. Not totally sure about the other 2.

cmd/log.go

lib/api.go

dustmop · 2021-04-02T00:43:55Z

lib/datasets.go

+		// "render":            {AE??, "POST"},
+		"remove":   {AERemove, "POST"},
+		"validate": {AEValidate, "POST"},
+		// "unpack": {AE??, "POST"},


lib/fsi.go

ramfox · 2021-04-02T02:14:41Z

lib/log.go

+		"history":        {AELog, "POST"},
 		"entries":        {AEEntries, "POST"},


To be clear here, I just made this name change from /logbook to /entries for clarify, but always intended it to be temporary until we adjust to the new spec. When we do our final refactor, /entries should be /log and /history should be removed!

Awesome, and yeah I'll be keeping rebasing this as I'm trailing all the other changes.

Arqu · 2021-04-06T13:59:53Z

Majority of the refactor is done and I would like to land this before making this PR not just a refactor but a big change in the logic of some remaining methods.

A list of remaining things:
- merge log <> history
- merge list <> list/peer
- clean up handleRefRoute
- align remaining routes in correct methods
- one more run through routes vs spec

Arqu · 2021-04-08T13:17:45Z

Quick observations from the current standing of the PR:

/remote/remove vs /ds/remove we currently have both I think
/peer/remove - this exists in the spec but not in code as far as I can see
/access/token - need to bring this one back seems to be lost in the rebases - only the API handler/route binding, code is there

ramfox · 2021-04-08T15:50:16Z

hmmm re /peer/remove I'm not sure what this would do? Remove the peer from your peerstore? Otherwise, we already have the ability to disconnect.

/remote/remove and /ds/remove is tricky & i think clashes with two thought processes we have. On one hand, we specifically decided that we wanted our api to have qri remove --remote qri_remote & /remote?remote="qri_remote" when we first established removing from a remote. However, the way it's set up we switch on local vs remote at the api/cmd level. This means we would need a custom api handler function to control the flow here. Another option is combining these at the lib level. But then this opens up more questions about remote behavior, resolution, and how we separate out concerns, that we will come up against in the future when we open up the ability to do more behavior on a remote (saving, triggering runs, changing permissions).

I think the easiest thing to do rn would be to remove the remote behavior from the /ds/remove endpoint, punt the convo, separate the concerns completely, and have this discussion when we open up more remote behaviors.

ramfox · 2021-04-08T15:55:32Z

This is so lovely. Noticed that you refactored away from sql.QueryHandler, but that the api/sql.go & api/sql_test.go files wasn't removed.

b5

@Arqu this is looking great. To avoid further rebasing I vote we merge this now & do follow-ups for any subsequent fixes

Arqu added API JSON API issues Breaking Change things that change existing API contracts. labels Apr 1, 2021

Arqu requested review from b5, dustmop and ramfox April 1, 2021 20:40

Arqu self-assigned this Apr 1, 2021

Arqu commented Apr 1, 2021

View reviewed changes

ramfox mentioned this pull request Apr 1, 2021

Qri API v0.10.0 tracker #1731

Closed

29 tasks

dustmop reviewed Apr 2, 2021

View reviewed changes

ramfox reviewed Apr 2, 2021

View reviewed changes

Arqu force-pushed the arqu/api_audit branch 2 times, most recently from 9f2e5bb to 9622a06 Compare April 6, 2021 13:54

Arqu force-pushed the arqu/api_audit branch from 9622a06 to 1e54086 Compare April 7, 2021 12:03

Arqu marked this pull request as ready for review April 7, 2021 12:03

Arqu requested review from ramfox and dustmop April 7, 2021 12:03

b5 added this to the v0.10.0 milestone Apr 7, 2021

Arqu force-pushed the arqu/api_audit branch from 1e54086 to bd21bee Compare April 8, 2021 10:16

refactor(api): aligning our API with spec

2d8026f

Arqu force-pushed the arqu/api_audit branch from bd21bee to 2d8026f Compare April 8, 2021 10:44

b5 approved these changes Apr 8, 2021

View reviewed changes

Arqu merged commit 5821567 into master Apr 8, 2021

Arqu deleted the arqu/api_audit branch April 8, 2021 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(api): aligning our API with spec #1734

refactor(api): aligning our API with spec #1734

Arqu commented Apr 1, 2021

Arqu Apr 1, 2021

dustmop Apr 2, 2021

ramfox Apr 2, 2021

Arqu Apr 2, 2021

Arqu Apr 1, 2021

dustmop Apr 2, 2021

Arqu Apr 1, 2021

dustmop Apr 2, 2021

Arqu Apr 1, 2021

Arqu Apr 1, 2021

dustmop Apr 2, 2021

ramfox Apr 2, 2021

dustmop left a comment

dustmop Apr 2, 2021

dustmop Apr 2, 2021

dustmop Apr 2, 2021

dustmop Apr 2, 2021

ramfox Apr 2, 2021

Arqu Apr 2, 2021

Arqu commented Apr 6, 2021

Arqu commented Apr 8, 2021

ramfox commented Apr 8, 2021

ramfox commented Apr 8, 2021

b5 left a comment

refactor(api): aligning our API with spec #1734

refactor(api): aligning our API with spec #1734

Conversation

Arqu commented Apr 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustmop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arqu commented Apr 6, 2021

Arqu commented Apr 8, 2021

ramfox commented Apr 8, 2021

ramfox commented Apr 8, 2021

b5 left a comment

Choose a reason for hiding this comment