qri-io / qri

you're invited to a data party!
https://qri.io
GNU General Public License v3.0
1.11k stars 66 forks source link

feat(collection): `List` should allow for ordering by "name", "updated" & "created" #1974

Closed ramfox closed 2 years ago

ramfox commented 2 years ago

We want the /list endpoint to replace the dataset_summary/{username} endpoint on cloud that returns a list of that user's datasets. In order to match the behavior we need the list to be able to be sorted.

The expected sorting behavior on frontend is updated (which is the default) & name. The cloud endpoint dataset_summary/{username} allows sorting by created (default) & name. In the collection.localSet (our core implementation of collection.Set), we need to examine the list params & order the list response before slicing for limit & offset. Let's allow for the union of options: update, created, & name.

Arqu commented 2 years ago

Quick question on this (or I'll just wait and have you take lead on this):

Either way, think it's time we implement this as a common pattern on core for all future use and embed it nicely into endpoints. On the wire we serialize and POST everything so it's not a huge issue but often these endpoints with ordering and filtering are supposed to be GET endpoints and would be good we have the query param serialization there. Also it's good to do it now on the list params while we don't have many dependencies on it as I think a raw string array is not the best representation.

Also, take a look at qri-io/qri/api/util/order.go for an existing implementation of some bits.

ramfox commented 2 years ago

I full agree that we need to get more specific and structured in our order, and the rest of this comment are some thoughts I've had about ways that these params should be refactored. However, I think we should hold off until the initial cursor work is finished, since some of that work depends on the lib.ListParams (which I feel needs a re-think), & for now just do the above work to get the collection do some basic sorting.

Opinions about lib.ListParams, params.List, util.Order: Visually I prefer the latter ?orderBy[name]=ASC&orderBy[created]=DESC, lmk if you have a strong opinion about this.

Def agree, our params.List needs a more specific OrderBy field. We can move the api/util/order implementation of Order into the base/params package & have OrderBy be a slice of params.Orders

Another confusing layer is that we have the lib.ListParams struct, that is embedded in the ActivityParams the way that I image params.List should be being used instead. Makes sense to me that we should remove lib.ListParams in favor of a new lib.CollectionParams (since that is the only lib method that directly uses the lib.ListParams), that embeds params.List.

Then the params.List struct should have a method UnmarshalFromRequest to satisfy the RequestUnmarshaller interface. In any get endpoint that uses listing (and since our lib packages only uses POST requests, this would be isolated to any sugar endpoints in the api package), we would need to call UnmarshalParams to correctly parse the list params from the request.

ramfox commented 2 years ago

After working through this, there is no reliable way to sort by "Created" in the core implementation of Collection, without dipping down into the logbook anytime a dataset gets added. Since this violates the current "standard" way we add to the collection (by pulling from the payload of events) and would mean that any Collection needs access to the logbook, as well as potentially adding a new field Created to the VersionInfo, I am holding off on doing this work until I get some feedback.

Some things to think about if we do move ahead with adding Created:

Arqu commented 2 years ago

Lots of stuff here, I'm just going to single out relevant points:

Visually I prefer the latter ?orderBy[name]=ASC&orderBy[created]=DESC, lmk if you have a strong opinion about this.

  • Agree, feels a lot more "strongly typed" and since we're at it why not do it in the less hacky way. Also this gets much easier once we do the UnmarshalFromRequest this seems like something cloud may have an easier time "migrating" and adding to the collection, but would need to check in with Asmir.
  • Yep, cloud is primed for this and already has an "internal" created field, it's just not loaded into VersionInfo as such. By default the endpoint currently sort's by updated

For the rest/implementation details I pretty much agree with all of it. Very much so on the separation of lib.ListParams and cleaning up it's usage.