Stop a null tag refusing to store the post it is on

Reported from a real migration falling over on NOT NULL for post_tags.tag.

Not a post with no tags -- a missing tags field, a null one and an empty list all
stored fine already.  It is a null *element* inside the array, which is what
Trog::DataModule::_process() left behind: it pushed a post's visibility into its
tags without defaulting it first, so every post written before that defaulting
carries a null in there.  A whole site's migration then stopped on the one
constraint, over a tag that was never a tag.

The index now leaves those out.  A null tag is one no query ever matches -- the
flat file model's filter compares it with // '' and it matches nothing -- so
leaving it out of the index changes no answer, while indexing it refused to store
the post at all.  The blob keeps whatever it had, so the way back is unaffected.

The more useful half: every trigger is now dropped and recreated when the schema
is applied, rather than created IF NOT EXISTS.  A trigger is code, and the schema
is applied on every connect, but IF NOT EXISTS meant a database that already had
one kept its old copy for good -- so this fix would have reached only sites that
had not migrated yet, which is precisely not the site that needs it.  Reapplying
now updates them, so an existing database is fixed by the next worker start.

Also make migrate.pl retry a failed batch one post at a time.  A batch is one
transaction, so a single bad post took the other four hundred and ninety nine
down with it and the run reported five hundred failures.  A migration wants
everything that can land to land, and a list naming exactly what could not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6Q1gRJ2Rr85S8FQ47EjnQ

bin/migrate.pl

