[Geoserver-devel] WFS-T Insert: Fids being returned in incorrect order

GeoServer GeoServer Developer
1

Hi,

I was looking at the WFS-T response for Inserts (it’s a list of FIDs). The spec says these should be in the same order as the features in the request’s Insert elements.

However, I’m seeing these in a “strange” order. I think I’ve tracked it down to the DefaultFeatureCollection implementation. This stores the features in a SortedMap<String, SimpleFeature>, and the iterator just returns them in the text-natural-id-order (ie. “new0”, “new1”, “new11”, “new2”, “new3”,… – “new11” is in the wrong order). This messes up the order of the new fids for the inserted features.

Here’s it using the DefaultFeatureCollection;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L98

and the passing it (incorrectly ordered) off to the underlying datastore;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L189

Here’s the DefaultFeatureCollection implementation;
https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/feature/DefaultFeatureCollection.java#L330

I’m surprised that no one’s seen this and there aren’t any test cases for it - am I seeing things?

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I havent been able to figure out how set it to use idgen=UseExisting instead of idgen=GenerateNew?

Thanks,
Dave

2

Interesting, DefaultFeatureCollection has always used a TreeSet (to support fast fid lookup since I figured that was important when initially designing feature collection, my mistake).

There is a quick fix we could try using a ListFeatureCollection in this section of code, care must be taken to avoid duplicate Fids since it is only a wrapper.

ArrayList list = new ArrayList();

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(1,2)”), “name1”}, null) );

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(4,4)”), “name2”}, null) );

SimpleFeatureCollection collection = new ListFeatureCollection(TYPE,list);

For more background/code examples see docs.

···

On 20 March 2017 at 21:45, Dave Blasby <dblasby@anonymised.com> wrote:

Hi,

I was looking at the WFS-T response for Inserts (it’s a list of FIDs). The spec says these should be in the same order as the features in the request’s Insert elements.

However, I’m seeing these in a “strange” order. I think I’ve tracked it down to the DefaultFeatureCollection implementation. This stores the features in a SortedMap<String, SimpleFeature>, and the iterator just returns them in the text-natural-id-order (ie. “new0”, “new1”, “new11”, “new2”, “new3”,… – “new11” is in the wrong order). This messes up the order of the new fids for the inserted features.

Here’s it using the DefaultFeatureCollection;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L98

and the passing it (incorrectly ordered) off to the underlying datastore;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L189

Here’s the DefaultFeatureCollection implementation;
https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/feature/DefaultFeatureCollection.java#L330

I’m surprised that no one’s seen this and there aren’t any test cases for it - am I seeing things?

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I havent been able to figure out how set it to use idgen=UseExisting instead of idgen=GenerateNew?

Thanks,
Dave

Check out the vibrant tech community on one of the world’s most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Geoserver-devel mailing list
Geoserver-devel@anonymised.com.366…sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

–
Jody Garnett

3

Hi, Jody,

I also refactored code using DefaultFeatureCollection to use ListFeatureCollection - so I expect this is a quick patch (wasn’t sure if it would cause issues). I will look at the test cases…

Thanks,
Dave

···

On Mon, Mar 20, 2017 at 10:02 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Interesting, DefaultFeatureCollection has always used a TreeSet (to support fast fid lookup since I figured that was important when initially designing feature collection, my mistake).

There is a quick fix we could try using a ListFeatureCollection in this section of code, care must be taken to avoid duplicate Fids since it is only a wrapper.

ArrayList list = new ArrayList();

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(1,2)”), “name1”}, null) );

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(4,4)”), “name2”}, null) );

SimpleFeatureCollection collection = new ListFeatureCollection(TYPE,list);

For more background/code examples see docs.

–
Jody Garnett

On 20 March 2017 at 21:45, Dave Blasby <dblasby@anonymised.com> wrote:

Hi,

I was looking at the WFS-T response for Inserts (it’s a list of FIDs). The spec says these should be in the same order as the features in the request’s Insert elements.

However, I’m seeing these in a “strange” order. I think I’ve tracked it down to the DefaultFeatureCollection implementation. This stores the features in a SortedMap<String, SimpleFeature>, and the iterator just returns them in the text-natural-id-order (ie. “new0”, “new1”, “new11”, “new2”, “new3”,… – “new11” is in the wrong order). This messes up the order of the new fids for the inserted features.

Here’s it using the DefaultFeatureCollection;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L98

and the passing it (incorrectly ordered) off to the underlying datastore;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L189

Here’s the DefaultFeatureCollection implementation;
https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/feature/DefaultFeatureCollection.java#L330

I’m surprised that no one’s seen this and there aren’t any test cases for it - am I seeing things?

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I havent been able to figure out how set it to use idgen=UseExisting instead of idgen=GenerateNew?

Thanks,
Dave

Check out the vibrant tech community on one of the world’s most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Geoserver-devel mailing list
Geoserver-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

4

Consider that this issue was not found by test cases, or cite tests even I assume you will be pretty well in the clear.

