Skip to content

Commit 1c27109

Browse files
rouaultrouault
authored
Merge pull request OSGeo#12808 from rouault/fix_12790
 Zarr: make GDALDataset::Close() report errors when write failure occurs
2 parents 7d9be72 + 36062e3 commit 1c27109

File tree

9 files changed

+185
-42
lines changed

9 files changed

+185
-42
lines changed

‎autotest/gdrivers/zarr_driver.py‎

Lines changed: 75 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4744,10 +4744,10 @@ def reopen_after_rename():
47444744
"format,create_z_metadata",
47454745
[("ZARR_V2", "YES"), ("ZARR_V2", "NO"), ("ZARR_V3", "NO")],
47464746
)
4747-
def test_zarr_multidim_rename_array_at_creation(tmp_vsimem, format, create_z_metadata):
4747+
def test_zarr_multidim_rename_array_at_creation(tmp_path, format, create_z_metadata):
47484748

47494749
drv = gdal.GetDriverByName("ZARR")
4750-
filename = str(tmp_vsimem / "test.zarr")
4750+
filename = str(tmp_path / "test.zarr")
47514751

47524752
def test():
47534753
ds = drv.CreateMultiDimensional(
@@ -5743,3 +5743,76 @@ def test_zarr_read_imagecodecs_tiff_errors(dirname):
57435743
with pytest.raises(Exception):
57445744
with gdal.Open(dirname) as ds:
57455745
ds.ReadRaster()
5746+
5747+
5748+
###############################################################################
5749+
#
5750+
5751+
5752+
@gdaltest.enable_exceptions()
5753+
@pytest.mark.parametrize("format", ["ZARR_V2", "ZARR_V3"])
5754+
def test_zarr_write_error_at_close_on_group(tmp_path, format):
5755+
out_filename = tmp_path / "test.zarr"
5756+
5757+
ds = gdal.GetDriverByName("ZARR").CreateMultiDimensional(
5758+
out_filename, options=["FORMAT=" + format]
5759+
)
5760+
rg = ds.GetRootGroup()
5761+
subgroup = rg.CreateGroup("subgroup")
5762+
attr = subgroup.CreateAttribute(
5763+
"str_attr", [], gdal.ExtendedDataType.CreateString()
5764+
)
5765+
assert attr.Write("my_string") == gdal.CE_None
5766+
del attr
5767+
del subgroup
5768+
del rg
5769+
5770+
gdal.RmdirRecursive(out_filename)
5771+
5772+
with pytest.raises(Exception, match="cannot be opened for writing"):
5773+
ds.Close()
5774+
5775+
5776+
###############################################################################
5777+
#
5778+
5779+
5780+
@gdaltest.enable_exceptions()
5781+
@pytest.mark.parametrize("format", ["ZARR_V2", "ZARR_V3"])
5782+
def test_zarr_write_error_at_close_on_array(tmp_path, format):
5783+
out_filename = tmp_path / "test.zarr"
5784+
5785+
ds = gdal.GetDriverByName("ZARR").CreateMultiDimensional(
5786+
out_filename, options=["FORMAT=" + format]
5787+
)
5788+
rg = ds.GetRootGroup()
5789+
dim0 = rg.CreateDimension("dim0", None, None, 2)
5790+
5791+
ar = rg.CreateMDArray("my_ar", [dim0], gdal.ExtendedDataType.Create(gdal.GDT_Byte))
5792+
attr = ar.CreateAttribute("str_attr", [], gdal.ExtendedDataType.CreateString())
5793+
assert attr.Write("my_string") == gdal.CE_None
5794+
del attr
5795+
del ar
5796+
del rg
5797+
5798+
gdal.RmdirRecursive(out_filename)
5799+
5800+
with pytest.raises(Exception, match="cannot be opened for writing"):
5801+
ds.Close()
5802+
5803+
5804+
###############################################################################
5805+
#
5806+
5807+
5808+
@gdaltest.enable_exceptions()
5809+
@pytest.mark.parametrize("format", ["ZARR_V2", "ZARR_V3"])
5810+
def test_zarr_write_vsizip(tmp_vsimem, format):
5811+
out_filename = "/vsizip/" + str(tmp_vsimem) + "test.zarr.zip/test.zarr"
5812+
5813+
gdal.GetDriverByName("Zarr").CreateCopy(
5814+
out_filename, gdal.Open("data/byte.tif"), options=["FORMAT=" + format]
5815+
)
5816+
5817+
ds = gdal.Open(out_filename)
5818+
assert ds.GetMetadata() == {"AREA_OR_POINT": "Area"}

‎frmts/zarr/zarr.h‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@ const CPLCompressor *ZarrGetQuantizeDecompressor();
3535
const CPLCompressor *ZarrGetTIFFDecompressor();
3636
const CPLCompressor *ZarrGetFixedScaleOffsetDecompressor();
3737

38+
class ZarrGroupBase;
39+
3840
/************************************************************************/
3941
/* ZarrDataset */
4042
/************************************************************************/
@@ -43,7 +45,7 @@ class ZarrDataset final : public GDALDataset
4345
{
4446
friend class ZarrRasterBand;
4547

46-
std::shared_ptr<GDALGroup> m_poRootGroup{};
48+
std::shared_ptr<ZarrGroupBase> m_poRootGroup{};
4749
CPLStringList m_aosSubdatasets{};
4850
GDALGeoTransform m_gt{};
4951
bool m_bHasGT = false;
@@ -55,7 +57,7 @@ class ZarrDataset final : public GDALDataset
5557
CSLConstList papszOpenOptions);
5658

5759
public:
58-
explicit ZarrDataset(const std::shared_ptr<GDALGroup> &poRootGroup);
60+
explicit ZarrDataset(const std::shared_ptr<ZarrGroupBase> &poRootGroup);
5961
~ZarrDataset() override;
6062

6163
CPLErr FlushCache(bool bAtClosing = false) override;
@@ -87,10 +89,7 @@ class ZarrDataset final : public GDALDataset
8789
CPLErr GetGeoTransform(GDALGeoTransform &gt) const override;
8890
CPLErr SetGeoTransform(const GDALGeoTransform &gt) override;
8991

90-
std::shared_ptr<GDALGroup> GetRootGroup() const override
91-
{
92-
return m_poRootGroup;
93-
}
92+
std::shared_ptr<GDALGroup> GetRootGroup() const override;
9493
};
9594

