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.
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, OSGeo, or GeoSolutions websites. However, in order to understand what we're dealing with, let's take a look at the interface of the server access panel:
On the first start, we get some initial data, so we can even visualize it on the map:
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), 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.
private GridSampleDimension[] getCoverageSampleDimensions(
GridCoverage2DReader reader, Map<String, Serializable> 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:
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 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 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.
Some analyzer messages describe the issue in detail, with no further explanations needed. For example, take a look at the following code fragment:
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 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
@Override
public <T extends CatalogInfo> CloseableIterator<T> list(
final Class<T> 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 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().
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 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 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 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.
@Test
public void testChallenge() throws Exception {
Map<String, Object> 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 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.
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 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:
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.
Take a look at three methods that share the same issue:
public TreeSet<Object> getTimeDomain() throws IOException {
if (!hasTime()) {
Collections.emptySet();
}
....
return values;
}
public TreeSet<Object> getTimeDomain(DateRange range, int maxEntries)
throws IOException {
if (!hasTime()) {
Collections.emptySet();
}
....
return result;
}
public TreeSet<Object> 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:
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<Object>().
public CoverageViewEditor(
String id,
final IModel<List<String>> inputCoverages,
final IModel<List<CoverageBand>> bands,
IModel<EnvelopeCompositionType> envelopeCompositionType,
IModel<SelectedResolution> selectedResolution,
IModel<String> resolutionReferenceCoverage,
List<String> availableCoverages) {
....
coveragesChoice.setOutputMarkupId(true);
add(coveragesChoice);
new ArrayList<CoverageBand>(); // <=
outputBandsChoice =
new ListMultipleChoice<>(
"outputBandsChoice",
new Model<>(),
new ArrayList<>(outputBands.getObject()),
new ChoiceRenderer<CoverageBand>() {
@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<CoverageBand>():
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.
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.
@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 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.
@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 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.
/** 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 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 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.
Now let's look at the following method:
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 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?
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<Layer> 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 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:
// 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.
Not so long ago, we discussed how an analyzer can help in writing clean code, 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.
Let's start with an exception thrown in a loop unconditionally.
@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 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:
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?
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 warning:
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. The necessary dependency might simply be missing. In any case, one should pay attention to such method calls and utilize best practices whenever possible.
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 for free!