Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qri API v0.10.0 tracker #1731

Closed
26 of 29 tasks
ramfox opened this issue Mar 31, 2021 · 10 comments
Closed
26 of 29 tasks

Qri API v0.10.0 tracker #1731

ramfox opened this issue Mar 31, 2021 · 10 comments
Assignees
Labels
discussion Open-ended request for feedback refactor A code change that neither fixes a bug nor adds a feature
Milestone

Comments

@ramfox
Copy link
Member

ramfox commented Mar 31, 2021

Qri 0.10.0 API Tracker

TODO:

  • Dispatch-ify project
    • profile methods
    • registry methods
    • remote methods
    • render methods
    • search methods
    • sql methods
    • transform methods
    • remaining dataset methods
  • unify params 1 unify parameters on lib methods #1732
    • ListParams - struct contains Limit, Offset, All
    • RefParams - struct contains Ref, Filter2
    • RefListParams - struct contains ListParams, RefParams
    • Audit ALL lib method params, have them extend the above if they can
  • Simplify api & Remove Lingering code that does not support RPC style api
    • remove UnmarshalFromRequest?
    • convert all api handler methods to lib.NewHTTPRequestHandlers
    • align the API with the spec refactor(api): aligning our API with spec #1734
    • add Instance.GiveAPIServer() that returns a mux with all the appropriate endpoints handled, aka only the endpoints that are expected in 0.10.0 3
  • Adding back RESTful api "Sugar"
    • add GetHandler that implements RESTful api expectations
    • /ds/get/{username}/{name}/at/{path}/{component}.{encoding} w/ RefListParams
	# REST-style:
	/ds/get/{username}/{name}
	$ curl http://localhost:2503/ds/get/b5/world_bank_population
	$ curl http://localhost:2503/ds/get/b5/world_bank_population/meta
	$ curl http://localhost:2503/ds/get/b5/world_bank_population/body.csv?all=true
	$ curl http://localhost:2503/ds/get/b5/world_bank_population/body.csv?limit=25&offset=0
	$ curl -H "Accept: text/csv" http://localhost:2503/ds/get/b5/world_bank_population/body
	$ curl -H "Accept: application/zip" http://localhost:2503/ds/get/b5/world_bank_population
  • Tests!
    The tests will transition to a JSON based test framework where we have a central set of expected JSON inputs and outputs. This should make the maintenance of the tests easier as well as more portable in terms of other clients relying on them or devs navigating/reusing them.

  • Automated api docs

  • release notes

  • dependency upgrades to ipfs punting for future release because it requires a repo configuration

  • shell script in make file to update api docs

  • add building api docs into ops folder

  • update release cycle to include building from make

Questions/Footnotes:

params: this could easily be ListParams RefParams & FilterParams separately, or all one MethodParams that each method extends, would love some debate/settled spec on this point

filter/selector: is Selector or Filter the agreed upon word? I seem to remember both Filter and Selector as options, but can't remember if it was settled.

denyRPC: Footnote content goes here We denyRPC for certain endpoints. How much does that overlap with endpoints that we do not expect to have in qri api 0.10.0? It would be helpful if this was one-to-one. We could rename denyRPC to denyHTTP and use the inst.regMethods to programmatically assign the api endpoints in the new inst.GiveAPIServer method

API Spec

Sugar

The purpose of the API package is to expose the lib.RPC api and add syntatic sugar for mapping RESTful HTTP requests to lib method calls

endpoint HTTP methods Lib Method Name
"/" GET api.HealthCheckHandler
"/health" GET api.HealthCheckHandler
"/qfs/ipfs/{path:.*}" GET qfs.Get
"/qfs/s3/{path:.*}" GET qfs.Get
"/webui" GET api.WebuiHandler
/ds/get/{username}/{name} GET api.GetHandler
/ds/get/{username}/{name}/at/{path} GET api.GetHandler
/ds/get/{username}/{name}/at/{path}/{component} GET api.GetHandler
/ds/get/{username}/{name}/at/{path}/{component}.{encoding} GET api.GetHandler

