Skip to content

Commit c854879

Browse files
rouaultrouault
authored
Merge pull request #12836 from rouault/fix_12832
SetNextByIndex(invalid_index): return OGRERR_NON_EXISTING_FEATURE and…
2 parents 85dbc71 + aa7a1b9 commit c854879

File tree

16 files changed

+123
-39
lines changed

16 files changed

+123
-39
lines changed

‎apps/test_ogrsf.cpp

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1384,7 +1384,7 @@ static int TestBasic(GDALDataset *poDS, OGRLayer *poLayer)
13841384
static int TestLayerErrorConditions(OGRLayer *poLyr)
13851385
{
13861386
int bRet = TRUE;
1387-
OGRFeature *poFeat = nullptr;
1387+
std::unique_ptr<OGRFeature> poFeat;
13881388

13891389
CPLPushErrorHandler(CPLQuietErrorHandler);
13901390

@@ -1422,18 +1422,16 @@ static int TestLayerErrorConditions(OGRLayer *poLyr)
14221422
}
14231423

14241424
poLyr->ResetReading();
1425-
poFeat = poLyr->GetNextFeature();
1425+
poFeat.reset(poLyr->GetNextFeature());
14261426
if (poFeat)
14271427
{
14281428
poFeat->SetFID(-10);
1429-
if (poLyr->SetFeature(poFeat) == OGRERR_NONE)
1429+
if (poLyr->SetFeature(poFeat.get()) == OGRERR_NONE)
14301430
{
14311431
printf("ERROR: SetFeature(-10) should have returned an error\n");
1432-
delete poFeat;
14331432
bRet = FALSE;
14341433
goto bye;
14351434
}
1436-
delete poFeat;
14371435
}
14381436

14391437
if (poLyr->DeleteFeature(-10) == OGRERR_NONE)
@@ -1451,18 +1449,40 @@ static int TestLayerErrorConditions(OGRLayer *poLyr)
14511449
goto bye;
14521450
}
14531451

1454-
if (LOG_ACTION(poLyr->SetNextByIndex(-10)) != OGRERR_FAILURE)
1452+
if (LOG_ACTION(poLyr->SetNextByIndex(-1)) != OGRERR_NON_EXISTING_FEATURE)
14551453
{
1456-
printf("ERROR: SetNextByIndex(-10) should have "
1457-
"returned OGRERR_FAILURE\n");
1454+
printf("ERROR: SetNextByIndex(-1) should have "
1455+
"returned OGRERR_NON_EXISTING_FEATURE\n");
1456+
bRet = FALSE;
1457+
goto bye;
1458+
}
1459+
if (LOG_ACTION(std::unique_ptr<OGRFeature>(poLyr->GetNextFeature())) !=
1460+
nullptr)
1461+
{
1462+
printf("ERROR: GetNextFeature() after SetNextByIndex(-1) "
1463+
"should have returned NULL\n");
14581464
bRet = FALSE;
14591465
goto bye;
14601466
}
14611467

