update metadata corresponding to spec edits (#2282)

* spec update: metadata keys are lowercase

* add batch parameter to metadata batches

* fix: connecting clients receive METADATA, not RPL_KEYVALUE

* spec update: send RPL_METADATASUBS in a metadata-subs batch

* move some helpers

* bump irctest to forked hash

This is progval/irctest#314 but I don't want to
couple the merges

* fix: empty value is valid

* fix: deleting a nonexistent key gets a FAIL

Author: slingamn

irc/client.go

@@ -828,7 +828,7 @@ func (client *Client) applyPreregMetadata(session *Session) {
 
 	target := client.Nick()
 	for k, v := range updates {
-		broadcastMetadataUpdate(client.server, maps.Keys(friends), session, target, k, v)
+		broadcastMetadataUpdate(client.server, maps.Keys(friends), session, target, k, v, true)
 	}
 }
 

irc/handlers.go

@@ -3195,14 +3195,15 @@ func metadataRegisteredHandler(client *Client, config *Config, subcommand string
 			// echo the value to the client whether or not there was a real update
 			rb.Add(nil, server.name, RPL_KEYVALUE, client.Nick(), target, key, "*", value)
 			if updated {
-				notifySubscribers(server, rb.session, targetObj, target, key, value)
+				notifySubscribers(server, rb.session, targetObj, target, key, value, true)
 			}
 		} else {
 			if updated := targetObj.DeleteMetadata(key); updated {
-				notifySubscribers(server, rb.session, targetObj, target, key, "")
+				notifySubscribers(server, rb.session, targetObj, target, key, "", false)
+				rb.Add(nil, server.name, RPL_KEYNOTSET, client.Nick(), target, key, client.t("Key deleted"))
+			} else {
+				rb.Add(nil, server.name, "FAIL", "METADATA", "KEY_NOT_SET", utils.SafeErrorParam(key), client.t("Metadata key not set"))
 			}
-			// acknowledge to the client whether or not there was a real update
-			rb.Add(nil, server.name, RPL_KEYNOTSET, client.Nick(), target, key, client.t("Key deleted"))
 		}
 
 	case "get":
@@ -3211,7 +3212,7 @@ func metadataRegisteredHandler(client *Client, config *Config, subcommand string
 			return
 		}
 
-		batchId := rb.StartNestedBatch("metadata")
+		batchId := rb.StartNestedBatch("metadata", target)
 		defer rb.EndNestedBatch(batchId)
 
 		for _, key := range params[2:] {
@@ -3355,6 +3356,9 @@ func metadataSubsHandler(client *Client, subcommand string, params []string, rb
 
 		subs := rb.session.MetadataSubscriptions()
 
+		batchID := rb.StartNestedBatch("metadata-subs")
+		defer rb.EndNestedBatch(batchID)
+
 		chunked := utils.ChunkifyParams(maps.Keys(subs), lineLength)
 		for _, line := range chunked {
 			params := append([]string{client.Nick()}, line...)
@@ -3364,16 +3368,6 @@ func metadataSubsHandler(client *Client, subcommand string, params []string, rb
 	return false
 }
 
-func playMetadataList(rb *ResponseBuffer, nick, target string, values map[string]string) {
-	batchId := rb.StartNestedBatch("metadata")
-	defer rb.EndNestedBatch(batchId)
-
-	for key, val := range values {
-		visibility := "*"
-		rb.Add(nil, rb.session.client.server.name, RPL_KEYVALUE, nick, target, key, visibility, val)
-	}
-}
-
 // REHASH
 func rehashHandler(server *Server, client *Client, msg ircmsg.Message, rb *ResponseBuffer) bool {
 	nick := client.Nick()

irc/metadata.go

@@ -30,7 +30,7 @@ type MetadataHaver = interface {
 	CountMetadata() int
 }
 
-func notifySubscribers(server *Server, session *Session, targetObj MetadataHaver, targetName, key, value string) {
+func notifySubscribers(server *Server, session *Session, targetObj MetadataHaver, targetName, key, value string, set bool) {
 	var recipientSessions iter.Seq[*Session]
 
 	switch target := targetObj.(type) {
@@ -48,17 +48,17 @@ func notifySubscribers(server *Server, session *Session, targetObj MetadataHaver
 		return // impossible
 	}
 
-	broadcastMetadataUpdate(server, recipientSessions, session, targetName, key, value)
+	broadcastMetadataUpdate(server, recipientSessions, session, targetName, key, value, set)
 }
 
-func broadcastMetadataUpdate(server *Server, sessions iter.Seq[*Session], originator *Session, target, key, value string) {
+func broadcastMetadataUpdate(server *Server, sessions iter.Seq[*Session], originator *Session, target, key, value string, set bool) {
 	for s := range sessions {
 		// don't notify the session that made the change
 		if s == originator || !s.isSubscribedTo(key) {
 			continue
 		}
 
-		if value != "" {
+		if set {
 			s.Send(nil, server.name, "METADATA", target, key, "*", value)
 		} else {
 			s.Send(nil, server.name, "METADATA", target, key, "*")
@@ -67,7 +67,7 @@ func broadcastMetadataUpdate(server *Server, sessions iter.Seq[*Session], origin
 }
 
 func syncClientMetadata(server *Server, rb *ResponseBuffer, target *Client) {
-	batchId := rb.StartNestedBatch("metadata")
+	batchId := rb.StartNestedBatch("metadata", target.Nick())
 	defer rb.EndNestedBatch(batchId)
 
 	subs := rb.session.MetadataSubscriptions()
@@ -81,7 +81,7 @@ func syncClientMetadata(server *Server, rb *ResponseBuffer, target *Client) {
 }
 
 func syncChannelMetadata(server *Server, rb *ResponseBuffer, channel *Channel) {
-	batchId := rb.StartNestedBatch("metadata")
+	batchId := rb.StartNestedBatch("metadata", channel.Name())
 	defer rb.EndNestedBatch(batchId)
 
 	subs := rb.session.MetadataSubscriptions()
@@ -106,7 +106,27 @@ func syncChannelMetadata(server *Server, rb *ResponseBuffer, channel *Channel) {
 	}
 }
 
-var validMetadataKeyRegexp = regexp.MustCompile("^[A-Za-z0-9_./-]+$")
+func playMetadataList(rb *ResponseBuffer, nick, target string, values map[string]string) {
+	batchId := rb.StartNestedBatch("metadata", target)
+	defer rb.EndNestedBatch(batchId)
+
+	for key, val := range values {
+		visibility := "*"
+		rb.Add(nil, rb.session.client.server.name, RPL_KEYVALUE, nick, target, key, visibility, val)
+	}
+}
+
+func playMetadataVerbBatch(rb *ResponseBuffer, target string, values map[string]string) {
+	batchId := rb.StartNestedBatch("metadata", target)
+	defer rb.EndNestedBatch(batchId)
+
+	for key, val := range values {
+		visibility := "*"
+		rb.Add(nil, rb.session.client.server.name, "METADATA", target, key, visibility, val)
+	}
+}
+
+var validMetadataKeyRegexp = regexp.MustCompile("^[a-z0-9_./-]+$")
 
 func metadataKeyIsEvil(key string) bool {
 	return !validMetadataKeyRegexp.MatchString(key)

irc/metadata_test.go

@@ -7,9 +7,12 @@ func TestKeyCheck(t *testing.T) {
 		input  string
 		isEvil bool
 	}{
-		{"ImNormal", false},
+		{"ImNormalButIHaveCaps", true},
+		{"imnormalandidonthavecaps", false},
+		{"ergo.chat/vendor-extension", false},
 		{"", true},
 		{":imevil", true},
+		{"im:evil", true},
 		{"key£with$not%allowed^chars", true},
 		{"key.thats_completely/normal-and.fine", false},
 	}

irc/server.go

@@ -499,7 +499,7 @@ func (server *Server) playRegistrationBurst(session *Session) {
 		server.RplISupport(c, rb)
 	}
 	if session.capabilities.Has(caps.Metadata) {
-		playMetadataList(rb, d.nick, d.nick, c.ListMetadata())
+		playMetadataVerbBatch(rb, d.nick, c.ListMetadata())
 	}
 	if d.account != "" && session.capabilities.Has(caps.Persistence) {
 		reportPersistenceStatus(c, rb, false)