RPC

The purpose of the lib pacakge is to expose a uniform interface for interacting with a qri instance

endpoint Return Type Lib Method Name
Aggregate Endpoints
"/list" []VersionInfo collection.List?
"/sql" [][]any sql.Exec
"/diff" Diff diff.Diff
"/changes" ChangeReport diff.Changes
Access Endpoints
"/access/token" JSON Web Token access.Token
Automation Endpoints
"/auto/apply" Dataset automation.apply
Dataset Endpoints
"/ds/componentstatus" []Status dataset.componentstatus
"/ds/get Dataset dataset.Get
"/ds/activity" []VersionInfo dataset.History
"/ds/rename" DSRef? dataset.Rename
"/ds/save" DSRef dataset.Save
"/ds/pull" DSRef dataset.Pull
"/ds/push" DSRef dataset.Push
"/ds/render" []byte dataset.Render
"/ds/remove" DSRef dataset.Remove
"/ds/validate" ValidateRes dataset.Validate
"/ds/unpack" Dataset dataset.Unpack
"/ds/manifest" Manifest dataset.Manifest
"/ds/manifestmissing" Manifest dataset.ManifestMissing
"/ds/daginfo" DagInfo dataset.DagInfo
Peer Endpoints
"/peer" Profile peer.Info
"/peer/connect" Profile peer.Connect
"/peer/disconnect" Profile peer.Disconnect
"/peer/list" []Profile peer.Profiles
Profile Endpoints
"/profile" Profile profile.GetProfile
"/profile/set" Profile profile.SetProfile
"/profile/photo" Profile profile.ProfilePhoto
"/profile/poster" Profile profile.PosterPhoto
Remote Endpoints
"/remote/feeds" Feed remote.Feeds
"/remote/preview" Dataset remote.Preview
"/remote/remove" - remote.Remove
"/remote/registry/profile/new" Profile registry.CreateProfile
"/remote/registry/profile/prove" Profile registry.ProveProfile
"/remote/search" SearchResult remote.Search
Working Directory Endpoints
"/wd/status" []StatusItem fsi.Status
"/wd/init" DSRef fsi.Init
"/wd/caninitworkdir" -- fsi.CanInitworkdir
"/wd/checkout" -- fsi.Checkout
"/wd/restore" -- fsi.Restore
"/wd/write" []StatusItem fsi.Write
"/wd/createlink" VersionInfo fsi.CreateLink
"/wd/unlink" string fsi.Unlink
"/wd/ensureref" -- fsi.EnsureRefNotLinked

"Protocol Endpoints"

Sync Endpoints
"/sync/dag" GET *1 dag.Info remote.DsyncHTTPHandler
"/sync/dag" GET *2 []byte remote.DsyncHTTPHandler
"/sync/dag" POST dag.Manifest remote.DsyncHTTPHandler
"/sync/dag" PUT -- remote.DsyncHTTPHandler
"/sync/dag" PATCH stream? remote.DsyncHTTPHandler
"/sync/logs" GET -- remote.LogbookHTTPHandlery
"/sync/logs" PUT []byte remote.LogbookHTTPHandlery
"/sync/logs" DELETE -- remote.LogbookHTTPHandlery
"/sync/resolve" GET DSRef remote.RefsHTTPHandler
"/sync/resolve" DELETE -- remote.RefsHTTPHandler
@ramfox ramfox self-assigned this Mar 31, 2021
@ramfox ramfox added discussion Open-ended request for feedback refactor A code change that neither fixes a bug nor adds a feature labels Mar 31, 2021
@ramfox ramfox added this to the v0.10.0 milestone Mar 31, 2021
@c-nv-s
Copy link

c-nv-s commented Apr 1, 2021

Just curious... will there eventually be an endpoint for profile/user creation?

@Arqu
Copy link
Contributor

Arqu commented Apr 1, 2021

