diff --git a/client/src/com/aerospike/client/exp/Exp.java b/client/src/com/aerospike/client/exp/Exp.java index 4c2e799cf..186bc14d0 100644 --- a/client/src/com/aerospike/client/exp/Exp.java +++ b/client/src/com/aerospike/client/exp/Exp.java @@ -1723,9 +1723,13 @@ private ListVal(List list) { public void pack(Packer packer) { // List values need an extra array and QUOTED in order to distinguish // between a multiple argument array call and a local list. + // Value literals must be in canonical form (AER-6930): unordered maps + // at any depth are packed with keys sorted in server msgpack order. + packer.sortMaps(true); packer.packArrayBegin(2); packer.packInt(QUOTED); packer.packList(list); + packer.sortMaps(false); } } @@ -1738,7 +1742,11 @@ private MapVal(Map map) { @Override public void pack(Packer packer) { + // Value literals must be in canonical form (AER-6930): unordered maps + // at any depth are packed with keys sorted in server msgpack order. + packer.sortMaps(true); packer.packMap(map); + packer.sortMaps(false); } } diff --git a/client/src/com/aerospike/client/util/Packer.java b/client/src/com/aerospike/client/util/Packer.java index c34ef4a64..bfdee5a29 100644 --- a/client/src/com/aerospike/client/util/Packer.java +++ b/client/src/com/aerospike/client/util/Packer.java @@ -19,12 +19,14 @@ import static com.aerospike.client.Value.MapValue.getMapOrder; import java.nio.ByteBuffer; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.UUID; import com.aerospike.client.AerospikeException; +import com.aerospike.client.ResultCode; import com.aerospike.client.Value; import com.aerospike.client.cdt.MapOrder; import com.aerospike.client.command.Buffer; @@ -91,11 +93,22 @@ public static byte[] pack(List> val, MapOrder order) { private byte[] buffer; private int offset; + private boolean sortMaps; public Packer() { // Default to null buffer in estimate buffer size mode. } + /** + * Pack unordered maps at any depth with entries sorted by key in the server's + * canonical msgpack order, without adding an order flag ext header. Servers + * that include AER-6930 (8.1.2.3+) require map value literals in expressions + * to be in canonical form. Default is false. + */ + public void sortMaps(boolean sortMaps) { + this.sortMaps = sortMaps; + } + public void packValueArray(Value[] values) { packArrayBegin(values.length); for (Value value : values) { @@ -131,6 +144,11 @@ else if (size < 65536) { public void packValueMap(Map map) { MapOrder order = getMapOrder(map); + + if (sortMaps && buffer != null && order == MapOrder.UNORDERED && map.size() > 1) { + packMapCanonical(map.entrySet(), map.size()); + return; + } packMapBegin(map.size(), order); for (Entry entry : map.entrySet()) { @@ -145,6 +163,10 @@ public void packMap(Map map) { } public void packMap(Map map, MapOrder order) { + if (sortMaps && buffer != null && order == MapOrder.UNORDERED && map.size() > 1) { + packMapCanonical(map.entrySet(), map.size()); + return; + } packMapBegin(map.size(), order); for (Entry entry : map.entrySet()) { @@ -154,6 +176,10 @@ public void packMap(Map map, MapOrder order) { } public void packMap(List> list, MapOrder order) { + if (sortMaps && buffer != null && order == MapOrder.UNORDERED && list.size() > 1) { + packMapCanonical(list, list.size()); + return; + } packMapBegin(list.size(), order); for (Entry entry : list) { @@ -162,6 +188,435 @@ public void packMap(List> list, MapOrder order) { } } + // Only called in the write pass (buffer != null). The packed size is + // independent of entry order, so the estimate pass uses the plain packMap + // body and the canonicalization work (key serialization, sort, duplicate + // check) runs exactly once per map. + private void packMapCanonical(Iterable> entries, int size) { + final byte[][] keys = new byte[size][]; + Object[] values = new Object[size]; + Integer[] ranks = new Integer[size]; + int i = 0; + + for (Entry entry : entries) { + Packer packer = new Packer(); + packer.sortMaps = true; + packer.packObject(entry.getKey()); + packer.createBuffer(); + packer.packObject(entry.getKey()); + keys[i] = packer.getBuffer(); + values[i] = entry.getValue(); + ranks[i] = i; + i++; + } + + CanonicalCompare c0 = new CanonicalCompare(); + CanonicalCompare c1 = new CanonicalCompare(); + + Arrays.sort(ranks, (a, b) -> { + c0.reset(keys[a]); + c1.reset(keys[b]); + return CanonicalCompare.compareElement(c0, c1); + }); + + for (i = 1; i < size; i++) { + c0.reset(keys[ranks[i - 1]]); + c1.reset(keys[ranks[i]]); + + if (CanonicalCompare.compareElement(c0, c1) == 0) { + throw new AerospikeException(ResultCode.PARAMETER_ERROR, + "Map keys pack to duplicate msgpack keys in expression map literal"); + } + } + + packMapBegin(size); + + for (i = 0; i < size; i++) { + byte[] key = keys[ranks[i]]; + packByteArray(key, 0, key.length); + packObject(values[ranks[i]]); + } + } + + /** + * Compare packed msgpack elements using the same ordering as the server's + * msgpack_cmp (cf/src/msgpack_in.c). + */ + private static final class CanonicalCompare { + // Type ranks from the server's msgpack_type enum (cf/include/msgpack_in.h). + private static final int TYPE_NIL = 1; + private static final int TYPE_FALSE = 2; + private static final int TYPE_TRUE = 3; + private static final int TYPE_NEGINT = 4; + private static final int TYPE_INT = 5; + private static final int TYPE_STRING = 6; + private static final int TYPE_LIST = 7; + private static final int TYPE_MAP = 8; + private static final int TYPE_BYTES = 9; + private static final int TYPE_DOUBLE = 10; + private static final int TYPE_GEOJSON = 11; + private static final int TYPE_EXT = 12; + private static final int TYPE_WILDCARD = 13; + private static final int TYPE_INF = 14; + + private byte[] buf; + private int offset; + + // State of most recently parsed element. + private int type; + private long iNum; + private double dNum; + private int dataOffset; + private int dataLen; + private int count; + + private void reset(byte[] buf) { + this.buf = buf; + this.offset = 0; + } + + private static int compareElement(CanonicalCompare c0, CanonicalCompare c1) { + c0.parse(); + c1.parse(); + + if (c0.type == TYPE_WILDCARD || c1.type == TYPE_WILDCARD) { + c0.skipParsed(); + c1.skipParsed(); + return 0; + } + + if (c0.type != c1.type) { + return Integer.compare(c0.type, c1.type); + } + + switch (c0.type) { + case TYPE_NEGINT: + case TYPE_INT: + return Long.compareUnsigned(c0.iNum, c1.iNum); + + case TYPE_STRING: + case TYPE_BYTES: + case TYPE_GEOJSON: + case TYPE_EXT: { + int len = Math.min(c0.dataLen, c1.dataLen); + + for (int i = 0; i < len; i++) { + int cmp = (c0.buf[c0.dataOffset + i] & 0xff) - (c1.buf[c1.dataOffset + i] & 0xff); + + if (cmp != 0) { + return cmp; + } + } + return Integer.compare(c0.dataLen, c1.dataLen); + } + + case TYPE_LIST: { + int n0 = c0.count; + int n1 = c1.count; + int n = Math.min(n0, n1); + + for (int i = 0; i < n; i++) { + int cmp = compareElement(c0, c1); + + if (cmp != 0) { + return cmp; + } + } + return Integer.compare(n0, n1); + } + + case TYPE_MAP: { + if (c0.count != c1.count) { + return Integer.compare(c0.count, c1.count); + } + + int n = c0.count * 2; + + for (int i = 0; i < n; i++) { + int cmp = compareElement(c0, c1); + + if (cmp != 0) { + return cmp; + } + } + return 0; + } + + case TYPE_DOUBLE: + // Match C comparison semantics. Do not use Double.compare which + // orders -0.0 before 0.0 and NaN above all values. + if (c0.dNum > c1.dNum) { + return 1; + } + if (c0.dNum < c1.dNum) { + return -1; + } + return 0; + + default: + // NIL, FALSE, TRUE and INF have no payload. + return 0; + } + } + + private void skipParsed() { + int n; + + switch (type) { + case TYPE_LIST: + n = count; + break; + case TYPE_MAP: + n = count * 2; + break; + default: + return; + } + + for (int i = 0; i < n; i++) { + parse(); + skipParsed(); + } + } + + private void parse() { + int b = buf[offset++] & 0xff; + + switch (b) { + case 0xc0: + type = TYPE_NIL; + return; + case 0xc2: + type = TYPE_FALSE; + return; + case 0xc3: + type = TYPE_TRUE; + return; + + case 0xcc: + iNum = buf[offset++] & 0xff; + type = TYPE_INT; + return; + case 0xcd: + iNum = readUint(2); + type = TYPE_INT; + return; + case 0xce: + iNum = readUint(4); + type = TYPE_INT; + return; + case 0xcf: + iNum = readUint(8); + type = TYPE_INT; + return; + + case 0xd0: + iNum = buf[offset++]; + type = (iNum < 0) ? TYPE_NEGINT : TYPE_INT; + return; + case 0xd1: + iNum = readSint(2); + type = (iNum < 0) ? TYPE_NEGINT : TYPE_INT; + return; + case 0xd2: + iNum = readSint(4); + type = (iNum < 0) ? TYPE_NEGINT : TYPE_INT; + return; + case 0xd3: + iNum = readSint(8); + type = (iNum < 0) ? TYPE_NEGINT : TYPE_INT; + return; + + case 0xca: + dNum = Float.intBitsToFloat((int)readUint(4)); + type = TYPE_DOUBLE; + return; + case 0xcb: + dNum = Double.longBitsToDouble(readUint(8)); + type = TYPE_DOUBLE; + return; + + case 0xc4: + case 0xd9: + setRaw(buf[offset++] & 0xff); + return; + case 0xc5: + case 0xda: + setRaw((int)readUint(2)); + return; + case 0xc6: + case 0xdb: + setRaw((int)readUint(4)); + return; + + case 0xdc: + count = (int)readUint(2); + type = TYPE_LIST; + return; + case 0xdd: + count = (int)readUint(4); + type = TYPE_LIST; + return; + case 0xde: + count = (int)readUint(2); + type = TYPE_MAP; + return; + case 0xdf: + count = (int)readUint(4); + type = TYPE_MAP; + return; + + case 0xd4: { + int extType = buf[offset++] & 0xff; + + if (extType == 0xff) { + int val = buf[offset] & 0xff; + + if (val == 0x00) { + offset++; + type = TYPE_WILDCARD; + return; + } + + if (val == 0x01) { + offset++; + type = TYPE_INF; + return; + } + } + dataOffset = offset++; + dataLen = 1; + type = TYPE_EXT; + return; + } + case 0xd5: + setExt(2); + return; + case 0xd6: + setExt(4); + return; + case 0xd7: + setExt(8); + return; + case 0xd8: + setExt(16); + return; + case 0xc7: { + int len = buf[offset++] & 0xff; + int extType = buf[offset++] & 0xff; + + if (extType == 0xff && len == 1) { + int val = buf[offset] & 0xff; + + if (val == 0x00) { + offset++; + type = TYPE_WILDCARD; + return; + } + + if (val == 0x01) { + offset++; + type = TYPE_INF; + return; + } + } + dataOffset = offset; + dataLen = len; + offset += len; + type = TYPE_EXT; + return; + } + case 0xc8: + setExt((int)readUint(2)); + return; + case 0xc9: + setExt((int)readUint(4)); + return; + + default: + if (b < 0x80) { + iNum = b; + type = TYPE_INT; + return; + } + + if (b >= 0xe0) { + iNum = (byte)b; + type = TYPE_NEGINT; + return; + } + + if ((b & 0xe0) == 0xa0) { + setRaw(b & 0x1f); + return; + } + + if ((b & 0xf0) == 0x80) { + count = b & 0x0f; + type = TYPE_MAP; + return; + } + + if ((b & 0xf0) == 0x90) { + count = b & 0x0f; + type = TYPE_LIST; + return; + } + throw new AerospikeException(ResultCode.PARAMETER_ERROR, "Unexpected msgpack header: " + b); + } + } + + private void setRaw(int len) { + dataOffset = offset; + dataLen = len; + offset += len; + + if (len == 0) { + type = TYPE_BYTES; + return; + } + + switch (buf[dataOffset] & 0xff) { + case ParticleType.STRING: + type = TYPE_STRING; + return; + case ParticleType.GEOJSON: + type = TYPE_GEOJSON; + return; + default: + type = TYPE_BYTES; + return; + } + } + + private void setExt(int len) { + // The ext type byte is not compared by the server. + offset++; + dataOffset = offset; + dataLen = len; + offset += len; + type = TYPE_EXT; + } + + private long readUint(int size) { + long val = 0; + + for (int i = 0; i < size; i++) { + val = (val << 8) | (buf[offset++] & 0xff); + } + return val; + } + + private long readSint(int size) { + long val = buf[offset++]; + + for (int i = 1; i < size; i++) { + val = (val << 8) | (buf[offset++] & 0xff); + } + return val; + } + } + public void packMapBegin(int size, MapOrder order) { if (order == MapOrder.UNORDERED) { packMapBegin(size); diff --git a/test/src/com/aerospike/test/sync/basic/TestListExp.java b/test/src/com/aerospike/test/sync/basic/TestListExp.java index 36cf18584..2807a4c1b 100644 --- a/test/src/com/aerospike/test/sync/basic/TestListExp.java +++ b/test/src/com/aerospike/test/sync/basic/TestListExp.java @@ -17,10 +17,15 @@ package com.aerospike.test.sync.basic; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import java.util.ArrayList; +import java.util.Arrays; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import org.junit.Before; import org.junit.Test; @@ -33,6 +38,7 @@ import com.aerospike.client.cdt.CTX; import com.aerospike.client.cdt.ListOperation; import com.aerospike.client.cdt.ListPolicy; +import com.aerospike.client.cdt.MapOrder; import com.aerospike.client.exp.Exp; import com.aerospike.client.exp.ExpOperation; import com.aerospike.client.exp.ExpReadFlags; @@ -40,6 +46,7 @@ import com.aerospike.client.exp.Expression; import com.aerospike.client.exp.ListExp; import com.aerospike.client.policy.Policy; +import com.aerospike.client.util.Packer; import com.aerospike.test.sync.TestSync; public class TestListExp extends TestSync { @@ -150,4 +157,135 @@ public void expReturnsList() { List results2 = record.getList("var"); assertEquals(4, results2.size()); } + + @Test + public void appendItemsUnsortedMapLiteral() { + // CLIENT-5039: server 8.1.2.3+ (AER-6930) rejects expression map literals + // that are not in canonical (key sorted) form. + client.operate(null, keyA, + ListOperation.appendItems(ListPolicy.Default, binA, + Arrays.asList(Value.get(0), Value.get(1)))); + + // LinkedHashMap is an unordered (non-SortedMap) HashMap with a + // deterministic, deliberately unsorted iteration order. + Map map = new LinkedHashMap<>(); + map.put("zz", 4L); + map.put("aa", 1L); + map.put("mm", 2L); + map.put("cc", 3L); + + Expression exp = Exp.build( + ListExp.size( + ListExp.appendItems(ListPolicy.Default, Exp.val(Arrays.asList((Object)map)), + Exp.listBin(binA)))); + + Record record = client.operate(null, keyA, + ExpOperation.read("result", exp, ExpReadFlags.DEFAULT)); + + assertEquals(3, record.getLong("result")); + } + + @Test + public void appendItemsUnsortedIntKeyMapLiteral() { + // Exact CLIENT-5039 ticket repro: integer keys in non-sorted order. + client.operate(null, keyA, + ListOperation.appendItems(ListPolicy.Default, binA, + Arrays.asList(Value.get(0), Value.get(1)))); + + Map map = new LinkedHashMap<>(); + map.put(1402L, 1802L); + map.put(2003L, 3946L); + map.put(834L, 1374L); + map.put(3117L, 1295L); + + Expression exp = Exp.build( + ListExp.appendItems(ListPolicy.Default, Exp.val(Arrays.asList((Object)map)), + Exp.listBin(binA))); + + Record record = client.operate(null, keyA, + ExpOperation.read("result", exp, ExpReadFlags.DEFAULT)); + + assertEquals(3, record.getList("result").size()); + } + + @Test + public void nestedMapLiteralPacksCanonical() { + Map inner = new LinkedHashMap<>(); + inner.put("z", 26L); + inner.put("a", 1L); + + Map innerSorted = new LinkedHashMap<>(); + innerSorted.put("a", 1L); + innerSorted.put("z", 26L); + + // Maps nested in list literals canonicalize at any depth. + assertArrayEquals( + Exp.build(Exp.val(Arrays.asList((Object)0L, inner))).getBytes(), + Exp.build(Exp.val(Arrays.asList((Object)0L, innerSorted))).getBytes()); + + // Maps nested as values of single-key maps canonicalize too. + Map outer = new LinkedHashMap<>(); + outer.put("k", inner); + + Map outerSorted = new LinkedHashMap<>(); + outerSorted.put("k", innerSorted); + + assertArrayEquals( + Exp.build(Exp.val(outer)).getBytes(), + Exp.build(Exp.val(outerSorted)).getBytes()); + } + + @Test + public void unsortedMapLiteralPacksCanonical() { + Map unsorted = new LinkedHashMap<>(); + unsorted.put("z", 26L); + unsorted.put(5L, "five"); + unsorted.put("a", 1L); + unsorted.put(-3L, "neg"); + + Map sorted = new LinkedHashMap<>(); + sorted.put(-3L, "neg"); + sorted.put(5L, "five"); + sorted.put("a", 1L); + sorted.put("z", 26L); + + // Canonicalization is deterministic and adds no order header, so packed + // bytes match a same-content map inserted in canonical order. + assertArrayEquals( + Exp.build(Exp.val(unsorted)).getBytes(), + Exp.build(Exp.val(sorted)).getBytes()); + } + + @Test + public void operationPathPreservesInsertionOrder() { + Map map = new LinkedHashMap<>(); + map.put("z", 26L); + map.put("a", 1L); + + Map reversed = new LinkedHashMap<>(); + reversed.put("a", 1L); + reversed.put("z", 26L); + + // Non-expression packing (record writes, CDT operation arguments) must + // keep map iteration order and stay byte-identical to previous releases. + assertFalse(Arrays.equals( + Packer.pack(map, MapOrder.UNORDERED), + Packer.pack(reversed, MapOrder.UNORDERED))); + } + + @Test + public void appendItemsOperationUnsortedMap() { + Map map = new LinkedHashMap<>(); + map.put("z", 26L); + map.put("a", 1L); + map.put("m", 13L); + + client.operate(null, keyB, + ListOperation.appendItems(ListPolicy.Default, binB, + Arrays.asList(Value.get(0), Value.get(map)))); + + Record record = client.get(null, keyB, binB); + assertRecordFound(keyB, record); + assertEquals(2, record.getList(binB).size()); + } } diff --git a/test/src/com/aerospike/test/sync/basic/TestMapExp.java b/test/src/com/aerospike/test/sync/basic/TestMapExp.java index 4188eacaf..398dcec06 100644 --- a/test/src/com/aerospike/test/sync/basic/TestMapExp.java +++ b/test/src/com/aerospike/test/sync/basic/TestMapExp.java @@ -21,6 +21,7 @@ import static org.junit.Assert.fail; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import java.util.TreeMap; @@ -66,6 +67,56 @@ public void sortedMapEquality() { } } + @Test + public void unsortedMapLiteral() { + // CLIENT-5039: unordered multi-key map literals must be canonicalized or + // server 8.1.2.3+ (AER-6930) rejects the expression with PARAMETER_ERROR. + // LinkedHashMap is an unordered (non-SortedMap) map with a deterministic, + // deliberately unsorted iteration order. + Map map = new LinkedHashMap<>(); + map.put("key5", "a"); + map.put("key1", "e"); + map.put("key4", "b"); + map.put("key2", "d"); + map.put("key3", "c"); + + Key key = new Key(args.namespace, args.set, "usml"); + client.put(null, key, new Bin("m", "unused")); + + Expression exp = Exp.build(MapExp.size(Exp.val(map))); + + Record rec = client.operate(null, key, ExpOperation.read("sz", exp, ExpReadFlags.DEFAULT)); + assertRecordFound(key, rec); + assertEquals(5L, rec.getLong("sz")); + } + + @Test + public void nestedUnsortedMapLiteral() { + Map inner = new LinkedHashMap<>(); + inner.put(1402L, 1802L); + inner.put(834L, 1374L); + + Map map = new LinkedHashMap<>(); + map.put("z", inner); + map.put("a", 1L); + + Key key = new Key(args.namespace, args.set, "nusml"); + client.put(null, key, new Bin("m", "unused")); + + // Look up the nested map inside the literal — the whole literal, at every + // depth, must be in canonical form for the server to accept it. + Expression exp = Exp.build( + MapExp.getByKey(MapReturnType.VALUE, Exp.Type.MAP, Exp.val("z"), Exp.val(map))); + + Record rec = client.operate(null, key, ExpOperation.read("res", exp, ExpReadFlags.DEFAULT)); + assertRecordFound(key, rec); + + Map expected = new HashMap<>(); + expected.put(1402L, 1802L); + expected.put(834L, 1374L); + assertEquals(expected, rec.getMap("res")); + } + @Test public void invertedMapExp() { HashMap map = new HashMap<>();