From 51f9e497d4d547d0ed0d6d98d62adbb36f08aed8 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Tue, 1 Oct 2024 19:13:41 -0600 Subject: [PATCH 1/3] feat(Content Analytics) fixes #30191 : Support multiple values for the `filters` attribute in our Content Analytics Query --- .../content/ContentAnalyticsAPIImpl.java | 7 +- .../analytics/query/AnalyticsQueryParser.java | 62 ++++++--- .../dotcms/analytics/query/FilterParser.java | 56 ++++++--- .../java/com/dotcms/cube/filters/Filter.java | 6 +- .../analytics/query/FilterParserTest.java | 118 +++++++++++++----- 5 files changed, 181 insertions(+), 68 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/analytics/content/ContentAnalyticsAPIImpl.java b/dotCMS/src/main/java/com/dotcms/analytics/content/ContentAnalyticsAPIImpl.java index 2417314de079..ef4c698a1101 100644 --- a/dotCMS/src/main/java/com/dotcms/analytics/content/ContentAnalyticsAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/analytics/content/ContentAnalyticsAPIImpl.java @@ -26,16 +26,15 @@ public ContentAnalyticsAPIImpl(final ContentAnalyticsFactory contentAnalyticsFac @Override public ReportResponse runReport(final AnalyticsQuery query, final User user) { - Logger.debug(this, ()-> "Running the report for the query: " + query); - // note: should check any permissions for an user. + // TODO: We should check for specific user permissions return this.contentAnalyticsFactory.getReport(query, user); } @Override - public ReportResponse runRawReport(CubeJSQuery cubeJSQuery, User user) { + public ReportResponse runRawReport(final CubeJSQuery cubeJSQuery, final User user) { Logger.debug(this, ()-> "Running the report for the raw query: " + cubeJSQuery); - // note: should check any permissions for an user. + // TODO: We should check for specific user permissions return this.contentAnalyticsFactory.getRawReport(cubeJSQuery, user); } diff --git a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java index 688c8080b96c..52207ee474cf 100644 --- a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java +++ b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java @@ -13,15 +13,23 @@ import javax.enterprise.context.ApplicationScoped; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.List; import java.util.Objects; import java.util.stream.Collectors; import java.util.stream.Stream; +import static com.liferay.util.StringPool.APOSTROPHE; +import static com.liferay.util.StringPool.BLANK; + /** - * Parser for the analytics query, it can parse a json string to a {@link AnalyticsQuery} or a {@link CubeJSQuery} + * This class exposes a parser for the {@link AnalyticsQuery} class. It can parse a JSON string + * into a {@link AnalyticsQuery} object or a {@link CubeJSQuery} object. The Analytics Query + * exposes a more readable and simple syntax compared to the CubeJS query + * * @author jsanca + * @since Sep 19th, 2024 */ @ApplicationScoped public class AnalyticsQueryParser { @@ -44,14 +52,14 @@ public class AnalyticsQueryParser { public AnalyticsQuery parseJsonToQuery(final String json) { if (Objects.isNull(json)) { - throw new IllegalArgumentException("Json can not be null"); + throw new IllegalArgumentException("JSON cannot be null"); } try { - Logger.debug(this, ()-> "Parsing json to query: " + json); + Logger.debug(this, ()-> "Parsing json query: " + json); return DotObjectMapperProvider.getInstance().getDefaultObjectMapper() .readValue(json, AnalyticsQuery.class); - } catch (JsonProcessingException e) { + } catch (final JsonProcessingException e) { Logger.error(this, e.getMessage(), e); throw new DotRuntimeException(e); } @@ -80,18 +88,20 @@ public CubeJSQuery parseJsonToCubeQuery(final String json) { } /** - * Parse an {@link AnalyticsQuery} to a {@link CubeJSQuery} - * @param query - * @return CubeJSQuery + * Parses an {@link AnalyticsQuery} object into a {@link CubeJSQuery} object, which represents + * the official CubeJS query. + * + * @param query The {@link AnalyticsQuery} object to be parsed. + * + * @return The {@link CubeJSQuery} object. */ public CubeJSQuery parseQueryToCubeQuery(final AnalyticsQuery query) { - if (Objects.isNull(query)) { - throw new IllegalArgumentException("Query can not be null"); + throw new IllegalArgumentException("Query cannot be null"); } final CubeJSQuery.Builder builder = new CubeJSQuery.Builder(); - Logger.debug(this, ()-> "Parsing query to cube query: " + query); + Logger.debug(this, ()-> "Parsing query: " + query); if (UtilMethods.isSet(query.getDimensions())) { builder.dimensions(query.getDimensions()); @@ -136,6 +146,15 @@ private Collection parseOrders(final String orders) { ).collect(Collectors.toList()); } + /** + * Parses the value of the {@code filters} attribute of the {@link AnalyticsQuery} object. This + * filter can have several logical operators, and be able to compare a variable against multiple + * values. + * + * @param filters the value of the {@code filters} attribute. + * + * @return A collection of {@link Filter} objects. + */ private Collection parseFilters(final String filters) { final Tuple2,List> result = FilterParser.parseFilterExpression(filters); @@ -144,14 +163,13 @@ private Collection parseFilters(final String filters) { final List simpleFilters = new ArrayList<>(); for (final FilterParser.Token token : result._1) { - simpleFilters.add( new SimpleFilter(token.member, - parseOperator(token.operator), - new Object[]{token.values})); + parseOperator(token.operator), + this.parseTokenValues(token.values))); } - // if has operators + // Are there any operators? if (UtilMethods.isSet(result._2())) { FilterParser.LogicalOperator logicalOperator = result._2().get(0); // first one @@ -177,6 +195,21 @@ private Collection parseFilters(final String filters) { return filterList; } + /** + * Takes the value of a token and parses its contents into an array of strings. A token can have + * both single and multiple values. + * + * @param values The value of the token. + * + * @return The value or values of a token as an array of strings. + */ + private String[] parseTokenValues(final String values) { + final String[] valueArray = values.split("\\s*,\\s*"); + return Arrays.stream(valueArray).map( + value -> value.replaceAll(APOSTROPHE, BLANK)) + .toArray(String[]::new); + } + private SimpleFilter.Operator parseOperator(final String operator) { switch (operator) { case "=": @@ -191,4 +224,5 @@ private SimpleFilter.Operator parseOperator(final String operator) { throw new DotRuntimeException("Operator not supported: " + operator); } } + } diff --git a/dotCMS/src/main/java/com/dotcms/analytics/query/FilterParser.java b/dotCMS/src/main/java/com/dotcms/analytics/query/FilterParser.java index c176db1fabdf..60e0a1dd69bf 100644 --- a/dotCMS/src/main/java/com/dotcms/analytics/query/FilterParser.java +++ b/dotCMS/src/main/java/com/dotcms/analytics/query/FilterParser.java @@ -1,5 +1,7 @@ package com.dotcms.analytics.query; +import com.dotmarketing.util.Logger; +import com.dotmarketing.util.UtilMethods; import io.vavr.Tuple; import io.vavr.Tuple2; @@ -9,23 +11,27 @@ import java.util.regex.Pattern; /** - * Parser for a filter expression - * Example: + * This class exposes a parser for the {@code filters} attribute in our Analytics Query object. For + * example, the following expression: * 
- *     FilterParser.parseFilterExpression("Events.variant = ['B'] or Events.experiments = ['C']");
+ *     {@code FilterParser.parseFilterExpression("Events.variant = ['B'] or Events.experiments = ['C']");}
  *
