[gkneighb]

Summary

Temporal read tools could only access a space time dataset from mapsets on the current search path, even when given a fully qualified name (name@mapset). This makes open_old_stds() resolve a fully qualified name from any accessible mapset, matching how other GRASS data types behave.

Part of #7641.

Approach

1. Add an idempotent SQLDatabaseInterfaceConnection.add_mapset() that pulls an accessible mapset's temporal database into an existing connection (building on the mapsets support from t.list: Add mapset options for t.list #7631 / temporal: add parameter to skip creation of TGIS DB in current mapset on tgis.init #7645; a no-op when the mapset is already present).
2. Call it in open_old_stds(), after the connection is initialized, so the dataset's own mapset is part of the connection before the dataset is queried.

Scope

Limited to tools that open datasets through open_old_stds. Tools that build their own database interface and bypass open_old_stds (e.g. t.info, which calls is_in_db(dbif) on an interface it created itself) must ensure the required mapset is added to that interface; that is left as follow-up. The write/create path is unchanged (datasets are still created only in the current mapset).

Test plan

New pytest suite python/grass/temporal/tests/grass_temporal_fully_qualified_names_test.py:

• add_mapset unit: adds an off-search-path mapset to a connection; idempotent.
• open_old_stds resolves name@mapset for a mapset off the search path.

pytest python/grass/temporal/tests/grass_temporal_fully_qualified_names_test.py

AI assistance

Implemented with the assistance of an AI coding assistant (Claude), reviewed and verified by the author.

[gkneighb]

Heads-up on the red macOS build check here: it's a pre-existing, unrelated test-isolation flake, not something in this PR. grass_temporal_core_init_skip_db_test.py::test_init_succeeds_without_db_creation depends on a sibling test running first to set the global tgis_backend; it fails standalone and under pytest-xdist even on main (pytest python/grass/temporal/tests/grass_temporal_core_init_skip_db_test.py -n 2). Adding test files in this PR just reshuffled the xdist worker distribution enough to expose it. Root cause is that init(skip_db_init=True) returns before setting tgis_backend (contrary to its docstring). Fix is up in #7671; once that merges I'll rebase this PR and CI should go green.

[ninsbl]

Hi @gkneighb and thanks for your PR. Extending the SQLDatabaseConnection class with an add_mapset method is one option to solve the issue that I have not thought about before. Could be a good approach.

However, injecting a call to that function to every attempt to fetch data from the TGIS DB is probably not what I would do, as the sql functions may get executed quite frequently, and there we would not want to have avoidable overhead.

I can imagine two slightly alternative approaches.

1. One approach may be to modify the open_stds functions to identify is the mapset in the ident is in the SQLDatabaseConnection and use your add_mapset function if not.
   I have not checked if all tools use that function set or rather e.g. dataset factory, or even the space_time_datasets We would have to research a bit. The advantade with such a solution on the library level is that we do not have to adjust all the tools.
2. Another approach would be that all tools that take STDS as input, do the following:
   a) get (all) required mapsets from user input (fully qualified dataset names)
   b) initialize SQLDatabaseConnection with those mapsets (in addition to the current mapset if write access is required).

[gkneighb]

On the overhead: agreed the connection's SQL methods (execute/fetchone/fetchall) are hot and shouldn't carry per-call work.

To scope the alternatives I checked which read paths actually reach an off-search-path mapset, and it's broader than open_stds:

• t.info doesn't use open_stds at all — it builds the dataset via dataset_factory() and calls is_in_db() / select() directly.
• Some reads issue their own dbif.execute(mapset=self.base.mapset) without going through is_in_db / select either — STRDSMetadata.get_semantic_labels() (metadata.py) and get_registered_maps() plus two other reads in abstract_space_time_dataset.py. t.info -g on a fully-qualified off-path STDS fails specifically at get_semantic_labels().

So Alt 1 (open_stds only) would leave t.info and the metadata reads unfixed.

Proposed middle ground that avoids the per-statement overhead: call add_mapset at the per-dataset read entry points instead of in the connection's SQL methods — i.e. is_in_db() / select() (base.py) plus the direct metadata reads (get_semantic_labels, get_registered_maps, and the two other execute(mapset=self.base.mapset) sites). That runs once per dataset access rather than per SQL statement, stays at the library level (no per-tool changes), and covers the bypass paths.

Does that direction work for you? If so I'll rework it that way.

For reference, I verified coverage on a two-mapset setup (STRDS in an off-search-path mapset, queried from PERMANENT): t.info, t.rast.list, t.rast.univar, t.topology all resolve name@mapset, and off-path writes stay blocked (t.remove still refuses). Full temporal suite passes under pytest-xdist.

[ninsbl]

OK, I think it would be reasonable to limit the scope of this PR to:

1. add the add_mapset method to SQLDatabaseConnection and
2. use it for now only in open_stds (dataset_factory is also used for e.g. registering single raster maps in STRDS, so I would try to avoid adding mapsets there).

Tools like t.info create the database interface separately, so they would have to make sure that the interface contains connections for the required mapsets. t.info then call database related function with that interface, e.g. is_in_db(dbif). So there the mapset should have been added earlier.

