Skip to content

Conversation

@dantownsend
Copy link
Member

Fixes #394

When you create a ForeignKey with Piccolo it automatically points to the primary key on the related table. With this PR you can specify a different column on the related table if you want (as long as it's unique).

Currently a proof of concept - seems to work fine, but needs some more tests.

@dantownsend dantownsend added the enhancement New feature or request label Jan 13, 2022
@theelderbeever
Copy link
Contributor

theelderbeever commented Jan 14, 2022

I pulled the branch down to play around with it and do some testing on my own. I maybe making a mistake but when I setup my tables and then autogenerate a migration and apply it the foreign keys ignore the target_column field and end up using the pk from the table instead.

# How it is in my table def
contract_address = ForeignKey(references=Contract, target_column=Contract.address)

# How it shows up in the migration file
    manager.add_column(
        table_class_name="Log",
        tablename="log",
        column_name="contract_address",
        db_column_name="contract_address",
        column_class_name="ForeignKey",
        column_class=ForeignKey,
        params={
            "references": Contract,
            "on_delete": OnDelete.cascade,
            "on_update": OnUpdate.cascade,
            "null": True,
            "primary_key": False,
            "unique": False,
            "index": False,
            "index_method": IndexMethod.btree,
            "choices": None,
            "db_column_name": None,
            "secret": False,
        },
    )

And this is from my table structure in postgres
image

I attempted to add "target_column": Contract.address amongst other variations to the params dict in the add_column method above however, I get an error when migrating that Contract as no attribute 'address' or that contract.address or address isn't a column. My guess is this is because the table doesn't exist yet as this is my init migration.

Edit:
I split the ForeignKeys into a second migration after the tables have been created but still received the same error about

No matching column found with name == contract.address

@dantownsend
Copy link
Member Author

@theelderbeever Thanks for taking a look at it. You're right - there's a bug. The target_column value should be pulling through to the migration. I'll push the fix now.

I need to write some more tests for migrations - as you say, the address column should be created before the foreign key.

@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #395 (9d6e741) into master (6b23206) will decrease coverage by 0.00%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #395      +/-   ##
==========================================
- Coverage   91.41%   91.40%   -0.01%     
==========================================
  Files         103      103              
  Lines        6523     6551      +28     
==========================================
+ Hits         5963     5988      +25     
- Misses        560      563       +3     
Impacted Files Coverage Δ
piccolo/query/methods/select.py 99.55% <ø> (ø)
piccolo/utils/pydantic.py 97.87% <ø> (ø)
piccolo/utils/sql_values.py 90.00% <83.33%> (-3.75%) ⬇️
piccolo/columns/base.py 95.16% <90.00%> (-0.19%) ⬇️
piccolo/apps/migrations/auto/serialisation.py 94.98% <94.44%> (-0.05%) ⬇️
piccolo/columns/column_types.py 92.93% <100.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b23206...9d6e741. Read the comment docs.

The file was getting too hard to understand with all of the different permutations of table.
When using a column reference instead of a string for `target_column`.
@dantownsend dantownsend marked this pull request as ready for review January 16, 2022 12:27
@dantownsend
Copy link
Member Author

It seems to work now.

Piccolo Admin will require some changes to support this new feature.

@theelderbeever
Copy link
Contributor

Awesome! Would the piccolo_admin issue show up as the tables not displaying values properly by chance?

@dantownsend
Copy link
Member Author

@theelderbeever Yes, I think the list view and foreign key selector will be a bit broken.

I'll release this, then will try and sort out the admin.

@dantownsend dantownsend merged commit db702b9 into master Jan 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Foreign Keys that reference non pk columns

4 participants