[Geoserver-devel] CatalogImpl allows to add multiple layers for a given ResourceInfo leaving the catalog in an inconsistent state

GeoServer GeoServer Developer
1

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel
--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

2

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com…1501…> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

3

Hey, sorry for the long time taken to get back to this.

On Mon, May 21, 2012 at 5:43 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

Yeah I know in the long term goal is to allow multiple layerinfos reference the same resourceinfo. Yet at the current state of affair that is not possible. Doing so leads to a broken catalog. So it’d be better to enforce the 1:1 relationship until we actually fix that.
The thing goes like that, as far as I understand it:

  • Add FeatureTypeInfo FT1
  • Add LayerInfo for it, it’s gonna be called FT1 too
  • Add a second LayerInfo for FT1, set its name to L2. LayerInfoImpl.setName(String name) does this.resource.setName(name)
  • Now the resourceinfo name changed under the hook, the layer L2 was saved, but both layers end up being called the same, L2, as LayerInfoImpl.getName() delegates to this.resource.getName();

In the end, you have multiple layers referencing the same resource but called the same. Moreover, the under-the-hook change for the resourceinfo is never persisted with the in-memory catalog, but the test that adds multiple layers for the same resource doesn’t check anything besides the add not failing.

Now, for the in-memory catalog the side effect is just that the ResourceInfo gets renamed under the hook. If that were actually done in application code, the result would be that after restart all the layers would be named as the resourceinfo was first named, as it was not persisted.

For the database implementation it is an impediment because the queries are evaluated against the persisted attribute value, not the in-memory one, and hence can’t go on. By either way, the Catalog is broken, and as far as I know, nowhere in application code this is being used. Hence the need to enforce the 1:1 relationship until we take care of it in a saner way (i.e. allowing layers to be named independently of its resource).

The following test (add it to CatalogImplTest) fails at the second assert. It should actually fail at the first one because there’s more than one layer named “LAYER-2”, but DefaultCatalogFacade short-circuits the evaluation and returns at the first match.

public void testAddMultipleLayersForTheSameResource()throws Exception{
addLayer();
final String initialName = ft.getName();

LayerInfo layer = catalog.getFactory().createLayer();
layer.setResource(ft);
layer.setName(“LAYER-2”);
catalog.add(layer);

assertNotNull(catalog.getLayerByName(“LAYER-2”));
assertNotNull(catalog.getLayerByName(initialName));
}

Hope it all makes sense now, let me know if I’m missing something.

Cheers,
Gabriel

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

4

Hey Gabriel,

Thanks for the explanation. Since indeed the many-to-one layer to resource is never really used i am fine with the change, just wanted to bring up the concern. I am fine with handling that at some later point when we have an actual driver for it.

SO fine with the change for now unless someone else can come up with a good reason not to enforce the 1-1 relationship.

-Justin

On Wed, Jun 6, 2012 at 8:02 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey, sorry for the long time taken to get back to this.

On Mon, May 21, 2012 at 5:43 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

Yeah I know in the long term goal is to allow multiple layerinfos reference the same resourceinfo. Yet at the current state of affair that is not possible. Doing so leads to a broken catalog. So it’d be better to enforce the 1:1 relationship until we actually fix that.
The thing goes like that, as far as I understand it:

  • Add FeatureTypeInfo FT1
  • Add LayerInfo for it, it’s gonna be called FT1 too
  • Add a second LayerInfo for FT1, set its name to L2. LayerInfoImpl.setName(String name) does this.resource.setName(name)
  • Now the resourceinfo name changed under the hook, the layer L2 was saved, but both layers end up being called the same, L2, as LayerInfoImpl.getName() delegates to this.resource.getName();

In the end, you have multiple layers referencing the same resource but called the same. Moreover, the under-the-hook change for the resourceinfo is never persisted with the in-memory catalog, but the test that adds multiple layers for the same resource doesn’t check anything besides the add not failing.

Now, for the in-memory catalog the side effect is just that the ResourceInfo gets renamed under the hook. If that were actually done in application code, the result would be that after restart all the layers would be named as the resourceinfo was first named, as it was not persisted.

