Check that columns used as foreign keys have unique or primary constraints

[jamesaoverton]

SQLite (at least) requires that a column T2.C used as a foreign key constraint needs to be unique or primary. If not, we'll get an error like error returned from database: (code: 1) foreign key mismatch - "T1" referencing "T2". I've been very confused by this error message, so I'd like a more user-friendly message. Based on just the 'column' table we have all the information we need to check this. There are some relevant lines around toolkit.rs get_table_ddl() around line 4080.

Options:

1. just report a friendlier message and return an error (or panic)
2. fix the problem by giving C a unique constraint, and warn the user about this change

To fix the problem (2), we need to act before the foreign key constraint is actually asserted in a SQL DDL. Before that, we need to update the VALVE config and warn the user that this change was made. I think it would be good to figure this out somewhere in read_config_files().

If we only do this, the change would not persist across instances of the Valve struct. The next fancier thing we could do would be to persist that change by updating the column table content. We could also insert an INFO message telling the user that this change was made, and why. Then Nanobot would display the message in the 'column' table.

[lmcmicu]

Some comments:

1. "To fix the problem (2), we need to act before the foreign key constraint is actually asserted in a SQL DDL."

Actually this isn't quite true. I don't believe that anything bad can happen if we add the unique constraint directly in the DDL and not in the column config. We already do this for "tree foreign keys". See this code. That said, we should be able to make the change in the config and arguably this is what we should have been doing from the beginning (even in the tree foreign case). So if we do this then I would suggest that we also do it for tree foreign keys in addition to regular foreign keys, and in that case the code that I just linked to above will be removed. Instead we will add the extra unique keys in the get_table_constraints() function rather than in the get_table_ddl() function.

2. "If we only do this, the change would not persist across instances of the Valve struct."

While this is technically true, note that the change will be reapplied every time valve is run, so in a sense it does persist. That said it will obviously be useful to persist the information for real.

[lmcmicu]

One more thing that is worth mentioning regarding adding the unique constraint to the config (and possibly persisting it) rather than only to the DDL (as we currently do in the case of tree-foreign keys):

• One problem is that Valve currently only supports one "structure" per column. So if table_1.column_1 refers to table_2.column_1, and further if table_2.column_1 already has a structure defined on it (say, from(table_3.column_1)), then technically we cannot add a unique structure to the column since there is already a structure defined on table_2.column_1.

It should be possible to change Valve so that more than one structure per column is allowed, but this needs to be investigated.

[lmcmicu]

After discussion with @jamesaoverton it was decided that we will move the logic to implicitly add a unique key, for both tree-foreign and regular foreign constraints, from get_table_ddl() to get_table_constraints().

As a first pass we can simply, in get_table_constraints(), add the extra columns to the uniques vector which will then be picked up automatically later by get_table_ddl(). We should emit a warning to let the user know that we had to implicitly add a unique key.

As a second pass we can do the fancier stuff mentioned in the issue description, with the following caveat prompted by my comment above: In the case where we would normally add a unique constraint, but we cannot because the column we would have added it to already has some structure defined on it, then in that case don't update the configuration (since valve currently doesn't support multiple structures per column). Instead just emit a warning to the user to tell her that we were unable to update the configuration and why.

[lmcmicu]

Since persisting the addition of a unique structure would break our round-trip tests (unless we also build in a way to override this) it was decided that we will not implement the "fancier thing" described in the issue description, at least for now.