fix: delete deletable tilesets and tiles when deleting style #47
Conversation
achou11
changed the title
achou11
force-pushed
the
ac/fully-delete-styles
branch
from
d291505
to
c055633
Compare
achou11
force-pushed
the
ac/fully-delete-styles
branch
from
44e9785
to
0224129
Compare
achou11
force-pushed
the
ac/fully-delete-styles
branch
2 times, most recently
from
3fc80f0
to
d8b3790
Compare
achou11
changed the title
src/api.ts
Outdated
| db.prepare( | ||
| ` | ||
| CREATE VIEW DeletableTilesetIds AS | ||
| SELECT SourceToTilesetIdOuter.value AS tilesetId, Style.id AS styleId | ||
| FROM Style, json_each(Style.sourceIdToTilesetId, '$') AS SourceToTilesetIdOuter | ||
| JOIN ( | ||
| SELECT SourceToTilesetIdInner.value AS tilesetId, COUNT(SourceToTilesetIdInner.value) AS freq | ||
| FROM Style, json_each(Style.sourceIdToTilesetId, '$') AS SourceToTilesetIdInner | ||
| GROUP BY SourceToTilesetIdInner.value | ||
| ) AS TilesetIdFreq ON SourceToTilesetIdOuter.value = TilesetIdFreq.tilesetId | ||
| WHERE freq = 1; | ||
| ` | ||
| ).run() |
There was a problem hiding this comment.
have no idea if there's a best practice around using views 🤷 at the moment, we create a view, run some statements using it, then delete it
also, view statements can't take params, which would've made this + the following statements a little less verbose
There was a problem hiding this comment.
Quick thoughts based on 5 mins looking at this. Do we already have / plan to add a deleteTileset function? Does it also delete all tiles that are part of that tileset?
I think that rather than trying to do everything in a single transaction (which I know has the advantage of being atomic), we separate this out:
- Query the tilesetIds that need to be deleted
- Call api.deleteTileset() on each of those ids (also handles tiledata deletions?)
- Delete the style and other resources (offline areas, sprites)
That will maybe make this more manageable? And avoid re-implementing logic in deleteTileset and deleteStyle.
| db.prepare( | ||
| 'DELETE FROM Import WHERE areaId IN (SELECT id FROM OfflineArea WHERE styleId = ?)' | ||
| ).run(id) |
There was a problem hiding this comment.
I don't think we want to delete anything from the import table? I guess we could, but it's fine I think to keep that record in there.
There was a problem hiding this comment.
what about the offline areas table? thought it would make sense to delete there, but in order to do that, we'd need to either delete the associated records in the import table, or update the import table to dereference the offline areas
There was a problem hiding this comment.
Yes, good point, let's delete the imports too.
|
yeah we already have something for that. not sure it deletes the tiles though at the moment. will look into it! |
was wrong about this, we don't have an implementation for deleting tilesets. something to work on! |
Just a hunch, wondering if having so many open connections to the same DB is causing the errors?
…-server into pr/achou11/47 * 'ac/fully-delete-styles' of github.com:digidem/mapeo-map-server: remove unused deleteTileset method feat: add offline sprite support (#42) improve test for style deletion
This reverts commit 71f538f.
|
I think this is good to go now. |
Fixes #3
potentially running into an issue with the statements that I use to delete the tilesets. see WiseLibs/better-sqlite3#842 for details