# Mapping paths through GeoServer source code Accurate and detailed maps are incredibly helpful to travelers, whether they're planning a trip trying not to get lost in the middle of nowhere\. GeoServer assists in processing such data on the server side\. Let's see what lurks inside this project\. ![1161_geoserver_article/image1.png](https://import.viva64.com/docx/blog/1161_geoserver_article/image1.png) ## Geo is back Suddenly, right after I finished writing about GeoGebra, I came across GeoServer\. Apparently, this prefix doesn't want to leave me yet\. However, unlike the previous project in the Geo family \(which aren't related\), this one isn't about geometry but about geography\. So, what's GeoServer? It's a server\! But not just any server, of course, but one that delivers data and maps to clients such as web browsers or geographic information systems\. Those interested can find a detailed description and the list of features on the [GeoServer](https://geoserver.org/), [OSGeo](https://www.osgeo.org/projects/geoserver/), or [GeoSolutions](https://geosolutionsgroup.com/technologies/geoserver/) websites\. However, in order to understand what we're dealing with, let's take a look at the interface of the server access panel: ![1161_geoserver_article/image2.png](https://import.viva64.com/docx/blog/1161_geoserver_article/image2.png) On the first start, we get some initial data, so we can even visualize it on the map: ![1161_geoserver_article/image3.png](https://import.viva64.com/docx/blog/1161_geoserver_article/image3.png) We get an interactive world map in the OpenLayers format, where clicking on a location reveals its details\. I've outlined the server capabilities\. If I had more time, I'd play around building an interesting local map using open data \(like [OpenStreetMap](https://www.openstreetmap.org)\), but I'll leave it as it is for now\. Now I suggest we dive deeper into such a great service\. We won't just look at it but also try to spot some errors using the PVS\-Studio static analyzer\. So, let's unfold the code maps and begin our journey\. ## Typos ### Two models are like one ```cpp private GridSampleDimension[] getCoverageSampleDimensions( GridCoverage2DReader reader, Map customParameters) throws TransformException, IOException, Exception { .... ColorModel cm = imageLayout.getColorModel(null); if (cm == null) { throw new Exception( "Unable to acquire test coverage and color model for format:" + format.getName()); } SampleModel sm = imageLayout.getSampleModel(null); if (cm == null) { throw new Exception( "Unable to acquire test coverage and sample model for format:" + format.getName()); } .... } ``` By creating two similar variables, _cm_ and _sm_, one can easily mix them up later on and overlook the issue during code review\. Clearly, the original intent was to check _sm_ for _null_, not to repeat the check that's already been done\. Let's fix the typo and move on\. We have two warnings that indicate an error here: * [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'cm \=\= null' is always false\. CatalogBuilder\.java 1242 * [V6080](https://pvs-studio.com/en/docs/warnings/v6080/) Consider checking for misprints\. It's possible that an assigned variable should be checked in the next condition\. CatalogBuilder\.java 1241, CatalogBuilder\.java 1242 ### Right version needed ```cpp public StyledLayerDescriptor run(final GetStylesRequest request) throws ServiceException { if (request.getSldVer() != null && "".equals(request.getSldVer()) && !"1.0.0".equals(request.getSldVer())) throw new ServiceException("SLD version " + request.getSldVer() + " not supported"); .... } ``` We see an unusual check here\. It makes no sense to check if _request\.getSldVer\(\)_ is equal to an empty string and unequal to _1\.0\.0_ at the same time\. The analyzer warns us about it: [V6057](https://pvs-studio.com/en/docs/warnings/v6057/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. GetStyles\.java 40 What happened? How to fix it? The message we should receive if the check is successful informs us of an SLD version that is not supported\. I set out to learn what SLD is and what versions of it GeoServer supports\. After all, that's what the [documentation](https://docs.geoserver.org/stable/en/user/styling/sld/reference/index.html) is for, and it reveals the following: > The OGC Styled Layer Descriptor \(SLD\) standard defines a language for expressing styling of geospatial data\. GeoServer uses SLD as its primary styling language\. We don't need to go into details, but it's still a good idea to clarify a few things about the versions of this standard\. Just a little further down we find the following: > GeoServer implements the SLD 1\.0\.0 standard, as well as some parts of the SE 1\.1\.0 and WMS\-SLD 1\.1\.0 standards\. Now we know that versions 1\.0\.0 and 1\.1\.0 are supported\. In our code snippet, we find a 1\.0\.0 version mismatch check, but the empty string equivalence check breaks it\. If the query contains the required version, everything is fine\. However, even if the version is something like 3\.5\.2, we still won't throw an exception\. I see two options here: either remove the check for an empty string, or add the _\!"1\.1\.0"\.equals\(request\.getSldVersion\(\)\)_ check, as the documentation states, this version is partially supported\. ### Tall and wide Some analyzer messages describe the issue in detail, with no further explanations needed\. For example, take a look at the following code fragment: ```cpp int width = w instanceof Integer ? ((Integer)w) : Integer.parseInt((String)w); int height = w instanceof Integer ? ((Integer)h) : Integer.parseInt((String)h); ``` And here's the analyzer message: [V6072](https://pvs-studio.com/en/docs/warnings/v6072/) Two similar code fragments were found\. Perhaps, this is a typo and 'h' variable should be used instead of 'w'\. Wcs10GetCoverageRequestReader\.java 229, Wcs10GetCoverageRequestReader\.java 229, Wcs10GetCoverageRequestReader\.java 230, Wcs10GetCoverageRequestReader\.java 230 ### Don't sort unsortable ```cpp @Override public CloseableIterator list( final Class of, final Filter filter, @Nullable Integer offset, @Nullable Integer count, @Nullable SortBy... sortOrder) { if (sortOrder != null) { // <= for (SortBy so : sortOrder) { if (sortOrder != null && // <= !canSort(of, so.getPropertyName().getPropertyName())) { throw new IllegalArgumentException( "Can't sort objects of type " + of.getName() + " by " + so.getPropertyName()); } } } .... } ``` [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'sortOrder \!\= null' is always true\. DefaultCatalogFacade\.java 1138 The same check repeats with no changes\. It's clear that the value of the second one is always _true_\. Is it just redundant then? I suspect that the developers wanted to check the _sortOrder_ elements for _null_\. Then it should be _so \!\= null_\. Such a check would protect against a potential _NullPointerException_ when using _so\.getPropertyName\(\)_\. ## Avoiding NullPointerException ### Did they check the right thing? ```cpp protected void updateAttributeStats(DataAttribute attribute) throws IOException { .... // check we can compute min and max PropertyDescriptor pd = fs.getSchema().getDescriptor(attribute.getName()); Class binding = pd.getType().getBinding(); if (pd == null || !Comparable.class.isAssignableFrom(binding) || Geometry.class.isAssignableFrom(binding)) { return; } .... } ``` What's suspicious here? It's the _pd \=\= null_ check\. Let's look at the PVS\-Studio analyzer warning and start investigating the case: [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'pd' reference was utilized before it was verified against null\. DataPanel\.java 117, DataPanel\.java 118 Isn't it strange to check the _pd_ variable after it has been used as _pd\.getType\(\)_? If it were _null_, we'd be dealing with a _NullPointerException_ now\. At this point, I came up with a hypothesis\. Take a look at the _isAssignableFrom_ method\. Referring to the Javadocs, I won't quote the entire description, we need only a particular part of it: > @throws NullPointerException if the specified Class parameter is null So, we have our first clue: _binding_ shouldn't be _null_\. Next, let's see if _pd\.getType\(\)\.getBinding\(\)_ can return _null_\. The method references the _PropertyType_ interface\. Here we're now, in a dependency called _geotools_\. Fortunately, once again, the [_Javadocs_](https://docs.geotools.org/stable/javadocs/org/geotools/api/feature/type/PropertyType.html) come to the rescue\. We read about _getBindings_ and find this: > This value is never null\. Well, I'm giving up my hypothesis\. What was it about? I thought they wanted to check _binding_\. Do we need _pd_ to be checked? In our case, _fs\.getSchema\(\)_ returns _ComplexType_\. We find _getDescriptor_, [read](https://docs.geotools.org/stable/javadocs/org/geotools/api/feature/type/ComplexType.html) it again, and see: > This method returns null if no such property is found\. Wonderful\. Indeed, we should check if pd gets any value, but the check in the code comes too late\. ### Where's my SpringSecurityException? ```cpp @Test public void testChallenge() throws Exception { Map raw = getWorld(); try { executeGetCoverageKvp(raw); fail("This should have failed with a security exception"); } catch (Throwable e) { // make sure we are dealing with some security exception Throwable se = null; while (e.getCause() != null && e.getCause() != e) { e = e.getCause(); if (SecurityUtils.isSecurityException(e)) { se = e; } } if (e == null) { // <= fail("We should have got some sort of SpringSecurityException"); } else { // some mumbling about not having enough privileges assertTrue(se.getMessage().contains("World")); assertTrue(se.getMessage().contains("privileges")); } } } ``` Here we are, right in the test\. This is where the check for a _SpringSecurityException_ takes place\. However, PVS\-Studio finds an anomaly in the _e \=\= null_ check: [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'e' reference was utilized before it was verified against null\. ResourceAccessManagerWCSTest\.java 186, ResourceAccessManagerWCSTest\.java 193 Indeed, why checking a variable for _null_ if it has already been used? Fortunately, the code provides plenty of comments and an explanatory message in _fail_\. We expect to catch a security exception, and it's written to the _se_ variable\. We need to check this variable for _null_\. By the way, since _e \=\= null_ never returns _true_, this test easily passes\. ### Early utilization ```cpp public CoverageViewAbstractPage( String workspaceName, String storeName, String coverageName, CoverageInfo coverageInfo) throws IOException { .... // grab the coverage view coverageViewInfo = coverageInfo != null ? coverageInfo : catalog.getResourceByStore(store, coverageName, CoverageInfo.class); CoverageView coverageView = coverageViewInfo .getMetadata() // <= .get(CoverageView.COVERAGE_VIEW, CoverageView.class); // the type can be still not saved if (coverageViewInfo != null) { // <= coverageInfoId = coverageViewInfo.getId(); } if (coverageView == null) { throw new IllegalArgumentException( "The specified coverage does not have a coverage view attached to it"); } .... } ``` [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'coverageViewInfo' reference was utilized before it was verified against null\. CoverageViewAbstractPage\.java 128, CoverageViewAbstractPage\.java 132 We encounter another suspicious check\. The _null_ check here raises some concerns\. If it's expected that _coverageViewInfo_ can be _null_, then calling _coverageViewInfo\.getMetadata\(\)_ may result in the _NullPointerException_\. It doesn't look like a typo, so, let's dig deeper into what's going on\. We'll start by checking the git blame\. All the code between the two comments belongs to one commit, the rest is from the other\. In total, there are two commits found in the history\. The original one looks like this: ```cpp public CoverageViewAbstractPage( String workspaceName, String storeName, String coverageName, CoverageInfo coverageInfo) throws IOException { .... // grab the coverage view coverageViewInfo = coverageInfo != null ? coverageInfo : catalog.getResourceByStore( store, coverageName, CoverageInfo.class); CoverageView coverageView = coverageViewInfo.getMetadata().get( CoverageView.COVERAGE_VIEW, CoverageView.class); // the type can be still not saved if (coverageViewInfo != null) { coverageInfoId = coverageViewInfo.getId(); } if (coverageView == null) { throw new IllegalArgumentException( "The specified coverage does not have a coverage view attached to it"); } .... } ``` The logic hasn't changed much as the check is done after the code is used, so no logic has been lost from merging different commits\. Moving the check block above wouldn't change anything: the logic would remain the same and _coverageViewInfo\.getMetadata\(\)_ would be executed even if _coverageViewInfo \=\= null_\. Another option would be to throw an _IllegalArgumentException_ if _coverageViewInfo \=\= null_ \(similar to checking if _coverageView_ _\=\=_ _null_\)\. So, are we just replacing one exception with another? Maybe we are\. However, now we can add an explanatory message to it\. ## Forgotten ones didn't return ### I want my collection back Take a look at three methods that share the same issue: ```cpp public TreeSet getTimeDomain() throws IOException { if (!hasTime()) { Collections.emptySet(); } .... return values; } public TreeSet getTimeDomain(DateRange range, int maxEntries) throws IOException { if (!hasTime()) { Collections.emptySet(); } .... return result; } public TreeSet getElevationDomain(NumberRange range, int maxEntries) throws IOException { if (!hasElevation()) { Collections.emptySet(); } .... return result; } ``` I think we've all noticed the strange _emptySet_ call\. Now let's look at the messages issued by the PVS\-Studio analyzer: * [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'emptySet' is required to be utilized\. ReaderDimensionsAccessor\.java 149 * [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'emptySet' is required to be utilized\. ReaderDimensionsAccessor\.java 173 * [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'emptySet' is required to be utilized\. ReaderDimensionsAccessor\.java 324 What should be here? The obvious assumption is that the devs wanted to return an empty _Set_, but they forgot to specify _return_\. Moreover, since the method returns a _TreeSet_ and not a _Set_, we can't use _Collections\.emptySet\(\)_\. We need to use _new TreeSet\(\)_\. ### Created and forgotten ```cpp public CoverageViewEditor( String id, final IModel> inputCoverages, final IModel> bands, IModel envelopeCompositionType, IModel selectedResolution, IModel resolutionReferenceCoverage, List availableCoverages) { .... coveragesChoice.setOutputMarkupId(true); add(coveragesChoice); new ArrayList(); // <= outputBandsChoice = new ListMultipleChoice<>( "outputBandsChoice", new Model<>(), new ArrayList<>(outputBands.getObject()), new ChoiceRenderer() { @Override public Object getDisplayValue(CoverageBand vcb) { return vcb.getDefinition(); } }); outputBandsChoice.setOutputMarkupId(true); add(outputBandsChoice); .... } ``` The constructor is large, but we're interested in _new ArrayList\(\)_: [V6010](https://pvs-studio.com/en/docs/warnings/v6010/) The return value of function 'new ArrayList' is required to be utilized\. CoverageViewEditor\.java 85 Why is it here? I have no idea\. It serves no purpose\. Maybe the developers forgot to assign a variable here, or they just should've deleted that line altogether\. Things happen\. ## Comparing identical Let's take a look at another test\. We don't catch any exceptions here, and it's much simpler overall\. In fact, it's almost too simple\. ```cpp @Test public void testStore() { Properties newProps = dao.toProperties(); // properties equality does not seem to work... Assert.assertEquals(newProps.size(), props.size()); for (Object key : newProps.keySet()) { Object newValue = newProps.get(key); Object oldValue = newProps.get(key); Assert.assertEquals(newValue, oldValue); } ``` \} A good comment\. I don't know if we can help clear up the doubts that have arisen, but we'll try\. The PVS\-Studio analyzer warning: [V6027](https://pvs-studio.com/en/docs/warnings/v6027/) Variables 'newValue', 'oldValue' are initialized through the call to the same function\. It's probably an error or un\-optimized code\. DataAccessRuleDAOTest\.java 110, DataAccessRuleDAOTest\.java 111 We have two _assertEquals_, one of which compares the sizes of _newProps_ and _props_\. The second one compares whether the values are the same or not\. Where do we get the values from? We get them from the same properties\. It seems that what's being tested here's whether _get_ returns the same value\. The first comparison and the intriguing comment suggest that the _props\.get\(key\)_ result should probably be assigned to _oldValue_\. ## Ternary priority ```cpp @Override public void encode(Object o) throws IllegalArgumentException { if (!(o instanceof GetCapabilitiesType)) { throw new IllegalArgumentException( "Not a GetCapabilitiesType: " + o != null ? o.toString() : "null"); } .... } ``` What's wrong with this simple code fragment? It's the ternary operator\! In fact, everything seems good and the idea is clear: to check _o_ for _null_ and return either its string representation or the "_null_" literal\. In reality, _"Not a GetCapabilitiesType: " \+ o_ is checked for _null_\. And the result is always _true_\. The analyzer message: [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression '"Not a GetCapabilitiesType: " \+ o \!\= null' is always true\. WCS20GetCapabilitiesTransformer\.java 183 To fix this, all we need to do is put _o \!\= null ? o\.toString\(\) : "null"_ in parentheses\. ## There's no branching ### Show us the way ```cpp /** Utility method to dump a single component/page to standard output */ public static void print(Component c, boolean dumpClass, boolean dumpValue, boolean dumpPath ) { WicketHierarchyPrinter printer = new WicketHierarchyPrinter(); printer.setPathDumpEnabled(dumpClass); // <= printer.setClassDumpEnabled(dumpClass); printer.setValueDumpEnabled(dumpValue); if (c instanceof Page) { printer.print(c); } else { printer.print(c); } } ``` This immediately raises two questions\. Let's start with the unused method argument\. The PVS\-Studio analyzer warning: [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'dumpPath' is not used inside method body\. WicketHierarchyPrinter\.java 35 From an external perspective, it's extremely difficult to tell whether this is an error, a leftover from the refactoring process, or perhaps nothing has happened at all\. However, we can easily see that the _dumpClass_ argument is passed to _setPathDumpEnabled_ here\. Although, I'd expect _dumpPath_ to be used instead\. Here's another code fragment containing a possible error: [V6004](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. WicketHierarchyPrinter\.java 40, WicketHierarchyPrinter\.java 42 Here, however, it's somewhat difficult to guess what needs to be changed\. ### Null or not Null, it makes no difference Now let's look at the following method: ```cpp public static String getMessage(Component c, Exception e) { if (e instanceof ValidationException) { ValidationException ve = (ValidationException) e; try { if (ve.getParameters() == null) { return new ParamResourceModel(ve.getKey(), c, ve.getParameters()).getString(); } else { return new ParamResourceModel(ve.getKey(), c, ve.getParameters()).getString(); } } catch (Exception ex) { LOGGER.log(Level.FINE, "i18n not found, proceeding with default message", ex); } } // just use the message or the toString instead return e.getMessage() == null ? e.toString() : e.getMessage(); } ``` Identical branching bodies catch the eye, the PVS\-Studio analyzer detects them as well: [V6004](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. GeoServerApplication\.java 116, GeoServerApplication\.java 118 The fact that _ve\.getParameters\(\)_, which has been checked for null, is passed to the constructor in both cases raises some suspicion\. Why did they need the check here? ### No antialiasing or no antialiasing ```cpp public RenderedImageMap produceMap(final WMSMapContent mapContent, final boolean tiled) throws ServiceException { .... if (AA_NONE.equals(antialias)) { // <= potentialPalette = mapContent.getPalette(); } else if (AA_NONE.equals(antialias)) { // <= PaletteExtractor pe = new PaletteExtractor(transparent ? null : bgColor); List layers = mapContent.layers(); for (Layer layer : layers) { pe.visit(layer.getStyle()); if (!pe.canComputePalette()) break; } if (pe.canComputePalette()) potentialPalette = pe.getPalette(); } .... } ``` We've already encountered identical _if_/_else_ bodies, but now we've found identical conditions\. That's where the analyzer warning comes in: [V6003](https://pvs-studio.com/en/docs/warnings/v6003/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. RenderedImageMapOutputFormat\.java 240, RenderedImageMapOutputFormat\.java 242 To understand what's going on here, I suggest looking at what alternatives _AA\_NONE_ has: ```cpp // antialiasing settings, no antialias, only text, full antialias private static final String AA_NONE = "NONE"; private static final String AA_TEXT = "TEXT"; private static final String AA_FULL = "FULL"; ``` We don't have much choice here\. Without digging deeper, my gut tells me that, in _else if_, there should have been a comparison of _antialias_ with _AA\_FULL_\. ## Some clean code Not so long ago, we discussed how an analyzer [can help in writing clean code](https://pvs-studio.com/en/blog/posts/cpp/1115/), using C\+\+ examples\. Now, let's explore a similar case using a Java project as an example\. Let's take a look at two warnings\. ### Unconditional exception Let's start with an exception thrown in a loop unconditionally\. ```cpp @Override public void remove(StyleInfo style) { // ensure no references to the style for (LayerInfo l : facade.getLayers(style)) { throw new IllegalArgumentException( "Unable to delete style referenced by '" + l.getName() + "'" ); } .... } ``` The PVS\-Studio analyzer issues a warning for exactly what I've written: [V6037](https://pvs-studio.com/en/docs/warnings/v6037/) An unconditional 'throw' within a loop\. CatalogImpl\.java 1711 If we think about it a bit, the idea becomes clear: to throw an exception with the layer name in the message\. If _façade\.getLayers\(style\)_ returns an empty list, no exception is thrown\. I suggest replacing it with this: ```cpp facade.getLayers(style).stream().findFirst().ifPresent(layerInfo -> { throw new IllegalArgumentException( "Unable to delete style referenced by '" + layerInfo.getName() + "'" ); }); ``` Of course, the number of code lines hasn't changed, but now we don't have to use _foreach_ in an unusual way\. Also, such an option is quite self\-descriptive\. Which option do you prefer? ### Comparing classes by name ```cpp static { try { NON_FEATURE_TYPE_PROXY = Class.forName("org.geotools.data.complex.config.NonFeatureTypeProxy"); } catch (ClassNotFoundException e) { // might be ok if the app-schema datastore is not around if (StreamSupport.stream( Spliterators.spliteratorUnknownSize( DataStoreFinder.getAllDataStores(), Spliterator.ORDERED), false) .anyMatch(f -> f != null && f.getClass().getSimpleName() .equals("AppSchemaDataAccessFactory"))) { // <= LOGGER.log( Level.FINE, "Could not find NonFeatureTypeProxy yet App-schema" + "is around, probably the class changed name," + "package or does not exist anymore", e); } NON_FEATURE_TYPE_PROXY = null; } } ``` In this static block, we get the [V6054](https://pvs-studio.com/en/docs/warnings/v6054/) warning: [V6054](https://pvs-studio.com/en/docs/warnings/v6054/) Classes should not be compared by their name\. DefaultComplexGeoJsonWriterOptions\.java 41 Could this comparison cause an error? I don't think so\. However, we can simplify the code a bit and use _f\.getClass\(\)\.equals\(AppSchemaDataAccessFactory\.class\)_\. I admit I failed to find this class in the dependencies, although it isn't listed as deprecated in the [documentation](https://docs.geotools.org/stable/javadocs/org/geotools/data/complex/AppSchemaDataAccessFactory.html)\. The necessary dependency might simply be missing\. In any case, one should pay attention to such method calls and utilize best practices whenever possible\. ## Conclusion Manually checking code in large projects is a mission impossible\. Even if you review individual commits yourself, you may still miss typos or lesser\-known errors\. This is where static code analyzers like PVS\-Studio come into play, as they can scan the entire code base for issues\. You can [try it](https://pvs-studio.com/en/pvs-studio/try-free/) for free\!