Fix #3579: Unexpected end of row when setup diesel

[dsp]

We use PRAGMA table_info for older versions of sqlite. This returns fewer columns that PRAGMA table_xinfo which we use in newer versions of sqlite, making the two code paths incompatbile with the Queryable implementations. We instead now use sql_query with an implementation of QueryableByName to return default values for the case of PRAGMA table_info.

Note this is a WIP implementation. It's my first contribution and dive into the diesel codebase so there is a good chance this is not the right way to do it.

Questions

1. Should I usederive instead of a manual implementation of QueryableByName?

Tests

1. Run cargo test --features sqlite --no-default-features successfully.
2. Run the shell script provided in Error: Unexpected end of row when setup diesel #3579 .

NOT TESTED!

I have not tested the case of an older sqlite version as described in #3579 as I currently do
not have a setup for this. I will refer to the author of the issue to test this branch for me.

[dsp]

Okay I just found an issue in the implementation:

The following yields different results between xinfo and info:

sqlite> CREATE TABLE generated (
   ...>     id integer primary key,
   ...>     generated integer generated always as (id * 3)
   ...> );
sqlite> PRAGMA table_info(generated);
0|id|INTEGER|0||1
sqlite> PRAGMA table_xinfo(generated);
0|id|INTEGER|0||1|0
1|generated|INTEGER|0||0|2

This is the diesel_cli/tests/print_schema/print_schema_generated_columns_generated_always testcase.

[dsp]

Okay looking more into this. PRAGMA table_info won't return information on generated or hidden columns. This makes tests like print_schema::print_schema_generated_columns_with_generated_always fail as they have generated columns that then get dropped in the schema. I think we need to make a call on how to approach this. In my opinion there are 3 choices:

1. Don't support sqlite < 3.26 without support for PRAGMA table_xinfo
2. Support 3.26 with PRAGMA table_info but don't support generated or hidden columns in the schema (as the information is not readily available via PRAGMA table_info.
3. Find an alternative way to generate the schema for sqlite < 3.26. One approach (and the only one I found thus far), is to parse the CREATE TABLE statement that one can obtain via SELECT sql FROM sqlite_master WHERE type='table' AND name='<TABLE_NAME>' to generate the schema.

@weiznich I'll defer to you to decide on the right approach. My personal opinion is that 2. is the worst solution, since it will generate incorrect schemas. 1. would be my preferred appraoch, given that 3.26 is over 5 years old at this point, but it depends on the guarantees you want to give users of diesel. If we go with 3. I can give it a shot, depending on time available on my end. Just let me know waht you think is best.

[weiznich]

Thanks for opening this PR and working on the underlying issue.

We want to go with the second variant (using PRAGMA table_info() on old versions). Its no problem that we don't get information about generated columns on such old sqlite versions, because generated columns itself are only supported since version 3.31.

The manual impl of QueryableByName is required there to set some of the fields to None.

[dsp]

@weiznich Okay cool. In this case i think the PR is actually fine as is, i just need a way to fix the unit tests for the case of PRAGMA table_info. Let me see that i can figure it out.

[weiznich]

I think its fine for the unit test to assume that it is run with an sqlite version that supports generated columns. That means it will always use the table_xinfo branch. After all there is a difference between: You can use diesel with a really old sqlite version and you canbrun the tests against that old version and all of them pass.

[weiznich]

Thanks for working on this. The fix looks good. I have some minor cleanup request that would be great to resolve, otherwise I approve that change.

[dsp]

Good catch. In fact this made me realised that getting the primary key information also relied upon pragma_info_table being Queryable.

Now the pull requests consists of 3 steps (I prefer smaller, logically isolated changes):

1. Implement QueryableByName for ColumnInformation.
2. Implement QueryableByName for PrimaryKeyInformation and remove FullTableInfo as it is no longer needed.
3. Remove table! definition of pragma_info_table as it is now unused.

Hope this is what you hoped for :). Thanks for the patience

[weiznich]

Thanks for the update. It looks good now 👍

I've locally verified that it fixes the problem by manually setting the sqlite version in the corresponding function to 3.20 (which uses the pragma table_info() path.)