changes to support non-primary key in Piccolo Admin #134

sinisaos · 2022-01-21T17:25:59Z

Related to Supporting foreign keys which reference non-primary keys #144

dantownsend · 2022-01-21T18:06:26Z

piccolo_api/crud/endpoints.py

+            target_pk_type = [
+                i._meta.params["target_column"].value_type not in (int, uuid)
+                for i in self.table._meta._foreign_key_references
+            ][0]


I see the idea. I think the problem though is it's having to do [0]. If the _foreign_key_references contains more than 1 item, we're guessing that the first one is correct.

I think we're going to have to add a GET parameter so the Piccolo Admin UI can explicitly tell us which column it's interested in.

Yes. That make sense. I will try to add a GET parameter.

Cool, thanks. I think another advantage of the GET param is if someone doesn't pass it we can return the same response as before, so there are no breaking API changes.

I didn't get far, but we can pass target_column as a query param

target_column = request.query_params.get("target_column") if target_column is not None: return JSONResponse({i["readable"]: i["readable"] for i in values})

and return the result like this

# ./api/tables/movie/ids/?target_column= { 'Star Wars': 'Star Wars', 'Return of the Jedi: 'Return of the Jedi, ... }

(we don't need to pass anything to query param and return all results because the KeySearchModal already needs all results)
or without the target_column return standard response

# ./api/tables/movie/ids/ { 1: 'Star Wars', 2: 'Return of the Jedi, ... }

I don't know yet how to trigger Vue frontend based on the existence of the column_name query parameter. I don't know is this ok, or I completely miss the point.

per table

codecov-commenter · 2022-01-22T16:39:37Z

Codecov Report

Merging #134 (2cd11bb) into master (92ea0ac) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   92.55%   92.59%   +0.04%     
==========================================
  Files          33       33              
  Lines        2027     2039      +12     
==========================================
+ Hits         1876     1888      +12     
  Misses        151      151

Impacted Files	Coverage Δ
piccolo_api/crud/endpoints.py	`94.89% <100.00%> (+0.12%)`	⬆️

dantownsend · 2022-01-23T22:39:04Z

piccolo_api/crud/endpoints.py

@@ -871,7 +889,16 @@ async def detail(self, request: Request) -> Response:
        try:
            row_id = self.table._meta.primary_key.value_type(row_id)
        except ValueError:
-            return Response("The ID is invalid", status_code=400)
+            for i in self.table._meta._foreign_key_references:
+                target = i._meta.params["target_column"]


This should be i._meta.params.get("target_column") in case the key doesn't exist.

dantownsend · 2022-01-23T22:40:14Z

piccolo_api/crud/endpoints.py

+                    reference_target_pk = (
+                        await self.table.select(self.table._meta.primary_key)
+                        .where(target == row_id)
+                        .first()
+                        .run()
+                    )
+                    row_id = reference_target_pk[self.table._meta.primary_key]


We can do this slightly easier:

reference_target_pk = ( await self.table.select(self.table._meta.primary_key) .where(target == row_id) .output(as_list=True) .first() .run() )[0]

Unfortunately this does not work if we have multiple non-primary keys per single table.

dantownsend · 2022-01-23T22:44:10Z

@sinisaos I see where you're going with this. I'll have another look tomorrow at the UI side.

dantownsend · 2022-01-23T23:10:44Z

piccolo_api/crud/endpoints.py

@@ -871,7 +889,16 @@ async def detail(self, request: Request) -> Response:
        try:
            row_id = self.table._meta.primary_key.value_type(row_id)
        except ValueError:
-            return Response("The ID is invalid", status_code=400)
+            for i in self.table._meta._foreign_key_references:


I think we can move the logic around a bit here.

target = i._meta.params.get("target_column") if target is None: try: row_id = self.table._meta.primary_key.value_type(row_id) except ValueError: return Response("The ID is invalid", status_code=400) else: # insert your new logic here

Unfortunately this also does not work if we have multiple non-primary keys per single table. e.g if we add director name as non-primary key (or mix primary or non-primary keys) to review reviewer this does not work. we should to leave this as is.

sinisaos · 2022-01-24T09:29:24Z

@dantownsend Basically we have a working list of rows with only primary keys, only with non-primary keys, or a mixture of both.
The problem is the filter search in sidebar and referencing tables and how to combine id and readable based on target_colum or primary key. I hope you got some ideas. This is tricky with a lot of edge cases.

sinisaos · 2022-01-28T13:51:22Z

@dantownsend After a lot of experimentation I managed to solve the filter search problem to some extent. My approach is to always use a primary key even if table has a non-primary key FK relation to table. Everything works if it only has a non-primary key or a combination of primary key and non-primary key. Also tests for that need to be written.
The only situation when it doesn't work well is when we have more than one non-primary key per table

class Review(Table):
    reviewer = ForeignKey(Director, target_column=Director.name)
    ...
    movie = ForeignKey(Movie, target_column=Movie.name)

because the loop in _apply_filters returns only the last result.

For referencing table I took same approach and we need to make this changes in Piccolo Admin

// changes in ReferencingTables.vue
<script lang="ts">
import Vue from "vue"
import { Location } from "vue-router"
import { TableReferencesAPIResponse, TableReference } from "../interfaces"

export default Vue.extend({
    props: ["tableName", "rowID"],
    data() {
        return {
            references: [] as TableReference[],
            showReferencing: false
        }
    },
    computed: {
        selectedRow() {
            return this.$store.state.selectedRow
        },
        schema() {
            return this.$store.state.schema
        }
    },
    methods: {
        async fetchTableReferences() {
            const response = await this.$store.dispatch(
                "fetchTableReferences",
                this.tableName
            )

            this.references = (
                response.data as TableReferencesAPIResponse
            ).references
        },
        clickedReference(reference: TableReference) {
            let columnName = reference.columnName
            let query = {}
            for (const key in this.selectedRow) {
                if (this.rowID == this.selectedRow[key]) {
                    query[columnName] =
                        this.selectedRow[this.schema.primary_key_name]
                }
            }

            let location: Location = {
                name: "rowListing",
                params: { tableName: reference.tableName },
                query
            }
            let vueUrl = this.$router.resolve(location).href
            window.open(
                `${document.location.origin}${document.location.pathname}${vueUrl}`,
                "_blank"
            )
        }
    },
    async mounted() {
        await this.fetchTableReferences()
    }
})
</script>

Can you try this changes with reviewer column like this

class Review(Table):
    reviewer = Varchar()
#and then
class Review(Table):
    reviewer = ForeignKey(Director)
# and then
class Review(Table):
    reviewer = ForeignKey(Director, target_column=Director.name)

What do you think about this?

dantownsend · 2022-01-28T15:24:44Z

@sinisaos It looks promising - I'll give it a go.

sinisaos · 2022-01-29T07:37:29Z

@dantownsend We also need to make these changes to add and edit records in Piccolo Admin to make everything work.

// changes in EditRowForm.vue
methods: {
    async submitForm(event) {
        console.log("Submitting...")

        const form = new FormData(event.target)

        const json = {}
        const targetColumn = []
        for (const i of form.entries()) {
            const key = i[0].split(" ").join("_")
            let value = i[1]

            if (value == "null") {
                value = null
            } else if (this.schema.properties[key].type == "array") {
                // @ts-ignore
                value = JSON.parse(value)
            } else if (
                this.schema?.properties[key].format == "date-time" &&
                value == ""
            ) {
                value = null
            } else if (
                this.schema?.properties[key].extra.foreign_key == true &&
                value == ""
            ) {
                value = null
            } else if (
                this.schema?.properties[key].extra.foreign_key == true &&
                this.schema?.properties[key].format != "uuid" &&
                this.schema?.properties[key].type != "integer" &&
                key == this.schema?.properties[key].extra.to
            ) {
                targetColumn.push(value)
            }
            json[key] =
                targetColumn[1] == value &&
                this.schema?.properties[key].format != "uuid" &&
                this.schema?.properties[key].type != "integer"
                    ? targetColumn[0]
                    : value
        }
...

and

// changes in AddRowForm.vue
methods: {
    async submitForm(event) {
        console.log("I was pressed")
        const form = new FormData(event.target)

        const json = {}
        const targetColumn = []
        for (const i of form.entries()) {
            const key = i[0].split(" ").join("_")
            let value = i[1]

            if (value == "null") {
                value = null
                // @ts-ignore
            } else if (this.schema.properties[key].type == "array") {
                // @ts-ignore
                value = JSON.parse(value)
                // @ts-ignore
            } else if (
                this.schema?.properties[key].format == "date-time" &&
                value == ""
            ) {
                value = null
                // @ts-ignore
            } else if (
                this.schema?.properties[key].extra.foreign_key == true &&
                value == ""
            ) {
                value = null
            } else if (
                this.schema?.properties[key].extra.foreign_key == true &&
                this.schema?.properties[key].format != "uuid" &&
                this.schema?.properties[key].type != "integer" &&
                key == this.schema?.properties[key].extra.to
            ) {
                targetColumn.push(value)
            }
            json[key] =
                targetColumn[1] == value &&
                this.schema?.properties[key].format != "uuid" &&
                this.schema?.properties[key].type != "integer"
                    ? targetColumn[0]
                    : value
        }
...

sinisaos · 2022-02-04T07:49:03Z

@dantownsend Did you have time to try this? If you agree with this, I can do PR in Piccolo Admin, with changes from the comments to make everything work.

dantownsend · 2022-02-04T08:07:36Z

@sinisaos I haven't had a chance, but I would like to get it sorted. If you could create the PR that would be great, thanks.

sinisaos · 2022-02-04T08:19:20Z

@dantownsend Great. I will wait with PR because these changes in Piccolo Admin are only relevant if the code from this PR is good enough.

dantownsend · 2022-02-04T08:25:39Z

@sinisaos OK, makes sense

sinisaos · 2022-02-08T09:58:06Z

@dantownsend Everything should work now. Combination of primary and non-primary keys per table, multiple non-primary keys per table etc.

sinisaos · 2022-04-15T06:10:49Z

@dantownsend Have you had a chance to try this?

dantownsend · 2022-04-15T06:12:36Z

@sinisaos Not yet - will try and review it this weekend.

sinisaos · 2022-04-15T06:34:53Z

@dantownsend Great. Thanks.

changes to support non-primary key in Piccolo Admin

6df9c07

sinisaos marked this pull request as draft January 21, 2022 17:33

sinisaos mentioned this pull request Jan 21, 2022

Supporting foreign keys which reference non-primary keys piccolo-orm/piccolo_admin#144

Draft

dantownsend reviewed Jan 21, 2022

View reviewed changes

fix for multiple reference non primary keys

d899cd1

per table

dantownsend reviewed Jan 23, 2022

View reviewed changes

fix filter search

1d9b3b2

correct type for Postgres

b7e402d

cleaning and renaming variables

346f3ee

sinisaos marked this pull request as ready for review February 4, 2022 07:47

final fix for multiple non-primary key per table

a828396

sinisaos mentioned this pull request Feb 9, 2022

Foreign Keys that reference non pk columns piccolo-orm/piccolo#394

Open

index moved to target_column_fk_name

a6f2cca

sinisaos mentioned this pull request Jul 1, 2022

Bulk delete and bulk update #145

Closed

sinisaos added 4 commits March 8, 2023 06:14

Merge branch 'master' into reference_non_primary_key

c94facb

fix linter errors

ac16f2b

Merge branch 'piccolo-orm:master' into reference_non_primary_key

2c0a351

Merge branch 'piccolo-orm:master' into reference_non_primary_key

2cd11bb

sinisaos closed this Sep 1, 2023

sinisaos deleted the reference_non_primary_key branch September 1, 2023 12:34

sinisaos mentioned this pull request Feb 20, 2024

Piccolo-Admin is unable to understand target_column arg of ForeignKey columns piccolo-orm/piccolo_admin#362

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes to support non-primary key in Piccolo Admin #134

changes to support non-primary key in Piccolo Admin #134

sinisaos commented Jan 21, 2022

dantownsend Jan 21, 2022 •

edited

Loading

sinisaos Jan 21, 2022

dantownsend Jan 22, 2022

sinisaos Jan 22, 2022

codecov-commenter commented Jan 22, 2022 •

edited

Loading

dantownsend Jan 23, 2022

dantownsend Jan 23, 2022

sinisaos Jan 24, 2022

dantownsend commented Jan 23, 2022

dantownsend Jan 23, 2022

sinisaos Jan 24, 2022

sinisaos commented Jan 24, 2022

sinisaos commented Jan 28, 2022 •

edited

Loading

dantownsend commented Jan 28, 2022

sinisaos commented Jan 29, 2022 •

edited

Loading

sinisaos commented Feb 4, 2022

dantownsend commented Feb 4, 2022

sinisaos commented Feb 4, 2022

dantownsend commented Feb 4, 2022

sinisaos commented Feb 8, 2022

sinisaos commented Apr 15, 2022

dantownsend commented Apr 15, 2022

sinisaos commented Apr 15, 2022

changes to support non-primary key in Piccolo Admin #134

changes to support non-primary key in Piccolo Admin #134

Conversation

sinisaos commented Jan 21, 2022

dantownsend Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 22, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantownsend commented Jan 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinisaos commented Jan 24, 2022

sinisaos commented Jan 28, 2022 • edited Loading

dantownsend commented Jan 28, 2022

sinisaos commented Jan 29, 2022 • edited Loading

sinisaos commented Feb 4, 2022

dantownsend commented Feb 4, 2022

sinisaos commented Feb 4, 2022

dantownsend commented Feb 4, 2022

sinisaos commented Feb 8, 2022

sinisaos commented Apr 15, 2022

dantownsend commented Apr 15, 2022

sinisaos commented Apr 15, 2022

dantownsend Jan 21, 2022 •

edited

Loading

codecov-commenter commented Jan 22, 2022 •

edited

Loading

sinisaos commented Jan 28, 2022 •

edited

Loading

sinisaos commented Jan 29, 2022 •

edited

Loading