Skip to content

Commit

Permalink
Fix NPE when trying to search a blank pattern
Browse files Browse the repository at this point in the history 
Issue: #297
  • Loading branch information
@mlopatkin
mlopatkin committed Nov 19, 2022
1 parent 46209db commit 971fad3
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 122 deletions.
18 changes: 6 additions & 12 deletions ...rd/src/main/java/name/mlopatkin/andlogview/search/logrecord/RowSearchStrategyFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@
import name.mlopatkin.andlogview.search.text.SearchStrategyFactory;

import com.google.common.base.CharMatcher;
import com.google.common.base.Strings;

import org.checkerframework.checker.nullness.qual.Nullable;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -53,19 +50,16 @@ private RowSearchStrategyFactory() {}

private static final List<String> PREFIXES = Arrays.asList(PREFIX_APP, PREFIX_MSG, PREFIX_TAG);

public static @Nullable RowSearchStrategy compile(@Nullable String pattern) throws RequestCompilationException {
if (CharMatcher.whitespace().matchesAllOf(Strings.nullToEmpty(pattern))) {
return null;
}
assert pattern != null;
pattern = pattern.trim();
public static RowSearchStrategy compile(String pattern) throws RequestCompilationException {
var trimmedPattern = CharMatcher.whitespace().trimFrom(pattern);

// check for prefix
for (String prefix : PREFIXES) {
if (pattern.startsWith(prefix)) {
return prepareWithPrefix(prefix, pattern.substring(prefix.length()));
if (trimmedPattern.startsWith(prefix)) {
return prepareWithPrefix(prefix, trimmedPattern.substring(prefix.length()));
}
}
return prepareWithoutPrefix(pattern);
return prepareWithoutPrefix(trimmedPattern);
}

private static RowSearchStrategy prepareWithPrefix(String prefix, String rest) throws RequestCompilationException {
Expand Down
188 changes: 82 additions & 106 deletions ...rc/test/java/name/mlopatkin/andlogview/search/logrecord/RowSearchStrategyFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,18 @@

package name.mlopatkin.andlogview.search.logrecord;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;

import name.mlopatkin.andlogview.logmodel.LogRecord;
import name.mlopatkin.andlogview.logmodel.LogRecordUtils;
import name.mlopatkin.andlogview.search.RequestCompilationException;

import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;

public class RowSearchStrategyFactoryTest {
static final String TAG_CONTACTS = "contacts";
Expand All @@ -47,161 +47,137 @@ private static LogRecord makeRecord(String tag, String app, String msg) {
return LogRecordUtils.forMessage(msg).withTag(tag).withAppName(app);
}

@Before
public void setUp() throws Exception {
@BeforeEach
void setUp() throws Exception {
tagContacts = makeRecord(TAG_CONTACTS, APP_NAME_SYSTEM_SERVER, MSG_SYSTEM_SERVER);
appnameContacts = makeRecord(TAG_SYSTEM, APP_CONTACTS, MSG_SYSTEM_SERVER);
msgContacts = makeRecord(TAG_SYSTEM, APP_NAME_SYSTEM_SERVER, MSG_CONTACTS);
system = makeRecord(TAG_SYSTEM, APP_NAME_SYSTEM_SERVER, MSG_SYSTEM_SERVER);
}

@Test
public void testCompile_Simple() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("contacts");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertTrue(simple.test(tagContacts));
assertFalse(simple.test(system));
public void simplePatternMatchesAllFields() throws Exception {
var strategy = RowSearchStrategyFactory.compile("contacts");

assertThat(strategy)
.accepts(msgContacts, appnameContacts, tagContacts)
.rejects(system);
}

@Test
public void testCompile_Regex() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("/contacts/");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertTrue(simple.test(tagContacts));
assertFalse(simple.test(system));
public void plainRegexPatternMatchesAllFields() throws Exception {
var strategy = RowSearchStrategyFactory.compile("/contacts/");

assertThat(strategy)
.accepts(msgContacts, appnameContacts, tagContacts)
.rejects(system);

}

@Test
public void testCompile_ComplexRegex() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("/con.*ts/");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertTrue(simple.test(tagContacts));
assertFalse(simple.test(system));
public void trickyRegex() throws Exception {
var strategy = RowSearchStrategyFactory.compile("/con.*ts/");

assertThat(strategy)
.accepts(msgContacts, appnameContacts, tagContacts)
.rejects(system);
}

@Test
public void testCompile_AppNameSimple() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("app:contacts");
var strategy = RowSearchStrategyFactory.compile("app:contacts");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(appnameContacts)
.rejects(msgContacts, tagContacts, system);
}

@Test
public void testCompile_TagSimple() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("tag:contacts");
var strategy = RowSearchStrategyFactory.compile("tag:contacts");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertFalse(simple.test(appnameContacts));
assertTrue(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(tagContacts)
.rejects(msgContacts, appnameContacts, system);
}

@Test
public void testCompile_MsgSimple() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("msg:contacts");
var strategy = RowSearchStrategyFactory.compile("msg:contacts");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertFalse(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(msgContacts)
.rejects(appnameContacts, tagContacts, system);
}

@Test
public void testCompile_AppNameRegex() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("app:/c.ntacts/");
var strategy = RowSearchStrategyFactory.compile("app:/c.ntacts/");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(appnameContacts)
.rejects(msgContacts, tagContacts, system);
}

@Test
public void testCompile_TagRegex() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("tag:/c.ntacts/");
var strategy = RowSearchStrategyFactory.compile("tag:/c.ntacts/");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertFalse(simple.test(appnameContacts));
assertTrue(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(tagContacts)
.rejects(msgContacts, appnameContacts, system);
}

@Test
public void testCompile_MsgRegex() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("msg:/c.ntacts/");
var strategy = RowSearchStrategyFactory.compile("msg:/c.ntacts/");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertFalse(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
}

@Test
@Ignore("Not supported yet")
public void testCompile_Several() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile("msg:/c.ntacts/ app:contacts");

assertNotNull(simple);
assertTrue(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(msgContacts)
.rejects(appnameContacts, tagContacts, system);
}

@Test
@Ignore("No escaping, just treat this as a usual string")
@Disabled("No escaping, just treat this as a usual string")
public void testCompile_Escape() throws Exception {
LogRecord withEscaped = makeRecord(TAG_CONTACTS, APP_CONTACTS, "This contains a escaped app:contacts");

RowSearchStrategy simple = RowSearchStrategyFactory.compile("app\\:contacts");
var strategy = RowSearchStrategyFactory.compile("app\\:contacts");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertFalse(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertTrue(simple.test(withEscaped));
}

@Test(expected = RequestCompilationException.class)
@Ignore("No escaping, just treat this as a usual string")
public void testCompile_InvalidPrefix() throws Exception {
RowSearchStrategyFactory.compile("foobar:contacts");
assertThat(strategy)
.accepts(withEscaped)
.rejects(appnameContacts, msgContacts, tagContacts, system);
}

@Test
public void testCompile_Spaces() throws Exception {
RowSearchStrategy simple = RowSearchStrategyFactory.compile(" \t app:/c.ntacts/ ");
public void testCompile_InvalidPrefix() throws Exception {
var strategy = RowSearchStrategyFactory.compile("foobar:contacts");

assertNotNull(simple);
assertFalse(simple.test(msgContacts));
assertTrue(simple.test(appnameContacts));
assertFalse(simple.test(tagContacts));
assertFalse(simple.test(system));
assertThat(strategy)
.accepts(LogRecordUtils.forMessage("Some foobar:contacts"))
.rejects(LogRecordUtils.forMessage("Some contacts"));
}

@Test
public void testCompile_Empty() throws Exception {
assertNull(RowSearchStrategyFactory.compile(""));
assertNull(RowSearchStrategyFactory.compile(" "));
assertNull(RowSearchStrategyFactory.compile(null));
public void testCompile_Spaces() throws Exception {
var strategy = RowSearchStrategyFactory.compile(" \t app:/c.ntacts/ ");

assertThat(strategy)
.accepts(appnameContacts)
.rejects(msgContacts, tagContacts, system);
}

@ParameterizedTest
@ValueSource(strings = {
"",
" ",
" app: ",
"\t\t\t\n",
"app:",
" "
})
public void testCompile_Empty(String invalidPattern) throws Exception {
assertThatExceptionOfType(RequestCompilationException.class)
.isThrownBy(() -> RowSearchStrategyFactory.compile(invalidPattern));
}
}
2 changes: 1 addition & 1 deletion src/name/mlopatkin/andlogview/ui/search/SearchPresenter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ private void tryStartSearchWithPrompt(String patternText) {
find(Search.Direction.FORWARD.alsoSearchCurrent());
} catch (RequestCompilationException e) {
searchPromptView.showPatternError(
String.format("%s isn't a valid search expression: %s", patternText, e.getMessage()));
String.format("'%s' isn't a valid search expression. %s", patternText, e.getMessage()));
}
}

Expand Down
4 changes: 1 addition & 3 deletions src/name/mlopatkin/andlogview/ui/search/logtable/LogRecordSearchPatternCompiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
import name.mlopatkin.andlogview.search.logrecord.RowSearchStrategyFactory;
import name.mlopatkin.andlogview.ui.search.SearchPatternCompiler;

import java.util.Objects;

import javax.inject.Inject;

public class LogRecordSearchPatternCompiler implements SearchPatternCompiler<RowSearchStrategy> {
Expand All @@ -32,6 +30,6 @@ public LogRecordSearchPatternCompiler() {}

@Override
public RowSearchStrategy compile(String patternText) throws RequestCompilationException {
return Objects.requireNonNull(RowSearchStrategyFactory.compile(patternText));
return RowSearchStrategyFactory.compile(patternText);
}
}

0 comments on commit 971fad3

Please sign in to comment.