feat: universal tags with app-specific paths#777
Conversation
f00aeb6 to
61d5705
Compare
ae50791 to
e515926
Compare
e515926 to
af41082
Compare
| pagination: Pagination, | ||
| order: SortOrder, | ||
| sorting: &ResourceSorting, | ||
| tags: &Option<Vec<String>>, |
There was a problem hiding this comment.
NIT: This forces callers to construct and own an Option<Vec<String>>. The function only reads the contents.
tags: Option<&[String]>,There was a problem hiding this comment.
Agreed, Option<&[String]> is more idiomatic. But the change ripples through get_resource_keys, get_resource_keys_from_graph, can_use_index, build_index_key, and the webapi caller deserialization. Tracking as follow-up to keep this diff focused on the review fixes.
|
|
||
| /// Determines whether a query can be satisfied by a pre-computed Redis sorted set. | ||
| fn can_use_index( | ||
| _sorting: &ResourceSorting, |
There was a problem hiding this comment.
If _sorting isn't used at all in the function, why not remove it as an arg?
| .param("tag_id", tag_id) | ||
| /// Deletes a tag relationship created by a user and retrieves relevant details about the tag's target. | ||
| /// When `app` is Some, filters by app to prevent cross-app deletion for Resource tags. | ||
| /// When `app` is None, behaves as before (Post/User tags have no app property). |
There was a problem hiding this comment.
When
appis None, behaves as before (Post/User tags have no app property).
Judging by the query DELETE_TAG_WITHOUT_APP below, this is not true.
Providing no app as arg will end up deleting that tag of any app, because the query won't filter by app. It doesn't filter for "app must be NULL".
There was a problem hiding this comment.
This behavior is intentional. The None path is used by two callers:
- Standard event flow (
events/mod.rs): Forpubky.apptags, these TAGGED relationships have noappproperty, so no filter is needed. - InternalKnown DEL fallback (
handle_del): When a tag at/pub/mapky/tags/TAG_IDtargets a Post/User,sync_put_resourcedelegates to the standard flow, which creates a TAGGED relationship WITHOUTapp. On DEL,handle_deltriesSome("mapky")first (no match), then falls back toNone, correctly matching the app-less relationship.
Resource tags created via the universal flow always have app set and are deleted via the Some(app) path. There's no collision risk because tag_id = blake3(uri:label) is deterministic per URI, a Post tag and a Resource tag would have different URIs and therefore different tag_ids.
Updated the doc comment to clarify this reasoning.
| } | ||
| } | ||
|
|
||
| fn normalize_parsed_url(parsed: &Url) -> String { |
There was a problem hiding this comment.
NIT: Wouldn't it make more sense to move this to pubky-app-specs and call it there too, on the uri target of a tag?
There was a problem hiding this comment.
Agreed this is the right long-term home, we need to move this to pubky-app-specs, needs release cycle and cross-repo coordination. How abou keeping in nexus-common for now; and proposing a migration to pubky-app-specs as a separate effort to ease this PR going trough?
|
|
||
| // Second-chance: try handling as universal tag at app-specific path | ||
| if maybe_event.is_none() { | ||
| if let Some(result) = crate::events::handlers::universal_tag::try_handle( |
There was a problem hiding this comment.
This can be incorporated into Event::parse_event. Conceptually its an extension of the event parsing logic, not of the event line processing (this fn).
That would
- remove the need for these 5-6 extra levels of nesting (i.e. hard-to-reason-about logic)
- remove the need for
parse_event_linewhich duplicates the event parsing steps
There was a problem hiding this comment.
Agree it would reduce nesting. The tradeoff: Event::parse_event lives in nexus-common and uses ParsedUri::try_from from pubky-app-specs, which only recognizes pubky.app paths. Adding universal tag fallback there would couple nexus-common to app-specific path parsing logic that currently lives cleanly in nexus-watcher. The second-chance pattern keeps the concern separated. Worth revisiting when pubky-app-specs gains native support for app-specific paths (which would make ParsedUri::try_from handle them directly).
There was a problem hiding this comment.
How about #782 ?
It addresses the nesting issue I raised, but avoids the coupling risk you mentioned.
Summary
universal_tags_specs.md specification
/pub/<appname>/tags/TAG_IDare now indexed as generic Resourcenodes, enabling cross-app tag aggregation without forking Nexus
by source app (e.g.,
?app=mapky,?app=eventky)What changed
nexus-common (models + graph):
resource/module: URI normalization, BLAKE3 resource_id, classify_uri, ResourceDetails, TagResource, ResourceStream with 8 Redis sorted set indexesnexus-watcher (event processing):
*/tags/*and processes the tagnexus-webapi (API):
GET /v0/resource/:resource_id/tags, tag details with WoT supportGET /v0/resource/by-uri?uri=..., lookup by raw URI (normalizes internally)GET /v0/resource/:resource_id/tags/:label/taggers, tagger listGET /v0/stream/resources?app=mapky&tags=bitcoin&sorting=taggers_count, resource feedGET /v0/stream/resources/ids, cursor-paginated resource IDsnexusd (migration):
Test plan