Can you or your team patch for wednesday? I am onto windows installer tomorrow.

···

On 20 March 2017 at 22:19, Dave Blasby <dblasby@anonymised.com> wrote:

Hi, Jody,

I also refactored code using DefaultFeatureCollection to use ListFeatureCollection - so I expect this is a quick patch (wasn’t sure if it would cause issues). I will look at the test cases…

Thanks,
Dave

–
Jody Garnett

On Mon, Mar 20, 2017 at 10:02 PM, Jody Garnett <jody.garnett@anonymised.com> wrote:

Interesting, DefaultFeatureCollection has always used a TreeSet (to support fast fid lookup since I figured that was important when initially designing feature collection, my mistake).

There is a quick fix we could try using a ListFeatureCollection in this section of code, care must be taken to avoid duplicate Fids since it is only a wrapper.

ArrayList list = new ArrayList();

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(1,2)”), “name1”}, null) );

list.add( SimpleFeatureBuilder.build( TYPE, new Object[]{ wkt.read(“POINT(4,4)”), “name2”}, null) );

SimpleFeatureCollection collection = new ListFeatureCollection(TYPE,list);

For more background/code examples see docs.

–
Jody Garnett

On 20 March 2017 at 21:45, Dave Blasby <dblasby@anonymised.com> wrote:

Hi,

I was looking at the WFS-T response for Inserts (it’s a list of FIDs). The spec says these should be in the same order as the features in the request’s Insert elements.

However, I’m seeing these in a “strange” order. I think I’ve tracked it down to the DefaultFeatureCollection implementation. This stores the features in a SortedMap<String, SimpleFeature>, and the iterator just returns them in the text-natural-id-order (ie. “new0”, “new1”, “new11”, “new2”, “new3”,… – “new11” is in the wrong order). This messes up the order of the new fids for the inserted features.

Here’s it using the DefaultFeatureCollection;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L98

and the passing it (incorrectly ordered) off to the underlying datastore;
https://github.com/geoserver/geoserver/blob/2.9.x/src/wfs/src/main/java/org/geoserver/wfs/InsertElementHandler.java#L189

Here’s the DefaultFeatureCollection implementation;
https://github.com/geotools/geotools/blob/master/modules/library/main/src/main/java/org/geotools/feature/DefaultFeatureCollection.java#L330

I’m surprised that no one’s seen this and there aren’t any test cases for it - am I seeing things?

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I havent been able to figure out how set it to use idgen=UseExisting instead of idgen=GenerateNew?

Thanks,
Dave

Check out the vibrant tech community on one of the world’s most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Geoserver-devel mailing list
Geoserver-devel@anonymised.comrge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

5

On Tue, Mar 21, 2017 at 5:45 AM, Dave Blasby <dblasby@anonymised.com>
wrote:

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I
havent been able to figure out how set it to use idgen=UseExisting instead
of idgen=GenerateNew?

I don't know what WFS-NG does, but mind one thing, this control over index
generation is available only in WFS 1.1, it was not there in WFS 1.0, it
got removed in WFS 2.0 (and I don't believe it got wide implementation in
WFS 1.1 either, even in GeoServer if memory serves me well it works only
for JDBC data sources with a non auto-generated primary key).
So... I would not try to rely on it too much unless you're sure about the
server and target store.

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

6

Thanks!

···

On Mar 21, 2017 12:29 AM, “Andrea Aime” <andrea.aime@anonymised.com> wrote:

On Tue, Mar 21, 2017 at 5:45 AM, Dave Blasby <dblasby@anonymised.com> wrote:

Aside - does anyone know how to have WFS-NG preserve FIDs for inserts? I havent been able to figure out how set it to use idgen=UseExisting instead of idgen=GenerateNew?

I don’t know what WFS-NG does, but mind one thing, this control over index generation is available only in WFS 1.1, it was not there in WFS 1.0, it got removed in WFS 2.0 (and I don’t believe it got wide implementation in WFS 1.1 either, even in GeoServer if memory serves me well it works only for JDBC data sources with a non auto-generated primary key).
So… I would not try to rely on it too much unless you’re sure about the server and target store.

Cheers
Andrea

–

==
GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime

@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313

fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

AVVERTENZE AI SENSI DEL D.Lgs. 196/2003

Le informazioni contenute in questo messaggio di posta elettronica e/o nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il loro utilizzo è consentito esclusivamente al destinatario del messaggio, per le finalità indicate nel messaggio stesso. Qualora riceviate questo messaggio senza esserne il destinatario, Vi preghiamo cortesemente di darcene notizia via e-mail e di procedere alla distruzione del messaggio stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso, divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od utilizzarlo per finalità diverse, costituisce comportamento contrario ai principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for the attention and use of the named addressee(s) and may be confidential or proprietary in nature or covered by the provisions of privacy act (Legislative Decree June, 30 2003, no.196 - Italy’s New Data Protection Code).Any use not in accord with its purpose, any disclosure, reproduction, copying, distribution, or either dissemination, either whole or partial, is strictly forbidden except previous formal approval of the named addressee(s). If you are not the intended recipient, please contact immediately the sender by telephone, fax or e-mail and delete the information in this message that has been received in error. The sender does not give any warranty or accept liability as the content, accuracy or completeness of sent messages and accepts no responsibility for changes made after they were sent or for other risks which arise as a result of e-mail transmission, viruses, etc.

