Skip to content

Show referring tables and rows when the referring foreign key is compound#2003

Open
fgregg wants to merge 4 commits intosimonw:mainfrom
fgregg:compound_foreign_reference_row_display
Open

Show referring tables and rows when the referring foreign key is compound#2003
fgregg wants to merge 4 commits intosimonw:mainfrom
fgregg:compound_foreign_reference_row_display

Conversation

@fgregg
Copy link
Copy Markdown
Contributor

@fgregg fgregg commented Jan 24, 2023

sqlite foreign keys can be compound, but that is not as well supported by datasette as single column foreign keys.

in particular,

  1. in a table view, there is not a link from the row to the referenced row if the foreign key is compound
  2. in a row view, there is no listing of tables and rows that refer to the focal row if those referencing foreign keys are compound.

Both of these issues are discussed in #1099.

This PR only fixes the second one, because it's not clear what the right UX is for the first issue.

Screenshot 2023-01-24 at 19-47-40 nlrb bargaining_unit

Some things that might not be desirable about this approach.

  1. it changes the external API, by changing column => columns and other_column => other_columns (see inline comment for more discussion.
  2. There are various places where the plural foreign keys have to be checked for length and discarded or transformed to singular.

@fgregg fgregg marked this pull request as draft January 24, 2023 21:31
@fgregg fgregg changed the title [WIP] spike to fix half of #1099 Show referring tables and rows when the referring foreign key is compound Jan 25, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 25, 2023

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.12%. Comparing base (e4ebef0) to head (1e5b42f).
⚠️ Report is 613 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2003      +/-   ##
==========================================
+ Coverage   92.11%   92.12%   +0.01%     
==========================================
  Files          38       38              
  Lines        5555     5565      +10     
==========================================
+ Hits         5117     5127      +10     
  Misses        438      438              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread datasette/app.py
foreign_key
for foreign_key in foreign_keys
if foreign_key["column"] == column
if foreign_key["columns"][0] == column
Copy link
Copy Markdown
Contributor Author

@fgregg fgregg Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we don't have a strong idea of how to display compound foreign keys in a table view, we have to do this operation in a few places.

Comment thread tests/test_api.py
"other_table": "roadside_attraction_characteristics",
"column": "pk",
"other_column": "characteristic_id",
"columns": ["pk"],
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how the API changes.

Comment thread tests/test_api.py
"other_columns": ["foreign_key_with_blank_label"],
"count": 0,
"link": "/fixtures/foreign_key_references?foreign_key_with_blank_label=1",
"other_columns_reference": "foreign_key_with_blank_label",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also another API change. this got introduced so we could have compound keys look good in the row template, but we could do that through a custom filter instead.

@fgregg fgregg marked this pull request as ready for review January 25, 2023 00:53
@fgregg
Copy link
Copy Markdown
Contributor Author

fgregg commented Jan 25, 2023

@simonw, let me know what you think about this approach!

@fgregg
Copy link
Copy Markdown
Contributor Author

fgregg commented Jan 25, 2023

see this related discussion to a change in API in sqlite-utils simonw/sqlite-utils#203 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant