Skip to content

Commit d348a98

Browse files
VeskeRVeskeR
committed
Address PR feedback
toUserFacingObjectOperation had the same issue with *CreateWithObjectId fields as apply-on-ACK did, so fix to resolve create ops from *Create or *CreateWithObjectId._derivedFrom and omit *CreateWithObjectId fields from the returned object.
1 parent 8abde10 commit d348a98

File tree

3 files changed

+77
-13
lines changed

3 files changed

+77
-13
lines changed

‎src/plugins/liveobjects/objectmessage.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,11 @@ function toUserFacingMapEntry(entry: ObjectsMapEntry<ObjectData>): ObjectsApi.Ob
487487
}
488488

489489
function toUserFacingObjectOperation(operation: ObjectOperation<ObjectData>): ObjectsApi.ObjectOperation {
490-
const { mapCreate: internalMapCreate, mapSet: internalMapSet, mapRemove, counterCreate, counterInc } = operation;
490+
const { mapSet: internalMapSet, mapRemove, counterInc, objectDelete } = operation;
491+
492+
// resolve *Create from direct property or from *CreateWithObjectId._derivedFrom
493+
const internalMapCreate = operation.mapCreate ?? operation.mapCreateWithObjectId?._derivedFrom;
494+
const counterCreate = operation.counterCreate ?? operation.counterCreateWithObjectId?._derivedFrom;
491495

492496
let mapCreate: ObjectsApi.MapCreate | undefined;
493497
if (internalMapCreate) {
@@ -525,13 +529,14 @@ function toUserFacingObjectOperation(operation: ObjectOperation<ObjectData>): Ob
525529
}
526530

527531
return {
528-
...operation,
529532
action: operationActions[operation.action] || 'unknown',
533+
objectId: operation.objectId,
530534
mapCreate,
531535
mapSet,
532536
mapRemove,
533537
counterCreate,
534538
counterInc,
539+
objectDelete,
535540
// deprecated fields
536541
mapOp,
537542
counterOp,

‎test/common/modules/private_api_recorder.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
1515
'call.Defaults.normaliseOptions',
1616
'call.EventEmitter.emit',
1717
'call.EventEmitter.listeners',
18-
'call.LiveCounterValueType.createCounterCreateMessage',
19-
'call.LiveMapValueType.createMapCreateMessage',
2018
'call.LiveObject.isTombstoned',
2119
'call.LiveObject.tombstonedAt',
2220
'call.Message.decode',
@@ -153,6 +151,8 @@ define(['test/support/output_directory_paths'], function (outputDirectoryPaths)
153151
'write.Defaults.ENDPOINT',
154152
'write.Defaults.ENVIRONMENT',
155153
'write.Defaults.wsConnectivityCheckUrl',
154+
'write.ObjectOperation.counterCreateWithObjectId._derivedFrom',
155+
'write.ObjectOperation.mapCreateWithObjectId._derivedFrom',
156156
'write.Platform.Config.push', // This implies using a mock implementation of the internal IPlatformPushConfig interface. Our mock (in push_channel_transport.js) then interacts with internal objects and private APIs of public objects to implement this interface; I haven’t added annotations for that private API usage, since there wasn’t an easy way to pass test context information into the mock. I think that for now we can just say that if we wanted to get rid of this private API usage, then we’d need to remove this mock entirely.
157157
'write.RealtimeObject._DEFAULTS.gcInterval',
158158
'write.RealtimeObject.gcGracePeriod',

‎test/realtime/liveobjects.test.js‎

Lines changed: 68 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
5050
return;
5151
}
5252

53+
// ensure actual is a non-null object when expected is an object (even an empty one)
54+
expect(actual, msg).to.be.an('object').and.not.be.null;
55+
5356
for (const [key, expectedVal] of Object.entries(expected)) {
5457
const desc = msg ? `${msg} (at key '${key}')` : `at key '${key}'`;
5558
expect(actual, desc).to.have.property(key);
@@ -8404,20 +8407,24 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
84048407
const client = RealtimeWithLiveObjects(helper, { autoConnect: false });
84058408
helper.recordPrivateApi('call.ObjectMessage.encode');
84068409
const encodedMessage = scenario.message.encode(client);
8407-
helper.recordPrivateApi('call.BufferUtils.utf8Encode'); // was called by a scenario to create buffers
8408-
helper.recordPrivateApi('call.ObjectMessage.fromValues'); // was called by a scenario to create an ObjectMessage instance
8409-
helper.recordPrivateApi('call.Utils.dataSizeBytes'); // was called by a scenario to calculated the expected byte size
8410+
helper.recordPrivateApi('call.BufferUtils.utf8Encode'); // called by a scenario to create buffers
8411+
helper.recordPrivateApi('call.ObjectMessage.fromValues'); // called by a scenario to create an ObjectMessage instance
8412+
helper.recordPrivateApi('call.Utils.dataSizeBytes'); // called by a scenario to calculated the expected byte size
8413+
helper.recordPrivateApi('write.ObjectOperation.counterCreateWithObjectId._derivedFrom'); // prop set by a scenario
8414+
helper.recordPrivateApi('write.ObjectOperation.mapCreateWithObjectId._derivedFrom'); // prop set by a scenario
84108415
helper.recordPrivateApi('call.ObjectMessage.getMessageSize');
84118416
expect(encodedMessage.getMessageSize()).to.equal(scenario.expected);
84128417
});
84138418
});
84148419