@c-nv-s depending on the context you had in mind when asking there are a couple possible answers:

  • If you mean "signing up for cloud" from a local node, then /remote/registry/profile/new would cover that usecase
  • If you mean creating a profile locally, typically running qri setup (and variations with flags) will allow you to create local profiles, for which there currently is no HTTP endpoint and unless there's a concrete need probably won't be in the short term
  • The third thing hovering around currently is adding multi-tenancy support to qri which would mean many profiles under a single node. Once we get there I do see some HTTP endpoints being surfaced to support some GUI clients to manage all that.

Hope this helps, let me know if I went a bit sideways here.

@dustmop
Copy link
Contributor

dustmop commented Apr 1, 2021

Dispatch for sql was implemented by #1729

Dispatch for transform was implemented by #1691

@dustmop
Copy link
Contributor

dustmop commented Apr 1, 2021

For lib method parameter unification, I'm going to file an issue to track it separately.

@c-nv-s
Copy link

c-nv-s commented Apr 1, 2021

@c-nv-s depending on the context you had in mind when asking there are a couple possible answers:

* If you mean "signing up for cloud" from a local node, then `/remote/registry/profile/new` would cover that usecase

* If you mean creating a profile locally, typically running `qri setup` (and variations with flags) will allow you to create local profiles, for which there currently is no HTTP endpoint and unless there's a concrete need probably won't be in the short term

* The third thing hovering around currently is adding multi-tenancy support to qri which would mean many profiles under a single node. Once we get there I do see some HTTP endpoints being surfaced to support some GUI clients to manage all that.

Hope this helps, let me know if I went a bit sideways here.

@Arqu Thanks for the response. Yes I was thinking more along the lines of your second and third bullet points.
If someone is keen on creating separate profiles for different types of projects or development environments then it would be useful to be able to automate that via api too, instead of having to run 'qri setup' each time.
Regardless, thanks for this super project guys.

@Arqu
Copy link
Contributor

Arqu commented Apr 1, 2021

Glad that helps. For handling multiple profiles right now you have to juggle multiple qri repos. There's a handy npm package to do it called qriswitch and some fancy shell scripting should help get around the automation piece, but hopefully that goes away very soon.

@ramfox
Copy link
Member Author

ramfox commented Apr 6, 2021

Question about /push
Right now, we have /push do 3 things depending on the http method

  1. GET - returns a list of your datasets on the given remote as []VersionInfo
  2. POST - expected behavior, pushes a dataset to a remote
  3. DELETE - removes a dataset from a remote

Seems like we need to add a few more endpoints to our spec to cover cases 1 & 3 or at least make sure they are covered under other endpoints.

Should we add /remote/list and /remote/remove, or should these be folded into /list and /ds/remove?


question moved to issue #1744

@ramfox
Copy link
Member Author

ramfox commented Apr 8, 2021

For our records, the outline and notes from the 0.10.0 meeting!

0.10.0 April 8th meeting

Topics

  1. Outstanding 0.10.0 endpoint questions
  2. POST JSON api
  3. Does this belong in 0.10.0 or 0.11.0 (or 0.10.1)?
  4. Known work

Outstanding 0.10.0 questions

  1. "Running list of http RPC bugs"
    Running list of http RPC bugs #1721
    the issue only talks about /get, but here is the full breakdown of non NewHttpRequestHandler conforming endpoints.

save

  • returns a dataset ref rather than a dataset (that comes back from dispatch)
    • Save should return a *Dataset
  • adjusts the bodypath to be relative rather than absolute
  • returns a "scriptOutput" along side the ref as a message
    OKAY TO NewHTTPRequestHandler-ize

remove

  • attempts to remove from a remote! this brings up more existential questions about source & remote
  • then this can be easily converted to NewHTTPRequestHandler

OKAY TO NewHTTPRequestHandler-ize

get

  • if result format json or Dataset.Structure.Format is json
    • inlines scripts to bytes
    • if there is a selector (any selector selector != ""), returns a lib.DataResponse w/ the body as the data
    • otherwise, converts the dataset to a DatasetRef & returns
  • any other format
    • returns entire dataset as Content-Disposition: attachment

