LayerInfo validation doesn't enforce ResourceType uniqueness.

Can you give me more context about the actual problem this produces? The catalog model, or more the api does actually allow for multiple layers for one resource. See the method on Catalog:

List<LayerInfo> getLayers( ResourceInfo resource );

Which is called in a number of places. Although I understand that with layer name bound to resource name this list will generally always contain a single element. However this was designed to be a short term workaround, eventually we would like to break this.

Well, let's back up a bit. As I mentioned on the developers' list I started looking at this code because of a Findbugs flag. The flag was for this line:

if ( existing.getResource().getNamespace().equals( layer.getName() ) ) {

LayerInfo.getResource().getNamespace() produces a NamespaceInfo. LayerInfo.getName() produces a String. A NamespaceInfo and a String can never be equal, so this test doesn't accomplish anything. I assumed that it needed to be rewritten to actually return true for some inputs, but it sounds like you are saying that it should be removed outright.

Ahh... ok, thanks for that, and sorry i missed the part about the type mismatch. So yeah, clearly that is a pointless check. The check was supposed to be designed to ensure that two layers with the same name never end up in a single workspace. The issue of resources being unique I believe is orthogonal to that.

This is a bit of a mess for two reasons. The first being that layer names are tied to resource names. THe second that namespace are tied to workspaces. These were designed to be temporary workarounds until the time where we can actually decouple the LayerINfo class from ResourceInfo which was the original design.

So... i would be tempted to simply name the check look like this:

if ( existing.getResource().getNamespace().equals( layer.getResource().getNamespace() ) ) {

And see what that results in terms of test failures.

Does that make sense?

At first I thought that just updating the if statement would be enough, but then I realized that:

LayerInfo existing = getLayerByName( layer.getName() );

is ambiguous since layer.getName() gives an unqualified name and getLayerByName will return one anyway. I made it a for-loop in my patch, but now I am thinking that maybe we could just use {{ layer.getResource().getNamespace().getPrefix() + ":" + layer.getName() }} to get a qualified name and avoid having to iterate through the whole list.

String qualifiedName =
    layer.getResource().getNamespace().getPrefix() + ":" +
    layer.getName();

LayerInfo existing = getLayerByName(qualifiedName);
if ( existing != null && !existing.getId().equals( layer.getId() ) ) { 
    //JD: since layers are not qualified by anything (yet), check 
    // namespace of the resource, if they are different then allow the 
    // layer to be added
    NamespaceInfo eNamespace = existing.getResource().getNamespace();
    NamespaceInfo lNamespace = layer.getResource().getNamespace();
    if (eNamespace.equals(lNamespace)) {
        throw new IllegalArgumentException(
            "Layer named '" + layer.getName() + "' already exists in " + lNamespace + ".");
    }   
}

And then I realized that now that existing is fully determined by the namespace and name, we actually don't need to check those values for equality. So I think the final check should look like this.

String qualifiedName =
    layer.getResource().getNamespace().getPrefix() + ":" +
    layer.getName();

LayerInfo existing = getLayerByName(qualifiedName);
if ( existing != null && !existing.getId().equals( layer.getId() ) ) { 
    throw new IllegalArgumentException(
        "Layer named '" + layer.getName() + "' already exists in " + lNamespace + ".");
}  

Finally, the null-check on the resource is redundant after all this. If we want to throw a custom exception there instead of a generic NPE, we should move that line to before the uniqueness check. *That* is the patch I'm attaching.

This causes failures in addLayer, I modified it to use a different resource (and thus a different name) for the new layer. I expected similar errors in the multithreaded case but don't see them - I guess changes to the name of the shared resource aren't visible to getLayerByName until after the layer is saved? Is that expected? The multithreaded test is generating a lot of layers with the same name. (I only checked with println but I think they end up all having the same name as the last one set - which you'd expect since they all delegate getName to the same resource.)

The changes make sense even though I am weary of tinkering with such subtle behaviour. I would like another pair of eyes to review this patch since its changing such core functionality. @Andrea, could you look it over.

I imagine by the "multithreaded case" you mean the testGetLayerByIdWithConcurrentAdd test case? What is happening there (from a quick scan i could be wrong) is that the underlying resource is never added to the catalog, so the resource lookup never finds anything and there is not a clash. We should probably do a check on the resource to see if its id has been set, and if not throw a validation exception since it has not yet been added to the catalog. I imagine that will kick out another batch of failures, including testGetLayerByIdWithConcurrentAdd which will have to create a new resource when adding the new layer.

Marking resolved as per the duplicate GEOS-5121

Switching all issues that have been in "resolved" state for more than one month without further comments to "closed" status