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

Batch updating column using /tables/ api should reference column using id instead of column_index #1167

Closed
Tracked by #839
silentninja opened this issue Mar 14, 2022 · 12 comments · Fixed by #1307
Closed
Tracked by #839
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@silentninja
Copy link
Contributor

silentninja commented Mar 14, 2022

Problem

Batch updating Columns using the columns field in the patch method of the /tables/{id}/ does not use id of the column rather the column to update is identified based on the position of the column objects. This is not ideal due to the following reasons:

  • Similar to the column_index which is deprecated in favour of id.
  • Column position could have changed resulting in updating a wrong column

Proposed solution

The following changes have to be made:

-PATCH method of the /tables/{id} api which has the following structure

{
    "columns": [
        {
            "name": "id",
            "type": "INTEGER"
        },
        {
            "name": "Center",
            "type": "TEXT"
        }
    ]
}

should be replaced with

{
    "columns": [
        {
            "id": 1, // Use column id to reference a column, rather than assuming the column based on its index.
            "name": "id",
            "type": "INTEGER"
        },
        {
            "id": 2,
            "name": "Center",
            "type": "TEXT"
        }
    ]
}
  • The column id's have to be converted into attnums which can be fetched from the column object before sending the column data from the service layer to the database layer(before calling any function from the db module)
@silentninja silentninja added type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL ready Ready for implementation labels Mar 14, 2022
@silentninja silentninja added this to the [07] Initial Data Types milestone Mar 14, 2022
@silentninja silentninja self-assigned this Mar 24, 2022
@silentninja silentninja added status: started and removed ready Ready for implementation labels Mar 24, 2022
@ratika-12
Copy link
Contributor

@silentninja Can I work on this?

@silentninja
Copy link
Contributor Author

silentninja commented Mar 24, 2022

@ratika-12 This issue needs some understanding of the codebase and Django. I will confirm in a few minutes, sorry for the wait.

@silentninja
Copy link
Contributor Author

@ratika-12 I have assigned the issue to you, thanks!

@silentninja silentninja assigned ratika-12 and unassigned silentninja Mar 24, 2022
@silentninja
Copy link
Contributor Author

@ratika-12 I have updated the proposed solution to make it easier to understand. Let me know if anything is not clear.

@kgodey
Copy link
Contributor

kgodey commented Apr 4, 2022

@ratika-12 Are you still working on this task? I figured I would follow up since it's been a while and there has not been a PR yet.

@ratika-12
Copy link
Contributor

ratika-12 commented Apr 4, 2022

@kgodey Yes I am working on this task. I will make a pr for this asap.

@silentninja
Copy link
Contributor Author

@ratika-12 Are you still working on this task? Just wanted to follow up. Also please push your work to a draft PR, so that it is easy to follow up and suggest changes

@Anish9901
Copy link
Member

@kgodey @silentninja Can I work on this if this is not being worked on currently by @ratika-12?

@ratika-12
Copy link
Contributor

Yeah sure !!

@kgodey kgodey assigned Anish9901 and unassigned ratika-12 Apr 19, 2022
@kgodey
Copy link
Contributor

kgodey commented Apr 19, 2022

Thanks @Anish9901, assigned to you. And thanks for the update @ratika-12.

@Anish9901
Copy link
Member

Anish9901 commented Apr 21, 2022

The column id's have to be converted into attnums which can be fetched from the column object before sending the column data from the service layer to the database layer(before calling any function from the db module)

@silentninja Does the attnums have to be passed as a list of attnums to a db layer function or does the id in validated_data is needed to be replaced with its corresponding attnum?

@silentninja
Copy link
Contributor Author

id in validated_data is needed to be replaced with its corresponding attnum?

This should be the preferred option db layer should not know about the Django model fields like id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants