Implemente federated instances view (fixes #6063)#6089
Conversation
Nutomic
requested review from
SleeplessOne1917,
dessalines,
dullbananas and
phiresky
as code owners
Nutomic
force-pushed
the
federated-instances-view
branch
from
b96e09c to
0845654
Compare
crates/db_views/site/src/impls.rs
Outdated
|
|
||
| // Show recently updated instances and those with valid metadata first | ||
| // TODO: doesnt support keys_last() or different order per key. | ||
| // should be `key::updated_at.desc(), instance::software.asc().nulls_last()` |
There was a problem hiding this comment.
@dullbananas Would be nice if the pagination library could support this.
There was a problem hiding this comment.
Changing the null order and mixing asc/desc can't be done in cursor pagination (without possible loss of efficient index scans) until it's supported by Postgresql's comparison operators.
For this query, use reversetimestampkey (from lemmy_db_schema) for updated_at, and remove nulls_last for software because nulls are greater than non-null by default (meaning nulls_last is the default for ascending order).
There was a problem hiding this comment.
I dont find reversetimestampkey, where exactly is it?
There was a problem hiding this comment.
iirc we removed that.
TBH I think SortDirection::Desc, even for software (or null software) like you have below is fine. updated_at is already going to be unique enough.
There was a problem hiding this comment.
Okay I simply removed the comment.
| }) | ||
| } | ||
|
|
||
| impl FederatedInstanceView { |
There was a problem hiding this comment.
Here are the main changes.
Nutomic
force-pushed
the
federated-instances-view
branch
from
8bf569b to
026e38f
Compare
| } | ||
|
|
||
| impl FederatedInstanceView { | ||
| pub async fn list(pool: &mut DbPool<'_>, data: GetFederatedInstances) -> LemmyResult<Vec<Self>> { |
There was a problem hiding this comment.
I'm not super big on passing the query struct directly into the list, especially since a lot of the objects get transformed, or need to make other DB calls and checks before doing list, which should be a single list query.
Everywhere else we use either a Query object with a list method on it, or pass in all the params individually. You should probably create aFederationQuery object, especially since you need to pre-fetch cursor data. Check out crates/api/api/src/federation/search.rs for an example.
There was a problem hiding this comment.
I know its done like that in other places, and there it usually makes sense. But here is no need for any transformations, and its only called from a single place so theres no real need for an extra query struct.
crates/db_views/site/src/impls.rs
Outdated
|
|
||
| // Show recently updated instances and those with valid metadata first | ||
| // TODO: doesnt support keys_last() or different order per key. | ||
| // should be `key::updated_at.desc(), instance::software.asc().nulls_last()` |
There was a problem hiding this comment.
iirc we removed that.
TBH I think SortDirection::Desc, even for software (or null software) like you have below is fine. updated_at is already going to be unique enough.
3 participants
type: "Linked" | "Allowed" | "Blocked"