Check the post that gets written, not just the one submitted

The null tags were not a gap in the schema, and the sidecar merge was not the
problem either.  schema_for() merges the base spec last precisely so a sidecar
cannot redefine core fields, validate() uses it, and the base spec describes tags
as an array of strings -- handed the bad post it rejects it on the spot with
"/tags/1: Expected string - got null".  It was simply never asked.

validate() runs on what was submitted.  _process() then builds the shape that is
actually stored: it turns a visibility into a tag, folds in the acls, resolves
uploads into hrefs.  Nothing looked at the result, so a post could be made wrong
on its way to disk and nobody ever heard about it.  Every post saved without a
visibility got an undef pushed into its tags, which is a tag no query matches,
which made the post invisible to all but an admin -- silently, for as long as
that was possible.

Two changes.  _process() no longer builds one wrong: it defaults the visibility
where it makes the tag rather than relying on its caller, and drops undefs when
folding the acls in and when uniq'ing.  That also silences two uninitialized
warnings it emitted on every such post.

And add() now runs validate_built() on what _process() produced.  It checks
rather than filters, unlike validate(): _process() adds fields the base schema
has never heard of -- content_type, is_video, attachments -- and dropping what it
does not recognise would throw away the work it just did.  So it looks only at
the fields the schema describes, on a copy, and changes nothing.  A post failing
there is a bug in this code rather than bad input, but the answer is still to
refuse to write it: the alternative is what happened before.

Every post type, upload and array body in the suite passes it unchanged, which
is the evidence that it does not reject things it should not.

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

lib/Trog/DataModule.pm

@@ -630,9 +630,14 @@ meant to round-trip.
 
 =cut
 
