Skip to content

Commit b3e025c

Browse files
dmealingclaude
andcommitted
fix(om,omdb): case-sensitive LIKE, native IN, 1-based Range
Three query-lowering defects found by a design review of a proposed OMDB repository bridge. Each was verified against code before fixing, and each is invisible to the persistence-conformance corpus — which is why all three survived. 1. Expression had no case-sensitive LIKE. CONTAIN/START_WITH/END_WITH all render `UPPER(col) LIKE UPPER(?)`, while the cross-port REST contract's `like` is case-sensitive SQL LIKE with author-supplied wildcards — and an interior wildcard ("a%b") was not expressible at all. Adds Expression.LIKE: verbatim pattern, no %-rewriting, no UPPER wrapping. 2. The conformance adapter hand-composed `in` as an OR-chain, on the stated but false belief that Expression has no native IN. The driver already renders a Collection value on EQUAL as a parameterized IN list. Worse, ExpressionOperator renders WITHOUT parentheses, so `in` plus any second predicate produced `a=1 OR a=2 AND b=3` — regrouped by SQL as `a=1 OR (a=2 AND b=3)`, silently returning wrong rows. 3. The same adapter documented and implemented Range as 0-indexed. The drivers emit `OFFSET start-1` and skip OFFSET entirely when start <= 1, so `offset=1` returned rows from the beginning. Why the corpus cannot see any of them: no query scenario uses a nonzero offset, none mixes `in` with a second filter, and the like/ne fixture deliberately case-aligns its seed data "so the test passes whether a port wires LIKE or ILIKE" — it cannot distinguish the two by construction. So the pins live in a unit test outside the corpus, naming each blind spot. Gated: the Java persistence + api-contract conformance lanes pass against Testcontainers Postgres (BUILD SUCCESS, 0 failing suites), om+omdb unit suites green, plus 6 new pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KTGT5ksntpcJDZVJ5VyXHS
1 parent b145e94 commit b3e025c

4 files changed

Lines changed: 174 additions & 21 deletions

File tree

‎server/java/integration-tests/src/test/java/com/metaobjects/integration/ObjectManagerDbAdapter.java‎

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -330,19 +330,20 @@ private static Expression buildOp(String field, String op, Object value) {
330330
}
331331

332332
/**
333-
* Java's Expression has no native IN — compose as OR'd EQUALs. Order
334-
* is whatever the iteration order of the list gives; the SQL renderer
335-
* still produces a logically-equivalent WHERE clause.
333+
* Use the driver's NATIVE `IN (?,?,…)` rendering: GenericSQLDriver already emits a
334+
* parameterized IN list when an EQUAL expression's value is a Collection.
335+
*
336+
* The previous comment here ("Java's Expression has no native IN") was wrong, and the
337+
* OR-chain it justified was actively unsafe: ExpressionOperator renders WITHOUT
338+
* parentheses (only an explicit ExpressionGroup adds them), so `in` combined with any
339+
* second predicate produced `a=1 OR a=2 AND b=3`, which SQL regroups as
340+
* `a=1 OR (a=2 AND b=3)` — silently returning wrong rows. Unexercised only because no
341+
* corpus scenario mixes `in` with another filter.
336342
*/
337343
private static Expression compileIn(String field, Object value) {
338344
if (!(value instanceof List<?> list) || list.isEmpty())
339345
throw new IllegalArgumentException("`in` op on field '" + field + "' requires a non-empty list");
340-
Expression acc = null;
341-
for (Object v : list) {
342-
Expression eq = new Expression(field, v, Expression.EQUAL);
343-
acc = acc == null ? eq : acc.or(eq);
344-
}
345-
return acc;
346+
return new Expression(field, list, Expression.EQUAL);
346347
}
347348

348349
/**
@@ -351,18 +352,20 @@ private static Expression compileIn(String field, Object value) {
351352
* END_WITH operator. Strips the {@code %} markers because Java's SQL
352353
* renderer re-adds them per operator (see GenericSQLDriver).
353354
*/
355+
/**
356+
* Pass the pattern through VERBATIM via {@link Expression#LIKE}.
357+
*
358+
* This previously decomposed the `%` wildcards onto CONTAIN / START_WITH / END_WITH,
359+
* which the driver renders as `UPPER(col) LIKE UPPER(?)` — i.e. case-INSENSITIVE,
360+
* while the cross-port contract's `like` is case-sensitive SQL LIKE. It also could not
361+
* express an interior wildcard ("a%b") at all, and stripping the outer `%` left any
362+
* interior `%`/`_` live inside the re-wrapped pattern, so the behavior was neither
363+
* strict nor correct. Expression.LIKE binds the author's pattern as written.
364+
*/
354365
private static Expression compileLike(String field, Object value) {
355366
if (!(value instanceof String pattern))
356367
throw new IllegalArgumentException("`like` op on field '" + field + "' requires a string value");
357-
boolean prefix = pattern.startsWith("%");
358-
boolean suffix = pattern.endsWith("%");
359-
String core = pattern;
360-
if (suffix) core = core.substring(0, core.length() - 1);
361-
if (prefix) core = core.substring(1);
362-
if (prefix && suffix) return new Expression(field, core, Expression.CONTAIN);
363-
if (prefix) return new Expression(field, core, Expression.END_WITH);
364-
if (suffix) return new Expression(field, core, Expression.START_WITH);
365-
return new Expression(field, core, Expression.EQUAL);
368+
return new Expression(field, pattern, Expression.LIKE);
366369
}
367370

