ShapefileReader.MoveNext() is aborted prematurely

[giovani-luigi]

I was dealing with some shapefiles with possibly incorrect data for some shapes and I came across a situation where there are two polygons. One contains only 2 points (which is invalid) followed by a second valid polygon of 21 points. However on this situation the enumerator is aborted prematurely before the end of the file, due to the catch() block (ShapefileReader.cs line 138)

try
{
    int recordNumber = _shpBinaryReader.ReadInt32BE();
    int contentLength = _shpBinaryReader.ReadInt32BE();
    _geometry = _handler.Read(_shpBinaryReader, contentLength, _parent._geometryFactory);
}
catch (Exception ex)
{
    // actually we should remove this crap...
    Trace.WriteLine(ex.Message);
    return false;                  // <-----------------   will ignore further valid shapes
}

I would suggest to proceed the enumeration until the end of the file is reached, so possibly valid shapes are not ignored.

[giovani-luigi]

Its worth noting that the reason why the Exception is thrown is because: throw new ArgumentException( "Ring has fewer than 4 points, so orientation cannot be determined", nameof(ring));

[DGuidi]

As the code suggests, this is the expected behavior, so changing this introduces a "breaking change"...

[giovani-luigi]

As the code suggests, this is the expected behavior, so changing this introduces a "breaking change"...

I had to dig deep until I located the comment there, so its not exactly much of a suggestion. You either bubble up the exception, or suppress it, in order to allow continuation on the execution flow. What was done is neither. I do understand your point though, but in my humble opinion, I would consider more a bug than a behavior. And therefore I would analyze from the bug fix perspective. Anyway... Unfortunately I had to use a different code in order to bypass that limitation. So, perhaps a flag with default value to enable/disable the suppression? if not specified, remains backwards compatible. Just an idea...

[DGuidi]

perhaps a flag with default value to enable/disable the suppression?

this is exactly what I have in mind, but I need to check the code a bit

[DGuidi]

@giovani-luigi can you add some test data, so I can check the patch?

[FObermaier]

We could also improve the PolygonHandler: https://github.com/NetTopologySuite/NetTopologySuite.IO.ShapeFile/blob/bef5ec80cc5b1b5a95f4df3bfc5a61ab837525a1/src/NetTopologySuite.IO.ShapeFile/Handlers/PolygonHandler.cs#L91-L102

e.g. after line 96 add

if (tmp == null) continue;

If this is the only ring, we should be getting POLYGON EMPTY, otherwise the result polygon just does not have the invalid hole.

[DGuidi]

thanks @FObermaier , hope that @giovani-luigi can add some "invalid data" so I can build some unit tests... maybe your suggestion fixes his problem directly.

[giovani-luigi]

@DGuidi sure, its my first time using Git platform so forgive any inconvenience . I believe you will be able to download this one: Victoria North.zip

[DGuidi]

@FObermaier your suggestion fixed the problem, so I think I can simply commit your line