9695
/************************************************************************/
@@ -147,6 +146,8 @@ class ZarrAttributeGroup
147146
explicit ZarrAttributeGroup(const std::string &osParentName,
148147
bool bContainerIsGroup);
149148

149+
bool Close();
150+
150151
void Init(const CPLJSONObject &obj, bool bUpdatable);
151152

152153
std::shared_ptr<GDALAttribute> GetAttribute(const std::string &osName) const
@@ -233,8 +234,6 @@ class ZarrAttributeGroup
233234
/* ZarrSharedResource */
234235
/************************************************************************/
235236

236-
class ZarrGroupBase;
237-
238237
class ZarrSharedResource
239238
: public std::enable_shared_from_this<ZarrSharedResource>
240239
{
@@ -398,6 +397,8 @@ class ZarrGroupBase CPL_NON_FINAL : public GDALGroup
398397
public:
399398
~ZarrGroupBase() override;
400399

400+
virtual bool Close();
401+
401402
std::shared_ptr<GDALAttribute>
402403
GetAttribute(const std::string &osName) const override
403404
{
@@ -516,6 +517,8 @@ class ZarrV2Group final : public ZarrGroupBase
516517
{
517518
}
518519

520+
bool Close() override;
521+
519522
public:
520523
static std::shared_ptr<ZarrV2Group>
521524
Create(const std::shared_ptr<ZarrSharedResource> &poSharedResource,
@@ -569,6 +572,8 @@ class ZarrV3Group final : public ZarrGroupBase
569572
const std::string &osParentName, const std::string &osName,
570573
const std::string &osDirectoryName);
571574

575+
bool Close() override;
576+
572577
public:
573578
~ZarrV3Group() override;
574579

@@ -1042,7 +1047,7 @@ class ZarrArray CPL_NON_FINAL : public GDALPamMDArray
10421047

10431048
void ParentRenamed(const std::string &osNewParentFullName) override;
10441049

1045-
virtual void Flush() = 0;
1050+
virtual bool Flush() = 0;
10461051

10471052
std::shared_ptr<GDALGroup> GetRootGroup() const override
10481053
{
@@ -1086,7 +1091,7 @@ class ZarrV2Array final : public ZarrArray
10861091
const std::vector<DtypeElt> &aoDtypeElts,
10871092
const std::vector<GUInt64> &anBlockSize, bool bFortranOrder);
10881093

1089-
void Serialize();
1094+
bool Serialize();
10901095

10911096
bool LoadTileData(const uint64_t *tileIndices, bool bUseMutex,
10921097
const CPLCompressor *psDecompressor,
@@ -1130,7 +1135,7 @@ class ZarrV2Array final : public ZarrArray
11301135

11311136
void SetFilters(const CPLJSONArray &oFiltersArray);
11321137

1133-
void Flush() override;
1138+
bool Flush() override;
11341139

11351140
protected:
11361141
std::string GetDataDirectory() const override;
@@ -1478,7 +1483,7 @@ class ZarrV3Array final : public ZarrArray
14781483
const std::vector<DtypeElt> &aoDtypeElts,
14791484
const std::vector<GUInt64> &anBlockSize);
14801485

1481-
void Serialize(const CPLJSONObject &oAttrs);
1486+
bool Serialize(const CPLJSONObject &oAttrs);
14821487

14831488
bool NeedDecodedBuffer() const;
14841489

@@ -1513,7 +1518,7 @@ class ZarrV3Array final : public ZarrArray
15131518
m_poCodecs = std::move(poCodecs);
15141519
}
15151520

1516-
void Flush() override;
1521+
bool Flush() override;
15171522

15181523
protected:
15191524
std::string GetDataDirectory() const override;

‎frmts/zarr/zarr_group.cpp‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,27 @@
2424

2525
ZarrGroupBase::~ZarrGroupBase()
2626
{
27-
// We need to explicitly flush arrays so that the _ARRAY_DIMENSIONS
28-
// is properly written. As it relies on checking if the dimensions of the
29-
// array have an indexing variable, then still need to be all alive.
27+
ZarrGroupBase::Close();
28+
}
29+
30+
/************************************************************************/
31+
/* Close() */
32+
/************************************************************************/
33+
34+
bool ZarrGroupBase::Close()
35+
{
36+
bool ret = true;
37+
38+
for (auto &kv : m_oMapGroups)
39+
{
40+
ret = kv.second->Close() && ret;
41+
}
42+
3043
for (auto &kv : m_oMapMDArrays)
3144
{
32-
kv.second->Flush();
45+
ret = kv.second->Flush() && ret;
3346
}
47+
return ret;
3448
}
3549

3650
/************************************************************************/

‎frmts/zarr/zarr_v2_array.cpp‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,17 @@ ZarrV2Array::~ZarrV2Array()
7777
/* Flush() */
7878
/************************************************************************/
7979

80-
void ZarrV2Array::Flush()
80+
bool ZarrV2Array::Flush()
8181
{
8282
if (!m_bValid)
83-
return;
83+
return true;
8484

85-
ZarrV2Array::FlushDirtyTile();
85+
bool ret = ZarrV2Array::FlushDirtyTile();
8686

8787
if (m_bDefinitionModified)
8888
{
89-
Serialize();
89+
if (!Serialize())
90+
ret = false;
9091
m_bDefinitionModified = false;
9192
}
9293

@@ -131,9 +132,12 @@ void ZarrV2Array::Flush()
131132
const std::string osAttrFilename =
132133
CPLFormFilenameSafe(CPLGetDirnameSafe(m_osFilename.c_str()).c_str(),
133134
".zattrs", nullptr);
134-
oDoc.Save(osAttrFilename);
135+
if (!oDoc.Save(osAttrFilename))
136+
ret = false;
135137
m_poSharedResource->SetZMetadataItem(osAttrFilename, oAttrs);
136138
}
139+
140+
return ret;
137141
}
138142