For the database implementation it is an impediment because the queries are evaluated against the persisted attribute value, not the in-memory one, and hence can’t go on. By either way, the Catalog is broken, and as far as I know, nowhere in application code this is being used. Hence the need to enforce the 1:1 relationship until we take care of it in a saner way (i.e. allowing layers to be named independently of its resource).

The following test (add it to CatalogImplTest) fails at the second assert. It should actually fail at the first one because there’s more than one layer named “LAYER-2”, but DefaultCatalogFacade short-circuits the evaluation and returns at the first match.

public void testAddMultipleLayersForTheSameResource()throws Exception{
addLayer();
final String initialName = ft.getName();

LayerInfo layer = catalog.getFactory().createLayer();
layer.setResource(ft);
layer.setName(“LAYER-2”);
catalog.add(layer);

assertNotNull(catalog.getLayerByName(“LAYER-2”));
assertNotNull(catalog.getLayerByName(initialName));
}

Hope it all makes sense now, let me know if I’m missing something.

Cheers,
Gabriel

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

5

Sounds like a dupe of http://jira.codehaus.org/browse/GEOS-5001?

–
David Winslow
OpenGeo - http://opengeo.org/

On Wed, Jun 6, 2012 at 6:10 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Thanks for the explanation. Since indeed the many-to-one layer to resource is never really used i am fine with the change, just wanted to bring up the concern. I am fine with handling that at some later point when we have an actual driver for it.

SO fine with the change for now unless someone else can come up with a good reason not to enforce the 1-1 relationship.

-Justin

On Wed, Jun 6, 2012 at 8:02 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey, sorry for the long time taken to get back to this.

On Mon, May 21, 2012 at 5:43 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

Yeah I know in the long term goal is to allow multiple layerinfos reference the same resourceinfo. Yet at the current state of affair that is not possible. Doing so leads to a broken catalog. So it’d be better to enforce the 1:1 relationship until we actually fix that.
The thing goes like that, as far as I understand it:

  • Add FeatureTypeInfo FT1
  • Add LayerInfo for it, it’s gonna be called FT1 too
  • Add a second LayerInfo for FT1, set its name to L2. LayerInfoImpl.setName(String name) does this.resource.setName(name)
  • Now the resourceinfo name changed under the hook, the layer L2 was saved, but both layers end up being called the same, L2, as LayerInfoImpl.getName() delegates to this.resource.getName();

In the end, you have multiple layers referencing the same resource but called the same. Moreover, the under-the-hook change for the resourceinfo is never persisted with the in-memory catalog, but the test that adds multiple layers for the same resource doesn’t check anything besides the add not failing.

Now, for the in-memory catalog the side effect is just that the ResourceInfo gets renamed under the hook. If that were actually done in application code, the result would be that after restart all the layers would be named as the resourceinfo was first named, as it was not persisted.

For the database implementation it is an impediment because the queries are evaluated against the persisted attribute value, not the in-memory one, and hence can’t go on. By either way, the Catalog is broken, and as far as I know, nowhere in application code this is being used. Hence the need to enforce the 1:1 relationship until we take care of it in a saner way (i.e. allowing layers to be named independently of its resource).

The following test (add it to CatalogImplTest) fails at the second assert. It should actually fail at the first one because there’s more than one layer named “LAYER-2”, but DefaultCatalogFacade short-circuits the evaluation and returns at the first match.

public void testAddMultipleLayersForTheSameResource()throws Exception{
addLayer();
final String initialName = ft.getName();

LayerInfo layer = catalog.getFactory().createLayer();
layer.setResource(ft);
layer.setName(“LAYER-2”);
catalog.add(layer);

assertNotNull(catalog.getLayerByName(“LAYER-2”));
assertNotNull(catalog.getLayerByName(initialName));
}

Hope it all makes sense now, let me know if I’m missing something.

Cheers,
Gabriel

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Live Security Virtual Conference
Exclusive live event will cover all the ways today’s security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

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

6

Indeed, good catch.
So can we say this would be the add(LayerInfo) contract?

