Skip to content

temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps #7901

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.

Sign up for GitHub

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

Open
ninsbl wants to merge 7 commits into
base: main
Choose a base branch
from
+44 −42
Open

temporal: create limited connection to TGIS DB in current mapset for (un-) registering maps #7901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
06a52d9
remove hack for connecting only current mapset
ninsbl Sep 5, 2026
e585ab4
remove hack for connecting only current mapset
ninsbl Sep 5, 2026
6e628be
remove usage of dbif
ninsbl Sep 6, 2026
3ffc7fb
remove usage of dbif
ninsbl Sep 6, 2026
a912b92
remove usage of dbif
ninsbl Sep 6, 2026
6ed7b2f
remove usage of dbif
ninsbl Sep 6, 2026
232e8fc
reword deprecation warning
ninsbl Sep 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 32 additions & 20 deletions python/grass/temporal/register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@
import grass.script as gs

from .abstract_map_dataset import AbstractMapDataset
from .core import get_current_mapset, get_tgis_message_interface, init_dbif
from .core import (
get_current_mapset,
get_tgis_message_interface,
init_dbif,
SQLDatabaseInterfaceConnection,
)
from .datetime_math import (
check_datetime_string,
increment_datetime_by_string,
Expand All @@ -33,15 +38,15 @@


def register_maps_in_space_time_dataset(
type,
name,
type: str,
name: str | None = None,
maps=None,
file=None,
start=None,
end=None,
unit=None,
increment=None,
dbif=None,
dbif: SQLDatabaseInterfaceConnection | None = None,

@echoix echoix Sep 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ninsbl ninsbl Sep 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

@echoix echoix Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

interval: bool = False,
fs: str = "|",
update_cmd_list: bool = True,
Expand Down Expand Up @@ -72,7 +77,8 @@ def register_maps_in_space_time_dataset(
:param increment: Time increment between maps for time stamp creation
(format absolute: NNN seconds, minutes, hours, days,
weeks, months, years; format relative: 1.0)
:param dbif: The database interface to be used
:param dbif: The database interface to be used (deprecated,
will be removed in future versions)
:param interval: If True, time intervals are created in case the start
time and an increment is provided
:param fs: Field separator used in input file
Expand Down Expand Up @@ -117,17 +123,19 @@ def register_maps_in_space_time_dataset(
if not maps and not file:
msgr.fatal(_("Please specify maps or file"))

# We may need the mapset
if dbif is not None:
msgr.warning(
_(
"The dbif argument is deprecated and will be removed in a future release."
)
)
# Create a new DB connection only for the current mapset
mapset = get_current_mapset()
dbif, connection_state_changed = init_dbif(dbif)

# create new stds only in the current mapset
# remove all connections to any other mapsets
# ugly hack !
currcon = {mapset: dbif.connections[mapset]}
dbif.connections = currcon
dbif = SQLDatabaseInterfaceConnection(mapsets=mapset)
dbif.connect()

# The name of the space time dataset is optional
sp = None
if name:
sp = open_old_stds(name, type, dbif)

Expand Down Expand Up @@ -175,7 +183,7 @@ def register_maps_in_space_time_dataset(
# Check if last column is an end time or a semantic label.
# Relative timestamps are integers, absolute timestamps are
# datetime strings; anything else is a semantic label.
if sp.is_time_relative():
if sp is not None and sp.is_time_relative():
try:
int(line_list[2])
except ValueError:
Expand Down Expand Up @@ -251,6 +259,7 @@ def register_maps_in_space_time_dataset(
map_object_layer = map_object.get_layer()
map_object_type = map_object.get_type()
if not map_object.map_exists():
dbif.close()
msgr.fatal(
_("Unable to update {t} map <{mid}>. The map does not exist.").format(
t=map_object_type, mid=map_object_id
Expand All @@ -277,7 +286,7 @@ def register_maps_in_space_time_dataset(
msgr.fatal(
_(
"Unable to register {t} map <{mid}> with "
"layer {l}. The map has timestamp and "
"layer {l}. The map has no timestamp and "
"the start time is not set."
).format(
t=map_object_type,
Expand All @@ -297,8 +306,10 @@ def register_maps_in_space_time_dataset(
# We need to check if the time is absolute and the unit was specified
time_object = check_datetime_string(start)
if isinstance(time_object, datetime) and unit:
dbif.close()
msgr.fatal(_("unit can only be set for relative time"))
if not isinstance(time_object, datetime) and not unit:
dbif.close()
msgr.fatal(_("unit must be set in case of relative time stamps"))

if unit:
Expand Down Expand Up @@ -375,8 +386,7 @@ def register_maps_in_space_time_dataset(
# Try to read an existing time stamp from the grass spatial database
# in case this map wasn't already registered in the temporal database
# Read the spatial database time stamp only, if no time stamp was provided for
# this map
# as method argument or in the input file
# this map as method argument or in the input file
if not is_in_db and not start:
map_object.read_timestamp_from_grass()

Expand Down Expand Up @@ -457,8 +467,7 @@ def register_maps_in_space_time_dataset(
ds.select(dbif)
ds.update_from_registered_maps(dbif)

if connection_state_changed is True:
dbif.close()
dbif.close()

msgr.percent(num_maps, num_maps, 1)

Expand Down Expand Up @@ -642,7 +651,10 @@ def register_map_object_list(
output_stds_id = output_stds.get_id() if output_stds else None

register_maps_in_space_time_dataset(
type, output_stds_id, unit=unit, file=filename, dbif=dbif
type,
output_stds_id,
unit=unit,
file=filename,
)

# Remove empty maps and unregister them from the temporal database
Expand Down
1 change: 0 additions & 1 deletion python/grass/temporal/stds_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,6 @@ def import_stds(
start="file",
end="file",
unit=relative_time_unit,
dbif=None,
fs=fs,
update_cmd_list=False,
)
Expand Down
2 changes: 0 additions & 2 deletions python/grass/temporal/testsuite/test_register_function.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,6 @@ def test_history_raster(self) -> None:
start="2001-01-01 10:30:01",
increment="1 year",
interval=True,
dbif=self.dbif,
)

map_1 = tgis.RasterDataset("elevation@PERMANENT")
Expand All @@ -340,7 +339,6 @@ def test_history_vector(self) -> None:
start="2001-01-01 10:30:01",
increment="1 year",
interval=True,
dbif=self.dbif,
)

map_1 = tgis.VectorDataset("lakes@PERMANENT")
Expand Down
1 change: 0 additions & 1 deletion temporal/t.register/t.register.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ def main():
end=end,
unit=unit,
increment=increment,
dbif=None,
interval=interval,
fs=separator,
)
Expand Down
30 changes: 12 additions & 18 deletions temporal/t.unregister/t.unregister.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,19 +62,13 @@ def main():
if not maps and not file:
gs.fatal(_("%s= or %s= must be specified") % ("input", "file"))

mapset = gs.gisenv()["MAPSET"]
mapset = tgis.get_current_mapset()

dbif = tgis.SQLDatabaseInterfaceConnection()
dbif = tgis.SQLDatabaseInterfaceConnection(mapsets=mapset)
dbif.connect()

# modify a stds only if it is in the current mapset
# remove all connections to any other mapsets
# ugly hack !
currcon = {}
currcon[mapset] = dbif.connections[mapset]
dbif.connections = currcon

# In case a space time dataset is specified
sp = None
if input:
sp = tgis.open_old_stds(input, type, dbif)

Expand All @@ -84,7 +78,7 @@ def main():

# Map names as comma separated string
if maps is not None and maps != "":
maplist = [maps] if maps.find(",") == -1 else maps.split(",")
maplist = maps.split(",")

# Build the maplist
for count in range(len(maplist)):
Expand Down Expand Up @@ -117,30 +111,30 @@ def main():
if count % 10 == 0:
gs.percent(count, num_maps, 1)

map = tgis.dataset_factory(type, mapid)
map_item = tgis.dataset_factory(type, mapid)

# Unregister map if in database
if map.is_in_db(dbif, mapset=mapset):
if map_item.is_in_db(dbif, mapset=mapset):
# Unregister from a single dataset
if input:
# Collect SQL statements
statement += sp.unregister_map(map=map, dbif=dbif, execute=False)
statement += sp.unregister_map(map=map_item, dbif=dbif, execute=False)

# Unregister from temporal database
else:
# 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)
datasets = map_item.get_registered_stds(dbif)
# Store all unique dataset ids in a dictionary
if datasets:
for dataset in datasets:
update_dict[dataset] = dataset
# Collect SQL statements
statement += map.delete(dbif=dbif, update=False, execute=False)
statement += map_item.delete(dbif=dbif, update=False, execute=False)
else:
gs.warning(
_("Unable to find %s map <%s> in temporal database")
% (map.get_type(), map.get_id())
% (map_item.get_type(), map_item.get_id())
)

count += 1
Expand All @@ -157,7 +151,7 @@ def main():
else:
gs.message(_("Unregister maps from the temporal database"))

if input:
if input and sp is not None:
sp.update_from_registered_maps(dbif)
sp.update_command_string(dbif=dbif)
elif len(update_dict) > 0:
Expand Down
Loading