139143
/************************************************************************/
@@ -154,7 +158,7 @@ static void StripUselessItemsFromCompressorConfiguration(CPLJSONObject &o)
154158
/* ZarrV2Array::Serialize() */
155159
/************************************************************************/
156160

157-
void ZarrV2Array::Serialize()
161+
bool ZarrV2Array::Serialize()
158162
{
159163
CPLJSONDocument oDoc;
160164
CPLJSONObject oRoot = oDoc.GetRoot();
@@ -254,9 +258,11 @@ void ZarrV2Array::Serialize()
254258
oRoot.Add("dimension_separator", m_osDimSeparator);
255259
}
256260

257-
oDoc.Save(m_osFilename);
261+
bool ret = oDoc.Save(m_osFilename);
258262

259263
m_poSharedResource->SetZMetadataItem(m_osFilename, oRoot);
264+
265+
return ret;
260266
}
261267

262268
/************************************************************************/

‎frmts/zarr/zarr_v2_group.cpp‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,15 +40,28 @@ ZarrV2Group::Create(const std::shared_ptr<ZarrSharedResource> &poSharedResource,
4040

4141
ZarrV2Group::~ZarrV2Group()
4242
{
43+
ZarrV2Group::Close();
44+
}
45+
46+
/************************************************************************/
47+
/* Close() */
48+
/************************************************************************/
49+
50+
bool ZarrV2Group::Close()
51+
{
52+
bool bRet = ZarrGroupBase::Close();
53+
4354
if (m_bValid && m_oAttrGroup.IsModified())
4455
{
4556
CPLJSONDocument oDoc;
4657
oDoc.SetRoot(m_oAttrGroup.Serialize());
4758
const std::string osAttrFilename =
4859
CPLFormFilenameSafe(m_osDirectoryName.c_str(), ".zattrs", nullptr);
49-
oDoc.Save(osAttrFilename);
60+
bRet = oDoc.Save(osAttrFilename) && bRet;
5061
m_poSharedResource->SetZMetadataItem(osAttrFilename, oDoc.GetRoot());
5162
}
63+
64+
return bRet;
5265
}
5366

5467
/************************************************************************/
@@ -1126,7 +1139,8 @@ std::shared_ptr<GDALMDArray> ZarrV2Group::CreateMDArray(
11261139
poArray->SetFilters(oFilters);
11271140
poArray->SetUpdatable(true);
11281141
poArray->SetDefinitionModified(true);
1129-
poArray->Flush();
1142+
if (!cpl::starts_with(osZarrayFilename, "/vsi") && !poArray->Flush())
1143+
return nullptr;
11301144
RegisterArray(poArray);
11311145

11321146
return poArray;

‎frmts/zarr/zarr_v3_array.cpp‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,12 @@ ZarrV3Array::~ZarrV3Array()
7474
/* Flush() */
7575
/************************************************************************/
7676

77-
void ZarrV3Array::Flush()
77+
bool ZarrV3Array::Flush()
7878
{
7979
if (!m_bValid)
80-
return;
80+
return true;
8181

82-
ZarrV3Array::FlushDirtyTile();
82+
bool ret = ZarrV3Array::FlushDirtyTile();
8383

8484
if (!m_aoDims.empty())
8585
{
@@ -112,16 +112,19 @@ void ZarrV3Array::Flush()
112112

113113
if (m_bDefinitionModified)
114114
{
115-
Serialize(oAttrs);
115+
if (!Serialize(oAttrs))
116+
ret = false;
116117
m_bDefinitionModified = false;
117118
}
119+
120+
return ret;
118121
}
119122

120123
/************************************************************************/
121124
/* ZarrV3Array::Serialize() */
122125
/************************************************************************/
123126

124-
void ZarrV3Array::Serialize(const CPLJSONObject &oAttrs)
127+
bool ZarrV3Array::Serialize(const CPLJSONObject &oAttrs)
125128
{
126129
CPLJSONDocument oDoc;
127130
CPLJSONObject oRoot = oDoc.GetRoot();
@@ -238,7 +241,7 @@ void ZarrV3Array::Serialize(const CPLJSONObject &oAttrs)
238241

239242
// TODO: codecs
240243

241-
oDoc.Save(m_osFilename);
244+
return oDoc.Save(m_osFilename);
242245
}
243246

244247
/************************************************************************/

0 commit comments

Comments
 (0)