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

Modify records API to use column id instead of column name #896

Closed
seancolsen opened this issue Dec 16, 2021 · 11 comments · Fixed by #1047
Closed

Modify records API to use column id instead of column name #896

seancolsen opened this issue Dec 16, 2021 · 11 comments · Fixed by #1047
Assignees
Labels
type: enhancement New feature or request work: backend Related to Python, Django, and simple SQL

Comments

@seancolsen
Copy link
Contributor

seancolsen commented Dec 16, 2021

I'd like to be able to use the records API when all I know and care about is the column id for my columns.

GET request params

The order_by, group_count_by, and filters params all expect references to columns to be given by name. For example:

  • order_by=[{"field":"name","direction":"asc"}]
  • group_count_by=["name"]
  • filters={"and":[{"field":"name","op":"eq","value":"Sean"}]}

I'd like to the API to accept column references by id.

As a breaking change

A simple way to address this need would be to break the API, no longer accepting column references by name

  • order_by=[{"field":"1","direction":"asc"}]
  • group_count_by=["1"]
  • filters={"and":[{"field":"1","op":"eq","value":"Sean"}]}

As a backwards compatible change

Another approach would be to accept both references by name and by id. If I want to use the column id, I would do:

  • order_by=[{"field_id":"1","direction":"asc"}]
  • group_count_by_ids=["1"]
  • filters={"and":[{"field_id":"1","op":"eq","value":"Sean"}]}

GET Response

Currently the records API response look like:

{
  "count": 2,
  "results": [
    {
      "id": 1,
      "first_name": "Bart",
      "last_name": "Simpson",
    },
    {
      "id": 2,
      "first_name": "Lisa",
      "last_name": "Simpson",
    }
  ]
}

To avoid relying on the column name, I'd like the response to give the column id as keys instead.

{
  "count": 2,
  "results": [
    {
      "1": 1,
      "2": "Bart",
      "3": "Simpson",
    },
    {
      "1": 2,
      "2": "Lisa",
      "3": "Simpson",
    }
  ]
}

This would need to be a breaking change.

POST and PATCH request and response

Request and response bodies look like:

{
  "name": "Sean"
}

I'd like to be able to update a record or add a new record by sending something like:

{
  "1": "Sean"
}

Additional context

@seancolsen seancolsen changed the title Modify records API to use column id instead of column name Modify records API response to use column id instead of column name Dec 16, 2021
@seancolsen seancolsen changed the title Modify records API response to use column id instead of column name Modify records API to use column id instead of column name Dec 16, 2021
@seancolsen seancolsen added the work: backend Related to Python, Django, and simple SQL label Dec 16, 2021
@mathemancer
Copy link
Contributor

I think the breaking change option is superior (though a bit of a pain).

@kgodey
Copy link
Contributor

kgodey commented Dec 17, 2021

Agreed re: breaking change.

@silentninja
Copy link
Contributor

I have a few concerns with this change

  1. I assume we would use the same format for views too and views might be having computed columns that would not go well with id.
  2. This would be useful if filters are long-lived, but I think we would be creating views rather than storing the filter queries
  3. I think we discussed having tables filtered/sorted by a user-specified default in which case we might have to store them. So in such cases, I think the backend should rather do the conversion(name->id) and store it.

@kgodey
Copy link
Contributor

kgodey commented Jan 14, 2022

  1. I assume we would use the same format for views too and views might be having computed columns that would not go well with id.

We can figure out how to reflect Views when it comes to it, I don't know if we'll end up following the same format or not.

  1. This would be useful if filters are long-lived, but I think we would be creating views rather than storing the filter queries

The frontend saves the filter queries that a user was last looking at. We might also cache this in the backend at some point.

  1. I think we discussed having tables filtered/sorted by a user-specified default in which case we might have to store them. So in such cases, I think the backend should rather do the conversion(name->id) and store it.

I'm not sure what you're getting at here, can you elaborate?


The general principle here is the column only has a single identifier throughout the API, which is its id. This will also be more safe to work with, if someone drops the original column and creates a new column with the same name outside of Mathesar, we won't operate on it thinking it's the same column because the id will have changed during reflection.

@silentninja
Copy link
Contributor

silentninja commented Jan 14, 2022

I think we discussed having tables filtered/sorted by a user-specified default in which case we might have to store them. So in such cases, I think the backend should rather do the conversion(name->id) and store it.

We discussed that a table can have a default filter or sort. So in case we end up not using id I was suggesting an alternate solution where the frontend would send it as name and backend would store it internally as id

@silentninja silentninja removed their assignment Jan 18, 2022
@mathemancer
Copy link
Contributor

  1. I assume we would use the same format for views too and views might be having computed columns that would not go well with id.

Views have their own oid and the columns in views have their own attnum, so it's perfectly fine (with me at least) to give them ids. I.e., we should be creating view column models which inherit from normal table columns, and have a couple extra attributes for their other metadata. @kgodey If we set things up in Django properly, is it possible for the standard generated Django ids to be "global" across both column types?

@mathemancer
Copy link
Contributor

Alternatively, we could extend the column model to simply note whether it's a column of a table or of a view, and have some optional fields for view-specific metadata.

@kgodey kgodey added ready Ready for implementation and removed status: started labels Jan 19, 2022
@kgodey
Copy link
Contributor

kgodey commented Jan 19, 2022

@kgodey If we set things up in Django properly, is it possible for the standard generated Django ids to be "global" across both column types?

Yup, we can have it be the same Django Column model if we want.

Alternatively, we could extend the column model to simply note whether it's a column of a table or of a view, and have some optional fields for view-specific metadata.

I think that's the same thing, not an alternative. We could have separate models for TableColumn and ViewColumn if we want alternatively.

@silentninja
Copy link
Contributor

My only concern is that this would limit us from using dynamically computed columns that could be generated by various means like group_by but general consensus is to use views, so I am okay with this change.

@kgodey
Copy link
Contributor

kgodey commented Jan 19, 2022

My only concern is that this would limit us from using dynamically computed columns that could be generated by various means like group_by but general consensus is to use views, so I am okay with this change.

If we do expose dynamically computed columns, we won't be using the same models as table columns or view columns to do so since they represent different underlying DB concepts. We can figure out the appropriate architecture for those when we get to them.

@seancolsen
Copy link
Contributor Author

seancolsen commented Feb 7, 2022

I moved this to [06] Working with Tables because it it blocking some other work in that milestone.

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
4 participants