Точные и подробные карты невероятно выручают туристов, будь это планирование путешествия или попытка не потеряться в незнакомом месте. Помочь в обслуживании таких данных на серверной стороне может GeoServer. Посмотрим же, что таится внутри этого проекта.
Уж не знаю, как так получилось, что сразу после написания статьи про GeoGebra я наткнулся на GeoServer. Видимо, этот префикс оставлять меня пока не хочет. Однако в отличие от предыдущего проекта семейства Geo (эти проекты никак не связаны) здесь речь пойдёт не о Geoметрии, а о Geoграфии.
Итак, что же такое GeoServer? Это сервер! Сервер, конечно, не простой, а такой, что обеспечивает данные и карты для клиентов вроде веб-браузеров или геоинформационных систем. Люди заинтересованные могут прочитать подробное описание и все его возможности на сайтах GeoServer, OSGeo или GeoSolutions. Но для того, чтобы понять, с чем имеем дело, предлагаю посмотреть интерфейс панели доступа сервера:
Уже на первом запуске предоставляются некоторые стартовые данные, так что можно даже визуально оценить их на карте:
Мы получаем в формате OpenLayers интерактивную карту мира и, кликнув по какому-либо месту, можем получить подробности.
Возможности сервера обрисовали. Будь время, я бы поигрался и попробовал на открытых данных (например, OpenStreetMap) собрать какую-нибудь интересную карту локального места, но пока остановлюсь на этом.
А теперь предлагаю заглянуть внутрь такого великолепного сервиса. И не просто посмотреть, а попробовать отыскать ошибки с использованием статического анализатора PVS-Studio. Разворачиваем карты кода и начинаем путешествие.
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());
}
....
}Создав две крайне похожие переменные cm и sm, легко перепутать их в будущем, а также не заметить проблемы на code review. Нетрудно понять, что здесь изначально планировали проверить sm на null, а не проверять уже проверенное. Исправляем опечатку и двигаемся дальше. На ошибку здесь указывают аж два срабатывания:
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");
....
}Здесь мы наблюдаем необычную проверку. Проверять request.getSldVer() одновременно на эквивалентность пустой строке и неэквивалентность 1.0.0 не имеет смысла, о чём и предупреждает анализатор:
V6057 Consider inspecting this expression. The expression is excessive or contains a misprint. GetStyles.java 40
Что же здесь произошло, и что должно быть?
Сообщение, которое мы должны получить в случае успешной проверки, сообщает нам о неподдерживаемой версии SLD. Я решил посмотреть, что есть SLD, и какие версии поддерживает GeoServer. На то и существует документация. Прямо из неё узнаём:
The OGC Styled Layer Descriptor (SLD) standard defines a language for expressing styling of geospatial data. GeoServer uses SLD as its primary styling language.
Вдаваться в подробности нет необходимости, но всё же неплохо бы уточнить что-то про версии этого стандарта. Дело нетрудное, смотрим немного ниже и находим:
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.
Понимаем, что поддерживаются версии 1.0.0 и 1.1.0. В нашем фрагменте кода мы действительно находим проверку на несовпадение версии 1.0.0, только вот необходимость эквивалентности пустой строке её ломает. Если в запросе будет нужная нам версия, то всё прекрасно. Но даже если версия будет, скажем, 3.5.2, то исключение всё равно не получим.
Я вижу два варианта: либо проверку на пустую строку необходимо удалить в принципе, либо на её месте должна быть проверка !"1.1.0".equals(request.getSldVersion()), поскольку, согласно документации, эта версия стандарта частично поддерживается.
Некоторые сообщения анализатора самостоятельно описывают проблему. Например, предлагаю посмотреть следующий фрагмент кода:
int width = w instanceof Integer ? ((Integer)w) : Integer.parseInt((String)w);
int height = w instanceof Integer ? ((Integer)h) : Integer.parseInt((String)h);И, собственно, сообщение:
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
Одна и та же проверка происходит дважды, причём никаких изменений по пути от одной к другой не происходит. Очевидно, что значение второй всегда будет true. Возможно, она просто лишняя?
Подозреваю, что проверить на null хотели элементы sortOrder, следовательно, должно быть so != null. Ведь такая проверка защитила бы от потенциального NullPointerException при использовании 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;
}
....
}Что здесь подозрительного? Проверка pd == null. Предлагаю посмотреть предупреждение анализатора PVS-Studio и начать расследование:
V6060 The 'pd' reference was utilized before it was verified against null. DataPanel.java 117 , DataPanel.java 118
Согласитесь, странно проверять переменную pd уже после её утилизации в виде pd.getType(). Будь она null, наши руки уже были бы заняты пойманным NullPointerException. Здесь у меня возникла некоторая гипотеза.
Предлагаю посмотреть на метод isAssignableFrom. Залезем в Javadocs, но я не стану приводить его целиком, ибо нас интересует лишь конкретная часть:
@throws NullPointerException if the specified Class parameter is null
Итак, у нас есть первая зацепка: binding не должен быть null. Далее предлагаю посмотреть, может ли pd.getType().getBinding() вернуть null. Сам метод отсылает нас к интерфейсу PropertyType. Здесь мы уже находимся в зависимости, которая называется geotools. И, к счастью, вновь на помощь приходят Javadocs. Читаем про getBindings и получаем:
This value is never null.
Что ж, от своей гипотезы отказываюсь. Какая была гипотеза? Что проверить хотели binding.
А вот нужна ли проверка у pd? fs.getSchema() в нашем случае возвращает ComplexType. Находим getDescriptor, вновь читаем и узнаём:
This method returns null if no such property is found.
Прекрасно. В получении какого-либо значения в pd действительно необходимо убедиться, однако эта проверка осуществляется слишком поздно.
@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"));
}
}
}Обнаруживаем себя в тесте. Здесь происходит проверка на получение SpringSecurityException. Однако PVS-Studio находит аномалию в проверке e == null:
V6060 The 'e' reference was utilized before it was verified against null. ResourceAccessManagerWCSTest.java 186, ResourceAccessManagerWCSTest.java 193
Действительно, зачем переменную проверять на null, если её уже использовали? К великому счастью, код здесь обладает хорошей концентрацией комментариев и пояснительным сообщением в fail. Ожидается, что мы получим исключение безопасности, а оно записывается в переменную se. Эту самую переменную на null и необходимо проверить. Кстати, поскольку e == null никогда не вернёт true, то этот тест спокойно проходит.
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
Встретили ещё одну сомнительную проверку. Наличие здесь проверки на null вызывает вопросы. Если ожидается, что coverageViewInfo может быть null, то coverageViewInfo.getMetadata() может выбросить NullPointerException. Не похоже на опечатку, так что необходимо попробовать разобраться, что происходит. Начнём с обращения к git blame.
Весь код, что находится между двумя комментариями, относится к одному коммиту, остальное — к другому. Всего в истории обнаружены два коммита. Оригинальный выглядит так:
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");
}
....
}В логике ничего не изменилось — проверка проводится после использования, так что потери логики из-за слияния различных коммитов не произошло.
Передвижение блока с проверкой выше не имеет смысла: логика останется прежней и coverageViewInfo.getMetadata() исполнится, даже если coverageViewInfo == null. Другой вариант — банально бросать IllegalArgumentException, если coverageViewInfo == null (подобно проверке coverageView == null). Выходит, мы просто заменим одно исключение другим? Возможно. Однако теперь к нему можно добавить пояснительное сообщение.
Вашему вниманию представляются сразу три метода, содержащие одну и ту же проблему:
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;
}Думаю, странный вызов emptySet заметили все. Теперь же посмотрим непосредственно на сообщения анализатора PVS-Studio:
Что же здесь должно быть? Очевидная мысль: хотели вернуть пустой Set, вот только return не указали. К слову, поскольку метод возвращает конкретно TreeSet, а не Set, то воспользоваться Collections.emptySet() не получится, и необходимо использовать 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);
....
}Конструктор большой, но нас интересует new ArrayList<CoverageBand>():
V6010 The return value of function 'new ArrayList' is required to be utilized. CoverageViewEditor.java 85
Зачем оно здесь? Не знаю. Никакого эффекта не вызывает. Возможно, было забыто присвоение переменной. А возможно, эту строку необходимо просто удалить. Всякое случается.
Взглянем на другой тест. Здесь никаких исключений не ловим, и в целом всё заметно проще. Даже слишком.
@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);
}}
Хороший комментарий. Не знаю, поможем ли мы пролить свет на возникшее сомнение, но всё же попробуем. Предупреждение анализатора PVS-Studio:
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
Находим два assertEquals, один из которых сравнивает размеры newProps и props. Второй же сравнивает, являются ли значения одинаковыми. Откуда берём значения? Из одних и тех же свойств. Можно предположить, что здесь тестируется то, что get возвращает одно и то же значение. Наличие первого сравнения и интересный комментарий намекают, что, скорее всего, oldValue следует присваивать результат props.get(key).
@Override
public void encode(Object o) throws IllegalArgumentException {
if (!(o instanceof GetCapabilitiesType)) {
throw new IllegalArgumentException(
"Not a GetCapabilitiesType: " + o != null ? o.toString() : "null");
}
....
}Что же не так в этом простом фрагменте? Тернарный оператор! На самом деле, всё выглядит хорошо и идея ясна: проверяем o на null и возвращаем либо его строковое представление, либо литерал "null". В реальности же на null будет проверяться "Not a GetCapabilitiesType: " + o. А результат всегда будет true. Сообщение анализатора:
V6007 Expression '"Not a GetCapabilitiesType: " + o != null' is always true. WCS20GetCapabilitiesTransformer.java 183
Для исправления достаточно включить o != null ? o.toString() : "null" в скобки.
/** 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);
}
}Здесь возникают сразу два вопроса. Предлагаю начать с неиспользуемого аргумента метода. Предупреждение анализатора PVS-Studio:
V6022 Parameter 'dumpPath' is not used inside method body. WicketHierarchyPrinter.java 35
Обычно для стороннего наблюдателя крайне трудно определить, является ли это ошибкой или остатками рефакторинга, а может ничего криминального и не случилось. Однако здесь можно легко обнаружить передачу в setPathDumpEnabled аргумента dumpClass. Хотя я бы ожидал там dumpPath.
Другое место с возможной ошибкой:
V6004 The 'then' statement is equivalent to the 'else' statement. WicketHierarchyPrinter.java 40, WicketHierarchyPrinter.java 42
Здесь же предположить, какие изменения необходимо внести, оказалось несколько затруднительно.
Теперь предлагаю посмотреть на следующий метод:
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();
}В глаза бросаются идентичные тела ветвлений, анализатор PVS-Studio это также обнаруживает:
V6004 The 'then' statement is equivalent to the 'else' statement. GeoServerApplication.java 116, GeoServerApplication.java 118
Масла в огонь подозрений подливает то, что проверяемый на null результат ve.getParameters() передаётся в конструктор в обоих случаях. Зачем тогда осуществлялась проверка?
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();
}
....
}Мы сталкивались с одинаковыми телами в if/else, но теперь нашли одинаковые условия. На это и ловим предупреждение анализатора:
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
Чтобы разобраться в происходящем, предлагаю посмотреть, какие есть альтернативы у AA_NONE:
// 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";Выбора немного, и без углубления чутьё подсказывает, что в else if предполагалось сравнение antialias с AA_FULL.
Не так давно мы писали о том, как анализатор подталкивает писать чистый код. Тогда это рассматривали на примере C++, но теперь мы можем посмотреть аналогичное на примере Java-проекта. Предлагаю к рассмотрению два срабатывания.
Начнём с выброшенного исключения в цикле без условий.
@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() + "'"
);
}
....
}Ровно на то, что я написал, и ругается анализатор PVS-Studio:
V6037 An unconditional 'throw' within a loop. CatalogImpl.java 1711
Если немного подумать, то идея понятна — выбросить исключение с указанием имени слоя в сообщении. Если façade.getLayers(style) вернёт пустой список, то исключение не будет выброшено.
Я предлагаю заменить это на:
facade.getLayers(style).stream().findFirst().ifPresent(layerInfo -> {
throw new IllegalArgumentException(
"Unable to delete style referenced by '" + layerInfo.getName() + "'"
);
});Конечно, количество строк кода не изменилось, однако теперь не придётся прибегать к необычному использованию foreach. А также такой вариант вполне сам себя описывает.
Какой вариант вам нравится больше?
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;
}
}Вот в таком статическом блоке мы получаем срабатывание диагностики V6054:
V6054 Classes should not be compared by their name. DefaultComplexGeoJsonWriterOptions.java 41
Возможна ли здесь ошибка из-за такого сравнения? Вряд ли. Однако можно слегка упростить код и использовать f.getClass().equals(AppSchemaDataAccessFactory.class). Признаюсь, я не смог найти этот класс в зависимостях, хотя в документации он не указан как deprecated. Возможно, что необходимой зависимости просто нет. В любом случае на подобные вызовы методов следует обращать внимание и по возможности использовать лучшие практики.
Проверять код в больших проектах вручную — миссия невыполнимая. Кроме того, даже проверив глазами отдельные коммиты, можно спокойно пропустить опечатки или малоизвестные ошибки. Именно в таких случаях на помощь приходят статические анализаторы кода вроде PVS-Studio, который проверяет весь исходный код. А попробовать его можно совершенно бесплатно!