diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/HttpObject.java b/lib/src/main/java/io/ably/pubsub/liveobjects/HttpObject.java index 08be06ace..03b1a8523 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/HttpObject.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/HttpObject.java @@ -70,7 +70,8 @@ public interface HttpObject { * * @param params the request params, built with {@link io.ably.pubsub.liveobjects.http.HttpObjectGetParams}, * or {@code null} to fetch the channel object. - * @return the compact representation, or {@code null} if the response has no body. + * @return the compact representation, or {@code null} if the response has no body; a JSON + * {@code null} value is {@link com.google.gson.JsonNull}. * @throws AblyException if the request fails or the response can't be decoded. */ @Blocking diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/DefaultHttpObject.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/DefaultHttpObject.java index 921cb834a..daa701f97 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/DefaultHttpObject.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/DefaultHttpObject.java @@ -102,7 +102,8 @@ public void getAsync(@Nullable Param[] params, @NotNull Callback compactImpl(@Nullable Param[] params) { // compact is the server default, so it isn't sent (RSO2d) - return getImpl(params, null, body -> body == null ? null : HttpObjectCodec.toJson(body)); + // a JSON null body is JsonNull, distinct from no body (null) + return getImpl(params, null, HttpObjectCodec::toJson); } /** RSO2g - the full body decoded into typed results. */ @@ -113,9 +114,11 @@ private HttpRequester.Request fullImpl(@Nullable Param[ private HttpRequester.Request getImpl(@Nullable Param[] params, @Nullable Param format, BodyDecoder decoder) { - // RSO2c - the objectId, if any, is a percent-encoded path segment + // RSO2c - the objectId, if any, is a percent-encoded path segment; an empty one is treated + // as absent, as in ably-js, rather than producing a trailing slash String objectId = Param.getFirst(params, HttpObjectGetParams.OBJECT_ID); - final String path = objectId != null ? basePath + "/" + HttpUtils.encodeURIComponent(objectId) : basePath; + final String path = objectId != null && !objectId.isEmpty() + ? basePath + "/" + HttpUtils.encodeURIComponent(objectId) : basePath; // RSO2d, RSO6 - every other param, such as path, goes in the query string unmodified; the // format is this method's, so a caller's compact param is dropped @@ -130,9 +133,12 @@ private HttpRequester.Request getImpl(@Nullable Param[] params, @Nullable if (client.options.addRequestIds) query.add(Crypto.generateRandomRequestId()); // RSC7c final Param[] queryParams = query.toArray(new Param[0]); - // RSO2e - the body is decoded per RSC8 before the decoder runs; no body is null + // RSO2e - the body is decoded per RSC8 before the decoder runs; no body is a null result + final HttpCore.ResponseHandler handler = responseHandler(decoder); return client.http.request((http, callback) -> - http.get(path, HttpUtils.defaultAcceptHeaders(binary()), queryParams, responseHandler(decoder), true, callback)); + http.get(path, HttpUtils.defaultAcceptHeaders(binary()), queryParams, (response, error) -> + error == null && (response.body == null || response.body.length == 0) + ? null : handler.handleResponse(response, error), true, callback)); } //endregion @@ -192,7 +198,7 @@ private HttpCore.RequestBody encodeOperations(List operatio return HttpObjectCodec.requestBody(wireOperations, binary()); } catch (RuntimeException e) { // e.g. a null where the typed API expects a value; report it as invalid input - throw HttpObjectCodec.invalidInput("Invalid operation: " + e); + throw HttpObjectCodec.invalidInput("Invalid operation: " + HttpObjectCodec.describe(e)); } } @@ -219,15 +225,17 @@ public void generateObjectIdAsync(@NotNull final HttpObjectGenerateIdBody create cb.onError(e.errorInfo); return; } + // A waiter can be notified on the thread of a blocking generateObjectId that fetched the + // time, so both outcomes are handed back to the scheduler. serverTime.currentTimeMillisAsync(new Callback() { @Override public void onSuccess(Long timestamp) { - cb.onSuccess(body.withTimestamp(timestamp)); + http.execute(() -> cb.onSuccess(body.withTimestamp(timestamp))); } @Override public void onError(ErrorInfo reason) { - cb.onError(reason); + http.execute(() -> cb.onError(reason)); } }); })).async(callback); @@ -251,7 +259,7 @@ static CreateBody of(HttpObjectGenerateIdBody createBody) throws AblyException { return encode(createBody); } catch (RuntimeException e) { // e.g. a null entry value; report it as invalid input - throw HttpObjectCodec.invalidInput("Invalid create body: " + e); + throw HttpObjectCodec.invalidInput("Invalid create body: " + HttpObjectCodec.describe(e)); } } diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpLiveObject.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpLiveObject.java index 761cd0c1c..b6f9b4769 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpLiveObject.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpLiveObject.java @@ -22,7 +22,7 @@ public interface HttpLiveObject extends HttpObjectGetFullResult { * For an unrecognised object type this holds its type-specific data. Values are JSON, as in a * {@code getCompact} result: binary values are Base64 strings for both protocols. * - * @return an unmodifiable map of field name to value; empty if there are none. + * @return a copy of the fields, by name; changes to it are not reflected. Empty if there are none. */ @NotNull JsonObject getUnrecognizedFields(); } diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectCodec.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectCodec.java index e35d77369..545fe86e5 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectCodec.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectCodec.java @@ -12,6 +12,7 @@ import io.ably.pubsub.types.AblyException; import io.ably.pubsub.types.ErrorInfo; import io.ably.pubsub.util.Base64Coder; +import io.ably.pubsub.util.Log; import org.msgpack.core.MessageBufferPacker; import org.msgpack.core.MessagePack; import org.msgpack.core.MessagePacker; @@ -40,6 +41,7 @@ */ final class HttpObjectCodec { + private static final String TAG = HttpObjectCodec.class.getName(); private static final String MSGPACK_CONTENT_TYPE = "application/x-msgpack"; private static final Charset UTF_8 = Charset.forName("UTF-8"); private static final List LEAF_VALUE_FIELDS = Arrays.asList("string", "number", "boolean", "bytes", "json"); @@ -54,6 +56,12 @@ private HttpObjectCodec() {} * field (RSO3d4). */ static Map encodeOperation(HttpObjectOperation op, boolean binary) throws AblyException { + // RSO5b1 - the factories set exactly one body, but a null passed to one leaves none + int bodies = count(op.getMapCreate(), op.getMapCreateWithObjectId(), op.getMapSet(), op.getMapRemove(), + op.getCounterCreate(), op.getCounterCreateWithObjectId(), op.getCounterInc()); + if (bodies != 1) { + throw invalidInput("An operation must have exactly one operation body, but it has " + bodies); + } Map wire = new LinkedHashMap<>(); putIfNotNull(wire, "id", op.getId()); putIfNotNull(wire, "objectId", op.getObjectId()); @@ -66,12 +74,12 @@ static Map encodeOperation(HttpObjectOperation op, boolean binar } if (op.getMapSet() != null) { Map mapSet = new LinkedHashMap<>(); - mapSet.put("key", op.getMapSet().getKey()); + mapSet.put("key", notNull(op.getMapSet().getKey(), "mapSet.key")); mapSet.put("value", encodeData(op.getMapSet().getValue(), binary)); wire.put("mapSet", mapSet); } if (op.getMapRemove() != null) { - wire.put("mapRemove", singleton("key", op.getMapRemove().getKey())); + wire.put("mapRemove", singleton("key", notNull(op.getMapRemove().getKey(), "mapRemove.key"))); } if (op.getCounterCreate() != null) { wire.put("counterCreate", encodeCounterCreate(op.getCounterCreate())); @@ -89,7 +97,7 @@ static Map encodeOperation(HttpObjectOperation op, boolean binar static Map encodeMapCreate(HttpObjectOperation.MapCreate body, boolean binary) throws AblyException { Map entries = new LinkedHashMap<>(); for (Map.Entry entry : body.getEntries().entrySet()) { - entries.put(entry.getKey(), singleton("data", encodeData(entry.getValue(), binary))); + entries.put(notNull(entry.getKey(), "mapCreate.entries key"), singleton("data", encodeData(entry.getValue(), binary))); } Map mapCreate = new LinkedHashMap<>(); mapCreate.put("semantics", encodeSemantics(body.getSemantics())); @@ -101,7 +109,10 @@ static Map encodeCounterCreate(HttpObjectOperation.CounterCreate return singleton("count", finite(body.getCount(), "counterCreate.count")); } - private static Map encodeCreateWithObjectId(HttpObjectOperation.CreateWithObjectId body) { + private static Map encodeCreateWithObjectId(HttpObjectOperation.CreateWithObjectId body) throws AblyException { + if (body.getInitialValue() == null || body.getNonce() == null) { + throw invalidInput("initialValue and nonce must not be null; use the values from generateObjectId"); + } Map wire = new LinkedHashMap<>(); wire.put("initialValue", body.getInitialValue()); wire.put("nonce", body.getNonce()); @@ -208,7 +219,8 @@ private static HttpLiveCounter decodeCounter(String objectId, Map counter, * OMP2 code ({@code 0}); accept both. Anything else is preserved as UNKNOWN. */ private static ObjectsMapSemantics decodeSemantics(Object value) { - if (value instanceof Number && ((Number) value).longValue() == 0L) return ObjectsMapSemantics.LWW; + // integral zero only: longValue() would also truncate 0.5 or a BigInteger of 2^64 to 0 + if (value instanceof Long && (Long) value == 0L) return ObjectsMapSemantics.LWW; if (value instanceof String && "lww".equalsIgnoreCase((String) value)) return ObjectsMapSemantics.LWW; return ObjectsMapSemantics.UNKNOWN; } @@ -217,13 +229,22 @@ private static ObjectsMapSemantics decodeSemantics(Object value) { * Decodes a leaf per OD5 (RSO2g3): Base64 {@code bytes} become binary (OD5b2), and the * JSON-encoded string in {@code json} is parsed (OD5a2, OD5b3). The result carries no * {@code objectId} or {@code encoding} (RSO9c1). + * + *

A {@code json} or {@code bytes} value that can't be decoded is logged and left out, so the + * leaf decodes as {@link io.ably.pubsub.liveobjects.ValueType#UNKNOWN} rather than failing the + * whole response. Like ably-js, which returns that leaf's raw fields, only the bad leaf is + * affected; the raw value isn't exposed, as {@link HttpObjectData} has no accessor for it. */ private static HttpObjectData decodeLeaf(Map fields) { final String string = asString(fields.get("string")); JsonElement json = null; Object rawJson = fields.get("json"); if (rawJson instanceof String) { - json = JsonParser.parseString((String) rawJson); + try { + json = JsonParser.parseString((String) rawJson); + } catch (RuntimeException e) { + Log.e(TAG, "Leaf json value is not valid JSON; leaving it out", e); + } } else if (rawJson != null) { json = toJson(rawJson); } @@ -232,7 +253,11 @@ private static HttpObjectData decodeLeaf(Map fields) { if (rawBytes instanceof byte[]) { bytes = (byte[]) rawBytes; } else if (rawBytes instanceof String) { - bytes = Base64Coder.decode((String) rawBytes); + try { + bytes = Base64Coder.decode((String) rawBytes); + } catch (RuntimeException e) { + Log.e(TAG, "Leaf bytes value is not valid Base64; leaving it out", e); + } } Object number = fields.get("number"); Object bool = fields.get("boolean"); @@ -373,7 +398,10 @@ private static void pack(MessagePacker packer, Object value) throws IOException packer.packString((String) value); } else if (value instanceof Boolean) { packer.packBoolean((Boolean) value); - } else if (value instanceof Double || value instanceof Float || value instanceof BigDecimal) { + } else if (value instanceof Float) { + // via the decimal string, as JSON writes it: widening 0.1f directly gives 0.10000000149011612 + packer.packDouble(Double.parseDouble(value.toString())); + } else if (value instanceof Double || value instanceof BigDecimal) { packer.packDouble(((Number) value).doubleValue()); } else if (value instanceof BigInteger) { packer.packBigInteger((BigInteger) value); @@ -464,6 +492,24 @@ private static Number finite(Number number, String field) throws AblyException { return number; } + private static V notNull(V value, String field) throws AblyException { + if (value == null) throw invalidInput("Invalid " + field + ": must not be null"); + return value; + } + + /** A short description of an unexpected exception for an invalid-input message. */ + static String describe(RuntimeException e) { + return e.getMessage() != null ? e.getMessage() : e.getClass().getSimpleName(); + } + + private static int count(Object... values) { + int count = 0; + for (Object value : values) { + if (value != null) count++; + } + return count; + } + private static String asString(Object value) { return value instanceof String ? (String) value : null; } diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGenerateIdBody.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGenerateIdBody.java index d3a27d806..a6f36b010 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGenerateIdBody.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGenerateIdBody.java @@ -49,10 +49,12 @@ private HttpObjectGenerateIdBody(@Nullable HttpObjectOperation.MapCreate mapCrea return counterCreate(new HttpObjectOperation.CounterCreate(count)); } + /** @return the map create body, or {@code null} if this is a counter create body. */ public @Nullable HttpObjectOperation.MapCreate getMapCreate() { return mapCreate; } + /** @return the counter create body, or {@code null} if this is a map create body. */ public @Nullable HttpObjectOperation.CounterCreate getCounterCreate() { return counterCreate; } diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGetParams.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGetParams.java index 940cf20be..bd86fe8a5 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGetParams.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectGetParams.java @@ -23,7 +23,7 @@ public final class HttpObjectGetParams { /** * The key of the ID of the object to fetch. It's sent as a path segment, not in the query - * string. If it's absent, the channel object is the target. + * string. If it's absent or empty, the channel object is the target. *

Spec: RSO2a1a */ public static final String OBJECT_ID = "objectId"; diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectOperation.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectOperation.java index 82d06ed28..ddf0ee784 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectOperation.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectOperation.java @@ -7,6 +7,7 @@ import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Objects; /** * An operation published with {@link io.ably.pubsub.liveobjects.HttpObject#publish}. @@ -202,42 +203,52 @@ private HttpObjectOperation(@Nullable String id, @Nullable String objectId, @Nul //region Getters + /** @return the operation ID set with {@link #withId}, or {@code null}; the library never generates one. */ public @Nullable String getId() { return id; } + /** @return the ID of the targeted or created object, or {@code null} if none is set. */ public @Nullable String getObjectId() { return objectId; } + /** @return the targeted path, or {@code null} if none is set. */ public @Nullable String getPath() { return path; } + /** @return the {@code mapCreate} body, or {@code null} if this is another kind of operation. */ public @Nullable MapCreate getMapCreate() { return mapCreate; } + /** @return the {@code mapCreateWithObjectId} body, or {@code null} if this is another kind of operation. */ public @Nullable CreateWithObjectId getMapCreateWithObjectId() { return mapCreateWithObjectId; } + /** @return the {@code mapSet} body, or {@code null} if this is another kind of operation. */ public @Nullable MapSet getMapSet() { return mapSet; } + /** @return the {@code mapRemove} body, or {@code null} if this is another kind of operation. */ public @Nullable MapRemove getMapRemove() { return mapRemove; } + /** @return the {@code counterCreate} body, or {@code null} if this is another kind of operation. */ public @Nullable CounterCreate getCounterCreate() { return counterCreate; } + /** @return the {@code counterCreateWithObjectId} body, or {@code null} if this is another kind of operation. */ public @Nullable CreateWithObjectId getCounterCreateWithObjectId() { return counterCreateWithObjectId; } + /** @return the {@code counterInc} body, or {@code null} if this is another kind of operation. */ public @Nullable CounterInc getCounterInc() { return counterInc; } @@ -261,13 +272,15 @@ public static final class MapCreate { */ public MapCreate(@NotNull ObjectsMapSemantics semantics, @NotNull Map entries) { this.semantics = semantics; - this.entries = Collections.unmodifiableMap(new LinkedHashMap<>(entries)); + this.entries = Collections.unmodifiableMap(new LinkedHashMap<>(Objects.requireNonNull(entries, "entries"))); } + /** @return the conflict-resolution semantics. */ public @NotNull ObjectsMapSemantics getSemantics() { return semantics; } + /** @return an unmodifiable map of the initial entries, in the order given. */ public @NotNull Map getEntries() { return entries; } @@ -281,6 +294,10 @@ public static final class CreateWithObjectId { private final @NotNull String initialValue; private final @NotNull String nonce; + /** + * @param initialValue the JSON-encoded initial value returned by {@code generateObjectId}. + * @param nonce the nonce returned by {@code generateObjectId}. + */ public CreateWithObjectId(@NotNull String initialValue, @NotNull String nonce) { this.initialValue = initialValue; this.nonce = nonce; @@ -302,15 +319,21 @@ public static final class MapSet { private final @NotNull String key; private final @NotNull PublishObjectData value; + /** + * @param key the key to set. + * @param value the value to set it to. + */ public MapSet(@NotNull String key, @NotNull PublishObjectData value) { this.key = key; this.value = value; } + /** @return the key to set. */ public @NotNull String getKey() { return key; } + /** @return the value to set the key to. */ public @NotNull PublishObjectData getValue() { return value; } @@ -320,10 +343,12 @@ public MapSet(@NotNull String key, @NotNull PublishObjectData value) { public static final class MapRemove { private final @NotNull String key; + /** @param key the key to remove. */ public MapRemove(@NotNull String key) { this.key = key; } + /** @return the key to remove. */ public @NotNull String getKey() { return key; } @@ -337,10 +362,12 @@ public MapRemove(@NotNull String key) { public static final class CounterCreate { private final @NotNull Number count; + /** @param count the counter's initial value; must be finite. */ public CounterCreate(@NotNull Number count) { this.count = count; } + /** @return the counter's initial value. */ public @NotNull Number getCount() { return count; } @@ -350,10 +377,12 @@ public CounterCreate(@NotNull Number count) { public static final class CounterInc { private final @NotNull Number number; + /** @param number the amount to add, negative to decrement; must be finite. */ public CounterInc(@NotNull Number number) { this.number = number; } + /** @return the amount to add. */ public @NotNull Number getNumber() { return number; } diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectResults.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectResults.java index e1ab7d1ea..50c093488 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectResults.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectResults.java @@ -230,12 +230,12 @@ static final class ObjectDataLeaf extends Result implements HttpObjectData { @Override public byte @Nullable [] getBytes() { - return bytes; + return bytes != null ? bytes.clone() : null; // byte[] is mutable; keep the result immutable } @Override public @Nullable JsonElement getJson() { - return json; + return json != null ? json.deepCopy() : null; // JsonElement is mutable; keep the result immutable } @Override diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectServerTime.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectServerTime.java index eca50474f..750494ffe 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectServerTime.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/HttpObjectServerTime.java @@ -41,6 +41,7 @@ private enum Fetch { NONE, BLOCKING, ASYNC } /** Callers waiting for the in-flight request. */ private final List> waiters = new ArrayList<>(); + /** @param client the client whose {@code /time} endpoint is queried. */ public HttpObjectServerTime(@NotNull PubSubBase client) { this.client = client; } @@ -73,9 +74,10 @@ long currentTimeMillis() throws AblyException { long serverTime; try { serverTime = client.time(); - } catch (AblyException | RuntimeException e) { - notifyWaiters(null, e instanceof AblyException ? ((AblyException) e).errorInfo : AblyException.fromThrowable(e).errorInfo); - throw e; + } catch (Throwable t) { + // any failure, even an Error, must end the in-flight request, or the waiters hang + notifyWaiters(null, errorInfo(t)); + throw t; } storeOffset(serverTime); notifyWaiters(serverTime, null); @@ -105,24 +107,43 @@ void currentTimeMillisAsync(@NotNull Callback callback) { return; } try { - client.timeAsync(new Callback() { - @Override - public void onSuccess(Long serverTime) { - storeOffset(serverTime); - notifyWaiters(serverTime, null); - } - - @Override - public void onError(ErrorInfo reason) { - notifyWaiters(null, reason != null ? reason : new ErrorInfo("Failed to retrieve server time", 500, 50000)); - } - }); + // The fetch runs as a blocking request on the scheduler rather than through timeAsync, + // whose callback is never invoked if decoding the /time response throws (for example + // on a 200 response of [] or HTML); fetchForWaiters releases the waiters on every path. + client.http.request((http, ignored) -> http.execute(this::fetchForWaiters)).async(IGNORED); } catch (RuntimeException e) { // the request never started; release the waiters so later calls can try again - notifyWaiters(null, AblyException.fromThrowable(e).errorInfo); + notifyWaiters(null, errorInfo(e)); + } + } + + /** The scheduled async fetch: always ends the in-flight request and notifies the waiters. */ + private void fetchForWaiters() { + long serverTime; + try { + serverTime = client.time(); + } catch (Throwable t) { + notifyWaiters(null, errorInfo(t)); + if (t instanceof Error) throw (Error) t; + return; } + storeOffset(serverTime); + notifyWaiters(serverTime, null); + } + + private static ErrorInfo errorInfo(Throwable t) { + return t instanceof AblyException ? ((AblyException) t).errorInfo : AblyException.fromThrowable(t).errorInfo; } + /** The scheduled fetch reports through {@link #notifyWaiters}, not through its own callback. */ + private static final Callback IGNORED = new Callback() { + @Override + public void onSuccess(Void result) {} + + @Override + public void onError(ErrorInfo reason) {} + }; + /** A blocking caller waiting for another caller's request. */ private static final class Waiter implements Callback { private final CountDownLatch done = new CountDownLatch(1); diff --git a/lib/src/main/java/io/ably/pubsub/liveobjects/http/PublishObjectData.java b/lib/src/main/java/io/ably/pubsub/liveobjects/http/PublishObjectData.java index e0c913143..7ae676801 100644 --- a/lib/src/main/java/io/ably/pubsub/liveobjects/http/PublishObjectData.java +++ b/lib/src/main/java/io/ably/pubsub/liveobjects/http/PublishObjectData.java @@ -9,6 +9,7 @@ /** * A leaf value in an {@link HttpObjectOperation}: the value of a {@code mapSet}, or the data of a * {@code mapCreate} entry. Exactly one field is set; create instances with the static factories. + * Instances are immutable: binary and JSON values are copied in and out. * *

Spec: RSO7 */ @@ -48,17 +49,17 @@ private PublishObjectData(@Nullable String string, @Nullable Number number, @Nul /** Spec: RSO7d */ public static @NotNull PublishObjectData bytes(byte @NotNull [] value) { - return new PublishObjectData(null, null, null, value, null, null); + return new PublishObjectData(null, null, null, value.clone(), null, null); } /** Spec: RSO7e */ public static @NotNull PublishObjectData json(@NotNull JsonObject value) { - return new PublishObjectData(null, null, null, null, value, null); + return new PublishObjectData(null, null, null, null, value.deepCopy(), null); } /** Spec: RSO7e */ public static @NotNull PublishObjectData json(@NotNull JsonArray value) { - return new PublishObjectData(null, null, null, null, value, null); + return new PublishObjectData(null, null, null, null, value.deepCopy(), null); } /** @@ -69,29 +70,34 @@ private PublishObjectData(@Nullable String string, @Nullable Number number, @Nul return new PublishObjectData(null, null, null, null, null, objectId); } + /** @return the string value, or {@code null} if this holds another kind of value. */ public @Nullable String getString() { return string; } + /** @return the number value, or {@code null} if this holds another kind of value. */ public @Nullable Number getNumber() { return number; } + /** @return the boolean value, or {@code null} if this holds another kind of value. */ public @Nullable Boolean getBoolean() { return bool; } + /** @return a copy of the binary value, or {@code null} if this holds another kind of value. */ public byte @Nullable [] getBytes() { - return bytes; + return bytes != null ? bytes.clone() : null; } /** - * @return the JSON value, a {@link JsonObject} or {@link JsonArray}, or {@code null}. + * @return a copy of the JSON value, a {@link JsonObject} or {@link JsonArray}, or {@code null}. */ public @Nullable JsonElement getJson() { - return json; + return json != null ? json.deepCopy() : null; } + /** @return the ID of the referenced object, or {@code null} if this holds another kind of value. */ public @Nullable String getObjectId() { return objectId; } diff --git a/lib/src/test/kotlin/io/ably/pubsub/liveobjects/http/HttpObjectEdgeCasesTest.kt b/lib/src/test/kotlin/io/ably/pubsub/liveobjects/http/HttpObjectEdgeCasesTest.kt index fad9060cb..c9bcaa650 100644 --- a/lib/src/test/kotlin/io/ably/pubsub/liveobjects/http/HttpObjectEdgeCasesTest.kt +++ b/lib/src/test/kotlin/io/ably/pubsub/liveobjects/http/HttpObjectEdgeCasesTest.kt @@ -1,13 +1,20 @@ package io.ably.pubsub.liveobjects.http import com.google.gson.JsonElement +import com.google.gson.JsonNull import com.google.gson.JsonObject +import com.google.gson.JsonParser import io.ably.pubsub.http.HttpUtils import io.ably.pubsub.liveobjects.ValueType +import io.ably.pubsub.liveobjects.message.ObjectsMapSemantics import io.ably.pubsub.types.AblyException import io.ably.pubsub.types.Callback import io.ably.pubsub.types.ErrorInfo import io.ably.pubsub.types.Param +import io.ably.pubsub.uts.unit.objects.encodeUriComponent +import io.ably.pubsub.uts.unit.objects.jsonEncode +import io.ably.pubsub.uts.unit.objects.msgpackDecode +import io.ably.pubsub.uts.unit.objects.parseJson import io.ably.pubsub.uts.unit.objects.path import io.ably.pubsub.uts.unit.objects.queryParams import io.ably.pubsub.uts.unit.objects.respondWithBody @@ -17,7 +24,11 @@ import java.math.BigInteger import java.util.Collections import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.assertContentEquals import kotlin.test.assertEquals +import kotlin.test.assertFails +import kotlin.test.assertFalse import kotlin.test.assertFailsWith import kotlin.test.assertIs import kotlin.test.assertNotNull @@ -37,22 +48,27 @@ class HttpObjectEdgeCasesTest { /** The outcome of an `...Async` call. */ private class Outcome(val result: T?, val error: ErrorInfo?) - /** Runs an `...Async` call and waits for its callback. */ + /** Runs an `...Async` call, waits for its callback and checks it isn't invoked a second time. */ private fun awaitCallback(call: (Callback) -> Unit): Outcome { val latch = CountDownLatch(1) + val invocations = AtomicInteger() var outcome: Outcome? = null call(object : Callback { override fun onSuccess(result: T) { + invocations.incrementAndGet() outcome = Outcome(result, null) latch.countDown() } override fun onError(reason: ErrorInfo?) { + invocations.incrementAndGet() outcome = Outcome(null, reason) latch.countDown() } }) assertTrue(latch.await(5, TimeUnit.SECONDS), "callback not invoked") + Thread.sleep(50) // give a second invocation, if any, the chance to happen + assertEquals(1, invocations.get(), "callback invoked more than once") return outcome!! } @@ -120,6 +136,42 @@ class HttpObjectEdgeCasesTest { assertTrue(callbackThread { obj.publishAsync(HttpObjectOperation.counterInc(Double.NaN), it) } != testThread) } + @Test + fun `an async callback runs on a background thread while a blocking call is fetching the time`() { + val timeRequested = CountDownLatch(1) + val asyncRegistered = CountDownLatch(1) + val setup = setupRestObject("edge-async-thread-waiter") { req -> + if (req.path == "/time") { + timeRequested.countDown() + asyncRegistered.await(5, TimeUnit.SECONDS) // hold the blocking fetch until the async call waits on it + req.respondWith(200, listOf(1700000000000L)) + } + } + val obj = setup.channel.`object` + val body = HttpObjectGenerateIdBody.counterCreate(1) + + var blockingThread: Thread? = null + val blocking = Thread { obj.generateObjectId(body) }.apply { blockingThread = this; start() } + assertTrue(timeRequested.await(5, TimeUnit.SECONDS)) + + val latch = CountDownLatch(1) + var callbackThread: Thread? = null + obj.generateObjectIdAsync(body, object : Callback { + override fun onSuccess(result: HttpObjectGenerateIdResult) { callbackThread = Thread.currentThread(); latch.countDown() } + override fun onError(reason: ErrorInfo?) { callbackThread = Thread.currentThread(); latch.countDown() } + }) + // Let the async call register as a waiter on the blocking fetch. If it hasn't by then, the + // time is already known when it runs and the test passes vacuously; it can't fail spuriously. + Thread.sleep(200) + asyncRegistered.countDown() + + assertTrue(latch.await(5, TimeUnit.SECONDS)) + blocking.join(5000) + assertTrue(callbackThread != blockingThread, "the async callback ran on the blocking caller's thread") + assertTrue(callbackThread != Thread.currentThread()) + assertEquals(1, setup.capturedRequests.count { it.path == "/time" }) + } + //endregion //region Get params @@ -145,6 +197,15 @@ class HttpObjectEdgeCasesTest { assertEquals(mapOf("path" to "a.b", "extra" to "1"), compact.queryParams) } + @Test + fun `get params - an empty objectId targets the channel object`() { + val setup = setupRestObject("edge-empty-objectid") { it.respondWithBody(200, mapOf("objectId" to "root"), false) } + + setup.channel.`object`.getCompact(HttpObjectGetParams.ofObjectId("")) + + assertEquals("/channels/edge-empty-objectid/object", setup.capturedRequests.single().path) + } + @Test fun `get params - factories build the objectId and path params`() { assertEquals(listOf(Param("objectId", "map:abc@1")), HttpObjectGetParams.ofObjectId("map:abc@1").toList()) @@ -184,6 +245,30 @@ class HttpObjectEdgeCasesTest { //region Errors and decoding + @Test + fun `operations left without a body or with null create values are rejected with 40003 before any request`() { + val setup = setupRestObject("edge-null-bodies") { it.respondWithBody(201, emptyMap(), false) } + val obj = setup.channel.`object` + // Kotlin can't pass null to these @NotNull parameters; Java callers can, so use reflection. + val type = HttpObjectOperation::class.java + val noMapCreate = type.getMethod("mapCreate", HttpObjectOperation.MapCreate::class.java).invoke(null, *arrayOf(null)) as HttpObjectOperation + val noCounterCreate = type.getMethod("counterCreate", HttpObjectOperation.CounterCreate::class.java).invoke(null, *arrayOf(null)) as HttpObjectOperation + val nullCreateValues = type.getMethod("mapCreateWithObjectId", String::class.java, String::class.java, String::class.java) + .invoke(null, "map:abc@1", null, null) as HttpObjectOperation + + for (op in listOf(noMapCreate, noCounterCreate, nullCreateValues)) { + assertEquals(40003, assertFailsWith { obj.publish(op) }.errorInfo.code) + assertEquals(40003, awaitCallback { obj.publishAsync(listOf(op), it) }.errorInfo().code) + } + + // likewise a null generate-ID body, through the async variant (the blocking one is RSO4c) + val generateAsync = obj.javaClass.getMethod("generateObjectIdAsync", HttpObjectGenerateIdBody::class.java, Callback::class.java) + val nullBody = awaitCallback { generateAsync.invoke(obj, null, it) }.errorInfo() + assertEquals(400, nullBody.statusCode) + assertEquals(40003, nullBody.code) + assertEquals(0, setup.capturedRequests.size) + } + @Test fun `null values are rejected with 40003, also through the async callback`() { val setup = setupRestObject("edge-null-values") { req -> @@ -198,6 +283,17 @@ class HttpObjectEdgeCasesTest { assertEquals(40003, awaitCallback { setup.channel.`object`.publishAsync(op, it) }.errorInfo().code) assertEquals(40003, assertFailsWith { setup.channel.`object`.generateObjectId(body) }.errorInfo.code) assertEquals(40003, awaitCallback { setup.channel.`object`.generateObjectIdAsync(body, it) }.errorInfo().code) + + // null keys, which Java callers can pass, are rejected rather than sent as JSON null + @Suppress("UNCHECKED_CAST") + val nullKeyEntries = mapOf(null to PublishObjectData.string("x")) as Map + val nullKeyRemove = HttpObjectOperation::class.java.getMethod("mapRemove", String::class.java) + .invoke(null, *arrayOf(null)) as HttpObjectOperation + for (invalid in listOf(HttpObjectOperation.mapCreate(nullKeyEntries), nullKeyRemove.withObjectId("root"))) { + val error = assertFailsWith { setup.channel.`object`.publish(invalid) } + assertEquals(40003, error.errorInfo.code) + assertFalse(error.errorInfo.message.contains("java.lang."), error.errorInfo.message) + } assertEquals(0, setup.capturedRequests.count { it.method == "POST" }) } @@ -214,21 +310,53 @@ class HttpObjectEdgeCasesTest { @Test - fun `malformed leaf in a full response fails without a fallback retry`() { + fun `a malformed leaf in a full response decodes as an unknown leaf without failing its siblings`() { val cases = listOf( mapOf("json" to "{not json"), mapOf("bytes" to "not*base64!"), ) for (leaf in cases) { - val setup = setupRestObject("edge-malformed") { it.respondWithBody(200, leaf, false) } + val setup = setupRestObject("edge-malformed") { req -> + if (req.queryParams["path"] == "leaf") { + req.respondWithBody(200, leaf, false) + } else { + req.respondWithBody(200, mapOf( + "objectId" to "root", + "map" to mapOf("semantics" to "LWW", "entries" to mapOf( + "bad" to mapOf("data" to leaf), + "good" to mapOf("data" to mapOf("string" to "ok")), + )), + ), false) + } + } val params = HttpObjectGetParams.ofPath("leaf") - val error = assertFailsWith { setup.channel.`object`.get(params) } - assertEquals(50000, error.errorInfo.code) - assertEquals(500, error.errorInfo.statusCode) - assertEquals(50000, awaitCallback { setup.channel.`object`.getAsync(params, it) }.errorInfo().code) - // one request per call: a decode error is not taken for a host failure and retried - assertEquals(2, setup.capturedRequests.size) + assertEquals(ValueType.UNKNOWN, setup.channel.`object`.get(params)!!.type) + assertEquals(ValueType.UNKNOWN, awaitCallback { setup.channel.`object`.getAsync(params, it) }.result!!.type) + val entries = setup.channel.`object`.get()!!.asLiveMap().entries + assertEquals(ValueType.UNKNOWN, entries.getValue("bad").data.type) + assertEquals("ok", entries.getValue("good").data.asString()) + } + } + + @Test + fun `map semantics decode from the name or the integer code, anything else is unknown`() { + val cases = listOf>( + "LWW" to ObjectsMapSemantics.LWW, + 0L to ObjectsMapSemantics.LWW, + 0.5 to ObjectsMapSemantics.UNKNOWN, + 1L to ObjectsMapSemantics.UNKNOWN, + "future" to ObjectsMapSemantics.UNKNOWN, + ) + // 2^64, whose longValue() is 0, only fits in JSON + val jsonOnly = BigInteger.ONE.shiftLeft(64) to ObjectsMapSemantics.UNKNOWN + for (binary in listOf(false, true)) { + for ((semantics, expected) in if (binary) cases else cases + jsonOnly) { + val setup = setupRestObject("edge-semantics", useBinaryProtocol = binary) { + it.respondWithBody(200, mapOf("objectId" to "root", "map" to mapOf("semantics" to semantics, "entries" to emptyMap())), binary) + } + assertEquals(expected, setup.channel.`object`.get()!!.asLiveMap().semantics, "semantics $semantics, binary $binary") + } } } @@ -250,6 +378,16 @@ class HttpObjectEdgeCasesTest { assertNull(setup.channel.`object`.getCompact()) } + @Test + fun `a JSON null compact body is JsonNull, distinct from no body`() { + for (binary in listOf(false, true)) { + val setup = setupRestObject("edge-null-body", useBinaryProtocol = binary) { it.respondWithBody(200, null, binary) } + + assertEquals(JsonNull.INSTANCE, setup.channel.`object`.getCompact()) + assertEquals(JsonNull.INSTANCE, awaitCallback { setup.channel.`object`.getCompactAsync(it) }.result) + } + } + @Test fun `publish with a null operation fails with 40003 and sends nothing`() { val setup = setupRestObject("edge-null-op") { it.respondWithBody(201, emptyMap(), false) } @@ -276,6 +414,23 @@ class HttpObjectEdgeCasesTest { assertEquals(0, setup.capturedRequests.size) } + @Test + fun `a Float value is sent as the same number with both protocols`() { + for (binary in listOf(false, true)) { + val setup = setupRestObject("edge-float", useBinaryProtocol = binary) { + it.respondWithBody(201, mapOf("messageId" to "m", "channel" to "c", "objectIds" to emptyList()), binary) + } + + setup.channel.`object`.publish(HttpObjectOperation.counterInc(0.1f).withPath("c")) + + val request = setup.capturedRequests.single() + val sent = if (binary) msgpackDecode(request.body) else parseJson(request.body) + @Suppress("UNCHECKED_CAST") + val counterInc = ((sent as List>)[0]["counterInc"] as Map) + assertEquals(0.1, (counterInc["number"] as Number).toDouble(), "binary $binary") + } + } + @Test fun `integers beyond the long range keep their exact value for both protocols`() { val big = BigInteger.ONE.shiftLeft(63) // just beyond Long.MAX_VALUE; still fits msgpack uint64 @@ -343,6 +498,36 @@ class HttpObjectEdgeCasesTest { assertEquals("future:f@1", entry("future").asLiveObject().objectId) } + @Test + fun `binary and JSON values are copied, so inputs and results stay immutable`() { + val bytes = byteArrayOf(1, 2, 3) + val json = JsonObject().apply { addProperty("a", 1) } + val bytesData = PublishObjectData.bytes(bytes) + val jsonData = PublishObjectData.json(json) + bytes[0] = 9 + json.addProperty("b", 2) + bytesData.bytes!![1] = 9 + (jsonData.json as JsonObject).addProperty("c", 3) + + assertContentEquals(byteArrayOf(1, 2, 3), bytesData.bytes) + assertEquals(JsonParser.parseString("{\"a\":1}"), jsonData.json) + + val setup = setupRestObject("edge-immutable") { + it.respondWithBody(200, mapOf("objectId" to "root", "map" to mapOf("semantics" to "LWW", "entries" to mapOf( + "bin" to mapOf("data" to mapOf("bytes" to "AQID")), + "obj" to mapOf("data" to mapOf("json" to "{\"a\":1}")), + ))), false) + } + val entries = setup.channel.`object`.get()!!.asLiveMap().entries + val bin = entries.getValue("bin").data.asObjectData() + val obj = entries.getValue("obj").data.asObjectData() + bin.bytes!![0] = 9 + (obj.json as JsonObject).addProperty("b", 2) + + assertContentEquals(byteArrayOf(1, 2, 3), bin.bytes) + assertEquals(JsonParser.parseString("{\"a\":1}"), obj.json) + } + @Test fun `as casts fail fast on a type mismatch`() { val body = mapOf("objectId" to "counter:c@1", "counter" to mapOf("data" to mapOf("number" to 1L))) @@ -363,6 +548,84 @@ class HttpObjectEdgeCasesTest { //endregion + //region Wire details + + @Test + fun `a msgpack publish response is decoded`() { + val setup = setupRestObject("edge-publish-msgpack", useBinaryProtocol = true) { + it.respondWithBody(201, mapOf("messageId" to "msg-1", "channel" to "edge-publish-msgpack", "objectIds" to listOf("counter:a@1", "map:b@2")), true) + } + + val result = setup.channel.`object`.publish(HttpObjectOperation.counterInc(1).withPath("c")) + + assertEquals("msg-1", result.messageId) + assertEquals("edge-publish-msgpack", result.channel) + assertEquals(listOf("counter:a@1", "map:b@2"), result.objectIds) + } + + @Test + fun `map entries without a data object are skipped`() { + val setup = setupRestObject("edge-skipped-entries") { + it.respondWithBody(200, mapOf("objectId" to "root", "map" to mapOf("semantics" to "LWW", "entries" to mapOf( + "noData" to mapOf("tombstone" to true), + "notAMap" to "x", + "kept" to mapOf("data" to mapOf("number" to 1L)), + ))), false) + } + + assertEquals(setOf("kept"), setup.channel.`object`.get()!!.asLiveMap().entries.keys) + } + + @Test + fun `compact values are returned verbatim, including json leaves and empty maps`() { + val bodies = listOf("{\"foo\":\"bar\"}", "[\"foo\",\"bar\"]", emptyMap()) + for (body in bodies) { + val setup = setupRestObject("edge-compact-verbatim") { it.respondWithBody(200, body, false) } + + val compact = setup.channel.`object`.getCompact(HttpObjectGetParams.ofPath("x")) + + // a json leaf stays its JSON-encoded string; an empty map is {} + assertEquals(JsonParser.parseString(jsonEncode(body)), compact) + } + } + + @Test + fun `request ids are added to get and publish when addRequestIds is set`() { + val setup = setupRestObject("edge-request-ids", clientOptions = { addRequestIds = true }) { req -> + if (req.method == "POST") { + req.respondWithBody(201, mapOf("messageId" to "m", "channel" to "c", "objectIds" to emptyList()), false) + } else { + req.respondWithBody(200, mapOf("objectId" to "root"), false) + } + } + + setup.channel.`object`.getCompact() + setup.channel.`object`.publish(HttpObjectOperation.counterInc(1).withPath("c")) + + assertEquals(2, setup.capturedRequests.size) + assertTrue(setup.capturedRequests.all { !it.queryParams["request_id"].isNullOrEmpty() }) + } + + @Test + fun `channel names are percent-encoded in the object path`() { + val name = "a/b c?d#e:f" + val setup = setupRestObject(name) { req -> + if (req.method == "POST") { + req.respondWithBody(201, mapOf("messageId" to "m", "channel" to name, "objectIds" to emptyList()), false) + } else { + req.respondWithBody(200, mapOf("objectId" to "root"), false) + } + } + + setup.channel.`object`.getCompact() + setup.channel.`object`.publish(HttpObjectOperation.counterInc(1).withPath("c")) + + val expected = "/channels/" + encodeUriComponent(name) + "/object" + assertEquals(listOf(expected, expected), setup.capturedRequests.map { it.path }) + } + + //endregion + //region Server time @Test @@ -396,9 +659,12 @@ class HttpObjectEdgeCasesTest { } val body = HttpObjectGenerateIdBody.counterCreate(1) - val threads = (1..5).map { Thread { setup.channel.`object`.generateObjectId(body) }.apply { start() } } + val failures = Collections.synchronizedList(mutableListOf()) + val threads = (1..5).map { Thread { runCatching { setup.channel.`object`.generateObjectId(body) }.onFailure { failures.add(it) } }.apply { start() } } threads.forEach { it.join(5000) } + assertTrue(threads.none { it.isAlive }, "a blocking call didn't finish") + assertEquals(emptyList(), failures) assertEquals(1, setup.capturedRequests.count { it.path == "/time" }) } @@ -425,20 +691,56 @@ class HttpObjectEdgeCasesTest { @Test fun `a blocking and an async first call share one time request`() { + val timeRequested = CountDownLatch(1) val setup = setupRestObject("edge-time-mixed") { req -> - if (req.path == "/time") req.respondWithDelay(300.milliseconds, 200, listOf(1700000000000L)) + if (req.path == "/time") { + timeRequested.countDown() + req.respondWithDelay(300.milliseconds, 200, listOf(1700000000000L)) + } } val body = HttpObjectGenerateIdBody.counterCreate(1) - val blocking = Thread { setup.channel.`object`.generateObjectId(body) }.apply { start() } - Thread.sleep(100) // let the blocking call start its request + var blockingFailure: Throwable? = null + val blocking = Thread { runCatching { setup.channel.`object`.generateObjectId(body) }.onFailure { blockingFailure = it } }.apply { start() } + // the blocking call has claimed the fetch once its request reaches the mock + assertTrue(timeRequested.await(5, TimeUnit.SECONDS)) val async = awaitCallback { setup.channel.`object`.generateObjectIdAsync(body, it) } blocking.join(5000) + assertTrue(!blocking.isAlive && blockingFailure == null, "blocking call failed or didn't finish: $blockingFailure") assertNotNull(async.result) assertEquals(1, setup.capturedRequests.count { it.path == "/time" }) } + @Test + fun `a blocking call while an async fetch is in flight fetches independently, and the time is then reused`() { + val timeRequested = CountDownLatch(1) + val setup = setupRestObject("edge-time-async-then-blocking") { req -> + if (req.path == "/time") { + timeRequested.countDown() + req.respondWithDelay(300.milliseconds, 200, listOf(1700000000000L)) + } + } + val obj = setup.channel.`object` + val body = HttpObjectGenerateIdBody.counterCreate(1) + + val async = CountDownLatch(1) + obj.generateObjectIdAsync(body, object : Callback { + override fun onSuccess(result: HttpObjectGenerateIdResult) = async.countDown() + override fun onError(reason: ErrorInfo?) = fail("unexpected error $reason") + }) + assertTrue(timeRequested.await(5, TimeUnit.SECONDS)) + // the blocking call doesn't wait on the scheduler's fetch; it makes its own request + assertTrue(obj.generateObjectId(body).objectId.endsWith("@1700000000000")) + assertTrue(async.await(5, TimeUnit.SECONDS)) + assertEquals(2, setup.capturedRequests.count { it.path == "/time" }) + + // once known, the time comes from the stored offset + obj.generateObjectId(body) + awaitCallback { obj.generateObjectIdAsync(body, it) } + assertEquals(2, setup.capturedRequests.count { it.path == "/time" }) + } + @Test fun `a failed time request fails generateObjectId and the next call retries`() { var timeRequests = 0 @@ -480,5 +782,53 @@ class HttpObjectEdgeCasesTest { assertEquals(2, timeRequests) } + @Test + fun `a malformed time response fails waiting generateObjectIdAsync calls and the next call retries`() { + // 200 responses whose body can't be read as [time] make the /time response handler throw + val malformed = ArrayDeque(listOf("[]", "[null]", "captive portal", "")) + val setup = setupRestObject("edge-time-malformed") { req -> + if (req.path == "/time") { + val body = synchronized(malformed) { malformed.removeFirstOrNull() } + if (body != null) req.respondWithDelay(100.milliseconds, 200, body) else req.respondWith(200, listOf(1700000000000L)) + } + } + val body = HttpObjectGenerateIdBody.counterCreate(1) + + repeat(4) { + // two concurrent callers share each failing request; both must be called back + val first = CountDownLatch(1) + val errors = Collections.synchronizedList(mutableListOf()) + val callback = object : Callback { + override fun onSuccess(result: HttpObjectGenerateIdResult) = fail("unexpected result $result") + override fun onError(reason: ErrorInfo?) { errors.add(reason); first.countDown() } + } + setup.channel.`object`.generateObjectIdAsync(body, callback) + setup.channel.`object`.generateObjectIdAsync(body, callback) + assertTrue(first.await(5, TimeUnit.SECONDS), "waiting callers were not released") + val deadline = System.currentTimeMillis() + 5000 + while (errors.size < 2 && System.currentTimeMillis() < deadline) Thread.sleep(10) + assertEquals(2, errors.size) + assertTrue(errors.all { it != null }) + } + + assertTrue(awaitCallback { setup.channel.`object`.generateObjectIdAsync(body, it) }.result!!.objectId.endsWith("@1700000000000")) + } + + @Test + fun `a malformed time response fails a blocking generateObjectId and the next call retries`() { + var timeRequests = 0 + val setup = setupRestObject("edge-time-malformed-sync") { req -> + if (req.path == "/time") { + timeRequests++ + if (timeRequests == 1) req.respondWith(200, "[]") else req.respondWith(200, listOf(1700000000000L)) + } + } + val body = HttpObjectGenerateIdBody.counterCreate(1) + + assertFails { setup.channel.`object`.generateObjectId(body) } + assertTrue(setup.channel.`object`.generateObjectId(body).objectId.endsWith("@1700000000000")) + assertEquals(2, timeRequests) + } + //endregion } diff --git a/lib/src/test/kotlin/io/ably/pubsub/uts/deviations.md b/lib/src/test/kotlin/io/ably/pubsub/uts/deviations.md index 987a95ca7..9f421e167 100644 --- a/lib/src/test/kotlin/io/ably/pubsub/uts/deviations.md +++ b/lib/src/test/kotlin/io/ably/pubsub/uts/deviations.md @@ -14,7 +14,7 @@ > deviations** entry format: each entry records the spec point, what the spec requires, what the SDK > actually does, and which test carries the deviation gate. Every realtime/rest deviation recorded here > is a **genuine SDK bug** (open; ably-js is compliant — fix the SDK), stated per entry under **Status**. -> The REST `HttpObject` entries (RSL16b, RSO2a1c, RSO2e, T-4) are the exception: they are typed-SDK +> The REST `HttpObject` entries (RSL16b, RSO2a1c, RSO2e, T-4, RSO9/RSO14/RSO15 result shapes) are the exception: they are typed-SDK > adaptations or intentional design divergences, as their **Status** lines say. Other entries of those > kinds are in the objects file. @@ -125,15 +125,24 @@ 5. **Test impact:** adapted assertion on the actual header; passes. 6. **Status:** intentional, SDK-wide. -### T-4 (objects) — operation and generate-ID bodies are unrepresentable without exactly one body (`objects.RestObjectPublishTest`, `objects.RestObjectGenerateIdTest`) +### T-4 (objects) — operation and generate-ID bodies can't be built with the wrong number of bodies (`objects.RestObjectPublishTest`, `objects.RestObjectGenerateIdTest`) 1. **Spec point:** RSO5b1 (`objects/unit/RSO5b1/no-body-field-error-0`, `objects/unit/RSO5b1/multiple-body-fields-error-1`), RSO4c (`objects/unit/RSO4c/missing-create-body-error-0`). 2. **What the spec says:** an operation with zero or several body fields, or a generate-ID body with neither `mapCreate` nor `counterCreate`, fails with 400 / 40003 before any request. Each test notes it is not applicable where the SDK makes these inputs unrepresentable. -3. **What the SDK does:** `HttpObjectOperation` and `HttpObjectGenerateIdBody` can only be built through factories that set exactly one body, and the `with*` methods only change the common fields. +3. **What the SDK does:** `HttpObjectOperation` and `HttpObjectGenerateIdBody` can only be built through factories that set one body, and the `with*` methods only change the common fields, so several bodies are unrepresentable. No body at all is reachable only through a `null` passed to a factory from Java (`HttpObjectOperation.mapCreate((MapCreate) null)`, `generateObjectId(null)`), and that fails with 400 / 40003 before any request, as the spec requires. 4. **Root cause:** typed-SDK construction (the same T-4 pattern as the objects suite, `liveobjects/.../uts/deviations.md`). -5. **Test impact:** the tests exist under their `@UTS` ids with a `// DEVIATION T-4` comment and assert only that no request was sent. The runtime-reachable 40003 case — an unrecognised semantics, `ObjectsMapSemantics.UNKNOWN` — is asserted for real in `RSO3d1/unrecognized-semantics-error-1` and `RSO4e1a/unrecognized-semantics-error-0`. +5. **Test impact:** `RSO5b1/multiple-body-fields-error-1` exists under its `@UTS` id with a `// DEVIATION T-4` comment and asserts only that no request was sent. `RSO5b1/no-body-field-error-0` and `RSO4c/missing-create-body-error-0` assert the 400 / 40003 for real, passing the `null` through reflection because Kotlin can't. The other runtime-reachable 40003 case — an unrecognised semantics, `ObjectsMapSemantics.UNKNOWN` — is asserted in `RSO3d1/unrecognized-semantics-error-1` and `RSO4e1a/unrecognized-semantics-error-0`. 6. **Status:** intentional — a typed-SDK adaptation, sanctioned by each spec test's own not-applicable note. +### RSO9, RSO14, RSO15 — typed result shapes, and a closed semantics enum (`objects.RestObjectGetTest`, `objects.RestObjectPublishTest`, `objects.RestObjectGenerateIdTest`) + +1. **Spec point:** RSO9c1 (`objects/unit/RSO2g3/...` leaf tests), RSO14 (`objects/unit/RSO2g1/...`), RSO15 (`objects/unit/RSO2g2/...`), RSO3d1 / RSO4e1a (`objects/unit/RSO3d1/unrecognized-semantics-error-1`, `objects/unit/RSO4e1a/unrecognized-semantics-error-0`). +2. **What the spec says:** a full-format leaf has `objectId` and `encoding` unset (RSO9c1); a map exposes `map.semantics` (as `"LWW"` in the mocked responses) and `map.entries`; a counter exposes `counter.data.number`; an unrecognised semantics such as `"not-a-semantics"` fails with 400 / 40003. +3. **What the SDK does:** `HttpObjectData` has no `encoding` accessor at all, so RSO9c1's `encoding` half holds by construction and isn't asserted. `HttpLiveMap` flattens `map.*` into `getSemantics()` (an `ObjectsMapSemantics` enum) and `getEntries()`, and `HttpLiveCounter` exposes `counter.data.number` as `getCount()`. `ObjectsMapSemantics` is a closed enum, so the only unrecognised value a caller can pass is `UNKNOWN`. +4. **Root cause:** typed-SDK result types and enums. +5. **Test impact:** the leaf tests note the unasserted `encoding` check in a comment; the map and counter tests read `entries` / `semantics == LWW` / `count`; the unrecognised-semantics tests pass `ObjectsMapSemantics.UNKNOWN` and assert 400 / 40003. All pass. +6. **Status:** intentional — typed-SDK adaptations. + ## Mock Infrastructure Limitations *(none)* diff --git a/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectGenerateIdTest.kt b/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectGenerateIdTest.kt index c97dfa9b9..18bc23923 100644 --- a/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectGenerateIdTest.kt +++ b/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectGenerateIdTest.kt @@ -179,10 +179,20 @@ class RestObjectGenerateIdTest { */ @Test fun `RSO4c - missing create body rejected`() = runTest { - // DEVIATION T-4 (not applicable, per the spec's own note): HttpObjectGenerateIdBody can only - // be built through the mapCreate / counterCreate factories, so an empty body is - // unrepresentable and the runtime 40003 check can't be reached. See deviations.md. + // Adapted (T-4): HttpObjectGenerateIdBody's factories always set a body, so an empty body is + // unrepresentable; the reachable equivalent is a null body, which Java callers can pass. + // Kotlin can't pass that null, so call through reflection. See deviations.md. val setup = setup("test-RSO4c-" + randomId()) + val generateObjectId = setup.channel.`object`.javaClass + .getMethod("generateObjectId", HttpObjectGenerateIdBody::class.java) + + val error = assertFailsWith { + generateObjectId.invoke(setup.channel.`object`, *arrayOf(null)) + }.targetException + + assertIs(error) + assertEquals(400, error.errorInfo.statusCode) + assertEquals(40003, error.errorInfo.code) assertEquals(0, setup.capturedRequests.size) } diff --git a/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectPublishTest.kt b/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectPublishTest.kt index a4b8e2b20..b873396ae 100644 --- a/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectPublishTest.kt +++ b/lib/src/test/kotlin/io/ably/pubsub/uts/unit/objects/RestObjectPublishTest.kt @@ -398,10 +398,17 @@ class RestObjectPublishTest { */ @Test fun `RSO5b1 - operation with no body field rejected before any request`() = runTest { - // DEVIATION T-4 (not applicable, per the spec's own note): an HttpObjectOperation can only be - // built through a factory that sets exactly one body, so `RestObjectOperation(objectId: "root")` - // with no body is unrepresentable and the runtime check can't be reached. See deviations.md. + // Adapted (T-4): the factories always set a body, but a null passed to one from Java, which + // doesn't enforce @NotNull, leaves none. Kotlin can't pass that null, so call through reflection. val setup = setup("test-RSO5b1-none-" + randomId()) + val noBody = HttpObjectOperation::class.java + .getMethod("mapCreate", HttpObjectOperation.MapCreate::class.java) + .invoke(null, *arrayOf(null)) as HttpObjectOperation + + val error = assertFailsWith { setup.channel.`object`.publish(noBody.withObjectId("root")) } + + assertEquals(400, error.errorInfo.statusCode) + assertEquals(40003, error.errorInfo.code) assertEquals(0, setup.capturedRequests.size) }