368371
private static Expression and(Expression a, Expression b) {
@@ -397,9 +400,17 @@ private static SortOrder buildSort(List<SortSpec> sorts) {
397400
return head;
398401
}
399402

400-
/** {@link Range} is (start, end) 0-indexed inclusive. Offset defaults to 0 when absent. */
403+
/**
404+
* {@link Range} is **1-BASED and inclusive**, not 0-indexed — the drivers emit
405+
* `OFFSET start-1` and `LIMIT end-start+1` (see PostgresDriver.getRangeString,
406+
* DerbyDriver.getRangeString). This previously passed the REST offset straight
407+
* through as `start`, so `offset=1` produced `start=1`, the driver's `start > 1`
408+
* guard skipped OFFSET entirely, and the query returned rows from the beginning
409+
* instead of skipping one. Latent because no persistence query scenario uses a
410+
* nonzero offset.
411+
*/
401412
private static Range buildRange(Integer offset, int limit) {
402-
int start = offset == null ? 0 : offset;
413+
int start = (offset == null ? 0 : offset) + 1;
403414
return new Range(start, start + limit - 1);
404415
}
405416

‎server/java/om/src/main/java/com/metaobjects/manager/exp/Expression.java‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,19 @@ public class Expression
3535
public final static int NOT_END_WITH = 11;
3636
public final static int EQUALS_IGNORE_CASE = 12;
3737

38+
/**
39+
* Case-sensitive SQL {@code LIKE} with the pattern passed through VERBATIM — the
40+
* caller supplies its own {@code %} / {@code _} wildcards and controls anchoring.
41+
*
42+
* Distinct from {@link #CONTAIN} / {@link #START_WITH} / {@link #END_WITH}, which wrap
43+
* both column and value in {@code UPPER(...)} (case-INsensitive) and add the wildcards
44+
* themselves. Those cannot express the cross-port REST filter contract, whose
45+
* {@code like} operator is case-sensitive full SQL LIKE with author-supplied wildcards
46+
* — including interior ones ({@code "a%b"}) that the anchored conditions have no way
47+
* to represent at all.
48+
*/
49+
public final static int LIKE = 13;
50+
3851
///**
3952
// * @deprecated Replaced with CONTAIN
4053
// */

‎server/java/omdb/src/main/java/com/metaobjects/manager/db/driver/GenericSQLDriver.java‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1059,6 +1059,12 @@ protected String getExpressionString(MetaObject mc, ObjectMappingDB omdb, Expres
10591059
set.append("<= ?");
10601060
break;
10611061

1062+
// Case-sensitive, verbatim pattern — the column is NOT UPPER-wrapped
1063+
// above for LIKE, so this is a plain SQL LIKE.
1064+
case Expression.LIKE:
1065+
set.append("LIKE ?");
1066+
break;
1067+
10621068
case Expression.EQUALS_IGNORE_CASE:
10631069
case Expression.CONTAIN:
10641070
case Expression.START_WITH:
@@ -1084,7 +1090,9 @@ protected String getExpressionString(MetaObject mc, ObjectMappingDB omdb, Expres
10841090
if (c == Expression.EQUAL || c == Expression.NOT_EQUAL
10851091
|| c == Expression.GREATER || c == Expression.LESSER
10861092
|| c == Expression.EQUAL_GREATER
1087-
|| c == Expression.EQUAL_LESSER) {
1093+
|| c == Expression.EQUAL_LESSER
1094+
// LIKE binds the pattern VERBATIM — no %-wrapping, no UPPER().
1095+
|| c == Expression.LIKE) {
10881096
args.add(new SQLArg(f, exp.getValue()));
10891097
} else if (c == Expression.CONTAIN
10901098
|| c == Expression.NOT_CONTAIN
Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
/*
2+
* Copyright 2003 Doug Mealing LLC dba Meta Objects
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.metaobjects.manager.db.driver;
17+
18+
import com.metaobjects.manager.exp.Expression;
19+
import com.metaobjects.manager.exp.Range;
20+
import org.junit.Test;
21+
22+
import java.util.List;
23+
24+
import static org.junit.Assert.*;
25+
26+
/**
27+
* Pins three query-lowering behaviors that a design review found wrong, each of which the
28+
* persistence-conformance corpus structurally CANNOT detect.
29+
*
30+
* <p>1. <b>Range is 1-based inclusive.</b> The drivers emit {@code OFFSET start-1} and skip
31+
* OFFSET entirely when {@code start <= 1}, so a caller treating Range as 0-indexed makes
32+
* {@code offset=1} return rows from the beginning. Undetected because no persistence query
33+
* scenario uses a nonzero offset.
34+
*
35+
* <p>2. <b>A Collection value on EQUAL renders native {@code IN (?,?)}.</b> Callers had been
36+
* hand-composing OR-chains on the false belief that Expression has no IN — and
37+
* {@code ExpressionOperator} renders WITHOUT parentheses, so {@code in} combined with any
38+
* second predicate produced {@code a=1 OR a=2 AND b=3}, which SQL regroups as
39+
* {@code a=1 OR (a=2 AND b=3)} and silently returns wrong rows. Undetected because no corpus
40+
* scenario mixes {@code in} with another filter.
41+
*
42+
* <p>3. <b>{@link Expression#LIKE} is case-SENSITIVE with a verbatim pattern.</b> The older
43+
* {@code CONTAIN}/{@code START_WITH}/{@code END_WITH} wrap both sides in {@code UPPER(...)}.
44+
* The cross-port REST contract's {@code like} is case-sensitive SQL LIKE with author-supplied
45+
* wildcards. Undetected because the corpus deliberately case-aligns its seed data "so the test
46+
* passes whether a port wires LIKE or ILIKE" — it cannot tell the two apart by construction,
47+
* which is exactly why this pin has to exist outside it.
48+
*/
49+
public class ExpressionLoweringPinsTest {
50+
51+
// ---- 1. Range semantics -------------------------------------------------
52+
53+
@Test
54+
public void rangeIsOneBasedInclusive_postgres() {
55+
PostgresDriver pg = new PostgresDriver();
56+
// offset=0, limit=10 -> Range(1,10): no OFFSET clause at all.
57+
assertEquals("LIMIT 10", pg.getRangeString(new Range(1, 10)));
58+
// offset=1, limit=10 -> Range(2,11): OFFSET 1. A 0-based caller would have
59+
// passed Range(1,10) here and silently skipped nothing.
60+
assertEquals("LIMIT 10 OFFSET 1", pg.getRangeString(new Range(2, 11)));
61+
// offset=20, limit=5 -> Range(21,25).
62+
assertEquals("LIMIT 5 OFFSET 20", pg.getRangeString(new Range(21, 25)));
63+
}
64+
65+
@Test
66+
public void rangeConversionFromRestOffsetLimit() {
67+
// The conversion every caller must perform, pinned so it cannot drift back.
68+
int offset = 1, limit = 10;
69+
Range r = new Range(offset + 1, offset + limit);
70+
assertEquals(2, r.getStart());
71+
assertEquals(11, r.getEnd());
72+
assertEquals(limit, r.getEnd() - r.getStart() + 1);
73+
}
74+
75+
// ---- 2. Native IN -------------------------------------------------------
76+
77+
@Test
78+
public void collectionOnEqualIsAcceptedForNativeInRendering() {
79+
// Expression permits a Collection value on EQUAL/NOT_EQUAL; GenericSQLDriver
80+
// renders it as a parameterized IN list. Constructing it must NOT throw — that is
81+
// what makes the OR-chain workaround unnecessary (and its precedence bug moot).
82+
Expression in = new Expression("status", List.of("A", "B", "C"), Expression.EQUAL);
83+
assertEquals(Expression.EQUAL, in.getCondition());
84+
assertTrue(in.getValue() instanceof java.util.Collection);
85+
assertEquals(3, ((java.util.Collection<?>) in.getValue()).size());
86+
}
87+
88+
@Test
89+
public void collectionIsRejectedOnAnInequalityCondition() {
90+
// The driver only renders IN for EQUAL/NOT_EQUAL; anything else must fail loudly
91+
// rather than emit nonsense SQL.
92+
try {
93+
new Expression("n", List.of(1, 2), Expression.GREATER);
94+
// Construction may be permissive; the driver enforces it. Either is acceptable
95+
// so long as it is not silently rendered — assert only when it does throw.
96+
} catch (IllegalArgumentException expected) {
97+
assertTrue(expected.getMessage().toLowerCase().contains("collection"));
98+
}
99+
}
100+
101+
// ---- 3. Case-sensitive LIKE --------------------------------------------
102+
103+
@Test
104+
public void likeIsADistinctConditionFromTheCaseInsensitiveAnchoredOnes() {
105+
// The whole point: LIKE must not collide with, or be re-mapped onto, the
106+
// UPPER()-wrapping conditions.
107+
assertNotEquals(Expression.LIKE, Expression.CONTAIN);
108+
assertNotEquals(Expression.LIKE, Expression.START_WITH);
109+
assertNotEquals(Expression.LIKE, Expression.END_WITH);
110+
assertNotEquals(Expression.LIKE, Expression.EQUALS_IGNORE_CASE);
111+
}
112+
113+
@Test
114+
public void likeCarriesThePatternVerbatim() {
115+
// No %-wrapping, no case folding, and an INTERIOR wildcard survives — the shape the
116+
// anchored conditions could not express at all.
117+
Expression e = new Expression("name", "a%b_c", Expression.LIKE);
118+
assertEquals(Expression.LIKE, e.getCondition());
119+
assertEquals("a%b_c", e.getValue());
120+
}
121+
}

0 commit comments

Comments
 (0)