Hey,
stumbled upon this code in CatalogImpl.validate(NamespaceInfo) and there seems to be a bug:
if (!namespace.isIsolated()) {
// not an isolated namespace \ workplace so we need to check for duplicates
existing = getNamespaceByURI(namespace.getURI());
if (existing != null && !existing.getId().equals(namespace.getId())) {
throw new IllegalArgumentException(
“Namespace with URI '” + namespace.getURI() + “’ already exists.”);
}
}
It calls getNamespaceByURI(namespace.getURI()), but that will get you only one of the possible many (say you have multiple isolated workspaces with the same namespace URI)
I think the following would be correct, provided my understanding is too:
if (!namespace.isIsolated()) {
// not an isolated namespace \ workplace so we need to check for duplicates
List currentList = facade.getNamespacesByURI(namespace.getURI());
for (NamespaceInfo current : currentList) {
if (!current.getId().equals(namespace.getId())) {
throw new IllegalArgumentException(
“Namespace with URI '” + namespace.getURI() + “’ already exists.”);
}
}
}
···
Gabriel Roldán
Tested and indeed, there’s a race condition the will leave you with an invalid configuration:
ws3:http://example.com
- Change the config of ws1 to isolated:false and save
now, depending existing = getNamespaceByURI(namespace.getURI());
may return ws1, in which case the validation proceeds and lets you with
ws1 being non isolated and the other two being isolated, but with the same ns uri.
···
Gabriel Roldán
Hola Gabriel,
The case you exposed about having same URI for one global workspace and several isolated ones is valid as stated on GSIP that introduced isolated workspaces:
https://github.com/geoserver/geoserver/wiki/GSIP-165---Add-isolated-workspaces-concept-to-GeoServer
The following situation will be valid:
But this one will not be a valid one:
Only one non isolated workspace can use a certain namespace.
Also on method:
IsolatedCatalogFacade.getNamespaceByURI(String)
Only global namespace can be returned, isolated ones are avoided so only one global workspace should match the lookup:
https://github.com/geoserver/geoserver/blob/290813cbc463e68459b7eb7d8e6b031b0e192657/src/main/src/main/java/org/geoserver/catalog/impl/IsolatedCatalogFacade.java#L347
Regards,
Fernando Mino
···
Gabriel Roldán
Hi Fernando,
thanks for the explanation.
My curiosity arose from IsolatedCatalogFacade.getNamespaceByURI showing up as a hotspot while doing some load testing for the REST config api, called by CatalogImpl.validate().
Then I thought I could guard the method as follows:
public NamespaceInfo getNamespaceByURI(String uri) {
if (Dispatcher.REQUEST.get() == null) {
return facade.getNamespaceByURI(uri);
}
…
Given my understanding was IsolatedCatalogFacade has nothing to do for non OWS requests. Would that be correct?
I guess that means CatalogFacade.getNamespaceByURI() will have to know it oughta return only global namespaces instead of the first one it finds?
···
Gabriel Roldán
Hi Gabriel,
Seems like IsolatedCatalogFacade.getNamespaceByURI method is working as a filtered lookup (not isolated namespaces results in the mix).
If we return directly facade.getNamespaceByURI(uri) this could match an isolated namespace since in DefaultCatalogFacade.getNamespaceByURI(String) there isn’t any filter for isolated NS URIs.
Also IsolatedCatalogFacade.getNamespaceByURI method uses getLocalNamespace() for getting the current local virtual service Namespace (if an OWS request is in context) to make available the isolated namespace in the lookup. So it seems like IsolatedCatalogFacade class was designed to support both: OWS and not OWS scoped requests.
Regards,
Fernando Mino
···
Gabriel Roldán