Skip to content

Add FixWinding and RequireWinding parse options (RFC 7946 §3.1.6)#29

Open
anativ wants to merge 2 commits into
tidwall:masterfrom
anativ:feat/rfc7946-winding
Open

Add FixWinding and RequireWinding parse options (RFC 7946 §3.1.6)#29
anativ wants to merge 2 commits into
tidwall:masterfrom
anativ:feat/rfc7946-winding

Conversation

@anativ

@anativ anativ commented Apr 19, 2026

Copy link
Copy Markdown

Summary

Adds two opt-in ParseOptions flags that enforce RFC 7946 §3.1.6 polygon ring winding (exterior CCW, holes CW) during parsing of Polygon and MultiPolygon:

  • FixWinding — silently rewrites non-compliant rings by reversing their points in place. Any z/m values in extra.values are reordered to stay aligned with their points, so 3D/4D polygons survive the fix.
  • RequireWinding — returns errCoordinatesInvalid when any ring has the wrong winding. Useful for strict validation pipelines.

Rings with zero signed area (degenerate/collinear) are left alone — they carry no orientation to correct.

Both flags default to false, so existing callers are unaffected.

Implementation notes

  • Orientation via the shoelace formula on 2D coordinates. Signed area > 0 → CCW; < 0 → CW; == 0 → skip.
  • Reversing a closed ring [P0, P1, ..., Pn, P0] preserves the first-last equality required by GeoJSON linear rings.
  • The fix is applied before the geometry.Poly is constructed, so the resulting polygon and its bbox/index are computed from the corrected coordinates.

Test plan

  • go test ./... passes
  • gofmt -l . / go vet ./... clean
  • New tests in winding_test.go cover:
    • Fix flips CW exterior to CCW
    • Fix flips CCW hole to CW
    • Fix is a no-op when rings are already compliant
    • Fix preserves z coordinates
    • Require rejects CW exterior / CCW hole and accepts compliant input
    • MultiPolygon fix and reject paths
    • ringOrientation unit tests for CCW / CW / degenerate rings

Enforce RFC 7946 §3.1.6 polygon ring winding during parse: exterior rings
counterclockwise, holes clockwise. FixWinding rewrites non-compliant rings
in place (reversing both points and any z/m extras); RequireWinding rejects
non-compliant polygons. Degenerate rings (zero signed area) are left
untouched. Applies to both Polygon and MultiPolygon.
@anativ

anativ commented Apr 19, 2026

Copy link
Copy Markdown
Author

Risk Assessment

Risk Score: Low

Justification

This PR introduces two new opt-in ParseOptions flags (FixWinding, RequireWinding) that both default to false, making the change fully additive and backward compatible. There are no database migrations, no removed or renamed APIs, no changed response shapes, and no dependency changes. Existing callers using DefaultParseOptions or any previously valid ParseOptions value retain identical behavior.

Database Migration Risks

None detected. This is a Go library with no persistence layer or schema.

Breaking API Changes

None detected.

  • ParseOptions struct gains two new boolean fields — this is additive. Zero-value (false) behavior preserves existing parse semantics.
  • DefaultParseOptions is updated to set both new fields to false explicitly, matching prior defaults.
  • No existing function signatures, return types, error values, or exported symbols were removed or renamed.
  • New exported surface is limited to the two struct fields; new internal helpers (ringOrientation, reverseRing, reverseExtraRange, applyRFC7946Winding) are unexported.

Backward Compatibility

Fully backward compatible. Existing clients continue to work without any code changes because:

  • Both new flags default to false, preserving legacy parse behavior for Polygon and MultiPolygon.
  • Ring winding enforcement only runs when a caller explicitly opts in.
  • Degenerate rings (zero signed area) are deliberately left alone, avoiding unexpected rejections when opting into RequireWinding.
  • The FixWinding path correctly keeps extra.values (z/m data) aligned with reversed point indices, so 3D/4D polygons are not silently corrupted.
  • Test coverage in winding_test.go exercises fix/require paths for both Polygon and MultiPolygon, the no-op case, z-coordinate preservation, and ringOrientation edge cases.

One minor note for callers: the doc comments state the two flags cannot be combined, but the implementation does not explicitly reject that combination — if both are set, RequireWinding takes precedence and returns an error before FixWinding can act. This is consistent behavior but worth confirming against the documented contract.


RISK_SCORE: Low

- Expand FixWinding/RequireWinding godoc: note AllowRects interaction, the
  antimeridian limitation of planar shoelace classification, RFC 7946's
  "SHOULD NOT reject" compatibility clause, and the FixWinding +
  RequireWinding precedence (strict wins).
- ringOrientation: note the antimeridian caveat.
- reverseExtraRange: bounds-check the computed slice range before swapping.
- Add tests: RequireWinding accepts degenerate rings, FixWinding on a
  polygon with a hole and z-coords, MultiPolygon FixWinding with per-child
  z-values.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant