From 0989ea6f736ab728954f389e7c0761ffa4c14838 Mon Sep 17 00:00:00 2001 From: Nidhi Date: Tue, 8 Dec 2020 16:22:05 +0530 Subject: [PATCH 1/5] Fixing SQLI Vulnerability --- .../postgres/PostgresDocStoreTest.java | 21 +++ .../core/documentstore/postgres/Params.java | 151 ++++++++++++++++++ .../postgres/PostgresCollection.java | 66 +++++--- .../postgres/PostgresCollectionTest.java | 124 +++++++------- 4 files changed, 282 insertions(+), 80 deletions(-) create mode 100644 document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java diff --git a/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java b/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java index 7e8c888b..ffc0ea6d 100644 --- a/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java +++ b/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java @@ -258,6 +258,27 @@ public void testIgnoreCaseLikeQuery() throws IOException { } } + @Test + public void testInQuery() throws IOException { + Collection collection = datastore.getCollection(COLLECTION_NAME); + collection.upsert(new SingleValueKey("default", "testKey1"), createDocument("name", "Bob")); + collection.upsert(new SingleValueKey("default", "testKey2"), createDocument("name", "Alice")); + collection.upsert(new SingleValueKey("default", "testKey3"), createDocument("name", "Halo")); + + List inArray = new ArrayList<>(); + inArray.add("Bob"); + inArray.add("Alice"); + + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.IN, "name", inArray)); + Iterator results = collection.search(query); + List documents = new ArrayList<>(); + for (; results.hasNext(); ) { + documents.add(results.next()); + } + Assertions.assertEquals(documents.size(), 2); + } + @Test public void testSearch() throws IOException { Collection collection = datastore.getCollection(COLLECTION_NAME); diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java new file mode 100644 index 00000000..058529f1 --- /dev/null +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java @@ -0,0 +1,151 @@ +package org.hypertrace.core.documentstore.postgres; + +import java.util.HashMap; +import java.util.Map; + +/** + * Holds the params that need to be set in the PreparedStatement for constructing the final PQL + * query + */ +public class Params { + + // Map of index to the corresponding param value + private final Map integerParams; + private final Map longParams; + private final Map stringParams; + private final Map floatParams; + private final Map doubleParams; + + private Params( + Map integerParams, + Map longParams, + Map stringParams, + Map floatParams, + Map doubleParams) { + this.integerParams = integerParams; + this.longParams = longParams; + this.stringParams = stringParams; + this.floatParams = floatParams; + this.doubleParams = doubleParams; + } + + public Map getIntegerParams() { + return integerParams; + } + + public Map getLongParams() { + return longParams; + } + + public Map getStringParams() { + return stringParams; + } + + public Map getFloatParams() { + return floatParams; + } + + public Map getDoubleParams() { + return doubleParams; + } + + @Override + public String toString() { + return "Params{" + + "integerParams=" + integerParams + + ", longParams=" + longParams + + ", stringParams=" + stringParams + + ", floatParams=" + floatParams + + ", doubleParams=" + doubleParams + + '}'; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + Params params = (Params) o; + + if (!integerParams.equals(params.integerParams)) { + return false; + } + if (!longParams.equals(params.longParams)) { + return false; + } + if (!stringParams.equals(params.stringParams)) { + return false; + } + if (!floatParams.equals(params.floatParams)) { + return false; + } + return doubleParams.equals(params.doubleParams); + } + + @Override + public int hashCode() { + int result = integerParams.hashCode(); + result = 31 * result + longParams.hashCode(); + result = 31 * result + stringParams.hashCode(); + result = 31 * result + floatParams.hashCode(); + result = 31 * result + doubleParams.hashCode(); + return result; + } + + public static Builder newBuilder() { + return new Builder(); + } + + public static class Builder { + + private int nextIndex; + private final Map integerParams; + private final Map longParams; + private final Map stringParams; + private final Map floatParams; + private final Map doubleParams; + + private Builder() { + nextIndex = 0; + integerParams = new HashMap<>(); + longParams = new HashMap<>(); + stringParams = new HashMap<>(); + floatParams = new HashMap<>(); + doubleParams = new HashMap<>(); + } + + public Builder addIntegerParam(int paramValue) { + integerParams.put(nextIndex++, paramValue); + return this; + } + + public Builder addLongParam(long paramValue) { + longParams.put(nextIndex++, paramValue); + return this; + } + + public Builder addStringParam(String paramValue) { + stringParams.put(nextIndex++, paramValue); + return this; + } + + public Builder addFloatParam(float paramValue) { + floatParams.put(nextIndex++, paramValue); + return this; + } + + public Builder addDoubleParam(double paramValue) { + doubleParams.put(nextIndex++, paramValue); + return this; + } + + public Params build() { + return new Params(integerParams, longParams, stringParams, floatParams, doubleParams); + } + } +} + diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java index a6b7b4f6..6b6577f9 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java @@ -48,6 +48,8 @@ public class PostgresCollection implements Collection { }}; private static final ObjectMapper MAPPER = new ObjectMapper(); private static final String DOC_PATH_SEPARATOR = "\\."; + private static String QUESTION_MARK = "?"; + private Connection client; private String collectionName; @@ -149,13 +151,13 @@ public boolean updateSubDoc(Key key, String subDocPath, Document subDocument) { @Override public Iterator search(Query query) { String filters = null; - String space = " "; - StringBuilder searchSQLBuilder = new StringBuilder("SELECT * FROM") - .append(space).append(collectionName); + StringBuilder sqlBuilder = new StringBuilder("SELECT * FROM ") + .append(collectionName); + Params.Builder paramsBuilder = Params.newBuilder(); // If there is a filter in the query, parse it fully. if (query.getFilter() != null) { - filters = parseQuery(query.getFilter()); + filters = parseFilter(query.getFilter(), paramsBuilder); } LOGGER.debug( @@ -164,27 +166,27 @@ public Iterator search(Query query) { filters); if (filters != null) { - searchSQLBuilder + sqlBuilder .append(" WHERE ").append(filters); } if (!query.getOrderBys().isEmpty()) { String orderBySQL = parseOrderByQuery(query.getOrderBys()); - searchSQLBuilder.append(" ORDER BY ").append(orderBySQL); + sqlBuilder.append(" ORDER BY ").append(orderBySQL); } Integer limit = query.getLimit(); if (limit != null && limit >= 0) { - searchSQLBuilder.append(" LIMIT ").append(limit); + sqlBuilder.append(" LIMIT ").append(limit); } Integer offset = query.getOffset(); if (offset != null && offset >= 0) { - searchSQLBuilder.append(" OFFSET ").append(offset); + sqlBuilder.append(" OFFSET ").append(offset); } try { - PreparedStatement preparedStatement = client.prepareStatement(searchSQLBuilder.toString()); + PreparedStatement preparedStatement = buildPreparedStatement(sqlBuilder.toString(), paramsBuilder.build()); ResultSet resultSet = preparedStatement.executeQuery(); return new PostgresResultIterator(resultSet); } catch (SQLException e) { @@ -195,16 +197,16 @@ public Iterator search(Query query) { } @VisibleForTesting - protected String parseQuery(Filter filter) { + protected String parseFilter(Filter filter, Params.Builder paramsBuilder) { if (filter.isComposite()) { - return parseQueryForCompositeFilter(filter); + return parseCompositeFilter(filter, paramsBuilder); } else { - return parseQueryForNonCompositeFilter(filter); + return parseNonCompositeFilter(filter, paramsBuilder); } } @VisibleForTesting - protected String parseQueryForNonCompositeFilter(Filter filter) { + protected String parseNonCompositeFilter(Filter filter, Params.Builder paramsBuilder) { Filter.Op op = filter.getOp(); Object value = filter.getValue(); String fieldName = filter.getFieldName(); @@ -236,7 +238,10 @@ protected String parseQueryForNonCompositeFilter(Filter filter) { List values = (List) value; String collect = values .stream() - .map(val -> "'" + val + "'") + .map(val -> { + paramsBuilder.addStringParam(String.valueOf(val)); + return QUESTION_MARK; + }) .collect(Collectors.joining(", ")); return filterString.append("(" + collect + ")").toString(); case CONTAINS: @@ -246,22 +251,23 @@ protected String parseQueryForNonCompositeFilter(Filter filter) { case NOT_EXISTS: // TODO: Checks if key does not exist case NEQ: - throw new UnsupportedOperationException("Only Equality predicate is supported"); default: throw new UnsupportedOperationException( String.format("Query operation:%s not supported", op)); } - return filterString.append("'").append(value).append("'").toString(); + String filters = filterString.append(QUESTION_MARK).toString(); + paramsBuilder.addStringParam(String.valueOf(value)); + return filters; } @VisibleForTesting - protected String parseQueryForCompositeFilter(Filter filter) { + protected String parseCompositeFilter(Filter filter, Params.Builder paramsBuilder) { Filter.Op op = filter.getOp(); switch (op) { case OR: { String childList = Arrays.stream(filter.getChildFilters()) - .map(this::parseQuery) + .map(childFilter -> parseFilter(childFilter, paramsBuilder)) .filter(str -> !StringUtils.isEmpty(str)) .map(str -> "(" + str + ")") .collect(Collectors.joining(" OR ")); @@ -270,7 +276,7 @@ protected String parseQueryForCompositeFilter(Filter filter) { case AND: { String childList = Arrays.stream(filter.getChildFilters()) - .map(this::parseQuery) + .map(childFilter -> parseFilter(childFilter, paramsBuilder)) .filter(str -> !StringUtils.isEmpty(str)) .map(str -> "(" + str + ")") .collect(Collectors.joining(" AND ")); @@ -278,10 +284,24 @@ protected String parseQueryForCompositeFilter(Filter filter) { } default: throw new UnsupportedOperationException( - String.format("Boolean operation:%s not supported", op)); + String.format("Query operation:%s not supported", op)); } } + @VisibleForTesting + protected PreparedStatement buildPreparedStatement(String sqlQuery, Params params) throws SQLException { + PreparedStatement preparedStatement = client.prepareStatement(sqlQuery); + params.getStringParams().forEach((k, v) -> { + try { + // Postgres Index starts from 1 + preparedStatement.setString(k+1, v); + } catch (SQLException e) { + LOGGER.error("SQLException querying documents. query: {}", sqlQuery, e); + } + }); + return preparedStatement; + } + @VisibleForTesting private String getJsonSubDocPath(String subDocPath) { return "{" + subDocPath.replaceAll(DOC_PATH_SEPARATOR, ",") + "}"; @@ -388,17 +408,19 @@ public long count() { public long total(Query query) { StringBuilder totalSQLBuilder = new StringBuilder("SELECT COUNT(*) FROM ") .append(collectionName); + Params.Builder paramsBuilder = Params.newBuilder(); + long count = -1; // on any in-correct filter input, it will return total without filtering if (query.getFilter() != null) { - String parsedQuery = parseQuery(query.getFilter()); + String parsedQuery = parseFilter(query.getFilter(), paramsBuilder); if (parsedQuery != null) { totalSQLBuilder.append(" WHERE ").append(parsedQuery); } } try { - PreparedStatement preparedStatement = client.prepareStatement(totalSQLBuilder.toString()); + PreparedStatement preparedStatement = buildPreparedStatement(totalSQLBuilder.toString(), paramsBuilder.build()); ResultSet resultSet = preparedStatement.executeQuery(); while (resultSet.next()) { count = resultSet.getLong(1); diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/PostgresCollectionTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/PostgresCollectionTest.java index 50e618d6..6de2eb55 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/PostgresCollectionTest.java +++ b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/PostgresCollectionTest.java @@ -25,132 +25,136 @@ public void setUp() { } @Test - public void testParseQueryForNonCompositeFilter() { + public void testParseNonCompositeFilter() { { Filter filter = new Filter(Filter.Op.EQ, ID, "val1"); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " = 'val1'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " = ?", query); } { Filter filter = new Filter(Filter.Op.GT, ID, 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " > '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " > ?", query); } { Filter filter = new Filter(Filter.Op.GTE, ID, 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " >= '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " >= ?", query); } { Filter filter = new Filter(Filter.Op.LT, ID, 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " < '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " < ?", query); } { Filter filter = new Filter(Filter.Op.LTE, ID, 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " <= '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " <= ?", query); } { Filter filter = new Filter(Filter.Op.LIKE, ID, "abc"); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " ILIKE '%abc%'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " ILIKE ?", query); } { Filter filter = new Filter(Filter.Op.IN, ID, List.of("abc", "xyz")); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals(ID + " IN ('abc', 'xyz')", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals(ID + " IN (?, ?)", query); } } @Test - public void testParseQueryForNonCompositeFilterForJsonField() { + public void testParseNonCompositeFilterForJsonField() { { Filter filter = new Filter(Filter.Op.EQ, "key1", "val1"); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' = 'val1'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' = ?", query); } { Filter filter = new Filter(Filter.Op.GT, "key1", 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' > '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' > ?", query); } { Filter filter = new Filter(Filter.Op.GTE, "key1", 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' >= '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' >= ?", query); } { Filter filter = new Filter(Filter.Op.LT, "key1", 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' < '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' < ?", query); } { Filter filter = new Filter(Filter.Op.LTE, "key1", 5); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' <= '5'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' <= ?", query); } { Filter filter = new Filter(Filter.Op.LIKE, "key1", "abc"); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' ILIKE '%abc%'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' ILIKE ?", query); } { Filter filter = new Filter(Filter.Op.IN, "key1", List.of("abc", "xyz")); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'key1' IN ('abc', 'xyz')", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'key1' IN (?, ?)", query); } { Filter filter = new Filter(Filter.Op.EQ, DOCUMENT_ID, "k1:k2"); - String query = collection.parseQueryForNonCompositeFilter(filter); - Assertions.assertEquals("document->>'_id' = 'k1:k2'", query); + String query = collection.parseNonCompositeFilter(filter, initParams()); + Assertions.assertEquals("document->>'_id' = ?", query); } } @Test - public void testParseQueryForNonCompositeFilterUnsupportedException() { - String expectedMessage = "Only Equality predicate is supported"; + public void testNonCompositeFilterUnsupportedException() { + String expectedMessage = "Query operation:%s not supported"; { Filter filter = new Filter(Filter.Op.EXISTS, "key1", null); + String expected = String.format(expectedMessage, Filter.Op.EXISTS); Exception exception = assertThrows(UnsupportedOperationException.class, - () -> collection.parseQueryForNonCompositeFilter(filter)); + () -> collection.parseNonCompositeFilter(filter, initParams())); String actualMessage = exception.getMessage(); - Assertions.assertTrue(actualMessage.contains(expectedMessage)); + Assertions.assertTrue(actualMessage.contains(expected)); } { Filter filter = new Filter(Filter.Op.NOT_EXISTS, "key1", null); + String expected = String.format(expectedMessage, Filter.Op.NOT_EXISTS); Exception exception = assertThrows(UnsupportedOperationException.class, - () -> collection.parseQueryForNonCompositeFilter(filter)); + () -> collection.parseNonCompositeFilter(filter, initParams())); String actualMessage = exception.getMessage(); - Assertions.assertTrue(actualMessage.contains(expectedMessage)); + Assertions.assertTrue(actualMessage.contains(expected)); } { Filter filter = new Filter(Filter.Op.NEQ, "key1", null); + String expected = String.format(expectedMessage, Filter.Op.NEQ); Exception exception = assertThrows(UnsupportedOperationException.class, - () -> collection.parseQueryForNonCompositeFilter(filter)); + () -> collection.parseNonCompositeFilter(filter, initParams())); String actualMessage = exception.getMessage(); - Assertions.assertTrue(actualMessage.contains(expectedMessage)); + Assertions.assertTrue(actualMessage.contains(expected)); } { Filter filter = new Filter(Filter.Op.CONTAINS, "key1", null); + String expected = String.format(expectedMessage, Filter.Op.CONTAINS); Exception exception = assertThrows(UnsupportedOperationException.class, - () -> collection.parseQueryForNonCompositeFilter(filter)); + () -> collection.parseNonCompositeFilter(filter, initParams())); String actualMessage = exception.getMessage(); - Assertions.assertTrue(actualMessage.contains(expectedMessage)); + Assertions.assertTrue(actualMessage.contains(expected)); } } @@ -158,11 +162,11 @@ public void testParseQueryForNonCompositeFilterUnsupportedException() { public void testParseQueryForCompositeFilterWithNullConditions() { { Filter filter = new Filter(Filter.Op.AND, null, null); - Assertions.assertNull(collection.parseQuery(filter)); + Assertions.assertNull(collection.parseFilter(filter, initParams())); } { Filter filter = new Filter(Filter.Op.OR, null, null); - Assertions.assertNull(collection.parseQuery(filter)); + Assertions.assertNull(collection.parseFilter(filter, initParams())); } } @@ -171,18 +175,18 @@ public void testParseQueryForCompositeFilter() { { Filter filter = new Filter(Filter.Op.EQ, ID, "val1").and(new Filter(Filter.Op.EQ, CREATED_AT, "val2")); - String query = collection.parseQueryForCompositeFilter(filter); + String query = collection.parseCompositeFilter(filter, initParams()); Assertions - .assertEquals(String.format("(%s = 'val1') AND (%s = 'val2')", ID, CREATED_AT), query); + .assertEquals(String.format("(%s = ?) AND (%s = ?)", ID, CREATED_AT), query); } { Filter filter = new Filter(Filter.Op.EQ, ID, "val1").or(new Filter(Filter.Op.EQ, CREATED_AT, "val2")); - String query = collection.parseQueryForCompositeFilter(filter); + String query = collection.parseCompositeFilter(filter, initParams()); Assertions - .assertEquals(String.format("(%s = 'val1') OR (%s = 'val2')", ID, CREATED_AT), query); + .assertEquals(String.format("(%s = ?) OR (%s = ?)", ID, CREATED_AT), query); } } @@ -191,18 +195,18 @@ public void testParseQueryForCompositeFilterForJsonField() { { Filter filter = new Filter(Filter.Op.EQ, "key1", "val1").and(new Filter(Filter.Op.EQ, "key2", "val2")); - String query = collection.parseQueryForCompositeFilter(filter); + String query = collection.parseCompositeFilter(filter, initParams()); Assertions - .assertEquals("(document->>'key1' = 'val1') AND (document->>'key2' = 'val2')", query); + .assertEquals("(document->>'key1' = ?) AND (document->>'key2' = ?)", query); } { Filter filter = new Filter(Filter.Op.EQ, "key1", "val1").or(new Filter(Filter.Op.EQ, "key2", "val2")); - String query = collection.parseQueryForCompositeFilter(filter); + String query = collection.parseCompositeFilter(filter, initParams()); Assertions - .assertEquals("(document->>'key1' = 'val1') OR (document->>'key2' = 'val2')", query); + .assertEquals("(document->>'key1' = ?) OR (document->>'key2' = ?)", query); } } @@ -215,9 +219,9 @@ public void testParseNestedQuery() { new Filter(Filter.Op.EQ, ID, "val3").and(new Filter(Filter.Op.EQ, "key4", "val4")); Filter filter = filter1.or(filter2); - String query = collection.parseQuery(filter); - Assertions.assertEquals(String.format("((%s = 'val1') AND (document->>'key2' = 'val2')) " + - "OR ((%s = 'val3') AND (document->>'key4' = 'val4'))", ID, ID), query); + String query = collection.parseFilter(filter, initParams()); + Assertions.assertEquals(String.format("((%s = ?) AND (document->>'key2' = ?)) " + + "OR ((%s = ?) AND (document->>'key4' = ?))", ID, ID), query); } @@ -230,10 +234,14 @@ public void testJSONFieldParseNestedQuery() { new Filter(Filter.Op.EQ, "key3", "val3").and(new Filter(Filter.Op.EQ, "key4", "val4")); Filter filter = filter1.or(filter2); - String query = collection.parseQuery(filter); - Assertions.assertEquals("((document->>'key1' = 'val1') AND (document->>'key2' = 'val2')) " + - "OR ((document->>'key3' = 'val3') AND (document->>'key4' = 'val4'))", query); + String query = collection.parseFilter(filter, initParams()); + Assertions.assertEquals("((document->>'key1' = ?) AND (document->>'key2' = ?)) " + + "OR ((document->>'key3' = ?) AND (document->>'key4' = ?))", query); } + private Params.Builder initParams() { + return Params.newBuilder(); + } + } From 68d5ca2441c5d9fa54e10d978199a4c5229ec985 Mon Sep 17 00:00:00 2001 From: Nidhi Date: Tue, 8 Dec 2020 17:13:15 +0530 Subject: [PATCH 2/5] Adding params test cases --- .../documentstore/postgres/ParamsTest.java | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java new file mode 100644 index 00000000..d24295c2 --- /dev/null +++ b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java @@ -0,0 +1,77 @@ +package org.hypertrace.core.documentstore.postgres; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +public class ParamsTest { + + Params.Builder paramBuilder; + + @BeforeEach + void setUp() { + paramBuilder = Params.newBuilder(); + } + + @Test + public void testAddAndGetIntegerParam() { + paramBuilder.addIntegerParam(1); + paramBuilder.addIntegerParam(2); + Params params = paramBuilder.build(); + Assertions.assertEquals(params.getIntegerParams().size(), 2); + } + + @Test + public void testAddAndGetLongParam() { + paramBuilder.addLongParam(1L); + paramBuilder.addLongParam(2L); + Params params = paramBuilder.build(); + Assertions.assertEquals(params.getLongParams().size(), 2); + } + + @Test + public void testAddAndGetStringParam() { + paramBuilder.addStringParam("Bob"); + paramBuilder.addStringParam("Alice"); + Params params = paramBuilder.build(); + Assertions.assertEquals(params.getStringParams().size(), 2); + } + + @Test + public void testAddAndGetFloatParam() { + paramBuilder.addFloatParam(1L); + paramBuilder.addFloatParam(2L); + Params params = paramBuilder.build(); + Assertions.assertEquals(params.getFloatParams().size(), 2); + } + + @Test + public void testAddAndGetDoubleParam() { + paramBuilder.addDoubleParam(1L); + paramBuilder.addDoubleParam(2L); + Params params = paramBuilder.build(); + Assertions.assertEquals(params.getDoubleParams().size(), 2); + } + + @Test + public void testAllParamsAndIndex() { + paramBuilder.addIntegerParam(1); + paramBuilder.addLongParam(2L); + paramBuilder.addStringParam("Alice"); + paramBuilder.addFloatParam(3L); + paramBuilder.addDoubleParam(4L); + Params params = paramBuilder.build(); + int index = 0; + Assertions.assertEquals(params.getIntegerParams().get(index++), 1); + Assertions.assertEquals(params.getLongParams().get(index++), 2L); + Assertions.assertEquals(params.getStringParams().get(index++), "Alice"); + Assertions.assertEquals(params.getFloatParams().get(index++), 3L); + Assertions.assertEquals(params.getDoubleParams().get(index), 4L); + Assertions.assertEquals(index, 4); + } + +} From 5104233e23dd27330374d354c691556305ff648c Mon Sep 17 00:00:00 2001 From: Nidhi Date: Tue, 8 Dec 2020 18:40:41 +0530 Subject: [PATCH 3/5] cleaning imports --- .../hypertrace/core/documentstore/postgres/ParamsTest.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java index d24295c2..db8b6d85 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java +++ b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java @@ -4,10 +4,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; - public class ParamsTest { Params.Builder paramBuilder; From 8f912b489646a9013f12eb477e5fb00f7440ca20 Mon Sep 17 00:00:00 2001 From: Nidhi Date: Thu, 10 Dec 2020 14:19:42 +0530 Subject: [PATCH 4/5] PR review changes --- .../org/hypertrace/core/documentstore/postgres/Params.java | 4 ++-- .../core/documentstore/postgres/PostgresCollection.java | 2 +- .../hypertrace/core/documentstore/postgres/ParamsTest.java | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java index 058529f1..faea812e 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java @@ -4,7 +4,7 @@ import java.util.Map; /** - * Holds the params that need to be set in the PreparedStatement for constructing the final PQL + * Holds the params that need to be set in the PreparedStatement for constructing the final SQL * query */ public class Params { @@ -110,7 +110,7 @@ public static class Builder { private final Map doubleParams; private Builder() { - nextIndex = 0; + nextIndex = 1; integerParams = new HashMap<>(); longParams = new HashMap<>(); stringParams = new HashMap<>(); diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java index 6b6577f9..2a799f06 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java @@ -294,7 +294,7 @@ protected PreparedStatement buildPreparedStatement(String sqlQuery, Params param params.getStringParams().forEach((k, v) -> { try { // Postgres Index starts from 1 - preparedStatement.setString(k+1, v); + preparedStatement.setString(k, v); } catch (SQLException e) { LOGGER.error("SQLException querying documents. query: {}", sqlQuery, e); } diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java index db8b6d85..d2f42397 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java +++ b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java @@ -61,13 +61,13 @@ public void testAllParamsAndIndex() { paramBuilder.addFloatParam(3L); paramBuilder.addDoubleParam(4L); Params params = paramBuilder.build(); - int index = 0; + int index = 1; Assertions.assertEquals(params.getIntegerParams().get(index++), 1); Assertions.assertEquals(params.getLongParams().get(index++), 2L); Assertions.assertEquals(params.getStringParams().get(index++), "Alice"); Assertions.assertEquals(params.getFloatParams().get(index++), 3L); Assertions.assertEquals(params.getDoubleParams().get(index), 4L); - Assertions.assertEquals(index, 4); + Assertions.assertEquals(index, 5); } } From eb3bea5b84d10ad153a0aac9598f1338e70a8328 Mon Sep 17 00:00:00 2001 From: Nidhi Date: Mon, 14 Dec 2020 14:22:08 +0530 Subject: [PATCH 5/5] Moving to Object Params --- .../postgres/PostgresDocStoreTest.java | 79 ++++++++++-- .../core/documentstore/postgres/Params.java | 118 ++---------------- .../postgres/PostgresCollection.java | 32 +++-- .../documentstore/postgres/ParamsTest.java | 60 ++------- 4 files changed, 113 insertions(+), 176 deletions(-) diff --git a/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java b/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java index ffc0ea6d..8ff70d06 100644 --- a/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java +++ b/document-store/src/integrationTest/java/org/hypertrace/core/documentstore/postgres/PostgresDocStoreTest.java @@ -28,6 +28,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertThrows; + public class PostgresDocStoreTest { public static final String ID = "id"; @@ -282,20 +284,75 @@ public void testInQuery() throws IOException { @Test public void testSearch() throws IOException { Collection collection = datastore.getCollection(COLLECTION_NAME); - String documentString = "{\"attributes\":{\"trace_id\":{\"value\":{\"string\":\"00000000000000005e194fdf9fbf5101\"}},\"span_id\":{\"value\":{\"string\":\"6449f1f720c93a67\"}},\"service_type\":{\"value\":{\"string\":\"JAEGER_SERVICE\"}},\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"createdTime\":1605692185945,\"entityId\":\"e3ffc6f0-fc92-3a9c-9fa0-26269184d1aa\",\"entityName\":\"driver\",\"entityType\":\"SERVICE\",\"identifyingAttributes\":{\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"tenantId\":\"__default\"}"; - Document document = new JSONDocument(documentString); - SingleValueKey key = new SingleValueKey("default", "testKey1"); - collection.upsert(key, document); + String docStr1 = "{\"amount\":1234.5,\"attributes\":{\"trace_id\":{\"value\":{\"string\":\"00000000000000005e194fdf9fbf5101\"}},\"span_id\":{\"value\":{\"string\":\"6449f1f720c93a67\"}},\"service_type\":{\"value\":{\"string\":\"JAEGER_SERVICE\"}},\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"createdTime\":1605692185945,\"entityId\":\"e3ffc6f0-fc92-3a9c-9fa0-26269184d1aa\",\"entityName\":\"driver\",\"entityType\":\"SERVICE\",\"identifyingAttributes\":{\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"tenantId\":\"__default\"}"; + Document document1 = new JSONDocument(docStr1); + SingleValueKey key1 = new SingleValueKey("default", "testKey1"); + collection.upsert(key1, document1); + + String docStr2 = "{\"amount\":1234,\"attributes\":{\"trace_id\":{\"value\":{\"string\":\"00000000000000005e194fdf9fbf5101\"}},\"span_id\":{\"value\":{\"string\":\"6449f1f720c93a67\"}},\"service_type\":{\"value\":{\"string\":\"JAEGER_SERVICE\"}},\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"createdTime\":1605692185945,\"entityId\":\"e3ffc6f0-fc92-3a9c-9fa0-26269184d1aa\",\"entityName\":\"driver\",\"entityType\":\"SERVICE\",\"identifyingAttributes\":{\"FQN\":{\"value\":{\"string\":\"driver\"}}},\"tenantId\":\"__default\"}"; + Document document2 = new JSONDocument(docStr2); + SingleValueKey key2 = new SingleValueKey("default", "testKey2"); + collection.upsert(key2, document2); + + // Search integer field + { + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.EQ, "amount", 1234)); + Iterator results = collection.search(query); + List documents = new ArrayList<>(); + for (; results.hasNext(); ) { + documents.add(results.next()); + } + Assertions.assertEquals(documents.size(), 1); + } + + // Search float field + { + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.EQ, "amount", 1234.5)); + Iterator results = collection.search(query); + List documents = new ArrayList<>(); + for (; results.hasNext(); ) { + documents.add(results.next()); + } + Assertions.assertEquals(documents.size(), 1); + } + + // Search integer and float field + { + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.GTE, "amount", 123)); + Iterator results = collection.search(query); + List documents = new ArrayList<>(); + for (; results.hasNext(); ) { + documents.add(results.next()); + } + Assertions.assertEquals(documents.size(), 2); + } // Search _id field in the document - Query query = new Query(); - query.setFilter(new Filter(Filter.Op.EQ, DOCUMENT_ID, key.toString())); - Iterator results = collection.search(query); - List documents = new ArrayList<>(); - for (; results.hasNext(); ) { - documents.add(results.next()); + { + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.EQ, DOCUMENT_ID, key1.toString())); + Iterator results = collection.search(query); + List documents = new ArrayList<>(); + for (; results.hasNext(); ) { + documents.add(results.next()); + } + Assertions.assertEquals(documents.size(), 1); } - Assertions.assertEquals(documents.size(), 1); + + // Unsupported Object Type in Filter, should throw an UnsupportedOperationException + { + Query query = new Query(); + query.setFilter(new Filter(Filter.Op.EQ, "amount", new Filter())); + String expected = "Un-supported object types in filter"; + Exception exception = assertThrows(UnsupportedOperationException.class, + () -> collection.search(query)); + String actualMessage = exception.getMessage(); + Assertions.assertTrue(actualMessage.contains(expected)); + } + } @Test diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java index faea812e..b3480b49 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/Params.java @@ -10,90 +10,22 @@ public class Params { // Map of index to the corresponding param value - private final Map integerParams; - private final Map longParams; - private final Map stringParams; - private final Map floatParams; - private final Map doubleParams; + private final Map objectParams; private Params( - Map integerParams, - Map longParams, - Map stringParams, - Map floatParams, - Map doubleParams) { - this.integerParams = integerParams; - this.longParams = longParams; - this.stringParams = stringParams; - this.floatParams = floatParams; - this.doubleParams = doubleParams; - } - - public Map getIntegerParams() { - return integerParams; - } - - public Map getLongParams() { - return longParams; - } - - public Map getStringParams() { - return stringParams; - } - - public Map getFloatParams() { - return floatParams; - } - - public Map getDoubleParams() { - return doubleParams; + Map objectParams) { + this.objectParams = objectParams; } @Override public String toString() { return "Params{" + - "integerParams=" + integerParams + - ", longParams=" + longParams + - ", stringParams=" + stringParams + - ", floatParams=" + floatParams + - ", doubleParams=" + doubleParams + + "objectParams=" + objectParams + '}'; } - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } - - Params params = (Params) o; - - if (!integerParams.equals(params.integerParams)) { - return false; - } - if (!longParams.equals(params.longParams)) { - return false; - } - if (!stringParams.equals(params.stringParams)) { - return false; - } - if (!floatParams.equals(params.floatParams)) { - return false; - } - return doubleParams.equals(params.doubleParams); - } - - @Override - public int hashCode() { - int result = integerParams.hashCode(); - result = 31 * result + longParams.hashCode(); - result = 31 * result + stringParams.hashCode(); - result = 31 * result + floatParams.hashCode(); - result = 31 * result + doubleParams.hashCode(); - return result; + public Map getObjectParams() { + return objectParams; } public static Builder newBuilder() { @@ -103,48 +35,20 @@ public static Builder newBuilder() { public static class Builder { private int nextIndex; - private final Map integerParams; - private final Map longParams; - private final Map stringParams; - private final Map floatParams; - private final Map doubleParams; + private final Map objectParams; private Builder() { nextIndex = 1; - integerParams = new HashMap<>(); - longParams = new HashMap<>(); - stringParams = new HashMap<>(); - floatParams = new HashMap<>(); - doubleParams = new HashMap<>(); - } - - public Builder addIntegerParam(int paramValue) { - integerParams.put(nextIndex++, paramValue); - return this; - } - - public Builder addLongParam(long paramValue) { - longParams.put(nextIndex++, paramValue); - return this; - } - - public Builder addStringParam(String paramValue) { - stringParams.put(nextIndex++, paramValue); - return this; - } - - public Builder addFloatParam(float paramValue) { - floatParams.put(nextIndex++, paramValue); - return this; + objectParams = new HashMap<>(); } - public Builder addDoubleParam(double paramValue) { - doubleParams.put(nextIndex++, paramValue); + public Builder addObjectParam(Object paramValue) { + objectParams.put(nextIndex++, paramValue); return this; } public Params build() { - return new Params(integerParams, longParams, stringParams, floatParams, doubleParams); + return new Params(objectParams); } } } diff --git a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java index 2a799f06..167ddf27 100644 --- a/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java +++ b/document-store/src/main/java/org/hypertrace/core/documentstore/postgres/PostgresCollection.java @@ -239,11 +239,13 @@ protected String parseNonCompositeFilter(Filter filter, Params.Builder paramsBui String collect = values .stream() .map(val -> { - paramsBuilder.addStringParam(String.valueOf(val)); + paramsBuilder.addObjectParam(val); return QUESTION_MARK; }) .collect(Collectors.joining(", ")); - return filterString.append("(" + collect + ")").toString(); + return filterString.append("(") + .append(collect) + .append(")").toString(); case CONTAINS: // TODO: Matches condition inside an array of documents case EXISTS: @@ -256,7 +258,7 @@ protected String parseNonCompositeFilter(Filter filter, Params.Builder paramsBui String.format("Query operation:%s not supported", op)); } String filters = filterString.append(QUESTION_MARK).toString(); - paramsBuilder.addStringParam(String.valueOf(value)); + paramsBuilder.addObjectParam(value); return filters; } @@ -289,19 +291,33 @@ protected String parseCompositeFilter(Filter filter, Params.Builder paramsBuilde } @VisibleForTesting - protected PreparedStatement buildPreparedStatement(String sqlQuery, Params params) throws SQLException { + protected PreparedStatement buildPreparedStatement(String sqlQuery, Params params) throws SQLException, RuntimeException { PreparedStatement preparedStatement = client.prepareStatement(sqlQuery); - params.getStringParams().forEach((k, v) -> { + params.getObjectParams().forEach((k, v) -> { try { - // Postgres Index starts from 1 - preparedStatement.setString(k, v); + if (isValidType(v)) { + preparedStatement.setString(k, String.valueOf(v)); + } else { + throw new UnsupportedOperationException("Un-supported object types in filter"); + } } catch (SQLException e) { - LOGGER.error("SQLException querying documents. query: {}", sqlQuery, e); + LOGGER.error("SQLException setting Param. key: {}, value: {}", k, v); } }); return preparedStatement; } + private boolean isValidType(Object v) { + Set> validClassez = new HashSet<>() {{ + add(Double.class); + add(Float.class); + add(Integer.class); + add(Long.class); + add(String.class); + }}; + return validClassez.contains(v.getClass()); + } + @VisibleForTesting private String getJsonSubDocPath(String subDocPath) { return "{" + subDocPath.replaceAll(DOC_PATH_SEPARATOR, ",") + "}"; diff --git a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java index d2f42397..6757bbd6 100644 --- a/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java +++ b/document-store/src/test/java/org/hypertrace/core/documentstore/postgres/ParamsTest.java @@ -13,60 +13,20 @@ void setUp() { paramBuilder = Params.newBuilder(); } - @Test - public void testAddAndGetIntegerParam() { - paramBuilder.addIntegerParam(1); - paramBuilder.addIntegerParam(2); - Params params = paramBuilder.build(); - Assertions.assertEquals(params.getIntegerParams().size(), 2); - } - - @Test - public void testAddAndGetLongParam() { - paramBuilder.addLongParam(1L); - paramBuilder.addLongParam(2L); - Params params = paramBuilder.build(); - Assertions.assertEquals(params.getLongParams().size(), 2); - } - - @Test - public void testAddAndGetStringParam() { - paramBuilder.addStringParam("Bob"); - paramBuilder.addStringParam("Alice"); - Params params = paramBuilder.build(); - Assertions.assertEquals(params.getStringParams().size(), 2); - } - - @Test - public void testAddAndGetFloatParam() { - paramBuilder.addFloatParam(1L); - paramBuilder.addFloatParam(2L); - Params params = paramBuilder.build(); - Assertions.assertEquals(params.getFloatParams().size(), 2); - } - - @Test - public void testAddAndGetDoubleParam() { - paramBuilder.addDoubleParam(1L); - paramBuilder.addDoubleParam(2L); - Params params = paramBuilder.build(); - Assertions.assertEquals(params.getDoubleParams().size(), 2); - } - @Test public void testAllParamsAndIndex() { - paramBuilder.addIntegerParam(1); - paramBuilder.addLongParam(2L); - paramBuilder.addStringParam("Alice"); - paramBuilder.addFloatParam(3L); - paramBuilder.addDoubleParam(4L); + paramBuilder.addObjectParam(1); + paramBuilder.addObjectParam(2L); + paramBuilder.addObjectParam("Alice"); + paramBuilder.addObjectParam(3L); + paramBuilder.addObjectParam(4L); Params params = paramBuilder.build(); int index = 1; - Assertions.assertEquals(params.getIntegerParams().get(index++), 1); - Assertions.assertEquals(params.getLongParams().get(index++), 2L); - Assertions.assertEquals(params.getStringParams().get(index++), "Alice"); - Assertions.assertEquals(params.getFloatParams().get(index++), 3L); - Assertions.assertEquals(params.getDoubleParams().get(index), 4L); + Assertions.assertEquals(params.getObjectParams().get(index++), 1); + Assertions.assertEquals(params.getObjectParams().get(index++), 2L); + Assertions.assertEquals(params.getObjectParams().get(index++), "Alice"); + Assertions.assertEquals(params.getObjectParams().get(index++), 3L); + Assertions.assertEquals(params.getObjectParams().get(index), 4L); Assertions.assertEquals(index, 5); }