Removed dangling logical filter methods
IMHO, These methods were esthetically pleasing, but they required complex logic, and make it easy for the user to do something incorrect.
Deleting them keeps things simple
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/ComplexLogicalFilterBuilder.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/ComplexLogicalFilterBuilder.java
index 7cae6af..b211808 100644
--- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/ComplexLogicalFilterBuilder.java
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/ComplexLogicalFilterBuilder.java
@@ -4,57 +4,56 @@
@Override
public FilterBuilder or(FilterExpression fe1) {
- if (filterExpression instanceof AttributeComparisonExpression) {
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(groupIfNeeded(filterExpression));
- logicalExpression.setRight(groupIfNeeded(fe1));
- logicalExpression.setOperator(LogicalOperator.OR);
- filterExpression = logicalExpression;
- return this;
+ if (filterExpression == null) {
+ throw new IllegalStateException("Cannot call or(Filter), call or(Filter, Filter) instead.");
}
-
LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(fe1);
+ logicalExpression.setLeft(groupIfNeeded(filterExpression));
+ logicalExpression.setRight(groupIfNeeded(fe1));
logicalExpression.setOperator(LogicalOperator.OR);
-
- return handleLogicalExpression(logicalExpression, LogicalOperator.OR);
+ filterExpression = logicalExpression;
+ return this;
}
@Override
public FilterBuilder or(FilterExpression fe1, FilterExpression fe2) {
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(groupIfNeeded(fe1));
- logicalExpression.setRight(groupIfNeeded(fe2));
- logicalExpression.setOperator(LogicalOperator.OR);
-
- return handleLogicalExpression(logicalExpression, LogicalOperator.OR);
+ if (filterExpression == null) {
+ LogicalExpression logicalExpression = new LogicalExpression();
+ logicalExpression.setLeft(groupIfNeeded(fe1));
+ logicalExpression.setRight(groupIfNeeded(fe2));
+ logicalExpression.setOperator(LogicalOperator.OR);
+ filterExpression = logicalExpression;
+ } else {
+ this.or(fe1).or(fe2);
+ }
+ return this;
}
@Override
public FilterBuilder and(FilterExpression fe1, FilterExpression fe2) {
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(groupIfNeeded(fe1));
- logicalExpression.setRight(groupIfNeeded(fe2));
- logicalExpression.setOperator(LogicalOperator.AND);
-
- return handleLogicalExpression(logicalExpression, LogicalOperator.AND);
+ if (filterExpression == null) {
+ LogicalExpression logicalExpression = new LogicalExpression();
+ logicalExpression.setLeft(groupIfNeeded(fe1));
+ logicalExpression.setRight(groupIfNeeded(fe2));
+ logicalExpression.setOperator(LogicalOperator.AND);
+ filterExpression = logicalExpression;
+ } else {
+ this.and(fe1).and(fe2);
+ }
+ return this;
}
@Override
public FilterBuilder and(FilterExpression fe1) {
- if (filterExpression instanceof AttributeComparisonExpression) {
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(filterExpression);
- logicalExpression.setRight(groupIfNeeded(fe1));
- logicalExpression.setOperator(LogicalOperator.AND);
- filterExpression = logicalExpression;
- return this;
+ if (filterExpression == null) {
+ throw new IllegalStateException("Cannot call and(Filter), call and(Filter, Filter) instead.");
}
-
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(fe1);
- logicalExpression.setOperator(LogicalOperator.AND);
- return handleLogicalExpression(logicalExpression, LogicalOperator.AND);
+ LogicalExpression logicalExpression = new LogicalExpression();
+ logicalExpression.setLeft(filterExpression);
+ logicalExpression.setRight(groupIfNeeded(fe1));
+ logicalExpression.setOperator(LogicalOperator.AND);
+ filterExpression = logicalExpression;
+ return this;
}
}
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterBuilder.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterBuilder.java
index 3c569cc..aa2fb3f 100644
--- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterBuilder.java
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/FilterBuilder.java
@@ -7,8 +7,6 @@
public interface FilterBuilder {
- FilterBuilder and();
-
FilterBuilder and(FilterExpression fe1);
default FilterBuilder and(UnaryOperator<FilterBuilder> filter) {
@@ -29,8 +27,6 @@
return and(left.apply(FilterBuilder.create()).build(), right.apply(FilterBuilder.create()).build());
}
- FilterBuilder or();
-
FilterBuilder or(FilterExpression fe1);
default FilterBuilder or(Filter filter) {
diff --git a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/SimpleLogicalFilterBuilder.java b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/SimpleLogicalFilterBuilder.java
index 8f87136..95c5bd5 100644
--- a/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/SimpleLogicalFilterBuilder.java
+++ b/scim-spec/scim-spec-schema/src/main/java/org/apache/directory/scim/spec/filter/SimpleLogicalFilterBuilder.java
@@ -7,83 +7,12 @@
protected FilterExpression filterExpression;
- @Override
- public FilterBuilder and() {
- if (filterExpression == null) {
- throw new IllegalStateException();
- }
-
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(groupIfNeeded(filterExpression));
- logicalExpression.setOperator(LogicalOperator.AND);
- filterExpression = logicalExpression;
-
- return this;
- }
-
- @Override
- public FilterBuilder or() {
- if (filterExpression == null) {
- throw new IllegalStateException();
- }
-
- LogicalExpression logicalExpression = new LogicalExpression();
- logicalExpression.setLeft(groupIfNeeded(filterExpression));
- logicalExpression.setOperator(LogicalOperator.OR);
- filterExpression = logicalExpression;
-
- return this;
- }
-
static FilterExpression groupIfNeeded(FilterExpression filterExpression) {
return (filterExpression instanceof LogicalExpression)
? new GroupExpression(false, filterExpression)
: filterExpression;
}
- protected FilterBuilder handleLogicalExpression(LogicalExpression expression, LogicalOperator operator) {
- log.info("In handleLogicalExpression");
- if (filterExpression == null) {
- filterExpression = expression;
- } else if (filterExpression instanceof AttributeComparisonExpression) {
- log.info("Adding a logical expression as the new root");
-
- log.info("Setting as left: " + filterExpression.toFilter());
- expression.setLeft(filterExpression);
- log.info("Setting as right: " + expression.toFilter());
-
- filterExpression = expression;
- } else if (filterExpression instanceof LogicalExpression) {
- log.info("filter exression is a logical expression");
- LogicalExpression le = (LogicalExpression) filterExpression;
-
- if (le.getLeft() == null) {
- log.info("Setting left to: " + expression.toFilter());
- le.setLeft(expression);
- } else if (le.getRight() == null) {
- log.info("Setting right to: " + expression.toFilter());
- le.setRight(expression);
- } else {
- log.info("The current base is complete, raising up one level");
- LogicalExpression newRoot = new LogicalExpression();
- log.info("Setting left to: " + expression);
- newRoot.setLeft(expression);
- filterExpression = newRoot;
- }
- } else if (filterExpression instanceof GroupExpression) {
- log.info("Found group expression");
- LogicalExpression newRoot = new LogicalExpression();
- newRoot.setLeft(filterExpression);
- newRoot.setRight(expression);
- newRoot.setOperator(operator);
- filterExpression = newRoot;
- }
-
- log.info("New filter expression: " + filterExpression.toFilter());
-
- return this;
- }
-
protected void handleComparisonExpression(FilterExpression expression) {
if (expression == null) {
@@ -94,7 +23,7 @@
filterExpression = expression;
} else {
if (!(filterExpression instanceof LogicalExpression)) {
- throw new IllegalStateException();
+ throw new IllegalStateException("Invalid filter state");
}
LogicalExpression le = (LogicalExpression) filterExpression;
diff --git a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderPresentTest.java b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderPresentTest.java
index f9cb497..0b6e5c9 100644
--- a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderPresentTest.java
+++ b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderPresentTest.java
@@ -20,9 +20,6 @@
package org.apache.directory.scim.spec.filter;
import lombok.extern.slf4j.Slf4j;
-import org.apache.directory.scim.spec.filter.Filter;
-import org.apache.directory.scim.spec.filter.FilterBuilder;
-import org.apache.directory.scim.spec.filter.FilterParseException;
import org.junit.jupiter.api.Test;
import static org.assertj.core.api.Assertions.assertThat;
@@ -41,8 +38,7 @@
public void testStartsWith() throws FilterParseException {
Filter filter = FilterBuilder.create()
.present("address.streetAddress")
- .and()
- .equalTo("address.region", "CA")
+ .and(f -> f.equalTo("address.region", "CA"))
.build();
Filter expected = new Filter("address.streetAddress PR AND address.region EQ \"CA\"");
assertThat(filter).isEqualTo(expected);
diff --git a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderTest.java b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderTest.java
index 641fad0..e044cda 100644
--- a/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderTest.java
+++ b/scim-spec/scim-spec-schema/src/test/java/org/apache/directory/scim/spec/filter/FilterBuilderTest.java
@@ -20,9 +20,6 @@
package org.apache.directory.scim.spec.filter;
import lombok.extern.slf4j.Slf4j;
-import org.apache.directory.scim.spec.filter.Filter;
-import org.apache.directory.scim.spec.filter.FilterBuilder;
-import org.apache.directory.scim.spec.filter.FilterParseException;
import org.junit.jupiter.api.Test;
import java.io.UnsupportedEncodingException;
@@ -37,8 +34,7 @@
public void testSimpleAnd() throws FilterParseException {
Filter filter = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .and()
- .equalTo("name.familyName", "Baggins")
+ .and(r -> r.equalTo("name.familyName", "Baggins"))
.build();
assertThat(filter).isEqualTo(new Filter("name.givenName EQ \"Bilbo\" AND name.familyName EQ \"Baggins\""));
}
@@ -47,8 +43,8 @@
public void testSimpleOr() throws FilterParseException {
Filter filter = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .or()
- .equalTo("name.familyName", "Baggins").build();
+ .or(r -> r.equalTo("name.familyName", "Baggins"))
+ .build();
assertThat(filter).isEqualTo(new Filter("name.givenName EQ \"Bilbo\" OR name.familyName EQ \"Baggins\""));
}
@@ -58,14 +54,13 @@
Filter filter = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .or()
- .equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins").build();
+ .or(f -> f.equalTo("name.givenName", "Frodo"))
+ .and(f -> f.equalTo("name.familyName", "Baggins"))
+ .build();
Filter expected = new Filter("(name.givenName EQ \"Bilbo\" OR name.givenName EQ \"Frodo\") AND name.familyName EQ \"Baggins\"");
- assertThat(filter).isEqualTo(expected);
+ assertThat(filter.getFilter()).isEqualTo(expected.getFilter());
}
@Test
@@ -75,8 +70,7 @@
.equalTo("name.givenName", "Bilbo")
.and(FilterBuilder.create()
.equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins")
+ .and(f -> f.equalTo("name.familyName", "Baggins"))
.build())
.build();
@@ -91,8 +85,7 @@
.equalTo("name.givenName", "Bilbo")
.or(FilterBuilder.create()
.equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins")
+ .and(f -> f.equalTo("name.familyName", "Baggins"))
.build())
.build();
@@ -106,36 +99,30 @@
FilterBuilder b1 = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .or()
- .equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins");
+ .or(f -> f.equalTo("name.givenName", "Frodo"))
+ .and(f -> f.equalTo("name.familyName", "Baggins"));
FilterBuilder b2 = FilterBuilder.create().equalTo("address.streetAddress", "Underhill")
- .or()
- .equalTo("address.streetAddress", "Overhill")
- .and()
- .equalTo("address.postalCode", "16803");
+ .or(f -> f.equalTo("address.streetAddress", "Overhill"))
+ .and(f -> f.equalTo("address.postalCode", "16803"));
Filter filter = FilterBuilder.create().and(b1.build(), b2.build()).build();
Filter expected = new Filter("((name.givenName EQ \"Bilbo\" OR name.givenName EQ \"Frodo\") AND name.familyName EQ \"Baggins\") AND ((address.streetAddress EQ \"Underhill\" OR address.streetAddress EQ \"Overhill\") AND address.postalCode EQ \"16803\")");
- assertThat(filter).isEqualTo(expected);
+ assertThat(filter.getFilter()).isEqualTo(expected.getFilter());
}
@Test
public void testNot() throws FilterParseException {
FilterBuilder b1 = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .or()
- .equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins");
+ .or(f -> f.equalTo("name.givenName", "Frodo"))
+ .and(f -> f.equalTo("name.familyName", "Baggins"));
Filter filter = FilterBuilder.create().not(b1.build()).build();
Filter expected = new Filter("NOT((name.givenName EQ \"Bilbo\" OR name.givenName EQ \"Frodo\") AND name.familyName EQ \"Baggins\")");
- assertThat(filter).isEqualTo(expected);
+ assertThat(filter.getFilter()).isEqualTo(expected.getFilter());
}
@Test
@@ -157,10 +144,8 @@
FilterBuilder b1 = FilterBuilder.create()
.equalTo("name.givenName", "Bilbo")
- .or()
- .equalTo("name.givenName", "Frodo")
- .and()
- .equalTo("name.familyName", "Baggins");
+ .or(f -> f.equalTo("name.givenName", "Frodo"))
+ .and(f -> f.equalTo("name.familyName", "Baggins"));
FilterBuilder b2 = FilterBuilder.create().attributeHas("address", b1.build());
@@ -176,11 +161,38 @@
@Test
public void testAttributeContainsDeeplyEmbedded() throws FilterParseException {
- FilterBuilder b1 = FilterBuilder.create().equalTo("name.givenName", "Bilbo").or().equalTo("name.givenName", "Frodo").and().equalTo("name.familyName", "Baggins");
+ FilterBuilder b1 = FilterBuilder.create()
+ .equalTo("name.givenName", "Bilbo")
+ .or(f -> f.equalTo("name.givenName", "Frodo"))
+ .and(f -> f.equalTo("name.familyName", "Baggins"));
FilterBuilder b2 = FilterBuilder.create().attributeHas("address", b1.build());
FilterBuilder b3 = FilterBuilder.create().equalTo("name.giveName", "Gandalf").and(b2.build());
FilterBuilder b4 = FilterBuilder.create().attributeHas("address", b3.build());
assertThrows(FilterParseException.class, () -> new Filter( b4.toString()));
}
+
+ @Test
+ public void complexOrPresent() throws FilterParseException {
+ Filter filter = FilterBuilder.create().or(l -> l.present("name.givenName"), r -> r.present("name.familyName")).build();
+ assertThat(filter).isEqualTo(new Filter("name.givenName PR OR name.familyName PR"));
+ }
+
+ @Test
+ public void complexPresentThenOr() throws FilterParseException {
+ Filter filter = FilterBuilder.create().present("name.givenName").or(f -> f.present("name.familyName")).build();
+ assertThat(filter).isEqualTo(new Filter("name.givenName PR OR name.familyName PR"));
+ }
+
+ @Test
+ public void expressionThenTwoAnds() {
+ Filter filter = FilterBuilder.create().present("name.givenName").and(l -> l.present("name.familyName"), r -> r.present("name.middleName")).build();
+ assertThat(filter.getFilter()).isEqualTo("(name.givenName PR AND name.familyName PR) AND name.middleName PR");
+ }
+
+ @Test
+ public void expressionThenTwoOrs() {
+ Filter filter = FilterBuilder.create().present("name.givenName").or(l -> l.present("name.familyName"), r -> r.present("name.middleName")).build();
+ assertThat(filter.getFilter()).isEqualTo("(name.givenName PR OR name.familyName PR) OR name.middleName PR");
+ }
}
