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

Alter table support for the JSON write API #2101

Closed
simonw opened this issue Jul 13, 2023 · 9 comments
Closed

Alter table support for the JSON write API #2101

simonw opened this issue Jul 13, 2023 · 9 comments

Comments

@simonw
Copy link
Owner

simonw commented Jul 13, 2023

Requested here: https://discord.com/channels/823971286308356157/823971286941302908/1129034187073134642

The former datasette-insert plugin had an option ?alter=1 to auto-add new columns. Does the JSON write API also have this?

Original issue title: alter: true support for JSON write API

@simonw
Copy link
Owner Author

simonw commented Jul 13, 2023

def insert_or_upsert_rows(conn):
table = sqlite_utils.Database(conn)[table_name]
kwargs = {}
if upsert:
kwargs["pk"] = pks[0] if len(pks) == 1 else pks
else:
kwargs = {"ignore": ignore, "replace": replace}
if should_return and not upsert:
rowids = []
method = table.upsert if upsert else table.insert
for row in rows:
rowids.append(method(row, **kwargs).last_rowid)
return list(
table.rows_where(
"rowid in ({})".format(",".join("?" for _ in rowids)),
rowids,
)
)
else:
method_all = table.upsert_all if upsert else table.insert_all
method_all(rows, **kwargs)

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

I just found my own need for this. I was trying to import a big bunch of JSON to create a table - https://schedules.ire.org/nicar-2024/nicar-2024-schedule.json - and I got an error because one of the later entries had new columns in it.

Thoughts:

  • When creating a table using a POST to /db/-/create with example "rows", should alter=True be assumed? Or should you need to pass it as a separate key?
  • "alter": true should definitely be supported for both insert and upsert (and row update as well)
  • Permissions is a challenge. Currently we have create-table and drop-table - but there is no alter-table permission

I think I'll need to add a new alter-table permission for that.

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

I'm really torn on whether or not you should need to pass alter: true to the create table API to handle later rows that have more columns.

That feels a bit confusing to me. The whole point of that API is "create a table that fits these example rows", so the fact that you had to alter later isn't really something the user should have to think about.

I'm going to set alter True on the cases where a table is created from example rows.

But...

https://docs.datasette.io/en/latest/json_api.html#creating-a-table-from-example-data

You can call the create endpoint multiple times for the same table provided you are specifying the table using the rows or row option. New rows will be inserted into the table each time. This means you can use this API if you are unsure if the relevant table has been created yet.

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

Ideal would be for POST to /db/-/create to be treated as alter: true if the table doesn't exist yet (ignoring if the user has alter-table permission), but if it DOES exist it only does alter: true if the user requested it AND they have the permission.

@simonw simonw changed the title alter: true support for JSON write API Alter table support for the JSON write API Feb 8, 2024
@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

It would be good to provide an explicit API for alter table as well, if we're adding a permission and an event type for it.

Maybe this:

POST /db/table/-/alter

Question is, what should the JSON look like? And that varies depending on what alter operations are supported.

If we're just adding columns it's pretty simple. But what about more advanced operations? Should this provide access to the full set of operations supported by sqlite-utils transform? https://sqlite-utils.datasette.io/en/stable/python-api.html#python-api-transform

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

I'm going to handle that /-/alter endpoint as a separate piece of work from the simpler "alter": true support.

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

In working on this I spotted a bug with the way events work for /db/-/create - it currently fires a create-table event any time you call that endpoint, even if calling it actually just inserted rows into an existing table. It also doesn't fire an insert-rows event ever.

Instead it should do this:

  • If table is created with example rows, fire create-table and then insert-rows
  • If table already exists and more rows are added, fire insert-rows

@simonw
Copy link
Owner Author

simonw commented Feb 8, 2024

Tested in Datasette Cloud - it worked, including requiring the permission:

CleanShot 2024-02-08 at 13 44 39@2x

simonw added a commit that referenced this issue Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant