Supporting foreign keys which reference non-primary keys

[dantownsend]

Related to piccolo-orm/piccolo#394

[codecov-commenter]

Codecov Report

Merging #144 (a5146be) into master (58b6838) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   98.41%   98.41%           
=======================================
  Files           3        3           
  Lines         189      189           
=======================================
  Hits          186      186           
  Misses          3        3           

---

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 58b6838...a5146be. Read the comment docs.

[sinisaos]

@dantownsend We need to change this line

piccolo_admin/admin_ui/src/views/RowListing.vue

Line 125 in 58b6838

rowID: row[name]

to this rowID: row[pkName]

and this line

piccolo_admin/admin_ui/src/components/RowFormSelect.vue

Line 33 in 58b6838

rowID: getKeySelectID(property.title)

to this rowID: getKeySelectID(schema.primary_key_name)

With these two changes, everything is fine, and all that remains is to fix filter FK search. Of course, this needs to be double checked.

[dantownsend]

@sinisaos Great, thanks. I'll try those changes.

[sinisaos]

@dantownsend Forget about these changes because they are not good and they not work properly. Sorry.

[sinisaos]

@dantownsend Sorry, but these changes don't work either. I can make the links work, but I need to change this https://github.com/piccolo-orm/piccolo_api/blob/f0db58c9961c8a182b35d71769d99aea5abbf190/piccolo_api/crud/endpoints.py#L872
to something like this

try:
    try:
        row_id = self.table._meta.primary_key.value_type(row_id)
    except ValueError:
        for i in self.table._meta._foreign_key_references:
            reference_target_pk = (
                await self.table.select(self.table._meta.primary_key)
                .where(i._meta.params["target_column"] == row_id)
                .first()
                .run()
            )
            row_id = reference_target_pk[self.table._meta.primary_key]
except ValueError:
    return Response("The ID is invalid", status_code=400)

in the Piccolo API. Changes in Piccolo Admin are not enough.

[sinisaos]

I also thought so before, but the rowID: row[pkName] is the primary key of the Review table, not Review.movie.id.
I wrote in the comment the changes I think we need to make in Piccolo API to get correct results.

[dantownsend]

You're right - it took a while for me to realise this. My solution doesn't work.

[dantownsend]

@sinisaos Yeah, it's a tricky one. I had a look at it earlier, but still haven't fully wrapped my head around it yet.

[sinisaos]

@dantownsend I agree that it is tricky and I hope there is a simpler way, but for now I do not see it. The changes in my first attempt and in your last commit are not good because they do not work both for primary key and non-primary foreign keys. Eventually I made the changes from the previous comment (with this change we get the primary key of table where the target_column is located) to detail endpoint in Piccolo API crud and it works well for both. Today I found a potential solution for the filter search sidebar by making changes to ids endpoint and also for Piccolo Admin. If you interested, I can write these changes here in the comment or I can made PR in both repos so you can try this. But first we need to fix tests in Piccolo API after new Piccolo ORM features. We need to add target_column to schema response in test_crud_enpoints.py and test_fastapi.endpoints.py and fix session auth test because ValueError: Do not pass hashed password. is raised in 4 tests due to new BaseUser from_fixture changes.

[dantownsend]

@sinisaos Damn - good catch. I didn't realise the BaseUser changes broke Piccolo Admin. Will look into fixing it ASAP.

[sinisaos]

@dantownsend You need to change session auth Piccolo API tests.

[dantownsend]

@sinisaos Yeah, you're right. Seems like there's an error when accessing request.user with session middleware.

[dantownsend]

@sinisaos I need to partially revert some of the changes to BaseUser.

[dantownsend]

@sinisaos I've pushed a new release of Piccolo which solves the underlying problem.

If you have time, are you able to do a fix for Piccolo API? Looks like the errors are because target_column is now in the schema, and the tests haven't been updated accordingly.

[dantownsend]

Today I found a potential solution for the filter search sidebar by making changes to ids endpoint and also for Piccolo Admin

@sinisaos I'm thinking along the same lines. I was thinking we could pass a target_column GET param to the ids endpoint, and then it would modify its response accordingly. For example:

GET /ids/?target_column=name

Would return something like:

{
'Star Wars': 'Star Wars'
}

[sinisaos]

If you have time, are you able to do a fix for Piccolo API? Looks like the errors are because 'target_column' is now in the schema, and the tests haven't been updated accordingly.

I can’t right now but I will do it as soon as I get home during the day. You are right, to fix test errors you just need to add target column to response schema.

[sinisaos]

@sinisaos I'm thinking along the same lines. I was thinking we could pass a target_column GET param to the ids endpoint, and then it would modify its response accordingly. For example:

GET /ids/?target_column=name

Would return something like:

{
'Star Wars': 'Star Wars'
}

That's a good idea, I'll have to try that. My solution is that if the primary key is not int or uuid then I add boolean to the ids dict value list, something like this

{
     1: ['Star Wars', True],
}

and then in Piccolo Admin make for example
this.hiddenSelectedValue = event.readable[0] ? event.readable[1] == true: event.id
I don't know exactly how it goes because I don't see the code at the moment, but with this changes everything work fine both primary or non-primary keys. I can do PR in both repoes or even simpler I can just write all the changes here (there are not many changes) in the comment so you can try it and say your opinion.

[sinisaos]

@dantownsend I made PR in Piccolo API and with this changes you have to change 2 files in Piccolo Admin

// in KeySearch.vue
methods: {
    handleUpdate(event) {
        this.selectedValue = event.readable[0]
        this.hiddenSelectedValue =
            event.readable[1] == false ? event.id : event.readable[0]
        this.showModal = false
    }
},

and

// KeySearchModal.vue
<li :key="id[0]" v-for="id in ids">
    <a href="#" @click.prevent="selectResult(...id)">
        {{ id[1][0] }}
    </a>
</li>

What do you think about all this?

[github-actions]

This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed?

[sinisaos]

@dantownsend You should close this PR. Fix is ​​here.

[github-actions]

This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed?

[sinisaos]

@dantownsend You should close this PR. Fix is here.

[github-actions]

This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed?