Fix invalid WKT empty geometries #160

Oreilles · 2023-08-13T00:32:59Z

Currently, empty geometries would be writter in WKT as:

LINESTRING()
POLYGON()
GEOMETRYCOLLECTION()

which isn't valid WKT.

This PR addresses this issue and output instead:

LINESTRING EMPTY
POLYGON EMPTY
GEOMETRYCOLLECTION EMPTY

michaelkirk

Good catch!

michaelkirk · 2023-08-13T01:14:33Z

geozero/src/wkt/wkt_writer.rs

        } else {
            self.out.write_all(b"(")?;
        }
        Ok(())
    }
    fn geom_end(&mut self) -> Result<()> {
-        self.out.write_all(b")")?;
+        if let Some(geometry_size) = self.geometry_sizes.pop() {


Would we ever expect this to be None?

A geometry would have to end before it began, so in theory no ? I just tried to get around the fact that Vec::pop always returns an option!

Consider adding something like else { debug_assert!(false, "ended geometry that didn't start") } as a "this should never happen but we don't want to blow up in production if it does"

Nice catch... #160 (comment)

michaelkirk · 2023-08-13T01:20:57Z

geozero/src/wkt/wkt_writer.rs

-        self.geom_begin(idx, b"POINT EMPTY")
-        // we intentionally omit calling geom_end(), because POINT EMPTY has no closing paren
+        self.geom_begin(b"POINT", true, 0, idx)?;
+        self.geom_end()


This seems less weird. 👍

…calls

Oreilles · 2023-08-13T18:32:29Z

We have a problem with the csv BufferingWktWriter... since it instanciate a new writer for every processor function call, the state isn't persisted and the implementation cannot work.

If we had access to the geometry size in processor functions polygon_end etc., then we wouldn't have to manage the geometry_sizes stack 🤔

michaelkirk · 2023-08-14T00:14:07Z

We have a problem with the csv BufferingWktWriter... since it instanciate a new writer for every processor function call, the state isn't persisted and the implementation cannot work.

I've just opened Oreilles#3 against your own branch that I think addresses this (and simplifies my old crappy code from yesteryear!). Take a look and merge into your branch if you think it makes sense.

michaelkirk · 2023-08-14T00:31:59Z

If we had access to the geometry size in processor functions polygon_end etc., then we wouldn't have to manage the geometry_sizes stack 🤔

I'm also open to this approach (or we could do both) but it seems like a bigger breaking change, so my preference at this point would be to avoid it.

michaelkirk · 2023-08-14T18:13:08Z

geozero/src/wkt/wkt_writer.rs

-impl<'a, W: Write> WktWriter<'a, W> {
-    pub fn new(out: &'a mut W) -> WktWriter<'a, W> {
+impl<W: Write> WktWriter<W> {
+    pub fn new(out: W) -> Self {


This LGTM, but wanted to highlight the change for other reviewers that we're changing WktWriter to take any impl Write rather than only &mut impl Write.

I'm all for simplifying writters so that they own the underlying impl Write - but should it possibly be a separate PR that changes them all? It feels like a counter-pattern to introduce it in just one writer as part of an unrelated empty geometry support. Then we can rebase this PR on top of it?

I'm fine with doing it for all writers. Who wants?

It's not unrelated, in that it's required to fix the bug. But yes, I agree it would be "cleaner" to switch all the API's over, rebase this PR and then merge it rather than follow up with changing the rest of the API's after this is merged. My judgement call was that it wasn't worth the extra paperwork, but if you feel differently thats fine.

I'm ok either way, just as long as we don't release until all writers are more-or-less lined up (and of course its a minor rather than a patch change

and of course its a minor rather than a patch change

Just to be clear, changing from &mut Write -> Write is a non-breaking change, right?

WktWriter is part of the public API - if you change its signature, just like we changed writers in #155 and #150, I think all of these should be released as 0.11, or else any existing code using this in dependencies: geozero = "0.10.0" and using MvtWriter::new() or using let w = WktWriter::new(); w.dims.z = true; will break.

Yes, there is a breaking change to WktWriter in this PR if that's what you mean (see the changelog entry).

So is everyone OK to merge this?

nyurik

lgtm

michaelkirk · 2023-08-16T19:53:39Z

bors r+

bors · 2023-08-16T19:53:55Z

Configuration problem:
bors.toml: not found

nyurik · 2023-08-16T19:56:59Z

i thought there were some discussions in rust itself to stop using bors because github now covers most such usecases?

michaelkirk · 2023-08-17T02:49:26Z

Yes, there were some of us trying to figure out GH merge queues a couple weeks ago. If you look at the proj crate’s recent commits for example, you’ll see some of my misadventures. As far as I know we don’t have any working examples on georust where GH’s merge queues function like bors: i.e. I trigger the process, then the tests run against an up-to-date branch and only get merged into main once they pass.It seems so basic, but i kept finding that the branches would merge immediately, regardless of if the tests pass.I’m sure it’s possible, I just didn’t personally dedicate the time to figuring it out. If you tell me what series of buttons to press I can press them.

michaelkirk · 2023-08-17T18:29:24Z

I just realized that this project does not, and never did, use bors. 😅

nyurik · 2023-08-18T19:53:58Z

@michaelkirk in Martin, we have branch protection rule for main:

Require a pull request before merging
Require approvals (1)
Require status checks to pass before merging (with most of the key CI jobs listed as required)

I think if you also enable Require branches to be up to date before merging, you will achieve what you want: a way to +1 something, plus a way to say "squash once CI passes" - this way it will not be squashed unless everything is good, and it will not pass if another PR merges ahead of it. Granted that it will fail if another PR merges first though.

michaelkirk · 2023-08-18T22:58:48Z

I got it working! Ultimately I think the issue was something else, but thanks for the sanity check @nyurik.

As of georust#160, this breaks with invalid output: GEOMETRYCOLLECTION EMPTYPOINT(-122.329051 47.6069),POINT(-122.266529 47.515984)

Oreilles added 2 commits August 13, 2023 02:32

Fix invalid empty geometries

333342d

Formatting

c730fc6

michaelkirk approved these changes Aug 13, 2023

View reviewed changes

michaelkirk reviewed Aug 13, 2023

View reviewed changes

Add debug assert in case of inconsistent geom_begin and geom_end …

3e7f6b8

…calls

remove unnecessary lifetime param

f094d93

michaelkirk and others added 2 commits August 13, 2023 17:33

WKTWriter can own its Vec, obviating BufferedWktWriter

2ba4875

Merge pull request #3 from michaelkirk/mkirk/empty-wkt

e79fd06

michaelkirk reviewed Aug 14, 2023

View reviewed changes

Oreilles force-pushed the empty-wkt branch from baeb26b to 782953c Compare August 15, 2023 19:32

Omit whitespace before EMPTY if geometry isn't tagged

b2381e5

Oreilles force-pushed the empty-wkt branch from 782953c to b2381e5 Compare August 15, 2023 20:16

nyurik approved these changes Aug 15, 2023

View reviewed changes

michaelkirk merged commit 47c02ea into georust:main Aug 17, 2023
1 check passed

michaelkirk added a commit to michaelkirk/geozero that referenced this pull request Aug 21, 2023

tests for invalid wkt output

c9cc5f6

As of georust#160, this breaks with invalid output: GEOMETRYCOLLECTION EMPTYPOINT(-122.329051 47.6069),POINT(-122.266529 47.515984)

michaelkirk added a commit to michaelkirk/geozero that referenced this pull request Aug 21, 2023

Fix: invalid WKT output for CSV's

c7b0c7a

As of georust#160, this breaks with invalid output: GEOMETRYCOLLECTION EMPTYPOINT(-122.329051 47.6069),POINT(-122.266529 47.515984)

michaelkirk mentioned this pull request Aug 21, 2023

Fix: invalid WKT output for CSV's #167

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid WKT empty geometries #160

Fix invalid WKT empty geometries #160

Oreilles commented Aug 13, 2023

michaelkirk left a comment

michaelkirk Aug 13, 2023

Oreilles Aug 13, 2023 •

edited

Loading

michaelkirk Aug 13, 2023

Oreilles Aug 13, 2023

Oreilles Aug 13, 2023

michaelkirk Aug 13, 2023

Oreilles commented Aug 13, 2023 •

edited

Loading

michaelkirk commented Aug 14, 2023

michaelkirk commented Aug 14, 2023

michaelkirk Aug 14, 2023

nyurik Aug 14, 2023

pka Aug 14, 2023

michaelkirk Aug 14, 2023

nyurik Aug 14, 2023

michaelkirk Aug 14, 2023

nyurik Aug 14, 2023

michaelkirk Aug 15, 2023

nyurik left a comment

michaelkirk commented Aug 16, 2023

bors bot commented Aug 16, 2023

nyurik commented Aug 16, 2023

michaelkirk commented Aug 17, 2023 via email •

edited

Loading

michaelkirk commented Aug 17, 2023

nyurik commented Aug 18, 2023

michaelkirk commented Aug 18, 2023

Fix invalid WKT empty geometries #160

Fix invalid WKT empty geometries #160

Conversation

Oreilles commented Aug 13, 2023

michaelkirk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oreilles Aug 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oreilles commented Aug 13, 2023 • edited Loading

michaelkirk commented Aug 14, 2023

michaelkirk commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik left a comment

Choose a reason for hiding this comment

michaelkirk commented Aug 16, 2023

bors bot commented Aug 16, 2023

nyurik commented Aug 16, 2023

michaelkirk commented Aug 17, 2023 via email • edited Loading

michaelkirk commented Aug 17, 2023

nyurik commented Aug 18, 2023

michaelkirk commented Aug 18, 2023

Oreilles Aug 13, 2023 •

edited

Loading

Oreilles commented Aug 13, 2023 •

edited

Loading

michaelkirk commented Aug 17, 2023 via email •

edited

Loading