- * should return 2 tokens and 1 logical operator - * Tokens are member, operator and values (Events.variant, =, B) and the operator is 'and' or 'or' + *

Should return 2 tokens and 1 logical operator. Tokens are composed of a member, an operator, + * and one or more values.

+ * * @author jsanca + * @since Sep 19th, 2024 */ public class FilterParser { - private static final String EXPRESSION_REGEX = "(\\w+\\.\\w+)\\s*(=|!=|in|!in)\\s*\\['(.*?)'"; + private static final String EXPRESSION_REGEX = "(\\w+\\.\\w+)\\s*(=|!=|in|!in)\\s*\\[\\s*((?:'([^']*)'|\\d+)(?:\\s*,\\s*(?:'([^']*)'|\\d+))*)\\s*]"; private static final String LOGICAL_OPERATOR_REGEX = "\\s*(and|or)\\s*"; + private static final Pattern TOKEN_PATTERN = Pattern.compile(EXPRESSION_REGEX); private static final Pattern LOGICAL_PATTERN = Pattern.compile(LOGICAL_OPERATOR_REGEX); - static class Token { + public static class Token { + String member; String operator; String values; @@ -37,37 +43,46 @@ public Token(final String member, this.operator = operator; this.values = values; } + } - enum LogicalOperator { + public enum LogicalOperator { AND, OR, UNKNOWN } /** - * This method parser the filter expression such as - * [Events.variant = [“B”] or Events.experiments = [“B”]] - * @param expression String - * @return return the token expression plus the logical operators + * Parses the value of the {@code filter} attribute, allowing both single and multiple values. + * For instance, the following expression: + * 
+     *     {@code request.whatAmI = ['PAGE','FILE'] and request.url in ['/blog']}
+     *
+ *

Allows you to retrieve results for both HTML Pages and Files whose URL contains the + * String {@code "/blog"}.

+ * + * @param expression The value of the {@code filter} attribute. + * + * @return A {@link Tuple2} object containing token expression plus the logical operators. */ public static Tuple2,List> parseFilterExpression(final String expression) { - final List tokens = new ArrayList<>(); final List logicalOperators = new ArrayList<>(); - // note:Need to use cache here + if (UtilMethods.isNotSet(expression)) { + return Tuple.of(tokens, logicalOperators); + } + // TODO: We need to use cache here final Matcher tokenMatcher = TOKEN_PATTERN.matcher(expression); - // Extract the tokens (member, operator, values) while (tokenMatcher.find()) { final String member = tokenMatcher.group(1); // Example: Events.variant final String operator = tokenMatcher.group(2); // Example: = - final String values = tokenMatcher.group(3); // Example: "B" + final String values = tokenMatcher.group(3); // Example: "'B'", or multiple values such as "'A', 'B'" tokens.add(new Token(member, operator, values)); } // Pattern for logical operators (and, or) - // Need to use cache here + // TODO: Need to use cache here final Matcher logicalMatcher = LOGICAL_PATTERN.matcher(expression); // Extract logical operators @@ -76,9 +91,9 @@ public static Tuple2,List> parseFilterExpression(fi logicalOperators.add(parseLogicalOperator(logicalOperator)); } - // if any unknown should fails - // note: should validate logical operators should be length - 1 of the tokens??? - + if (tokens.isEmpty() && logicalOperators.isEmpty()) { + Logger.warn(FilterParser.class, String.format("Filter expression failed to be parsed: %s", expression)); + } return Tuple.of(tokens, logicalOperators); } @@ -93,4 +108,5 @@ private static LogicalOperator parseLogicalOperator(final String logicalOperator return LogicalOperator.UNKNOWN; } } + } diff --git a/dotCMS/src/main/java/com/dotcms/cube/filters/Filter.java b/dotCMS/src/main/java/com/dotcms/cube/filters/Filter.java index 5092ae147991..6b9d41dddceb 100644 --- a/dotCMS/src/main/java/com/dotcms/cube/filters/Filter.java +++ b/dotCMS/src/main/java/com/dotcms/cube/filters/Filter.java @@ -6,6 +6,9 @@ * Represents a CubeJs Query Filter * * @see Filters format + * + * @author Freddy Rodriguez + * @since Jan 27th, 2023 */ public interface Filter { @@ -15,7 +18,8 @@ static LogicalFilter.Builder and(){ Map asMap(); - public enum Order { + enum Order { ASC, DESC; } + } diff --git a/dotCMS/src/test/java/com/dotcms/analytics/query/FilterParserTest.java b/dotCMS/src/test/java/com/dotcms/analytics/query/FilterParserTest.java index 8f5dc3bcc630..7e3a8f1cbb36 100644 --- a/dotCMS/src/test/java/com/dotcms/analytics/query/FilterParserTest.java +++ b/dotCMS/src/test/java/com/dotcms/analytics/query/FilterParserTest.java @@ -1,14 +1,15 @@ package com.dotcms.analytics.query; - import io.vavr.Tuple2; -import org.junit.Assert; import org.junit.Test; import java.util.List; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; + /** - * Test for {@link FilterParser} + * Tests for {@link FilterParser} class. * * @author jsanca */ @@ -20,23 +21,23 @@ public class FilterParserTest { * should return 2 tokens and 1 logical operator */ @Test - public void test_2_tokens_parseFilterExpression_should_be_OK() throws Exception { + public void test_2_tokens_parseFilterExpression_should_be_OK() { final Tuple2,List> result = FilterParser.parseFilterExpression("Events.variant = ['B'] or Events.experiments = ['C']"); - Assert.assertNotNull(result); - Assert.assertEquals(2, result._1.size()); // two tokens - Assert.assertEquals(1, result._2.size()); // one operator + assertNotNull(result); + assertEquals(2, result._1.size()); // two tokens + assertEquals(1, result._2.size()); // one operator final FilterParser.Token token1 = result._1.get(0); final FilterParser.Token token2 = result._1.get(1); - Assert.assertEquals("Events.variant", token1.member); - Assert.assertEquals("=", token1.operator); - Assert.assertEquals("B", token1.values); - Assert.assertEquals("Events.experiments", token2.member); - Assert.assertEquals("=", token2.operator); - Assert.assertEquals("C", token2.values); + assertEquals("Events.variant", token1.member); + assertEquals("=", token1.operator); + assertEquals("'B'", token1.values); + assertEquals("Events.experiments", token2.member); + assertEquals("=", token2.operator); + assertEquals("'C'", token2.values); - Assert.assertEquals(FilterParser.LogicalOperator.OR, result._2.get(0)); + assertEquals(FilterParser.LogicalOperator.OR, result._2.get(0)); } /** @@ -45,29 +46,88 @@ public void test_2_tokens_parseFilterExpression_should_be_OK() throws Exception * should return 3 tokens and 2 logical operator */ @Test - public void test_3_tokens_parseFilterExpression_should_be_OK() throws Exception { + public void test_3_tokens_parseFilterExpression_should_be_OK() { final Tuple2,List> result = FilterParser.parseFilterExpression("Events.variant in ['B'] and Events.experiments != ['C'] or Events.goals !in ['C']"); - Assert.assertNotNull(result); - Assert.assertEquals(3, result._1.size()); // two tokens - Assert.assertEquals(2, result._2.size()); // one operator + assertNotNull(result); + assertEquals(3, result._1.size()); // two tokens + assertEquals(2, result._2.size()); // one operator final FilterParser.Token token1 = result._1.get(0); final FilterParser.Token token2 = result._1.get(1); final FilterParser.Token token3 = result._1.get(2); - Assert.assertEquals("Events.variant", token1.member); - Assert.assertEquals("in", token1.operator); - Assert.assertEquals("B", token1.values); - Assert.assertEquals("Events.experiments", token2.member); - Assert.assertEquals("!=", token2.operator); - Assert.assertEquals("C", token2.values); - Assert.assertEquals("Events.goals", token3.member); - Assert.assertEquals("!in", token3.operator); - Assert.assertEquals("C", token3.values); + assertEquals("Events.variant", token1.member); + assertEquals("in", token1.operator); + assertEquals("'B'", token1.values); + assertEquals("Events.experiments", token2.member); + assertEquals("!=", token2.operator); + assertEquals("'C'", token2.values); + assertEquals("Events.goals", token3.member); + assertEquals("!in", token3.operator); + assertEquals("'C'", token3.values); + + assertEquals(FilterParser.LogicalOperator.AND, result._2.get(0)); + assertEquals(FilterParser.LogicalOperator.OR, result._2.get(1)); + } + + /** + *
    + *
  • Method to test: {@link FilterParser#parseFilterExpression(String)}
    • + *
  • Given Scenario: Parse a filter that includes multiple values.
    • + *
  • Expected Result: All values should be returned.
    • + *
+ */ + @Test + public void parseExpressionWithMultipleValues() { + // ╔════════════════════════╗ + // ║ Generating Test Data ║ + // ╚════════════════════════╝ + final Tuple2,List> result = + FilterParser.parseFilterExpression("request.whatAmI = ['PAGE','FILE']"); - Assert.assertEquals(FilterParser.LogicalOperator.AND, result._2.get(0)); - Assert.assertEquals(FilterParser.LogicalOperator.OR, result._2.get(1)); + // ╔══════════════╗ + // ║ Assertions ║ + // ╚══════════════╝ + assertNotNull(result); + assertEquals("Only one token is expected", 1, result._1.size()); + assertEquals("No logical operators are expected", 0, result._2.size()); + final FilterParser.Token token1 = result._1.get(0); + assertEquals("request.whatAmI", token1.member); + assertEquals("=", token1.operator); + assertEquals("'PAGE','FILE'", token1.values); } + /** + *
    + *
  • Method to test: {@link FilterParser#parseFilterExpression(String)}
    • + *
  • Given Scenario: Parse a filter that includes multiple values and two logical + * operators.
    • + *
  • Expected Result: All values and expressions should be returned.
    • + *
+ */ + @Test + public void parseExpressionWithMultipleValuesAndTwoLogicalOperators() { + // ╔════════════════════════╗ + // ║ Generating Test Data ║ + // ╚════════════════════════╝ + final Tuple2,List> result = + FilterParser.parseFilterExpression("request.whatAmI = ['PAGE','FILE'] and request.url in ['/blog']"); + + // ╔══════════════╗ + // ║ Assertions ║ + // ╚══════════════╝ + assertNotNull(result); + assertEquals("Two tokens are expected", 2, result._1.size()); + assertEquals("Only one logical operator is expected", 1, result._2.size()); + final FilterParser.Token token1 = result._1.get(0); + final FilterParser.Token token2 = result._1.get(1); + assertEquals("request.whatAmI", token1.member); + assertEquals("=", token1.operator); + assertEquals("'PAGE','FILE'", token1.values); + assertEquals("request.url", token2.member); + assertEquals("in", token2.operator); + assertEquals("'/blog'", token2.values); + assertEquals(FilterParser.LogicalOperator.AND, result._2.get(0)); + } } From 74c1f718366bd38acbc6a1fb1389665bd1cb27f6 Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Wed, 2 Oct 2024 08:56:33 -0600 Subject: [PATCH 2/3] Implementing SonarQube feedback --- .../com/dotcms/analytics/query/AnalyticsQueryParser.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java index 52207ee474cf..07703d0907a5 100644 --- a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java +++ b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java @@ -204,9 +204,10 @@ private Collection parseFilters(final String filters) { * @return The value or values of a token as an array of strings. */ private String[] parseTokenValues(final String values) { - final String[] valueArray = values.split("\\s*,\\s*"); + //final String[] valueArray = values.split("\\s*,\\s*"); + final String[] valueArray = values.split(","); return Arrays.stream(valueArray).map( - value -> value.replaceAll(APOSTROPHE, BLANK)) + value -> value.trim().replaceAll(APOSTROPHE, BLANK)) .toArray(String[]::new); } From 304236255268d78723af12c532a9c816d6bf1ebc Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Wed, 2 Oct 2024 10:35:58 -0600 Subject: [PATCH 3/3] Implementing SonarQube feedback --- .../java/com/dotcms/analytics/query/AnalyticsQueryParser.java | 1 - 1 file changed, 1 deletion(-) diff --git a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java index 07703d0907a5..1f96f86871f1 100644 --- a/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java +++ b/dotCMS/src/main/java/com/dotcms/analytics/query/AnalyticsQueryParser.java @@ -204,7 +204,6 @@ private Collection parseFilters(final String filters) { * @return The value or values of a token as an array of strings. */ private String[] parseTokenValues(final String values) { - //final String[] valueArray = values.split("\\s*,\\s*"); final String[] valueArray = values.split(","); return Arrays.stream(valueArray).map( value -> value.trim().replaceAll(APOSTROPHE, BLANK))