1462-
if (LOG_ACTION(poLyr->SetNextByIndex(2000000000)) == OGRERR_NONE &&
1463-
LOG_ACTION(poLyr->GetNextFeature()) != nullptr)
1468+
if (poFeat)
1469+
{
1470+
poLyr->ResetReading();
1471+
poFeat.reset(poLyr->GetNextFeature());
1472+
if (!poFeat)
1473+
{
1474+
printf("ERROR: GetNextFeature() after SetNextByIndex(-1) and "
1475+
"ResetReading() should have returned a non-NULL feature\n");
1476+
bRet = FALSE;
1477+
goto bye;
1478+
}
1479+
}
1480+
1481+
CPL_IGNORE_RET_VAL(LOG_ACTION(poLyr->SetNextByIndex(2000000000)));
1482+
if (LOG_ACTION(std::unique_ptr<OGRFeature>(poLyr->GetNextFeature())) !=
1483+
nullptr)
14641484
{
1465-
printf("ERROR: SetNextByIndex(2000000000) and then GetNextFeature() "
1485+
printf("ERROR: GetNextFeature() after SetNextByIndex(2000000000) "
14661486
"should have returned NULL\n");
14671487
bRet = FALSE;
14681488
goto bye;

‎autotest/ogr/ogr_sql_rfc28.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1265,11 +1265,11 @@ def test_ogr_rfc28_47(data_ds):
12651265
ogrtest.check_features_against_list(lyr, "EAS_ID", [171, 170])
12661266

12671267
with data_ds.ExecuteSQL("SELECT * FROM POLY LIMIT 1") as lyr:
1268-
assert lyr.SetNextByIndex(1) == ogr.OGRERR_FAILURE
1268+
assert lyr.SetNextByIndex(1) == ogr.OGRERR_NON_EXISTING_FEATURE
12691269
assert lyr.GetNextFeature() is None
12701270

12711271
with data_ds.ExecuteSQL("SELECT * FROM POLY LIMIT 1 OFFSET 1") as lyr:
1272-
assert lyr.SetNextByIndex(1) == ogr.OGRERR_FAILURE
1272+
assert lyr.SetNextByIndex(1) == ogr.OGRERR_NON_EXISTING_FEATURE
12731273
assert lyr.GetNextFeature() is None
12741274

12751275
with data_ds.ExecuteSQL("SELECT * FROM POLY LIMIT 2 OFFSET 1") as lyr:
@@ -1286,15 +1286,15 @@ def test_ogr_rfc28_47(data_ds):
12861286
assert lyr.GetNextFeature() is None
12871287

12881288
with data_ds.ExecuteSQL("SELECT * FROM POLY LIMIT 1 OFFSET 1") as lyr:
1289-
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_FAILURE
1289+
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_NON_EXISTING_FEATURE
12901290
assert lyr.GetNextFeature() is None
12911291

12921292
with data_ds.ExecuteSQL("SELECT * FROM POLY OFFSET 1") as lyr:
1293-
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_FAILURE
1293+
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_NON_EXISTING_FEATURE
12941294
assert lyr.GetNextFeature() is None
12951295

12961296
with data_ds.ExecuteSQL("SELECT * FROM POLY") as lyr:
1297-
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_FAILURE
1297+
assert lyr.SetNextByIndex((1 << 63) - 1) == ogr.OGRERR_NON_EXISTING_FEATURE
12981298
assert lyr.GetNextFeature() is None
12991299

13001300

‎frmts/mem/ogrmemlayer.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,9 @@ void OGRMemLayer::ResetReading()
127127
OGRFeature *OGRMemLayer::GetNextFeature()
128128

129129
{
130+
if (m_iNextReadFID < 0)
131+
return nullptr;
132+
130133
while (true)
131134
{
132135
OGRFeature *poFeature = nullptr;
@@ -172,7 +175,10 @@ OGRErr OGRMemLayer::SetNextByIndex(GIntBig nIndex)
172175
return OGRLayer::SetNextByIndex(nIndex);
173176

174177
if (nIndex < 0 || nIndex >= m_nMaxFeatureCount)
175-
return OGRERR_FAILURE;
178+
{
179+
m_iNextReadFID = -1;
180+
return OGRERR_NON_EXISTING_FEATURE;
181+
}
176182

177183
m_iNextReadFID = nIndex;
178184

‎ogr/ogrsf_frmts/generic/ogr_gensql.cpp

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -617,8 +617,13 @@ void OGRGenSQLResultsLayer::ResetReading()
617617
OGRErr OGRGenSQLResultsLayer::SetNextByIndex(GIntBig nIndex)
618618

619619
{
620+
m_bEOF = false;
621+
620622
if (nIndex < 0)
621-
return OGRERR_FAILURE;
623+
{
624+
m_bEOF = true;
625+
return OGRERR_NON_EXISTING_FEATURE;
626+
}
622627

623628
swq_select *psSelectInfo = m_pSelectInfo.get();
624629

@@ -627,7 +632,7 @@ OGRErr OGRGenSQLResultsLayer::SetNextByIndex(GIntBig nIndex)
627632
m_nIteratedFeatures = nIndex;
628633
if (m_nIteratedFeatures >= psSelectInfo->limit)
629634
{
630-
return OGRERR_FAILURE;
635+
return OGRERR_NON_EXISTING_FEATURE;
631636
}
632637
}
633638

@@ -636,7 +641,7 @@ OGRErr OGRGenSQLResultsLayer::SetNextByIndex(GIntBig nIndex)
636641
if (nIndex > std::numeric_limits<GIntBig>::max() - psSelectInfo->offset)
637642
{
638643
m_bEOF = true;
639-
return OGRERR_FAILURE;
644+
return OGRERR_NON_EXISTING_FEATURE;
640645
}
641646
if (psSelectInfo->query_mode == SWQM_SUMMARY_RECORD ||
642647
psSelectInfo->query_mode == SWQM_DISTINCT_LIST || !m_anFIDIndex.empty())

‎ogr/ogrsf_frmts/generic/ogrlayer.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -814,18 +814,15 @@ OGRErr OGRLayer::SetNextByIndex(GIntBig nIndex)
814814

815815
{
816816
if (nIndex < 0)
817-
return OGRERR_FAILURE;
817+
nIndex = GINTBIG_MAX;
818818

819819
ResetReading();
820820

821-
OGRFeature *poFeature = nullptr;
822821
while (nIndex-- > 0)
823822
{
824-
poFeature = GetNextFeature();
823+
auto poFeature = std::unique_ptr<OGRFeature>(GetNextFeature());
825824
if (poFeature == nullptr)
826-
return OGRERR_FAILURE;
827-
828-
delete poFeature;
825+
return OGRERR_NON_EXISTING_FEATURE;
829826
}
830827

831828
return OGRERR_NONE;

‎ogr/ogrsf_frmts/gpkg/ogrgeopackagetablelayer.cpp

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3481,6 +3481,7 @@ OGRErr OGRGeoPackageTableLayer::SetAttributeFilter(const char *pszQuery)
34813481

34823482
void OGRGeoPackageTableLayer::ResetReading()
34833483
{
3484+
m_bEOF = false;
34843485
if (m_bDeferredCreation && RunDeferredCreationIfNecessary() != OGRERR_NONE)
34853486
return;
34863487

@@ -3519,7 +3520,10 @@ void OGRGeoPackageTableLayer::ResetReading()
35193520
OGRErr OGRGeoPackageTableLayer::SetNextByIndex(GIntBig nIndex)
35203521
{
35213522
if (nIndex < 0)
3522-
return OGRERR_FAILURE;
3523+
{
3524+
m_bEOF = true;
3525+
return OGRERR_NON_EXISTING_FEATURE;
3526+
}
35233527
if (m_soColumns.empty())
35243528
BuildColumns();
35253529
return ResetStatementInternal(nIndex);
@@ -3620,6 +3624,8 @@ OGRErr OGRGeoPackageTableLayer::ResetStatementInternal(GIntBig nStartIndex)
36203624

36213625
OGRFeature *OGRGeoPackageTableLayer::GetNextFeature()
36223626
{
3627+
if (m_bEOF)
3628+
return nullptr;
36233629
if (!m_bFeatureDefnCompleted)
36243630
GetLayerDefn();
36253631
if (m_bDeferredCreation && RunDeferredCreationIfNecessary() != OGRERR_NONE)

‎ogr/ogrsf_frmts/ngw/ogr_ngw.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ class OGRNGWLayer final : public OGRLayer
177177
std::string osWhere;
178178
std::string osSpatialFilter;
179179
bool bClientSideAttributeFilter;
180+
bool m_bEOF = false;
180181

181182
explicit OGRNGWLayer(const std::string &osResourceIdIn,
182183
OGRNGWDataset *poDSIn,

‎ogr/ogrsf_frmts/ngw/ogrngwlayer.cpp

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,7 @@ OGRErr OGRNGWLayer::Rename(const char *pszNewName)
591591
*/
592592
void OGRNGWLayer::ResetReading()
593593
{
594+
m_bEOF = false;
594595
SyncToDisk();
595596
FreeFeaturesCache();
596597
if (poDS->GetPageSize() > 0)
@@ -653,7 +654,7 @@ OGRErr OGRNGWLayer::SetNextByIndex(GIntBig nIndex)
653654
CPLError(CE_Failure, CPLE_AppDefined,
654655
"Feature index must be greater or equal 0. Got " CPL_FRMT_GIB,
655656
nIndex);
656-
return OGRERR_FAILURE;
657+
return OGRERR_NON_EXISTING_FEATURE;
657658
}
658659
if (poDS->GetPageSize() > 0)
659660
{
@@ -719,6 +720,8 @@ OGRErr OGRNGWLayer::SetNextByIndex(GIntBig nIndex)
719720
*/
720721
OGRFeature *OGRNGWLayer::GetNextFeature()
721722
{
723+
if (m_bEOF)
724+
return nullptr;
722725
std::string osUrl;
723726

724727
if (poDS->GetPageSize() > 0)

‎ogr/ogrsf_frmts/ogrsf_frmts.dox

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1881,6 +1881,12 @@ error code.
18811881
fast seeking is available on the current layer use the TestCapability()
18821882
method with a value of OLCFastSetNextByIndex.
18831883

1884+
Starting with GDAL 3.12, when implementations can detect that nIndex is
1885+
invalid (at the minimum all should detect negative indices), they should
1886+
return OGRERR_NON_EXISTING_FEATURE, and following calls to GetNextFeature()
1887+
should return nullptr, until ResetReading() or a valid call to
1888+
SetNextByIndex() is done.
1889+
18841890
This method is the same as the C function OGR_L_SetNextByIndex().
18851891

18861892
@param nIndex the index indicating how many steps into the result set
@@ -1909,6 +1915,12 @@ error code.
19091915
fast seeking is available on the current layer use the TestCapability()
19101916
method with a value of OLCFastSetNextByIndex.
19111917

1918+
Starting with GDAL 3.12, when implementations can detect that nIndex is
1919+
invalid (at the minimum all should detect negative indices), they should
1920+
return OGRERR_NON_EXISTING_FEATURE, and following calls to GetNextFeature()
1921+
should return nullptr, until ResetReading() or a valid call to
1922+
SetNextByIndex() is done.
1923+
19121924
This method is the same as the C++ method OGRLayer::SetNextByIndex()
19131925

19141926
@param hLayer handle to the layer

‎ogr/ogrsf_frmts/openfilegdb/ogropenfilegdblayer.cpp

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2022,21 +2022,29 @@ OGRErr OGROpenFileGDBLayer::SetNextByIndex(GIntBig nIndex)
20222022
if (!BuildLayerDefinition())
20232023
return OGRERR_FAILURE;
20242024

2025+
m_bEOF = false;
2026+
20252027
if (m_eSpatialIndexState == SPI_IN_BUILDING)
20262028
m_eSpatialIndexState = SPI_INVALID;
20272029

20282030
if (m_nFilteredFeatureCount >= 0)
20292031
{
20302032
if (nIndex < 0 || nIndex >= m_nFilteredFeatureCount)
2031-
return OGRERR_FAILURE;
2033+
{
2034+
m_bEOF = true;
2035+
return OGRERR_NON_EXISTING_FEATURE;
2036+
}
20322037
m_iCurFeat = nIndex;
20332038
return OGRERR_NONE;
20342039
}
20352040
else if (m_poLyrTable->GetValidRecordCount() ==
20362041
m_poLyrTable->GetTotalRecordCount())
20372042
{
20382043
if (nIndex < 0 || nIndex >= m_poLyrTable->GetValidRecordCount())
2039-
return OGRERR_FAILURE;
2044+
{
2045+
m_bEOF = true;
2046+
return OGRERR_NON_EXISTING_FEATURE;
2047+
}
20402048
m_iCurFeat = nIndex;
20412049
return OGRERR_NONE;
20422050
}

0 commit comments

Comments
 (0)