/**

  • @precondition layer != null
  • @precondition layer.getId() == null
  • @precondition layer.getName() != null
  • @precondition layer.getResource() != null
  • @precondition layer.getResource().getId() != null
  • @precondition getLayerByName(layer.prefixedName()) == null
  • @postcondition layer.getType() != null
  • @postcondition layer.getId() != null
  • @see org.geoserver.catalog.Catalog#add(org.geoserver.catalog.LayerInfo)
    */
    public void add(LayerInfo layer) ;

(@pre/postcondition annotations don’t come from any library we’re using, although it’d be nice to state the contracts that way and configure the maven javadoc plugin with those tags?)

On Thu, Jun 7, 2012 at 10:05 AM, David Winslow <dwinslow@anonymised.com> wrote:

Sounds like a dupe of http://jira.codehaus.org/browse/GEOS-5001?

–
David Winslow
OpenGeo - http://opengeo.org/

On Wed, Jun 6, 2012 at 6:10 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Thanks for the explanation. Since indeed the many-to-one layer to resource is never really used i am fine with the change, just wanted to bring up the concern. I am fine with handling that at some later point when we have an actual driver for it.

SO fine with the change for now unless someone else can come up with a good reason not to enforce the 1-1 relationship.

-Justin

On Wed, Jun 6, 2012 at 8:02 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey, sorry for the long time taken to get back to this.

On Mon, May 21, 2012 at 5:43 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

Yeah I know in the long term goal is to allow multiple layerinfos reference the same resourceinfo. Yet at the current state of affair that is not possible. Doing so leads to a broken catalog. So it’d be better to enforce the 1:1 relationship until we actually fix that.
The thing goes like that, as far as I understand it:

  • Add FeatureTypeInfo FT1
  • Add LayerInfo for it, it’s gonna be called FT1 too
  • Add a second LayerInfo for FT1, set its name to L2. LayerInfoImpl.setName(String name) does this.resource.setName(name)
  • Now the resourceinfo name changed under the hook, the layer L2 was saved, but both layers end up being called the same, L2, as LayerInfoImpl.getName() delegates to this.resource.getName();

In the end, you have multiple layers referencing the same resource but called the same. Moreover, the under-the-hook change for the resourceinfo is never persisted with the in-memory catalog, but the test that adds multiple layers for the same resource doesn’t check anything besides the add not failing.

Now, for the in-memory catalog the side effect is just that the ResourceInfo gets renamed under the hook. If that were actually done in application code, the result would be that after restart all the layers would be named as the resourceinfo was first named, as it was not persisted.

For the database implementation it is an impediment because the queries are evaluated against the persisted attribute value, not the in-memory one, and hence can’t go on. By either way, the Catalog is broken, and as far as I know, nowhere in application code this is being used. Hence the need to enforce the 1:1 relationship until we take care of it in a saner way (i.e. allowing layers to be named independently of its resource).

The following test (add it to CatalogImplTest) fails at the second assert. It should actually fail at the first one because there’s more than one layer named “LAYER-2”, but DefaultCatalogFacade short-circuits the evaluation and returns at the first match.

public void testAddMultipleLayersForTheSameResource()throws Exception{
addLayer();
final String initialName = ft.getName();

LayerInfo layer = catalog.getFactory().createLayer();
layer.setResource(ft);
layer.setName(“LAYER-2”);
catalog.add(layer);

assertNotNull(catalog.getLayerByName(“LAYER-2”));
assertNotNull(catalog.getLayerByName(initialName));
}

Hope it all makes sense now, let me know if I’m missing something.

Cheers,
Gabriel

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Live Security Virtual Conference
Exclusive live event will cover all the ways today’s security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

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

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

7

Looks good. All for enforcing the current contract but I will admit i am still a bit hesitant since in my mind the one layer per resource is one we are likely going to break in the future, so weary of digging our heels in to much in terms of the contract.

On Fri, Jun 8, 2012 at 7:20 PM, Gabriel Roldan <groldan@anonymised.com> wrote:

Indeed, good catch.
So can we say this would be the add(LayerInfo) contract?

/**

  • @precondition layer != null
  • @precondition layer.getId() == null
  • @precondition layer.getName() != null
  • @precondition layer.getResource() != null
  • @precondition layer.getResource().getId() != null
  • @precondition getLayerByName(layer.prefixedName()) == null
  • @postcondition layer.getType() != null
  • @postcondition layer.getId() != null
  • @see org.geoserver.catalog.Catalog#add(org.geoserver.catalog.LayerInfo)
    */
    public void add(LayerInfo layer) ;