84158420
/** @nospec */
8416-
it('toUserFacingMessage exposes deprecated ObjectOperation fields alongside new fields for all operation types', function () {
8421+
it('toUserFacingMessage returns correct public ObjectOperation for all operation types', function () {
84178422
const helper = this.test.helper;
84188423
const client = RealtimeWithLiveObjects(helper, { autoConnect: false });
84198424
const channel = client.channels.get('channel');
84208425

8426+
helper.recordPrivateApi('write.ObjectOperation.counterCreateWithObjectId._derivedFrom');
8427+
helper.recordPrivateApi('write.ObjectOperation.mapCreateWithObjectId._derivedFrom');
84218428
const scenarios = [
84228429
{
84238430
description: 'MAP_CREATE',
@@ -8433,6 +8440,31 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
84338440
map: { semantics: 'lww', entries: { foo: { tombstone: false, data: { value: 'bar' } } } },
84348441
},
84358442
},
8443+
{
8444+
description: 'MAP_CREATE with mapCreateWithObjectId._derivedFrom',
8445+
operation: {
8446+
action: 0,
8447+
objectId: 'obj-1',
8448+
mapCreateWithObjectId: {
8449+
nonce: '1234567890',
8450+
initialValue: JSON.stringify({
8451+
semantics: 0,
8452+
entries: { foo: { tombstone: false, data: { string: 'bar' } } },
8453+
}),
8454+
_derivedFrom: {
8455+
semantics: 0,
8456+
entries: { foo: { tombstone: false, data: { string: 'bar' } } },
8457+
},
8458+
},
8459+
},
8460+
expectedCurrentFields: {
8461+
mapCreate: { semantics: 'lww', entries: { foo: { tombstone: false, data: { string: 'bar' } } } },
8462+
},
8463+
expectedDeprecatedFields: {
8464+
map: { semantics: 'lww', entries: { foo: { tombstone: false, data: { value: 'bar' } } } },
8465+
},
8466+
expectedAbsentFields: ['mapCreateWithObjectId'],
8467+
},
84368468
{
84378469
description: 'MAP_SET',
84388470
operation: { action: 1, objectId: 'obj-1', mapSet: { key: 'foo', value: { string: 'bar' } } },
@@ -8451,25 +8483,52 @@ define(['ably', 'shared_helper', 'chai', 'liveobjects', 'liveobjects_helper'], f
84518483
expectedCurrentFields: { counterCreate: { count: 42 } },
84528484
expectedDeprecatedFields: { counter: { count: 42 } },
84538485
},
8486+
{
8487+
description: 'COUNTER_CREATE with counterCreateWithObjectId._derivedFrom',
8488+
operation: {
8489+
action: 3,
8490+
objectId: 'obj-2',
8491+
counterCreateWithObjectId: {
8492+
nonce: '1234567890',
8493+
initialValue: JSON.stringify({ count: 42 }),
8494+
_derivedFrom: { count: 42 },
8495+
},
8496+
},
8497+
expectedCurrentFields: { counterCreate: { count: 42 } },
8498+
expectedDeprecatedFields: { counter: { count: 42 } },
8499+
expectedAbsentFields: ['counterCreateWithObjectId'],
8500+
},
84548501
{
84558502
description: 'COUNTER_INC',
84568503
operation: { action: 4, objectId: 'obj-2', counterInc: { number: 5 } },
84578504
expectedCurrentFields: { counterInc: { number: 5 } },
84588505
expectedDeprecatedFields: { counterOp: { amount: 5 } },
84598506
},
8507+
{
8508+
description: 'OBJECT_DELETE',
8509+
operation: { action: 5, objectId: 'obj-3', objectDelete: {} },
8510+
expectedCurrentFields: { objectDelete: {} },
8511+
},
84608512
];
84618513

8462-
for (const { description, operation, expectedCurrentFields, expectedDeprecatedFields } of scenarios) {
8514+
for (const {
8515+
description,
8516+
operation,
8517+
expectedCurrentFields,
8518+
expectedDeprecatedFields,
8519+
expectedAbsentFields,
8520+
} of scenarios) {
84638521
helper.recordPrivateApi('call.ObjectMessage.fromValues');
84648522
const msg = objectMessageFromValues({ operation });
84658523
helper.recordPrivateApi('call.ObjectMessage.toUserFacingMessage');
84668524
const result = msg.toUserFacingMessage(channel);
84678525

8468-
for (const [field, expected] of Object.entries(expectedCurrentFields)) {
8469-
expectDeepSubset(result.operation[field], expected, `${description}: check current field '${field}'`);
8526+
expectDeepSubset(result.operation, expectedCurrentFields, `${description}: Check current fields`);
8527+
if (expectedDeprecatedFields) {
8528+
expectDeepSubset(result.operation, expectedDeprecatedFields, `${description}: Check deprecated fields`);
84708529
}
8471-
for (const [field, expected] of Object.entries(expectedDeprecatedFields)) {
8472-
expectDeepSubset(result.operation[field], expected, `${description}: check deprecated field '${field}'`);
8530+
for (const field of expectedAbsentFields ?? []) {
8531+
expect(result.operation[field], `${description}: check '${field}' is not set`).to.not.exist;
84738532
}
84748533
}
84758534
});

0 commit comments

Comments
 (0)