Add writable flag to meta stores #256

kiksekage · 2022-02-28T14:42:25Z

Fixes #255

The MetaStore class is set to be writable by default:
https://github.com/DHI-GRAS/terracotta/blob/a3aedcbfcd95aed279dd9754434b6409462b4bc1/terracotta/drivers/base_classes.py#L41

Added a very thin decorator to throw the new DatabaseNotWritable exception:
https://github.com/DHI-GRAS/terracotta/blob/a3aedcbfcd95aed279dd9754434b6409462b4bc1/terracotta/drivers/terracotta_driver.py#L23-L34

And then we access the flag directly when getting metadata from the meta store:
https://github.com/DHI-GRAS/terracotta/blob/22353ca9c3f99dbe4cdb70b7497d93162827713a/terracotta/drivers/terracotta_driver.py#L187-L194

Me and @nickeopti were discussing whether or not it was cleanest to handle the database "writability" in TerracottaDriver or all meta stores individually.

…/terracotta into issue225-lazy-writeable

codecov · 2022-02-28T15:00:08Z

Codecov Report

Merging #256 (f07de5e) into main (7011a23) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
+ Coverage   98.54%   98.76%   +0.22%     
==========================================
  Files          48       49       +1     
  Lines        2136     2194      +58     
  Branches      299      306       +7     
==========================================
+ Hits         2105     2167      +62     
+ Misses         19       17       -2     
+ Partials       12       10       -2

Impacted Files	Coverage Δ
terracotta/drivers/base_classes.py	`100.00% <100.00%> (ø)`
terracotta/drivers/relational_meta_store.py	`98.92% <100.00%> (+0.01%)`	⬆️
terracotta/drivers/sqlite_remote_meta_store.py	`100.00% <100.00%> (ø)`
terracotta/drivers/terracotta_driver.py	`100.00% <100.00%> (ø)`
terracotta/exceptions.py	`100.00% <100.00%> (ø)`
terracotta/server/flask_api.py	`96.20% <100.00%> (+5.66%)`	⬆️
terracotta/config.py	`100.00% <0.00%> (ø)`
terracotta/drivers/__init__.py	`100.00% <0.00%> (ø)`
terracotta/drivers/postgresql_meta_store.py	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7011a23...f07de5e. Read the comment docs.

dionhaefner · 2022-03-01T09:31:53Z

I like introducing a new exception. But I think it would be cleaner if the meta stores themselves would check for writeability and raise the exception if necessary, and we can just let that bubble up to the client. Then we can also have this:

class RemoteSQLiteRasterStore:
    @requires_writable
    def insert(self, *args, **kwargs):
        pass  # always unwritable

In this case writable could be a private attribute self._writable, and all interactions could happen via this exception. For example, to raise a better error in conjunction with lazy loading we could catch the exception and re-raise it with a different message:

try:
    self.insert(keys, path, metadata=metadata) 
except DatabaseNotWritable as exc:
    raise DatabaseNotWritable("Lazy loading requires a writable database") from exc

kiksekage · 2022-03-07T15:12:26Z

I moved the handling into the meta stores instead, with the explicit catching as well. If it looks good to you @dionhaefner I'll write some tests to up the coverage 👍

dionhaefner

Yes that looks pretty good. Thanks!

I think we will also need an error handler for the new exception but the tests should reveal that.

BTW, can we check whether the other drivers are writable via SQLalchemy? It should just boil down to checking whether the current user has write permissions on the used database.

terracotta/drivers/sqlite_remote_meta_store.py

terracotta/drivers/terracotta_driver.py

terracotta/exceptions.py

kiksekage · 2022-03-23T12:11:43Z

The failing macos tests go through now and the PR should be ready to merge if you have the time @dionhaefner @mrpgraae 👍

dionhaefner · 2022-03-23T13:48:00Z

I think we will also need an error handler for the new exception but the tests should reveal that.

BTW, can we check whether the other drivers are writable via SQLalchemy? It should just boil down to checking whether the current user has write permissions on the used database.

Could you follow up on these points? What's the status?

kiksekage · 2022-03-23T13:59:25Z

Could you follow up on these points? What's the status?

Good point, I will have a look at that.

kiksekage · 2022-03-28T13:36:17Z

BTW, can we check whether the other drivers are writable via SQLalchemy? It should just boil down to checking whether the current user has write permissions on the used database.

I can't seem to find any ORM/core way of doing this in sqlalchemy, though I might be looking in the wrong directions. This and this (very old) issue seem to indicate that interfacing with user privileges is done the raw SQL query way, and then we are back to having to discern between which backend we are using and what syntax to use.
I guess we could do a write to any/all of the four tables and a rollback at meta_store instantiation time, but that also doesn't sit right with me.

I think we will also need an error handler for the new exception but the tests should reveal that.

I confused myself, does this refer to adding exceptions.DatabaseNotWritableError to the API error handlers here: https://github.com/DHI-GRAS/terracotta/blob/b5ee523be89dba5a622633751f4181ad7f827246/terracotta/server/flask_api.py#L41

dionhaefner · 2022-03-29T09:00:07Z

Too bad with the permissions. Maybe doing it in SQL is not so bad? But then again it might not be portable between different DB vendors? Your call.

I confused myself, does this refer to adding exceptions.DatabaseNotWritableError to the API error handlers here:

Yes, exactly. Right now this will throw a 500 internal server error, but maybe we would want something more descriptive.

nickeopti

I think we will also need an error handler for the new exception but the tests should reveal that.

I confused myself, does this refer to adding exceptions.DatabaseNotWritableError to the API error handlers here:

