Skip to content

Don't call all records deleted if record count is not set #4

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pramsey wants to merge 1 commit into from
Closed

Don't call all records deleted if record count is not set #4

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dbfopen.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1744,7 +1744,7 @@ int SHPAPI_CALL DBFIsRecordDeleted( DBFHandle psDBF, int iShape )
/* -------------------------------------------------------------------- */
/* Verify selection. */
/* -------------------------------------------------------------------- */
if( iShape < 0 || iShape >= psDBF->nRecords )
if( iShape < 0 || (iShape > 0 && iShape >= psDBF->nRecords) )
Copy link
Member

@rouault rouault Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be ? (I changed the iShape > 0 to psDBF->nRecords > 0 since that matches more your rationale + added the comment)

Suggested change
if( iShape < 0 || (iShape > 0 && iShape >= psDBF->nRecords) )
/* Some DBF files are written without a record count (because, say */
/* the SHP file or SHX file already give a record count. In that */
/* case, checking the shape index being > number of records always */
/* returns deleted, which is not correct. */
if( iShape < 0 || ( psDBF->nRecords > 0 && iShape >= psDBF->nRecords) )

I didn't check the DBF spec but I don't think a DBF header with nRecords == 0 would be valid ?
And as far as I can see DBFReadAttribute() would have the same issue., and quite a few other places How much is your proposed change tested ;-) ?

Copy link
Author

@pramsey pramsey Dec 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha ha. Well, in theory it's been tested widely in PostGIS forever, but I only had to make the one change to pass regression... and your change is correct, the comment is right and my code is not. I think our test files are so simple we aren't even seeing the DBFReadAttribute case because we have no attributes in our DBF files.

Copy link
Member

@rouault rouault Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point in trying to support such broken DBF files, unless you see them in great use in the wild ? I don't have memories of issues reported in GDAL regarding such cases

Copy link
Author

@pramsey pramsey Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible we are generating them ourselves... leave this PR open, I will check tomorrow... somehow we got these shape files in the very early days and added them to our regression suite... https://github.com/postgis/postgis/tree/master/regress/loader

Copy link
Author

@pramsey pramsey Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either we generated them ourselves with early versions of shapelib or maybe created them with ArcView 3.

Copy link
Member

@rouault rouault Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe regenerate them with a recent ogr2ogr ?

return TRUE;

/* -------------------------------------------------------------------- */
Expand Down