temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps - #7901
temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps#7901ninsbl wants to merge 10 commits into
Conversation
github-actions
Bot
added
temporal
|
Related to #7670 |
| unit=None, | ||
| increment=None, | ||
| dbif=None, | ||
| dbif: SQLDatabaseInterfaceConnection | None = None, |
There was a problem hiding this comment.
You say you will remove this in a future version. I have two problems with that:
If you remove, you'll need to make the argument passed keyword only at least at that point of the argument list, so it isn't used as positional arguments by other code, that will break when changing versions. We looked up this pattern in July, with * and /.
Next, I'm not sure it's going to do the right thing. Usually, a design pattern named dependency injection (part of the broad IoC: inversion of control), says that you should be giving your functions all the dependencies from the outside. The finality is that the function doesn't really depend on the outside, and shouldn't be affected by global scope: way easier to test.
So, will removing it be an improvement because you'll actually don't need it, or it will be increasing the coupling with either other functions, o the global state?
There was a problem hiding this comment.
I see. The reason for me to suggest to deprecate the dbif parameter is, that it will be ignored after the change. Creating the connection is relatively cheap and if the function would take dbif as input it would have to make sure the DB interface does not contain connections to other mapsets. So disallowing dbif as input would be clearer and safer. Of all ~115 usages of the function in the code base (mainly unittests), less than a hand-full actually ever passed a real DB-interface other than None or defalt by ommission.
Unfortunately, almost all temporal functions depend on users running tgis.init() which sets a couple of globals (and creates a temporal database if it does not exist).
We could reduce the globals-dependency by using "." as input to SQLDatabaseInterfaceConnection instead of get_current_mapset() (with the latter reading from globals).
In any case, coupling with other functions or global state will at least not increase by the suggested changes - as far as I can see - because if the user passes None as dbif (before this change), dbif would be created, just like after this change, only with init_dbif function which has the same dependencies to tgis.init() as the SQLDatabaseInterfaceConnection....
That said, I removed all usage of the dbif parameter in this function in the code base, to make sure a deprecation warning would not trigger.
I am more than open to suggestion for handling the little useful dbif parameter going forward...
There was a problem hiding this comment.
Is there an impact on user code that would use that function explicitly? Meaning: do we know if there's a lot of important usages of that function?
There was a problem hiding this comment.
Only usage of the function I found in user-facing code is t.register and stds_import. It is also used in a hand full of addons: https://github.com/search?q=repo%3AOSGeo%2Fgrass-addons+register_maps_in_space_time_dataset&type=code&p=1, but none passes a dbif object to the function.
I checked if the changes can have any impact on the dbif object a user may pass to the function. Before this change, dbif was modified into an inconsistent change where all connections to other mapsets are removed. After the change, dbif is left unchanged.
While investigating, I noticed issues with selecting info of a raster map form the TGIS database. Fixed it in: 939ea9e
There was a problem hiding this comment.
I am open to making dbif a key-word-only argument. But I am wondering if that would not change the signature of the function already now?
There was a problem hiding this comment.
It is a change, it would fail, but at least it would give a clear error that is easy to address and it wouldn't silently break anything.
There was a problem hiding this comment.
Now I made dbif a keyword only argument. I hope I understood correctly.
There was a problem hiding this comment.
Now I made dbif a keyword only argument. I hope I understood correctly.
dbif and all the following after too, just so you know (interval, fa and update_cmd_list too are keyword only now)
There was a problem hiding this comment.
Right. I was not sure what the best course of action would be here. Re-ordering of arguments (moving dbif down) or converting more other arguments... Hope that is OK as is, but I happily adjust if preferable...
| "are different." | ||
| ).format( | ||
| t=map_object_type, | ||
| mid=map_object_id, |
There was a problem hiding this comment.
This is pre-existing, but 'id' and 'mid' don't match.
| unit=None, | ||
| increment=None, | ||
| dbif=None, | ||
| dbif: SQLDatabaseInterfaceConnection | None = None, |
There was a problem hiding this comment.
It is a change, it would fail, but at least it would give a clear error that is easy to address and it wouldn't silently break anything.
| # We need to update all datasets after the removement of maps | ||
| map.metadata.select(dbif) | ||
| datasets = map.get_registered_stds(dbif) | ||
| map_item.metadata.select(dbif) |
There was a problem hiding this comment.
Should 'mapset' be added here as well?
3 participants
Replaces a hack that modified a broad SQLDatabaseInterfaceConnection with a clean DB connection limited only for the current mapset.