For such cases like it may make sense to create a helper function to check if an ID references a dataset on the search path mapset. But I am not sure where or how such a function should be implemented in detail. Would have to think about it a bit more...

[gkneighb]

Reworked to that scope: add_mapset is now used only in open_old_stds (removed from the connection's SQL methods, and not touching dataset_factory). Fully-qualified names resolve for open_old_stds-based tools; t.info and other tools that build their own interface are left as follow-up (noted in the commit and PR description). Updated the tests accordingly and repointed the PR to "Part of #7641" so it won't auto-close the issue while that follow-up remains. Force-pushed.

[ninsbl]

Now, that I edited the PR, I hope others can review.
Sorry, for messing around with your work @gkneighb . We have other stuff coming up depending on this, so I took the liberty to modify. Still, it is a very valuable contibution from you side.

Updated since.

[ninsbl]

I start wondering if it would be good to deprecate the open_stds functions and replace them with one generic open() function, with read, write and append mode, so users could do:

tgis.open(
    "my_series@my_mapset",
    "w",
    stds_type="stvds",
    **{
        "title": "stds_title",
        "description": "stds description",
        "temporal_type": "absolute"
    }
)
tgis.open("my_series@my_mapset", "a", stds_type="stvds")
tgis.open("my_series@my_mapset", "r", stds_type="stvds")

Probably with support for a context manager...

That would give a more familiar API....

But is a different PR of course...

[ninsbl]

The tgis_dbmi_paramstyle change is a relatively trivial regression fix. It is not entirely related to the other changes in this PR, but I noticed weird SQL error messages and since the fix is tiny, I just added it here. I can easily separate it out though if you prefer...

[gkneighb]

No worries at all, @ninsbl — please don't apologize. It's great to see it land in a form that unblocks the work you have depending on it, and your refactor was instructive to read (the check_id helper and the explicit search-path handling especially). Happy to help further if anything comes up.

[ninsbl]

@petrasovaa could you have another look at this when your time allows? Would be great to get it merged, as based on this one, more temporal improvements can be added...

[petrasovaa]

It looks like this PR triggers a problem in t.(un)register because they use a hack. Here are the details from the AI analysis:

Details

t.register/t.unregister crash instead of reporting a missing dataset

From a mapset with another temporal mapset on the search path (PERMANENT always is), a dataset name not found in the current mapset crashes:

$ t.register input=does_not_exist maps=m1
ERROR: 'PERMANENT'
$ t.unregister input=does_not_exist maps=m1
KeyError: 'PERMANENT' # full traceback

main branch gives for an unqualified name:

ERROR: Space time raster dataset does_not_exist@user1 not found

Cause. SQLDatabaseInterfaceConnection keeps tgis_mapsets (permitted) and connections (open handles) in sync. register.py:126-130 and t.unregister.py:80-83 deliberately break that — they prune connections to the current mapset ("ugly hack !") and leave tgis_mapsets alone. All six accessors (core.py:1215, 1242, 1260, 1275, 1287, 1306) then validate against tgis_mapsets and dereference connections, so a pruned mapset passes the guard and fails the lookup.

Relation to #7670. Pre-existing, not introduced there: main already raises the same KeyError for t.register input=ds@OTHERMAPSET. It stayed rare because open_old_stds never left the current mapset for an unqualified name. #7670's search-path loop iterates dbif.tgis_mapsets, so every failed unqualified lookup now hits it — a rare crash becomes an everyday one.

Repro. Project with PERMANENT + user1, both with a temporal database, raster m1 in user1:
grass /user1 --exec t.register input=does_not_exist maps=m1

Fix. In core.py, guard on the dict actually dereferenced (if mapset not in self.connections: at all six sites, and list self.connections.keys() in _create_mapset_error_message). No-op in normal use, since the dicts only diverge inside the hack. Better still, drop the hack: register_map already refuses cross-mapset writes on its own (verified — nothing gets written), so the connection surgery is redundant protection that exists only to break the invariant.

Here is a patch that (hopefully) solves this issue:
tgis-mapset-keyerror.patch
But I didn't want to apply it and see if you agree first.

[ninsbl]

It looks like this PR triggers a problem in t.(un)register because they use a hack. Here are the details from the AI analysis:

I see. I would probably rather change t.register and t.unregister and replace the "ugly hack" with creating a SQLDatabaseInterfaceConnection only for the current mapset:

dbif = tgis.SQLDatabaseInterfaceConnection(mapsets=mapsets)

and raise an error when the requested STDS is not in the current mapset.

Not sure if that is sufficient for t.unregister. Will look into it...

[ninsbl]

It looks like this PR triggers a problem in t.(un)register because they use a hack.

Solved in #7901

[gkneighb]

@petrasovaa — friendly nudge when you have a minute. Stefan addressed your Sep 4 notes in 527e856/e11f8fb, and the t.(un)register KeyError path landed separately in #7901. CI is green on the current head. Would you mind another look / dismissing the older changes-requested when it looks good?

[petrasovaa]

Sorry, I've been busy...