(@pre/postcondition annotations don’t come from any library we’re using, although it’d be nice to state the contracts that way and configure the maven javadoc plugin with those tags?)

On Thu, Jun 7, 2012 at 10:05 AM, David Winslow <dwinslow@anonymised.com> wrote:

Sounds like a dupe of http://jira.codehaus.org/browse/GEOS-5001?

–
David Winslow
OpenGeo - http://opengeo.org/

On Wed, Jun 6, 2012 at 6:10 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Thanks for the explanation. Since indeed the many-to-one layer to resource is never really used i am fine with the change, just wanted to bring up the concern. I am fine with handling that at some later point when we have an actual driver for it.

SO fine with the change for now unless someone else can come up with a good reason not to enforce the 1-1 relationship.

-Justin

On Wed, Jun 6, 2012 at 8:02 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey, sorry for the long time taken to get back to this.

On Mon, May 21, 2012 at 5:43 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hey Gabriel,

Not so sure… the catalog api is set up to allow multiple layers for a single resource. I realize that at the moment there is a 1-1 relationship but one of the long term goals for the catalog is to move all publishing concerns from REsourceInfo to LayerInfo, at which time I think the case for having multiple layers for a single resource will become stronger.

What exactly is the impediment?

Yeah I know in the long term goal is to allow multiple layerinfos reference the same resourceinfo. Yet at the current state of affair that is not possible. Doing so leads to a broken catalog. So it’d be better to enforce the 1:1 relationship until we actually fix that.
The thing goes like that, as far as I understand it:

  • Add FeatureTypeInfo FT1
  • Add LayerInfo for it, it’s gonna be called FT1 too
  • Add a second LayerInfo for FT1, set its name to L2. LayerInfoImpl.setName(String name) does this.resource.setName(name)
  • Now the resourceinfo name changed under the hook, the layer L2 was saved, but both layers end up being called the same, L2, as LayerInfoImpl.getName() delegates to this.resource.getName();

In the end, you have multiple layers referencing the same resource but called the same. Moreover, the under-the-hook change for the resourceinfo is never persisted with the in-memory catalog, but the test that adds multiple layers for the same resource doesn’t check anything besides the add not failing.

Now, for the in-memory catalog the side effect is just that the ResourceInfo gets renamed under the hook. If that were actually done in application code, the result would be that after restart all the layers would be named as the resourceinfo was first named, as it was not persisted.

For the database implementation it is an impediment because the queries are evaluated against the persisted attribute value, not the in-memory one, and hence can’t go on. By either way, the Catalog is broken, and as far as I know, nowhere in application code this is being used. Hence the need to enforce the 1:1 relationship until we take care of it in a saner way (i.e. allowing layers to be named independently of its resource).

The following test (add it to CatalogImplTest) fails at the second assert. It should actually fail at the first one because there’s more than one layer named “LAYER-2”, but DefaultCatalogFacade short-circuits the evaluation and returns at the first match.

public void testAddMultipleLayersForTheSameResource()throws Exception{
addLayer();
final String initialName = ft.getName();

LayerInfo layer = catalog.getFactory().createLayer();
layer.setResource(ft);
layer.setName(“LAYER-2”);
catalog.add(layer);

assertNotNull(catalog.getLayerByName(“LAYER-2”));
assertNotNull(catalog.getLayerByName(initialName));
}

Hope it all makes sense now, let me know if I’m missing something.

Cheers,
Gabriel

On Mon, May 21, 2012 at 10:15 AM, Gabriel Roldan <groldan@anonymised.com> wrote:

Hey there,

may I draw your attention to http://jira.codehaus.org/browse/GEOS-5121
for a minute and ask to comment out on whether the description and
patch look good?

I found this an impediment on implementing the GSIP-69 jdbc backend so
please check it at your earliest convenience.

TIA,
Gabriel

Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Live Security Virtual Conference
Exclusive live event will cover all the ways today’s security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/

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

–
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.