@@ -126,12 +126,27 @@ sub flush_batch () {
         1;
     } or do {
         my $error = $@;
-        $failed += scalar(@batch);
-
-        # The batch was a transaction, so none of it landed.  Name what was in
-        # it, or the operator has an error and no idea which posts to look at.
-        say STDERR "Could not write a batch of " . scalar(@batch) . " post version(s): $error";
-        say STDERR "  the batch held: " . join( ', ', map { "$_->{id}\@" . ( $_->{version} // 0 ) } @batch );
+        $error =~ s/\n.*//s;
+
+        # A batch is one transaction, so one bad post took every other post in
+        # the batch down with it.  Go back over them one at a time: a migration
+        # wants everything that can land to land, and a list naming exactly what
+        # could not -- not five hundred posts rejected on account of one.
+        say STDERR "A batch of " . scalar(@batch) . " failed ($error); retrying it one post at a time.";
+
+        foreach my $post (@batch) {
+            local $@;
+            eval {
+                $to->write( [$post] );
+                $written++;
+                1;
+            } or do {
+                my $why = $@;
+                $why =~ s/\n.*//s;
+                $failed++;
+                say STDERR "  could not write $post->{id} version " . ( $post->{version} // 0 ) . ": $why";
+            };
+        }
     };
 
     @batch = ();

schema/sqlite.schema

@@ -62,10 +62,16 @@ CREATE TABLE IF NOT EXISTS post_versions (
 CREATE INDEX IF NOT EXISTS post_versions_created ON post_versions(first_created);
 CREATE INDEX IF NOT EXISTS post_versions_latest  ON post_versions(latest_id);
 
+/* Every trigger below is dropped and recreated rather than created IF NOT       */
+/* EXISTS.  The schema is applied on every connect, and a trigger is code: with   */
+/* IF NOT EXISTS a database that already had one kept its old copy for good, so   */
+/* fixing a trigger would have fixed it only for sites that had not run yet.      */
+
 /* Versions are only ever appended, so an insert can settle this incrementally.  */
 /* Every expression in DO UPDATE sees the row as it was before this statement,   */
 /* which is what makes the comparisons below read the way they do.               */
-CREATE TRIGGER IF NOT EXISTS posts_versions_insert AFTER INSERT ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_versions_insert;
+CREATE TRIGGER posts_versions_insert AFTER INSERT ON posts BEGIN
     INSERT INTO post_versions (uuid, version_min, version_max, first_id, latest_id, first_created)
         VALUES (new.uuid, new.version, new.version, new.id, new.id, new.created)
     ON CONFLICT(uuid) DO UPDATE SET
@@ -78,7 +84,8 @@ END;
 
 /* Deletes take whole posts rather than single versions, and are rare, so this  */
 /* recomputes rather than trying to unpick the increments above.                */
-CREATE TRIGGER IF NOT EXISTS posts_versions_delete AFTER DELETE ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_versions_delete;
+CREATE TRIGGER posts_versions_delete AFTER DELETE ON posts BEGIN
     DELETE FROM post_versions WHERE uuid = old.uuid;
     INSERT INTO post_versions (uuid, version_min, version_max, first_id, latest_id, first_created)
         SELECT old.uuid,
@@ -113,13 +120,23 @@ CREATE TABLE IF NOT EXISTS post_tags (
 CREATE INDEX IF NOT EXISTS post_tags_tag ON post_tags(tag, post_id);
 CREATE INDEX IF NOT EXISTS post_tags_post ON post_tags(post_id);
 
-CREATE TRIGGER IF NOT EXISTS posts_tags_insert AFTER INSERT ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_tags_insert;
+CREATE TRIGGER posts_tags_insert AFTER INSERT ON posts BEGIN
     INSERT INTO post_tags (post_id, tag)
         SELECT new.id, value FROM json_each(json_extract(new.post_data, '$.tags'))
-        WHERE json_type(new.post_data, '$.tags') = 'array';
+        WHERE json_type(new.post_data, '$.tags') = 'array'
+        /* A null in the array is not a tag.  Trog::DataModule::_process() used  */
+        /* to push a post's visibility into its tags without defaulting it       */
+        /* first, so posts written before that carry a null there -- and a null  */
+        /* tag is one no query ever matches, which is exactly what leaving it    */
+        /* out of the index amounts to.  Indexing it instead would refuse to     */
+        /* store the post at all, which is a great deal worse than the           */
+        /* untaggable post it already was.                                       */
+          AND value IS NOT NULL;
 END;
 
-CREATE TRIGGER IF NOT EXISTS posts_tags_delete AFTER DELETE ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_tags_delete;
+CREATE TRIGGER posts_tags_delete AFTER DELETE ON posts BEGIN
     DELETE FROM post_tags WHERE post_id = old.id;
 END;
 
@@ -141,10 +158,12 @@ CREATE VIRTUAL TABLE IF NOT EXISTS posts_fts USING fts5(
     tokenize='trigram'
 );
 
-CREATE TRIGGER IF NOT EXISTS posts_fts_insert AFTER INSERT ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_fts_insert;
+CREATE TRIGGER posts_fts_insert AFTER INSERT ON posts BEGIN
     INSERT INTO posts_fts (rowid, title, data) VALUES (new.id, new.title, new.data);
 END;
 
-CREATE TRIGGER IF NOT EXISTS posts_fts_delete AFTER DELETE ON posts BEGIN
+DROP TRIGGER IF EXISTS posts_fts_delete;
+CREATE TRIGGER posts_fts_delete AFTER DELETE ON posts BEGIN
     INSERT INTO posts_fts (posts_fts, rowid, title, data) VALUES ('delete', old.id, old.title, old.data);
 END;

t/Trog-Data-SQLite.t

@@ -325,6 +325,82 @@ subtest 'writing' => sub {
     unlike( $blob, qr/version_max|"modified"|display_name|user_class/, 'derived fields are not stored' );
 };
 
+subtest 'tags a post might really have' => sub {
+    my $dbh = Trog::SQLite::dbh( undef, 'data/posts.sqlite' );
+
+    my $tags_of = sub {
+        my ($id) = @_;
+        my $rows = $dbh->selectall_arrayref(
+            'SELECT tag FROM post_tags t JOIN posts p ON p.id = t.post_id WHERE p.uuid = ? ORDER BY tag',
+            { Slice => {} }, $id
+        );
+        return [ map { $_->{tag} } @$rows ];
+    };
+
+    # A post is storable whatever its tags look like.  Refusing one is refusing
+    # to store the post, which is never the right answer for an index.
+    my %shapes = (
+        'p-tag-missing' => undef,
+        'p-tag-null'    => undef,
+        'p-tag-empty'   => [],
+        'p-tag-normal'  => [qw{blog public}],
+        'p-tag-holed'   => [ 'public', undef ],
+        'p-tag-numeric' => [ 1,        2 ],
+    );
+
+    foreach my $id ( sort keys %shapes ) {
+        my $post = post( id => $id );
+        if   ( $id eq 'p-tag-missing' ) { delete $post->{tags} }
+        else                            { $post->{tags} = $shapes{$id} }
+
+        is( exception { $sqlite->write( [$post] ) }, undef, "a post with $id tags is storable" );
+    }
+
+    # The one that actually bit: Trog::DataModule::_process() pushed a post's
+    # visibility into its tags without defaulting it first, so posts written
+    # before that carry a null there, and a whole site's migration fell over on
+    # the NOT NULL constraint rather than on anything wrong with the post.
+    is_deeply( $tags_of->('p-tag-holed'),   ['public'],        'a null in the array is left out of the index' );
+    is_deeply( $tags_of->('p-tag-normal'),  [qw{blog public}], 'and a normal one is indexed whole' );
+    is_deeply( $tags_of->('p-tag-empty'),   [],                'an empty list indexes nothing' );
+    is_deeply( $tags_of->('p-tag-null'),    [],                'and neither does a null tags field' );
+    is_deeply( $tags_of->('p-tag-missing'), [],                'nor no tags field at all' );
+
+    # A null tag matched nothing in the flat file model either, so leaving it
+    # out of the index changes no answer.
+    my @found = $sqlite->get( limit => 0, acls => ['public'], tags => ['public'] );
+    ok( scalar( grep { $_->{id} eq 'p-tag-holed' } @found ), 'the post is still found by the tag it does have' );
+};
+
+subtest 'the triggers can be fixed after the fact' => sub {
+
+    # Every trigger is dropped and recreated when the schema is applied, rather
+    # than created IF NOT EXISTS.  With IF NOT EXISTS a database that already
+    # had one kept its old copy for good, so fixing a trigger would have fixed
+    # it only for sites that had not run yet -- which is no use at all, since
+    # the site that needs the fix is by definition one that has.
+    my $dbh = Trog::SQLite::dbh( undef, 'data/posts.sqlite' );
+
+    $dbh->do('DROP TRIGGER posts_tags_insert');
+    $dbh->do( 'CREATE TRIGGER posts_tags_insert AFTER INSERT ON posts BEGIN INSERT INTO post_tags (post_id, tag) VALUES (new.id, %s); END;' =~ s/%s/'wrong'/r );
+
+    # What a worker starting up does.
+    Trog::Data::SQLite::_dbh();
+    my $schema = File::Slurper::read_text('schema/sqlite.schema');
+    $dbh->{sqlite_allow_multiple_statements} = 1;
+    $dbh->do($schema);
+    $dbh->{sqlite_allow_multiple_statements} = 0;
+
+    # post() appends the visibility to the tags, so this one carries both.
+    $sqlite->write( [ post( id => 'p-after-fix', tags => ['afterfix'] ) ] );
+    my $rows = $dbh->selectall_arrayref(
+        'SELECT tag FROM post_tags t JOIN posts p ON p.id = t.post_id WHERE p.uuid = ? ORDER BY tag',
+        { Slice => {} }, 'p-after-fix'
+    );
+    is_deeply( [ map { $_->{tag} } @$rows ], [qw{afterfix public}], 'reapplying the schema replaces a stale trigger' );
+    unlike( join( ',', map { $_->{tag} } @$rows ), qr/wrong/, 'and the stale one stops firing' );
+};
+
 subtest 'indexing a custom field' => sub {
 
     # index_fields() logs what it did, and with no logger configured that comes