Yeah, that will need a flask error handler, like the ones you referenced, @kiksekage. However, it seems that we don't have any tests for lazy-loading metadata on API/server level. I guess we should add that. I must admit I haven't quite grasped how that setup works, so I haven't added one.

terracotta/drivers/relational_meta_store.py

terracotta/drivers/sqlite_remote_meta_store.py

terracotta/drivers/terracotta_driver.py

kiksekage · 2022-03-30T13:19:11Z

Too bad with the permissions. Maybe doing it in SQL is not so bad? But then again it might not be portable between different DB vendors? Your call.

I think, with all the work that went into sqlachemy-fying the DB handling it hit me the wrong way to start doing some vendor-specific things again. I'm in favor of not checking it and letting it be up to the user.

I have added an error handler, and as far as I could tell, the only way for a API user to hit this error would be to request metadata for a dataset which is present but doesn't have any pre-calculated metadata. The test uses the already existing fixtures for generating sqlite-remote databases for testing so I moved that functionality to conftest.py.

dionhaefner · 2022-03-31T08:43:02Z

I think, with all the work that went into sqlachemy-fying the DB handling it hit me the wrong way to start doing some vendor-specific things again. I'm in favor of not checking it and letting it be up to the user.

Sounds like sunk cost fallacy to me :) But it's fine for now, the error from SQLalchemy should be descriptive enough to alert users as to what's going on.

tests/server/test_flask_api.py

tests/conftest.py

Co-authored-by: Dion Häfner <[email protected]>

tests/drivers/test_raster_drivers.py

dionhaefner

LGTM! Thanks @kiksekage

kiksekage · 2022-04-01T08:01:41Z

LGTM! Thanks @kiksekage

Should I do the honors of merging or will you @dionhaefner? 😄

kiksekage and others added 2 commits February 28, 2022 14:48

Add writable flag to meta store classes

a3aedcb

Always compute metadata

22353ca

kiksekage mentioned this pull request Feb 28, 2022

TerracottaDriver.get_metadata assumes meta store is writeable when lazy-loading metadata #255

Closed

kiksekage added 2 commits February 28, 2022 15:54

🐛

8cd1c6a

Merge branch 'issue225-lazy-writeable' of https://github.com/DHI-GRAS…

ef4d2ec

…/terracotta into issue225-lazy-writeable

Don't assert always true statement

70d314b

Handle writability in meta stores

372ee17

dionhaefner reviewed Mar 7, 2022

View reviewed changes

terracotta/drivers/sqlite_remote_meta_store.py Outdated Show resolved Hide resolved

terracotta/drivers/terracotta_driver.py Show resolved Hide resolved

terracotta/exceptions.py Outdated Show resolved Hide resolved

kiksekage added 2 commits March 8, 2022 10:51

Error naming and specific try-catch

a423d6e

Add unwritable TC driver test

b5ee523

Also require writable for database creation

4c88475

nickeopti reviewed Mar 29, 2022

View reviewed changes

terracotta/drivers/relational_meta_store.py Show resolved Hide resolved

terracotta/drivers/sqlite_remote_meta_store.py Outdated Show resolved Hide resolved

terracotta/drivers/terracotta_driver.py Show resolved Hide resolved

kiksekage added 3 commits March 30, 2022 14:28

Add non-writable error handler

7c4fb65

Add flask debug exception tests

cad011e

💄

1c647d0

dionhaefner reviewed Mar 31, 2022

View reviewed changes

tests/server/test_flask_api.py Outdated Show resolved Hide resolved

Monkeypatch meta_store._WRITABLE

e081355

dionhaefner reviewed Mar 31, 2022

View reviewed changes

tests/conftest.py Outdated Show resolved Hide resolved

Actually use the monkeypatch...

d476ac9

Co-authored-by: Dion Häfner <[email protected]>

dionhaefner reviewed Mar 31, 2022

View reviewed changes

tests/drivers/test_raster_drivers.py Outdated Show resolved Hide resolved

Use the contexts

f07de5e

dionhaefner approved these changes Mar 31, 2022

View reviewed changes

dionhaefner merged commit 0f975e7 into main Apr 1, 2022

dionhaefner deleted the issue225-lazy-writeable branch April 1, 2022 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add writable flag to meta stores #256

Add writable flag to meta stores #256

kiksekage commented Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

dionhaefner commented Mar 1, 2022 •

edited

Loading

kiksekage commented Mar 7, 2022

dionhaefner left a comment

kiksekage commented Mar 23, 2022

dionhaefner commented Mar 23, 2022

kiksekage commented Mar 23, 2022

kiksekage commented Mar 28, 2022

dionhaefner commented Mar 29, 2022

nickeopti left a comment

kiksekage commented Mar 30, 2022

dionhaefner commented Mar 31, 2022

dionhaefner left a comment

kiksekage commented Apr 1, 2022

Add writable flag to meta stores #256

Add writable flag to meta stores #256

Conversation

kiksekage commented Feb 28, 2022

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

dionhaefner commented Mar 1, 2022 • edited Loading

kiksekage commented Mar 7, 2022

dionhaefner left a comment

Choose a reason for hiding this comment

kiksekage commented Mar 23, 2022

dionhaefner commented Mar 23, 2022

kiksekage commented Mar 23, 2022

kiksekage commented Mar 28, 2022

dionhaefner commented Mar 29, 2022

nickeopti left a comment

Choose a reason for hiding this comment

kiksekage commented Mar 30, 2022

dionhaefner commented Mar 31, 2022

dionhaefner left a comment

Choose a reason for hiding this comment

kiksekage commented Apr 1, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

dionhaefner commented Mar 1, 2022 •

edited

Loading