-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add OR operator to QuerySpec constraints #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |||||||||||||||||||||||||||||||||||||
| * #%L | ||||||||||||||||||||||||||||||||||||||
| * Commons Backend - Model | ||||||||||||||||||||||||||||||||||||||
| * %% | ||||||||||||||||||||||||||||||||||||||
| * Copyright (C) 2020 - 2021 Flowing Code | ||||||||||||||||||||||||||||||||||||||
| * Copyright (C) 2020 - 2026 Flowing Code | ||||||||||||||||||||||||||||||||||||||
| * %% | ||||||||||||||||||||||||||||||||||||||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||||||||||||||||
| * you may not use this file except in compliance with the License. | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -19,12 +19,32 @@ | |||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| package com.flowingcode.backendcore.model; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| import com.flowingcode.backendcore.model.constraints.DisjunctionConstraint; | ||||||||||||||||||||||||||||||||||||||
| import com.flowingcode.backendcore.model.constraints.NegatedConstraint; | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| public interface Constraint { | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| default Constraint not() { | ||||||||||||||||||||||||||||||||||||||
| return new NegatedConstraint(this); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||
| * Returns a constraint that is satisfied when this constraint or any of the given constraints is | ||||||||||||||||||||||||||||||||||||||
| * satisfied (logical OR). | ||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||
| * @param first the first additional constraint | ||||||||||||||||||||||||||||||||||||||
| * @param rest optional additional constraints | ||||||||||||||||||||||||||||||||||||||
| * @return a {@link DisjunctionConstraint} combining this and the given constraints | ||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||
| default Constraint or(Constraint first, Constraint... rest) { | ||||||||||||||||||||||||||||||||||||||
| return DisjunctionConstraint.of(this, prepend(first, rest)); | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| private static Constraint[] prepend(Constraint first, Constraint[] rest) { | ||||||||||||||||||||||||||||||||||||||
| Constraint[] result = new Constraint[1 + rest.length]; | ||||||||||||||||||||||||||||||||||||||
| result[0] = first; | ||||||||||||||||||||||||||||||||||||||
| System.arraycopy(rest, 0, result, 1, rest.length); | ||||||||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+39
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Handle explicit Line 43 assumes Suggested fix import com.flowingcode.backendcore.model.constraints.DisjunctionConstraint;
import com.flowingcode.backendcore.model.constraints.NegatedConstraint;
+import java.util.Objects;
@@
default Constraint or(Constraint first, Constraint... rest) {
- return DisjunctionConstraint.of(this, prepend(first, rest));
+ return DisjunctionConstraint.of(this, prepend(first, Objects.requireNonNull(rest, "rest must not be null")));
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai please check if the comment has been addressed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
default Constraint or(Constraint first, Constraint... rest) {
return DisjunctionConstraint.of(this, prepend(first, rest));
}A call like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai Note that DisjunctionConstraint.of already checks whether the varargs is null There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 1759
So the null guard in That said, explicitly casting
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @coderabbitai I consider this acceptable behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /*- | ||
| * #%L | ||
| * Commons Backend - Model | ||
| * %% | ||
| * Copyright (C) 2020 - 2026 Flowing Code | ||
| * %% | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * #L% | ||
| */ | ||
| package com.flowingcode.backendcore.model.constraints; | ||
|
|
||
| import com.flowingcode.backendcore.model.Constraint; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import lombok.AccessLevel; | ||
| import lombok.Getter; | ||
| import lombok.NonNull; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.experimental.FieldDefaults; | ||
|
|
||
| /** A constraint that is satisfied when any of its member constraints is satisfied (logical OR). */ | ||
| @Getter | ||
| @RequiredArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @FieldDefaults(level = AccessLevel.PRIVATE, makeFinal = true) | ||
| public final class DisjunctionConstraint implements Constraint { | ||
|
|
||
| @NonNull List<Constraint> constraints; | ||
|
|
||
| public static DisjunctionConstraint of(Constraint first, Constraint... rest) { | ||
| List<Constraint> list = new java.util.ArrayList<>(); | ||
| Objects.requireNonNull(rest, "constraints must not be null"); | ||
| add(list, Objects.requireNonNull(first, "constraint must not be null")); | ||
| for (Constraint c : rest) { | ||
| add(list, Objects.requireNonNull(c, "constraint must not be null")); | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return new DisjunctionConstraint(List.copyOf(list)); | ||
| } | ||
|
|
||
| private static void add(List<Constraint> list, Constraint c) { | ||
| if (c instanceof DisjunctionConstraint) { | ||
|
Check warning on line 50 in backend-core-model/src/main/java/com/flowingcode/backendcore/model/constraints/DisjunctionConstraint.java
|
||
| list.addAll(((DisjunctionConstraint) c).constraints); | ||
| } else { | ||
| list.add(c); | ||
| } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| /*- | ||
| * #%L | ||
| * Commons Backend - Model | ||
| * %% | ||
| * Copyright (C) 2020 - 2026 Flowing Code | ||
| * %% | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * #L% | ||
| */ | ||
| package com.flowingcode.backendcore.model.constraints; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertSame; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| import com.flowingcode.backendcore.model.Constraint; | ||
| import com.flowingcode.backendcore.model.ConstraintBuilder; | ||
| import java.util.List; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| class DisjunctionConstraintTest { | ||
|
|
||
| private static final Constraint A = ConstraintBuilder.of("a").equal(1); | ||
| private static final Constraint B = ConstraintBuilder.of("b").equal(2); | ||
| private static final Constraint C = ConstraintBuilder.of("c").equal(3); | ||
|
|
||
| @Test | ||
| void testOfProducesCorrectMembers() { | ||
| DisjunctionConstraint d = DisjunctionConstraint.of(A, B, C); | ||
| assertEquals(List.of(A, B, C), d.getConstraints()); | ||
| } | ||
|
|
||
| @Test | ||
| void testChainedOrFlattens() { | ||
| // a.or(b).or(c) must produce OR(a, b, c), not OR(OR(a, b), c) | ||
| Constraint chained = A.or(B).or(C); | ||
| DisjunctionConstraint d = (DisjunctionConstraint) chained; | ||
| assertEquals(3, d.getConstraints().size()); | ||
| assertSame(A, d.getConstraints().get(0)); | ||
| assertSame(B, d.getConstraints().get(1)); | ||
| assertSame(C, d.getConstraints().get(2)); | ||
| } | ||
|
|
||
| @Test | ||
| void testOrWithExistingDisjunctionInRestFlattens() { | ||
| // DisjunctionConstraint passed as a rest element is also flattened | ||
| DisjunctionConstraint ab = DisjunctionConstraint.of(A, B); | ||
| DisjunctionConstraint d = DisjunctionConstraint.of(C, ab); | ||
| assertEquals(List.of(C, A, B), d.getConstraints()); | ||
| } | ||
|
|
||
| @Test | ||
| void testNullFirstThrows() { | ||
| assertThrows(NullPointerException.class, () -> DisjunctionConstraint.of(null, B)); | ||
| } | ||
|
|
||
| @Test | ||
| void testNullRestArrayThrows() { | ||
| assertThrows(NullPointerException.class, () -> DisjunctionConstraint.of(A, (Constraint[]) null)); | ||
| } | ||
|
|
||
| @Test | ||
| void testNullElementInRestThrows() { | ||
| assertThrows(NullPointerException.class, () -> DisjunctionConstraint.of(A, B, null)); | ||
| } | ||
|
|
||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: FlowingCode/backend-core
Length of output: 1805
🏁 Script executed:
Repository: FlowingCode/backend-core
Length of output: 1388
Make join reuse join-type-aware to preserve OR semantics.
The
join()method (lines 92-95) reuses any existing join by attribute name only. WhentransformDisjunctionConstraint()switchescurrentJoinTypetoLEFT(line 183), a previousINNERjoin with the same attribute name will be incorrectly reused, causing the OR evaluation to apply INNER join semantics (dropping nullable-association rows) before OR logic.The suggested patch correctly adds a join-type check to ensure reused joins match both the attribute name and the current join type.
Suggested patch
`@SuppressWarnings`("rawtypes") private From<?,?> join(From<?,?> source, String attributeName) { - Optional<Join> existingJoin = source.getJoins().stream().filter(join->join.getAttribute().getName().equals(attributeName)).map(join->(Join)join).findFirst(); - return existingJoin.orElseGet(()->source.join(attributeName, currentJoinType)); + Optional<Join> existingJoin = source.getJoins().stream() + .map(join -> (Join) join) + .filter(join -> join.getAttribute().getName().equals(attributeName)) + .filter(join -> join.getJoinType() == currentJoinType) + .findFirst(); + return existingJoin.orElseGet(() -> source.join(attributeName, currentJoinType)); }🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai The finding is technically correct — the filter on line 93 does not check join type — but it has no observable effect on query results. When a prior non-disjunction constraint has already created an INNER JOIN on an association, rows with a null foreign key are eliminated by that join before any WHERE predicate is evaluated, including the OR predicate from the disjunction. Conversely, when the disjunction runs first and creates a LEFT JOIN, any subsequent non-disjunction predicate on that association (e.g. city.population > 0) evaluates to NULL for no-city rows, which SQL treats as UNKNOWN and filters out — the same rows excluded by an INNER JOIN. In both orderings the final result set is identical. The fix would not change which rows are returned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javier-godoy, that's a well-reasoned explanation — thank you for the detailed walkthrough. You're right on both orderings:transformDisjunctionConstraintproduces no different outcome.UNKNOWNfor the null-padded rows (SQL three-valued logic), causing them to be silently excluded — effectively the same result as an INNER join would produce.I'll retract the suggestion. The code is functionally correct as written; the join-type mismatch has no observable effect on query results.
ᓚᘏᗢ