Failed to list collections with unset optional fields in SQLite

[Akron]

When listing a collection with optional unset fields, in SQLite the backend responds with

{"errors":[{"message":"Expected string - got null.","path":"\/items\/0\/xy"}],"status":500}

There is no difference, whether the field is a listed column or not. Tested with: 1.001 Example app:

use Mojolicious::Lite;
use Mojo::SQLite;
use Test::More;
use Test::Mojo;
use Mojo::File qw/tempfile/;

my $db_file = tempfile;

helper sqlite => sub {
  state $sql = Mojo::SQLite->new->from_filename($db_file);
};

plugin Yancy => {
  backend => {
    sqlite => app->sqlite
  },
  collections => {
    people => {
      required => ['name'],
      'x-list-columns' => [qw/name/], # No difference though
      type => 'object',
      properties => {
        id => {
          type => 'integer',
          readOnly => 1,
        },
        name => {
          type => 'string'
        },
        email => {
          type => 'string'
        }
      },
      example => {
        name => 'Philip J. Fry'
      }
    },
  }
};

# Load schema
app->sqlite->migrations->from_string(<<SCHEMA)->migrate;
-- 1 up
CREATE TABLE people (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    name VARCHAR NOT NULL,
    email VARCHAR NULL
);
-- 1 down
drop table people;
SCHEMA

get '/' => sub {
  my $c = shift;
  $c->render(inline => <<'TEMPLATE');
% my @people = app->yancy->list('people');
<h1>People!</h1>
<ul>
  % for my $person (@people) {
    <li><%= $person->{name} %><% if ($person->{email}) { %> (<%= $person->{email} %>)<% } %></li>
  % }
</ul>
TEMPLATE
};

my $t = Test::Mojo->new;

$t->get_ok('/')
  ->status_is(200)
  ->text_is('h1', 'People!')
  ->element_exists_not('ul > li');

$t->get_ok('/yancy')
  ->status_is(200)
  ->text_is('head > title', 'Yancy CMS');

$t->post_ok('/yancy/api/people' => json => {
  name => 'akron'
})->status_is(201)
  ->content_is(1);

$t->post_ok('/yancy/api/people' => json => {
  name => 'frank',
  email => 'frank@sojolicio.us',
})->status_is(201)
  ->content_is(2);

# This fails
$t->get_ok('/yancy/api/people')
  ->status_is(200)
  ->content_is('');

$t->get_ok('/')
  ->status_is(200)
  ->element_exists('ul > li')
  ->text_is('ul > li:nth-of-type(1)', 'akron');

$t->get_ok('/')
  ->status_is(200)
  ->element_exists('ul > li')
  ->text_is('ul > li:nth-of-type(2)', 'frank (frank@sojolicio.us)');

done_testing;

(Sorry for submitting an example app again instead of doing a pull request, but I wasn't able to find a backend specific full integration test in the test suite and had no luck using TEST_YANCY_BACKEND="sqlite:myfile.db" prove -lr t/).

[preaction]

Sorry for submitting an example app again instead of doing a pull request

Not a problem. When using TEST_YANCY_BACKEND, did your myfile.db have the schema written (from t/share/sqlite.sql)? Without that, the tests won't work: They don't know how to set up a schema. I should add some docs in CONTRIBUTING about running the integration tests, and where all the various tests are (like how backend tests are actually in t/lib/Local/Test.pm).

As for your problem: It seems like the solution to this is to allow the email field to be null, by setting multiple types:

plugin Yancy => {
  backend => {
    sqlite => app->sqlite
  },
  collections => {
    people => {
      required => ['name'],
      'x-list-columns' => [qw/name/], # No difference though
      type => 'object',
      properties => {
        id => {
          type => 'integer',
          readOnly => 1,
        },
        name => {
          type => 'string'
        },
        email => {
          type => [ 'string', 'null' ] # Can be a string, or since it is not required, null
        }
      },
      example => {
        name => 'Philip J. Fry'
      }
    },
  }
};

This should be picked up automatically if using read_schema => 1:

plugin Yancy => {
  backend => {
    sqlite => app->sqlite
  },
  read_schema => 1, # Fill in the parts we don't define below
  collections => {
    people => {
      'x-list-columns' => [qw/name/], # No difference though
      example => {
        name => 'Philip J. Fry'
      }
    },
  }
};

But, it's an annoying problem, and I got bit by it a few times myself, so I'm going to add a DIAGNOSTICS section to the config help document about this problem of Expected <type> - got null. and its solution. And possibly a section called Nullable Fields to make it really obvious what's going on.

I could also add some sanity checks to the config: A command or a startup check to see if there are problematic type differences between the schema as it is (read_schema) and the configuration. If there are conflicts, write a warning to the log.

What do you think would have been most helpful when you came across this problem?

[preaction]

The underlying problem is that the JSON schema sees the email field and checks its type. It sees that it's not one of the allowed types and throws an error.

Another alternative to solve this problem could be for the backend, or the API controller, to explicitly remove any fields whose value is null: Unless the field is required by the required list of fields, it's optional and does not have to be defined.

I'm not sure I like that idea: It makes it more difficult to loop over all the fields in a collection when you only have an item of the collection. But, this solution would have the benefit of not requiring users to configure explicitly null fields, removing this problem entirely. And it'd only have to be in the API controller, everywhere else could get the rows exactly as they are.

Thoughts / Opinions?

[preaction]

Alright. I went with the API controller changes so that the configuration doesn't have to change. The editor already goes through and prunes out null values when adding/editing items, so this is just the other side of that: The API controller serves the editor the same thing the editor would send back.

I also added some docs for using ./share/run_backend_tests.pl, which should help with running the integration tests, and some docs about the error you came across if someone later comes across it again (it's a misconfiguration in any case, it's just more difficult to reach now).

The config vs. schema sanity check would still be a pretty awesome feature, but it's something that can be done later.

Thanks for taking the time to try things out and write up these detailed bug reports. I really appreciate the help!

[Akron]

Thank you very much! That did indead the trick!

Sorry for submitting an example app again instead of doing a pull request

Not a problem. When using TEST_YANCY_BACKEND, did your myfile.db have the schema written (from t/share/sqlite.sql)?

Ah - no, sorry. I didn't know that and I thought that there were various different schemas involved in testing so the individual tests would do the setup.

What do you think would have been most helpful when you came across this problem?

I wouldn't have considered null to be a type - so that was the main source of confusion to me, I guess. I also didn't know that it's valid to set multiple types, but I probably missed that in the documentation.

I'm not sure I like that idea: It makes it more difficult to loop over all the fields in a collection when you only have an item of the collection.

Yes, I now see the problem as well. On the other hand, it ensures the data to be sparse, in case the majority of fields are optional and unset. I would have preferred your solution as well (mainly because of the "null as a type" confusion).

[preaction]

What do you think would have been most helpful when you came across this problem?

I wouldn't have considered null to be a type - so that was the main source of confusion to me, I guess. I also didn't know that it's valid to set multiple types, but I probably missed that in the documentation.

It wasn't in the docs, and was merely implied by pointing at JSON Schema's docs. It's funny that you mention that, because now I'm remembering that I also did not know that when I came across the error you did, and had to read the JSON Schema docs to find it out ;). Now there's an explicit reference to it in Yancy's docs, so that should help as well. We'll find out when the next patsy ... I mean person... tries Yancy out ;)