Merge pull request #624 from dcs4cop/forman-623-fix_fs_data_accessors

Fix FsDataAccessor.write_data() impls

Author: forman

CHANGES.md

@@ -4,9 +4,13 @@
 
 ### Fixes
 
-* Images with ascending y-values are tiled correctly. This fixes an issue where
-  some datasets seemed to be shifted in the y-(latitude-) direction and were
-  misplaced on maps. (#626)
+* Fixed `FsDataAccessor.write_data()` implementations, 
+  which now always return the passed in `data_id`. (#623)
+
+* Fixes an issue where some datasets seemed to be shifted in the 
+  y-(latitude-) direction and were misplaced on maps whose tiles 
+  are served by `xcube serve`. Images with ascending y-values are 
+  now tiled correctly. (#626)
 
 ### Other
 

test/core/store/fs/test_registry.py

@@ -117,7 +117,8 @@ def assertDatasetSupported(
         self.assertEqual([], list(data_store.get_data_ids()))
 
         data = new_cube(variables=dict(A=8, B=9))
-        data_store.write_data(data, data_id)
+        written_data_id = data_store.write_data(data, data_id)
+        self.assertEqual(data_id, written_data_id)
         self.assertEqual({expected_data_type_alias},
                          set(data_store.get_data_types_for_data(data_id)))
         self.assertEqual(True, data_store.has_data(data_id))

xcube/core/store/fs/impl/dataset.py

@@ -191,7 +191,7 @@ def write_data(self,
                    data: xr.Dataset,
                    data_id: str,
                    replace=False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, xr.Dataset, name='data')
         assert_instance(data_id, str, name='data_id')
         fs, write_params = self.load_fs(write_params)
@@ -209,6 +209,7 @@ def write_data(self,
         except ValueError as e:
             raise DataStoreError(f'Failed to write'
                                  f' dataset {data_id!r}: {e}') from e
+        return data_id
 
     def delete_data(self,
                     data_id: str,
@@ -277,7 +278,7 @@ def write_data(self,
                    data: xr.Dataset,
                    data_id: str,
                    replace=False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, xr.Dataset, name='data')
         assert_instance(data_id, str, name='data_id')
         fs, write_params = self.load_fs(write_params)
@@ -298,3 +299,4 @@ def write_data(self,
         data.to_netcdf(file_path, engine=engine, **write_params)
         if not is_local:
             fs.put_file(file_path, data_id)
+        return data_id

xcube/core/store/fs/impl/geodataframe.py

@@ -79,7 +79,10 @@ def get_write_data_params_schema(self) -> JsonObjectSchema:
             ),
         )
 
-    def write_data(self, data: gpd.GeoDataFrame, data_id: str, **write_params):
+    def write_data(self,
+                   data: gpd.GeoDataFrame,
+                   data_id: str,
+                   **write_params) -> str:
         # TODO: implement me correctly,
         #  this is not valid for shapefile AND geojson
         assert_instance(data, (gpd.GeoDataFrame, pd.DataFrame), 'data')
@@ -92,6 +95,7 @@ def write_data(self, data: gpd.GeoDataFrame, data_id: str, **write_params):
         data.to_file(file_path, driver=self.get_driver_name(), **write_params)
         if not is_local:
             fs.put_file(file_path, data_id)
+        return data_id
 
 
 class GeoDataFrameShapefileFsDataAccessor(GeoDataFrameFsDataAccessor, ABC):

xcube/core/store/fs/impl/mldataset.py

@@ -71,7 +71,7 @@ def write_data(self,
                    data: Union[xr.Dataset, MultiLevelDataset],
                    data_id: str,
                    replace=False,
-                   **write_params):
+                   **write_params) -> str:
         assert_instance(data, (xr.Dataset, MultiLevelDataset), name='data')
         assert_instance(data_id, str, name='data_id')
         if isinstance(data, MultiLevelDataset):
@@ -108,6 +108,7 @@ def write_data(self,
                 level_dataset = xr.open_zarr(zarr_store,
                                              consolidated=consolidated)
                 ml_dataset.set_dataset(index, level_dataset)
+        return data_id
 
 
 class FsMultiLevelDataset(LazyMultiLevelDataset):
@@ -139,7 +140,8 @@ def _get_dataset_lazily(self, index: int, parameters) \
 
     def _get_tile_grid_lazily(self) -> TileGrid:
         """
-        Retrieve, i.e. read or compute, the tile grid used by the multi-level dataset.
+        Retrieve, i.e. read or compute, the tile grid used
+        by the multi-level dataset.
 
         :return: the dataset for the level at *index*.
         """

xcube/core/store/fs/store.py

@@ -34,13 +34,14 @@
 from xcube.util.assertions import assert_given
 from xcube.util.assertions import assert_in
 from xcube.util.assertions import assert_instance
+from xcube.util.assertions import assert_true
 from xcube.util.extension import Extension
 from xcube.util.jsonschema import JsonBooleanSchema
 from xcube.util.jsonschema import JsonIntegerSchema
 from xcube.util.jsonschema import JsonObjectSchema
 from xcube.util.jsonschema import JsonStringSchema
-from .accessor import STORAGE_OPTIONS_PARAM_NAME
 from .accessor import FsAccessor
+from .accessor import STORAGE_OPTIONS_PARAM_NAME
 from .helpers import is_local_fs
 from .helpers import normalize_path
 from ..accessor import DataOpener
@@ -296,11 +297,22 @@ def write_data(self,
         data_id = self._ensure_valid_data_id(writer_id, data_id=data_id)
         fs_path = self._convert_data_id_into_fs_path(data_id)
         self.fs.makedirs(self.root, exist_ok=True)
-        writer.write_data(data,
-                          fs_path,
-                          replace=replace,
-                          fs=self.fs,
-                          **write_params)
+        written_fs_path = writer.write_data(data,
+                                            fs_path,
+                                            replace=replace,
+                                            fs=self.fs,
+                                            **write_params)
+        # Verify, accessors fulfill their write_data() contract
+        assert_true(fs_path == written_fs_path,
+                    message='FsDataAccessor implementations must '
+                            'return the data_id passed in.')
+        # Return original data_id (which is a relative path).
+        # Note: it would be cleaner to return written_fs_path
+        # here, but it is an absolute path.
+        # --> Possible solution: FsDataAccessor implementations
+        # should be responsible for resolving their data_id into a
+        # filesystem path by providing them both with fs and root
+        # arguments.
         return data_id
 
     def get_delete_data_params_schema(self, data_id: str = None) \