Explanation of what is going on in ogrgeojsonlayer.cpp

[Kurt Schwehr]

Hi Even,

Another Google engineer and I were taking a look at AddFeature and could use some explanation of what the code is doing. Can you help us better understand this code and how FIDs work? Maybe we can add some comments to several places in code based on this ticket.

This came up because of r34333 and r34330.

void OGRGeoJSONLayer::AddFeature( OGRFeature* poFeature )
{
    GIntBig nFID = poFeature->GetFID();

    // Detect potential FID duplicates and make sure they are eventually
    // unique.
    if( -1 == nFID )
    {
        nFID = GetFeatureCount(FALSE);
        OGRFeature* poTryFeature = NULL;
        while( (poTryFeature = GetFeature(nFID) ) != NULL )
        {
            nFID++;
            delete poTryFeature;
        }

GetFeature() returns a copy of the feature that we must delete, yes? Looking at OGRMemLayer::GetNextFeature() I see a return poFeature->Clone().

I have no idea why this block does what the comment says. It is looking for the first available FID that will be SetFID below?

Here's how he interpret it:

• start with "nFID <-- feature count", i.e., if there is an imaginary list of features, nFID should now be the first empty position after the end of this list.
• start a while loop with the first thing: ask for the feature at nFID
  • wait what? That must be an invalid position right after the list-of-features end
  • OK, so maybe there is a left over feature past the end of the list, and GetFeature does not care that it is asked elements past GetFeatureCount
    • hm, maybe this makes sense, seems like we proceed with nFID++ and delete every non-null element we get
• so maybe this while loop is to clean up left-over features past GetFeatureCount?
• But these pointers .. do they ever get nullified after they get deleted? If we ask GetFeature(GetFeatureCount()) again, it will return non-NULL value again, right?

    }
    else
    {
        OGRFeature* poTryFeature = NULL;
        if( (poTryFeature = GetFeature(nFID) ) != NULL )
        {
            if( !bOriginalIdModified_ )
            {
                CPLError(
                    CE_Warning, CPLE_AppDefined,
                    "Several features with id = " CPL_FRMT_GIB " have been "
                    "found. Altering it to be unique. This warning will not "
                    "be emitted for this layer",
                    nFID );
                bOriginalIdModified_ = true;
            }
            delete poTryFeature;
            nFID = GetFeatureCount(FALSE);
            while( (poTryFeature = GetFeature(nFID) ) != NULL )

He has some questions here:

oops, .. do we expect nFID here to be different from the nFID we have from the initial poTryFeature = GetFeature(nFID)?

If it happens to be the same, then poTryFeature = GetFeature(nFID) will return the same pointer we just deleted in line 144, no? and guess what? it won't be == NULL, so we will try to delete it again?

            {
                nFID++;
                delete poTryFeature;
            }
        }
    }
    poFeature->SetFID( nFID );

    if( !CPL_INT64_FITS_ON_INT32(nFID) )
        SetMetadataItem(OLMD_FID64, "YES");

    SetUpdatable( true );  // Temporary toggle on updatable flag.
    CPL_IGNORE_RET_VAL(OGRMemLayer::SetFeature(poFeature));
    SetUpdatable( poDS_->IsUpdatable() );
    SetUpdated( false );
}

How do FID's work and where is it documented? I don't see any explanation in ogr_feature.h/ogrfeature.cpp or ogrsf_frmts.h, which seem to be the roots of FID definition. Or in ogr_api.h (which seems a less logical location being the indirect C layer). Or these

• ​http://www.gdal.org/ogr__api_8h.html#afc99defd1fb1f8f71ff89c38982f2103
• ​http://www.gdal.org/classOGRFeature.html#a45da957be1eb8aa824e3ee9dbfb9604c

The best I found was from SetFID() in ogrfeature.cpp.

/**
 * For specific types of features this operation may fail on illegal
 * features ids.  Generally it always succeeds.  Feature ids should be
 * greater than or equal to zero, with the exception of OGRNullFID (-1)
 * indicating that the feature id is unknown.
 */