7

On Tue, Mar 21, 2017 at 5:45 AM, Dave Blasby <dblasby@anonymised.com>
wrote:

I was looking at the WFS-T response for Inserts (it's a list of FIDs).
The spec says these should be in the same order as the features in the
request's Insert elements.

I never heard of this requirement so I went and check, indeed all 3 spec
say so. However, it seems that at least the old
WFS CITE test we're running did not verify that.

So, you're not seeing things, but I believe you're the first one to care
about it enough to report the problem

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------

8

On mardi 21 mars 2017 09:24:13 CET Andrea Aime wrote:

On Tue, Mar 21, 2017 at 5:45 AM, Dave Blasby dblasby@anonymised.com

wrote:

I was looking at the WFS-T response for Inserts (it’s a list of FIDs).

The spec says these should be in the same order as the features in the

request’s Insert elements.

I never heard of this requirement so I went and check, indeed all 3 spec

say so. However, it seems that at least the old

WFS CITE test we’re running did not verify that.

So, you’re not seeing things, but I believe you’re the first one to care

about it enough to report the problem

Well, getting out of order IDs would certainly mess up things in the QGIS WFS client when doing multiple inserts at once, especially if doing later updates of the features (the wrong feature would be modified on the server)

I’m surprised this wasn’t raised before, but I can see we have this logic :

/* Fix issue with GeoServer and shapefile feature stores when no real

feature id are returned but new0 returned instead of the featureId*/

Q_FOREACH ( const QString &v, idList )

{

if ( v.startsWith( “new” ) )

{

reloadData();

return true;

}

}

which makes that we don’t trust such Ids and have to re-issue a GetFeature request. But if the ids do not start with “new”, we would have the issue.

Even

–

Spatialys - Geospatial professional services

http://www.spatialys.com

9

On Tue, Mar 21, 2017 at 9:41 AM, Even Rouault <even.rouault@anonymised.com>
wrote:

/* Fix issue with GeoServer and shapefile feature stores when no real

feature id are returned but new0 returned instead of the featureId*/

Q_FOREACH ( const QString &v, idList )

{

if ( v.startsWith( "new" ) )

{

reloadData();

return true;

}

}

Nope, this is another issue. With shapefiles our current code has no way to
know the new feature ids generated
(they are being collected before the transaction ends) so it gives back
fake results.
Well known issue, ticket here, second oldest open ticket in the system:

https://osgeo-org.atlassian.net/browse/GEOS-662

And then a repeat here with some other discussion (we should really close
one of the two):
https://osgeo-org.atlassian.net/browse/GEOS-6390

What David reports instead happens also for database backends and was not
known until today.

Cheers
Andrea

--

GeoServer Professional Services from the experts! Visit
http://goo.gl/it488V for more information.

Ing. Andrea Aime
@geowolf
Technical Lead

GeoSolutions S.A.S.
Via di Montramito 3/A
55054 Massarosa (LU)
phone: +39 0584 962313
fax: +39 0584 1660272
mob: +39 339 8844549

http://www.geo-solutions.it
http://twitter.com/geosolutions_it

*AVVERTENZE AI SENSI DEL D.Lgs. 196/2003*

Le informazioni contenute in questo messaggio di posta elettronica e/o
nel/i file/s allegato/i sono da considerarsi strettamente riservate. Il
loro utilizzo è consentito esclusivamente al destinatario del messaggio,
per le finalità indicate nel messaggio stesso. Qualora riceviate questo
messaggio senza esserne il destinatario, Vi preghiamo cortesemente di
darcene notizia via e-mail e di procedere alla distruzione del messaggio
stesso, cancellandolo dal Vostro sistema. Conservare il messaggio stesso,
divulgarlo anche in parte, distribuirlo ad altri soggetti, copiarlo, od
utilizzarlo per finalità diverse, costituisce comportamento contrario ai
principi dettati dal D.Lgs. 196/2003.

The information in this message and/or attachments, is intended solely for
the attention and use of the named addressee(s) and may be confidential or
proprietary in nature or covered by the provisions of privacy act
(Legislative Decree June, 30 2003, no.196 - Italy's New Data Protection
Code).Any use not in accord with its purpose, any disclosure, reproduction,
copying, distribution, or either dissemination, either whole or partial, is
strictly forbidden except previous formal approval of the named
addressee(s). If you are not the intended recipient, please contact
immediately the sender by telephone, fax or e-mail and delete the
information in this message that has been received in error. The sender
does not give any warranty or accept liability as the content, accuracy or
completeness of sent messages and accepts no responsibility for changes
made after they were sent or for other risks which arise as a result of
e-mail transmission, viruses, etc.

-------------------------------------------------------