lib.Get return an interface w/ a type?
default -> json dataset
request a component -> w/ json returns a json response
request a component -> no json, plain bytes
request a zip -> returns raw bytes as attachment

GetResult -> returns a ref, returns a byte slice
Perhaps instead GetResult returns an interface{} and a type (how to interpret the interface{})
type field v/ strongly typed
Mix between serialization formats and response shapes
get w/selector and serialization format, give me the meta as a zip, how does that interact (right now, if you have zip then you return the whole dataset)
Add validation -> selector + "zip", don't let it happen
We have other serialization formats that we support, how do we handle?

type GetResult struct {
    res interface{}
    type GetResultType
    format GetSerializationType // ?
}

asmir this is your problem nowwwwwhahahahah

PeerList

  • is this even an endpoint we are supporting?
    Should be absorbed into the normal list with a peer filter

Pull
dataset.Pull returns a dataset.Dataset, but the PullHandler returns a reporef.DatasetRef, are we a-okay to just return a dataset.Dataset
should only return dsref.Ref

UnpackHandler

  • this isn't even associated with a lib method, it just reads the body of the request, unzips it, marshals it and sends to back as json. Should this be converted to a lib method? Or should it stay in the api as sugar?
    stays as sugar

SQL done in aligning PR

  • has special "returnWithSQLResponse". If the requested format is not json (not the content header type, specifically a field on the lib.SQLQueryParams) it returns Content-Type: text/plain. Should we just assume the return type is json?
  • looks like this change is already made in the aligning PR

ApplyHandler can just be converted to use NewHTTPRequestHandler done in aligning pr

FSI status whatchanged init, remove handleRefRoute/newrefRouteParams done in aligning PR
DATASET validate manifest daginfo remove handleRefRoute/newrefRouteParams done in aligning PR

Remote all http methods that are not http.MethodPost

  1. "merging log and history endpoints"
    Merging log and history endpoints #1743
    Need to settle this convo

History of a dataset should encapsulate a log of commits & the automation history

/history needs to be able to return a slice of a variety of things

asmir:
/ds/history rename to /ds/activity and this handles userland requests
and /ds/log is just raw log and you do your thing

don't need to have /ds/activity return a heterogenous type yet
rename /ds/history to /ds/activity & remvoe /ds/log as an api endpoint

  1. /ds/remove & /remote/remove, keep separate or merge? Answer: keep separate

  2. Are there endpoints we've spoken about that have not ended up documented in the tracker issue? Qri API v0.10.0 tracker #1731

Are we married to or just dating a POST/JSON api?

  1. "Dispatch should be able to call http rpc using GET in addition to POST" dispatch should be able to call http rpc using GET in addition to POST #1658
    Should be closed w/ comment, we are only supporting rpc POST

  2. "mux.Vars and other details should be abstracted away from lib"
    mux.Vars and other details should be abstracted away from lib #1696
    This issue was written when we were still in GET world, is it still relevant?

  3. "How to get the http client to add a ref to the given url, setting the stage for the option to GET over http rpc"
    How to get the http client to add a ref to the given url, setting the stage for the option to GET over http rpc #1720
    The issue we resolved/decided to commit to http rpc speaking in only POST/JSON w/ GET functionality as sugar added to the api level. Should be closed when 0.10.0 spec is met

  4. We still use UnmarshalParams in NewHTTPRequestHandler. Since this only applies to POST/JSON situations, we should just default to using json.NewDecoder in NewHttpRequestHandler. Do we need to keep theUnmarshalParams func and should we be removing UnmarshalFromRequest methods from params. Should those move to the api level?

  • strip away any that are not in use, but if GetParams, for example, is being used as an Unmarshaller.
  1. "Remove Support for readOnly API mode, use auth tokens for privileged access"
    Remove Support for readOnly API mode, use auth tokens for privileged access #1723
    We should remove all readOnly endpoints

