changes to allow for compound foreign keys

[drkane]

Add support for compound foreign keys, as per issue #117

Not sure if this is the right approach. In particular I'm unsure about:

• the new ForeignKey class, which replaces the namedtuple in order to ensure that column and other_column are forced into tuples. The class does the job, but doesn't feel very elegant.
• I haven't rewritten guess_foreign_table to take account of multiple columns, so it just checks for the first column in the foreign key definition. This isn't ideal.
• I haven't added any ability to the CLI to add compound foreign keys, it's only in the python API at the moment.

The PR also contains a minor related change that columns and tables are always quoted in foreign key definitions.

[simonw]

Sorry for not reviewing this yet! I'll try to carve out time to look at it in the next few days.

[simonw]

It looks like ForeignKeyBase is missing - I added this and the tests started passing again:

ForeignKeyBase = namedtuple(
    "ForeignKey", ("table", "column", "other_table", "other_column")
)

[simonw]

Sorry for taking so long to review this!

This approach looks great to me - being able to optionally pass a tuple anywhere the API currently expects a column is smart, and it's consistent with how the pk= parameter works elsewhere.

There's just one problem I can see with this: the way it changes the ForeignKey(...) interface to always return a tuple for .column and .other_column, even if that tuple only contains a single item.

This represents a breaking change to the existing API - any code that expects ForeignKey.column to be a single string (which is any code that has been written against that) will break.

As such, I'd have to bump the major version of sqlite-utils to 4.0 in order to ship this.

Ideally I'd like to make this change in a way that doesn't represent an API compatibility break. I need to think a bit harder about how that might be achieved.

[simonw]

One way that this could avoid a breaking change would be to have fk.column and fk.other_column remain as strings for non-compound-foreign-keys, but turn into tuples for a compound foreign key.

This is a bit of an ugly API design, and it could still break existing code that encounters a compound foreign key for the first time - but it would leave code working for the more common case of a non-compound-foreign-key.

[simonw]

Another option: expand the ForeignKey object to have .columns and .other_columns properties in addition to the existing .column and .other_column properties. These new plural properties would always return a tuple, which would be a one-item tuple for a non-compound-foreign-key.

The question then is what should .column and .other_column return for compound foreign keys?

I'd be inclined to say they should return None - which would trigger errors in code that encounters a compound foreign key for the first time, but those errors would at least be a strong indicator as to what had gone wrong.

We can label .column and .other_column as deprecated and then remove them in sqlite-utils 4.0.

Since this would still be a breaking change in some minor edge-cases I'm thinking maybe 4.0 needs to happen in order to land this feature. I'm not opposed to doing that, I was just hoping it might be avoidable.

[drkane]

Thanks for looking at this - home schooling kids has prevented me from replying.

I'd struggled with how to adapt the API for the foreign keys too - I definitely tried the String/Tuple approach. I hadn't considered the breaking changes that would introduce though. I can take a look at this and try and make the change - see which of your options works best.

I've got a workaround for the use-case I was looking at this for, so it wouldn't be a problem for me if it was put on the back burner until a hypothetical v4.0 anyway.

[psychemedia]

Is there any progress elsewhere on the handling of compound / composite foreign keys, or is this PR still effectively open?

[fgregg]

i'll adopt this PR to make the changes @simonw suggested #203 (comment)