Bug fix for del() with multiple Foreign Key aliases to the same Primary.

[zitrosolrac]

Fix #379

[zfj1]

@guzman-raphael Requesting a further fix to get this working

[kabilar]

Thanks for the report @zfj1. We will review and get back to you.

[guzman-raphael]

@zfj1 Thanks for the report. :handshake:

We've addressed this now in our new release 3.5.0. Please let us know if you run into any further issues.

[zfj1]

Hi @guzman-raphael @kabilar,

We've run into more issues here. The offending table:

:: BreedingPair
-> AnimalSource #gives an id number to this breeding pair
---
(male_id) -> Animal
(female_id) -> Animal

This table has 2/3 aliased foreign keys (so all([fks.aliased]) is false), but the restriction fails. Specifically, it seems to always try to delete every entry from this table when deleting an animal from the Animal table, even when the given animal is not a member of a BreedingPair. I've implemented a fix that works for this particular table on my branch which you can take a look at.

Basically, one needs to check if any() foreign keys are aliased, not all(). But the foreign keys should also be cross-referenced with the referee, thus ~any([fks.aliased] & fks_index_i) is the appropriate check. This change requires a change to the else block of the condition to parse properly.

I would strongly suggest testing this on a more pathological table, with several aliased and unaliased FKs, and FKs to tables with multiple primary keys. Accidentally deleting an entire table due to a failed restriction is certainly something we're keen to avoid :).

[kabilar]

Thanks for the report @zfj1. We will review and provide you with an estimated timeline for the fix.

[guzman-raphael]

Thanks @zfj1 for the detailed report! :handshake: This is very similar to a test case we already have that passes, however, without the FK on the primary.

A few questions come to mind:

1. Is the definition you've provided exactly how you are currently defining it? I would expect some error in ambiguity related to the renaming.
2. Have you by chance ran the tests to see if your fix passes our entire test suite?

In an initial review, nothing obvious comes to mind yet but I'd generally approach this by:

1. Creating a new test case that reproduces per above resulting in a failure case
2. Apply the fix
3. Iterate until the entire suite of tests pass

@zitrosolrac Would you help starting to investigate this?

We'll have a better idea on the estimated time to solution once we understand better why the prior logic did not account for this case.

[zitrosolrac]

@zfj1 @guzman-raphael As an update to your inquiry, I can relay that you're suggested changes were implemented into relvar.m and all of the tests in our test suite passed. Thank you for you contribution and we will consider merging this into the app soon!