Does this belong in 0.10.0 or 0.11.0 (or 0.10.1)

remove 0.10.1 & only use 0.11.0 for now

  1. "scope needs additional data in order to properly scope the method call"
    scope needs additional data in order to properly scope the method call #1659
    mentions abstracting the "Owner" of the ProfileStore as scope.Owner. Should this be included in our current refactoring? We've been relying on scope.ActiveProfile. Assuming scope.Owner is the actual repo profile, where scope.ActiveProfile is the profile of the user making the request, which may or my not also be the Owner

  2. "dispatch should know about read-only vs read-write methods"
    dispatch should know about read-only vs read-write methods #1660
    Should the bones of this be implemented now? Or is this a post 0.10.0 problem

  3. "internal data structures should serialize to disk after dispatch completes"
    internal data structures should serialize to disk after dispatch completes #1661
    Mentioned in pr refactor(config): adjust ConfigMethods to use dispatch #1694 when calling scope.Loader().ChangeConfig(cfg). Also can see this playing out in the ProfileMethod path, if we "prove" our profileID, we re-write our logbook calling scope.SetLogbook(book)

  4. "remove repo from FSISubsystem, use scope instead"
    remove repo from FSISubsystem, use scope instead #1665
    tbh this is just here for completion sake, I don't know that it directly relates to our 0.10.0 goals

  5. "event bus should be bound to scopes"
    event bus should be bound to scopes #1666
    This very helpfully has a 0.10.1 tag. However we have two "future" api tags floating around now, 0.11.0 & 0.10.1, just want to make sure we a) want two tags & if so b) are specific about when we use which

  6. "list can be scoped to a single user, to view only their collection"
    feat: list can be scoped to a single user, to view only their collection #1683
    mark for later release?

  7. "want to clean up the dsref.Loader and lib/load.go situation"
    want to clean up the dsref.Loader and lib/load.go situation #1704
    Even after this round of dispatchifying, I know we don't have a unified way to load a dataset. To load a dataset, lib.Get, lib.Validate, lib.Stats use scope.ParseAndResolveRefWithWorkingDir & then separately scope.LoadDataset, while others use scope.ParseResolveFunc. There is no verison of scope.ParseResolveFunc that also adds the fsi subsystem, so this seems necessary, but wanted to point out that there is a pattern difference. Is this clean up 0.10.0 material, or should it be pushed

This also ties into some questions we need to answer around /ds/remove and /remote/remove. What's the right way to resolve a ref. Is it always via configuration, or does param.Remote ever make sense. Feels like we should have one way of doing things. If not, what's the dividing line? Is it different when you are explicitly telling the remote to do something for you rather than receiving something. Why do we have /remote/remove separate? In the future, when its possible to tell a remote to save a version or run an update, should those fold into /ds/save etc?

0.10.0 work that is not mentioned elsewhere but is happening

  1. "unify parameters on lib methods"
    unify parameters on lib methods #1732
    Okay this is definitely 0.10.0 work, Expectation is every lib method param will conform to these specifications

  2. "unify list aggregation fields"
    Unify list aggregation fields #1737
    Similar to unify parameters on lib methods #1732
    Issue is scoped to defining (not implementing) filter & orderBy, but having all aggregation. methods use the proposed fields.

  3. t-t-t-tests

@ramfox
Copy link
Member Author

ramfox commented Apr 13, 2021

slim notes from our meeting today:

notes 04/13/2021 API 0.10.0 checkin

TODO

  • tests - body checking WIP (per b5, no need to test body)
  • /get use fill struct for unmarshaling and be strict about params
  • handle get multi responses / unify response structure for get
  • one last pass to make sure API == spec
  • template/structure based body test result checking - open issue
  • removed DsCache flag until we have better configuration
  • Need open api doc file

@b5
Copy link
Member

b5 commented May 10, 2021

v 0.10.0 is out! closing

@b5 b5 closed this as completed May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open-ended request for feedback refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

No branches or pull requests

5 participants