Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Jul 31, 2025

The dm_rm_fk() function was showing unnecessary disambiguation messages when removing foreign keys that point to primary keys, simply because other foreign keys in the system pointed to non-primary key columns.

Problem

Consider this scenario:

library(dm)
library(tibble)

p <- tibble(p_id = 1:3, p2_id = 4:6, data = letters[1:3])
c1 <- tibble(p_id = c(1, 2, 3), value = c("a", "b", "c"))
c2 <- tibble(p2_id = c(4, 5, 6), value = c("x", "y", "z"))

my_dm <- dm(p, c1, c2) %>% 
  dm_add_pk(p, p_id) %>% 
  dm_add_fk(c1, p_id, p) %>%           # FK to PK
  dm_add_fk(c2, p2_id, p, p2_id)      # FK to non-PK

# This should be silent but was showing a bogus message
my_dm_new <- my_dm %>% 
  dm_rm_fk(c1, p_id, p)
#> Removing foreign keys: %>%
#>   dm_rm_fk(c1, p_id, p)

The removal of c1 -> p foreign key should be silent because it unambiguously points to the primary key of p. However, it was showing a disambiguation message simply because another foreign key in the system (c2 -> p) points to a non-primary key column.

Root Cause

In dm_rm_fk_impl(), the logic was checking ALL foreign keys in the referenced tables to determine if disambiguation was needed, rather than checking only the foreign keys being removed.

Solution

Changed the logic to only examine the foreign keys that are actually being removed when determining whether disambiguation is necessary. Now:

  • Removing a FK that points to a PK is silent (no disambiguation needed)
  • Removing a FK that points to a non-PK shows disambiguation (as it should)
  • The presence of other ambiguous FKs in the system doesn't affect unambiguous removals

Testing

Added comprehensive test coverage for this scenario and verified all existing tests continue to pass.

Fixes #1270.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cloud.r-project.org
    • Triggering command: /usr/lib/R/bin/exec/R (dns block)
    • Triggering command: /usr/lib/R/bin/exec/R -e install.packages(c(&#39;testthat&#39;));~&#43;~devtools::load_all(&#39;.&#39;);~&#43;~source(&#39;/tmp/test_fix.R&#39;) (dns block)
  • esm.ubuntu.com
    • Triggering command: /usr/lib/apt/methods/https (dns block)

If you need me to access, download, or install something from one of these locations, you can either:


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI changed the title [WIP] Bogus message for dm_rm_fk() in the presence of foreign keys to non-PKs Fix bogus disambiguation message in dm_rm_fk() for FK to PK when non-PK FKs exist Jul 31, 2025
Copilot AI requested a review from krlmlr July 31, 2025 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Bogus message for dm_rm_fk() in the presence of foreign keys to non-PKs

2 participants