-sub validate ($post) {
+sub _validator {
     state $validator;
     $validator //= JSON::Validator::Schema::Troglodyne->new();
+    return $validator;
+}
+
+sub validate ($post) {
+    my $validator = _validator();
 
     my $schema = schema_for( $post->{form} );
 
@@ -661,6 +666,34 @@ sub validate ($post) {
     return $validator->validate( $post, $schema );
 }
 
+=head2 validate_built($post)
+
+Check the post that is actually about to be written.
+
+validate() runs on what was submitted.  _process() then builds the shape that
+gets stored -- it turns a visibility into a tag, folds the acls in, resolves
+uploads into hrefs -- and until now nothing looked at the result.  That is how
+every post saved without a visibility came to carry a null in its tags: the
+schema describes tags as an array of strings and would have rejected it on the
+spot, but it was never asked.
+
+Checks rather than filters, unlike validate().  _process() adds fields the base
+schema has never heard of -- content_type, is_video, attachments -- and dropping
+what it does not recognise would throw away the work it just did.  So this looks
+only at the fields the schema actually describes, on a copy, and changes nothing.
+
+Returns the list of validation errors, empty when the post is fit to store.
+
+=cut
+
+sub validate_built ($post) {
+    my $schema = schema_for( $post->{form} );
+
+    my %described = map { $_ => $post->{$_} } grep { exists $schema->{properties}{$_} } keys(%$post);
+
+    return _validator()->validate( \%described, $schema );
+}
+
 sub add ( $self, @posts ) {
     my @to_write;
 
@@ -719,6 +752,17 @@ sub add ( $self, @posts ) {
 
         $post = _process($post);
 
+        # And check what _process() built, not just what arrived.  A post that
+        # fails here is one this code has made wrong on its way to disk, so it
+        # is a bug rather than bad input -- but the answer is still to refuse to
+        # write it, because the alternative is what we did before, which was to
+        # write it and find out years later.
+        my @built = validate_built($post);
+        if (@built) {
+            WARN( "Refusing to store a post this built wrong: " . join( ', ', map { "$_" } @built ) );
+            die [ map { "$_" } @built ];
+        }
+
         push @to_write, $post;
     }
     $self->write( \@to_write );
@@ -730,6 +774,12 @@ sub add ( $self, @posts ) {
 # Not actually a subprocess, kek
 sub _process ($post) {
 
+    # Here as well as in add(), because this is the sub that turns a visibility
+    # into a tag: without one it pushed an undef into tags, which is a tag no
+    # query ever matches, so the post went invisible to everyone but an admin.
+    # Nothing complained, and the bunk tag was written to disk and stayed there.
+    $post->{visibility} //= 'private';
+
     # If the post is private, make sure it's associated assets are too.
     my $is_private_post = $post->{visibility} eq 'private';
 
@@ -754,7 +804,7 @@ sub _process ($post) {
         my $subj = $_;
         !grep { $_ eq $subj } qw{public private unlisted}
     } @{ $post->{tags} };
-    push( @{ $post->{tags} }, @{ $post->{acls} } ) if $is_private_post;
+    push( @{ $post->{tags} }, grep { defined } @{ $post->{acls} } ) if $is_private_post;
     delete $post->{acls};
     push( @{ $post->{tags} }, $post->{visibility} );
 
@@ -764,9 +814,11 @@ sub _process ($post) {
         push( @{ $post->{tags} }, $post->{series} );
     }
 
-    #Filter adding the same acl twice
-    @{ $post->{tags} }    = List::Util::uniq( @{ $post->{tags} } );
-    @{ $post->{aliases} } = List::Util::uniq( @{ $post->{aliases} } );
+    #Filter adding the same acl twice, and anything that isn't a tag at all --
+    # an undef here is what every query below compares against, and it warns
+    # its way through the lot before matching nothing.
+    @{ $post->{tags} }    = List::Util::uniq( grep { defined && length } @{ $post->{tags} } );
+    @{ $post->{aliases} } = List::Util::uniq( grep { defined && length } @{ $post->{aliases} } );
 
     # Handle multimedia content types
     $post->{content_type}       = Trog::Utils::mime_type("www/$post->{href}")       if $post->{href};

t/Trog-DataModule-schema.t

@@ -164,6 +164,95 @@ subtest 'validate() filters, coerces and reports' => sub {
     is_deeply( [ Trog::DataModule::validate( { form => 'recipe.tx', photo => '/assets/x.jpg' } ) ],     [], "so does the href it becomes" );
 };
 
+subtest 'what gets written is checked, not just what was submitted' => sub {
+    set_sidecars();
+
+    # validate() runs on the submitted post.  _process() then builds the shape
+    # that is actually stored, and until it was checked too, a post could be
+    # made wrong on its way to disk and nobody would ever hear about it.
+    my $good = { form => 'blog.tx', title => 'Fine', tags => [qw{blog public}], visibility => 'public' };
+    is_deeply( [ Trog::DataModule::validate_built($good) ], [], 'a well built post passes' );
+
+    my $holed  = { form => 'blog.tx', title => 'Holed', tags => [ 'public', undef ], visibility => 'public' };
+    my @errors = Trog::DataModule::validate_built($holed);
+    ok( scalar(@errors), 'a null in the tags is caught' );
+    like( "$errors[0]", qr{/tags/1}, 'and named by where it is' );
+
+    # It checks rather than filters, unlike validate(): _process() adds fields
+    # the base schema has never heard of, and throwing those away would discard
+    # the work it just did.
+    my $built = {
+        form         => 'blog.tx',
+        title        => 'Built',
+        tags         => ['public'],
+        visibility   => 'public',
+        content_type => 'text/html',
+        is_video     => 1,
+        attachments  => ['/assets/thing.txt'],
+        preview      => '/assets/thumb.png',
+    };
+    is_deeply( [ Trog::DataModule::validate_built($built) ], [], "the fields _process() invents do not fail it" );
+    ok( exists $built->{is_video},    'and are not deleted' );
+    ok( exists $built->{attachments}, 'any of them' );
+};
+
+subtest '_process no longer builds a post wrong' => sub {
+
+    # The bug: a post saved without a visibility had an undef pushed into its
+    # tags, which is a tag no query ever matches -- so the post went invisible
+    # to all but an admin, silently, and the bunk tag was written to disk.
+    my $post = Trog::DataModule::_process( { form => 'blog.tx', title => 'No Visibility', tags => ['blog'] } );
+
+    is_deeply( [ grep { !defined } @{ $post->{tags} } ], [], 'no undef survives into the tags' );
+    is( $post->{visibility}, 'private', 'and the visibility is defaulted where the tag is made' );
+    ok( scalar( grep { $_ eq 'private' } @{ $post->{tags} } ), 'so the tag it produces is a real one' );
+
+    # Which is the same thing validate_built() is there to notice.
+    is_deeply( [ Trog::DataModule::validate_built($post) ], [], 'and the result passes the check' );
+
+    # An undef among the acls of a private post went the same way.
+    my $acled = Trog::DataModule::_process( { form => 'blog.tx', title => 'Acled', tags => ['blog'], visibility => 'private', acls => [ 'members', undef ] } );
+    is_deeply( [ grep { !defined } @{ $acled->{tags} } ], [], 'nor from the acls' );
+    ok( scalar( grep { $_ eq 'members' } @{ $acled->{tags} } ), 'while the real acl still lands' );
+};
+
+subtest 'add() refuses to store a post it built wrong' => sub {
+    set_sidecars();
+
+    # Break _process on purpose: the point is that add() no longer takes its
+    # word for it.  This is the shape the real bug produced.
+    #
+    # One mock object for all three, and unmocked by hand at the end.
+    # Test::MockModule keeps one per package, so a second new() on the same
+    # package hands back the first -- and leaving it to go out of scope left
+    # _process broken for every subtest after this one.
+    my @written;
+    my $mock = Test::MockModule->new('Trog::DataModule');
+    $mock->redefine(
+        '_process',
+        sub ($post) {
+            push( @{ $post->{tags} }, undef );
+            return $post;
+        }
+    );
+    $mock->redefine( 'write', sub { my ( $self, $data ) = @_; push( @written, @$data ); return 1 } );
+    $mock->redefine( 'get',   sub { return () } );
+
+    my $model = bless {}, 'Trog::DataModule';
+    my $err   = exception { $model->add( { form => 'blog.tx', title => 'Doomed', tags => ['blog'], visibility => 'public' } ) };
+
+    $mock->unmock_all();
+
+    ok( $err, 'the save fails' );
+    is( ref $err, 'ARRAY', 'with the errors the submitter is shown' );
+    like( "$err->[0]", qr{/tags/}, 'saying what was wrong' );
+    is_deeply( \@written, [], 'and nothing was written' );
+
+    # The mock really is gone, or every subtest after this one is testing it.
+    my $sane = Trog::DataModule::_process( { form => 'blog.tx', title => 'Sane', tags => ['blog'], visibility => 'public' } );
+    is_deeply( [ grep { !defined } @{ $sane->{tags} } ], [], '_process is itself again afterwards' );
+};
+
 subtest 'empty inputs are treated as absent, except for strings' => sub {
     set_sidecars( 'recipe.json' => sidecar( properties => { servings => { type => 'integer' } } ) );