Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting foreign keys which reference non-primary keys #144

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dantownsend
Copy link
Member

@dantownsend dantownsend added the enhancement New feature or request label Jan 16, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2022

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Member

@dantownsend We need to change this line


to this rowID: row[pkName]

and this line

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
Copy link
Member Author

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

@sinisaos
Copy link
Member

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

@sinisaos
Copy link
Member

sinisaos commented Jan 20, 2022

@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.

"
>
<router-link
:to="{
name: 'editRow',
params: {
tableName: getTableName(name),
rowID: row[name]
rowID: row[pkName]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@dantownsend
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member Author

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

@sinisaos
Copy link
Member

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

@dantownsend
Copy link
Member Author

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

@dantownsend
Copy link
Member Author

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

@dantownsend
Copy link
Member Author

dantownsend commented Jan 21, 2022

@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
Copy link
Member Author

dantownsend commented Jan 21, 2022

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

@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
Copy link

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?

@github-actions github-actions bot added the Stale label Mar 25, 2022
@sinisaos
Copy link
Member

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

@github-actions github-actions bot removed the Stale label Mar 28, 2022
@github-actions
Copy link

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?

@github-actions github-actions bot added the Stale label Apr 28, 2022
@sinisaos
Copy link
Member

sinisaos commented Apr 4, 2023

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

@github-actions github-actions bot removed the Stale label Apr 7, 2023
@github-actions
Copy link

github-actions bot commented May 7, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants