Conversation
tipogi
added
🐞 bug
tipogi
requested review from
aintnostressin and
ok300
and removed request for
ok300
|
|
||
| /// If a referenced user is unknown, not ingested in the graph yet, resolves their homeserver | ||
| /// and persists the user node in the graph. | ||
| pub async fn maybe_ingest_for_user(user_id: &str) -> ModelResult<()> { |
There was a problem hiding this comment.
| pub async fn maybe_ingest_for_user(user_id: &str) -> ModelResult<()> { | |
| #[tracing::instrument(name = "user.ingest", skip_all)] | |
| pub async fn maybe_ingest_for_user(user_id: &str) -> ModelResult<()> { |
The tracing annotation was not moved together with the method.
| pub async fn ingest_user_handler(Path(user_id): Path<String>) -> Result<()> { | ||
| debug!("PUT {INGEST_USER_ROUTE}, user_id:{user_id}"); | ||
|
|
||
| PubkyId::try_from(&user_id) |
There was a problem hiding this comment.
Why not keep this PubkyId validation? It fails early, in case API is called with malformed PK.
| /// ### Arguments | ||
| /// | ||
| /// - `referenced_post_uri`: The parent post (if current post is a reply to it), or a reposted post (if current post is a Repost) | ||
| pub async fn maybe_ingest_for_post(referenced_post_uri: &ParsedUri) -> ModelResult<()> { |
There was a problem hiding this comment.
Before, "ingesting" meant "ingesting a HS".
Now with this PR, "ingesting" means "ingesting a user".
Then maybe_ingest_for_post becomes a bit confusing: what does it mean to ingest a user for a post?
Therefore I would rename maybe_ingest_for_post -> maybe_ingest_author_of_post.
(A)
|
|
||
| /// If a referenced user is unknown, not ingested in the graph yet, resolves their homeserver | ||
| /// and persists the user node in the graph. | ||
| pub async fn maybe_ingest_for_user(user_id: &str) -> ModelResult<()> { |
There was a problem hiding this comment.
With the new meaning (see (A)), I would rename maybe_ingest_for_user -> maybe_ingest_user.
| OperationOutcome::MissingDependency => { | ||
| if let Err(e) = Homeserver::maybe_ingest_for_user(followee_id.as_str()).await { | ||
| if let Err(e) = UserDetails::maybe_ingest_for_user(followee_id.as_str()).await { | ||
| tracing::error!("Failed to ingest homeserver: {e}"); |
There was a problem hiding this comment.
| tracing::error!("Failed to ingest homeserver: {e}"); | |
| tracing::error!("Failed to ingest user: {e}"); |
| OperationOutcome::MissingDependency => { | ||
| if let Err(e) = Homeserver::maybe_ingest_for_user(tagged_user_id.as_str()).await { | ||
| if let Err(e) = UserDetails::maybe_ingest_for_user(tagged_user_id.as_str()).await { | ||
| tracing::error!("Failed to ingest homeserver: {e}"); |
There was a problem hiding this comment.
| tracing::error!("Failed to ingest homeserver: {e}"); | |
| tracing::error!("Failed to ingest user: {e}"); |
| let hs_id = &hs_pk.into_inner().to_z32(); | ||
|
|
||
| user_details | ||
| .put_to_graph() |
There was a problem hiding this comment.
maybe_ingest_for_user only writes the new UserDetails to graph.
I think we should add it to the index, too, by adding
user_details.put_index_json(&[user_id], None, None).await?;right after put_to_graph.
| Ok(()) | ||
| } | ||
|
|
||
| /// If a referenced post is hosted on a new, unknown homeserver, this method triggers ingestion of that user. |
There was a problem hiding this comment.
| /// If a referenced post is hosted on a new, unknown homeserver, this method triggers ingestion of that user. | |
| /// If a referenced post is authored by a new, unknown user, this method triggers ingestion of that user. |
There was a problem hiding this comment.
Looks like there is some duplication between
nexus-watcher/tests/homeserver/active_homeservers.rsnexus-watcher/tests/user_ingestion/active_homeservers.rs
I'd say they should be consolidated into the first path.
2 participants
Pre-submission Checklist
cargo nextest run.cargo bench -p nexus-webapiWhat this PR fixes
Wrong thing was persisted for unknown users (living outside the default homeserver). When an event referenced a user Nexus did not have yet, the code resolved their homeserver and called
Homeserver::persist_if_unknown, so Nexus mainly created/stored a homeserver in the graph and cache. The referenced user still was not represented as an ingested User in the graph in that path.This PR
Ingests the user (minimal profile) when they are unknown in the graph, writes the
USER_HS_CURSORmapping in Redis, and aligns the bootstrap route naming/docs with user ingestion.