-
Notifications
You must be signed in to change notification settings - Fork 3
[PUB-1041] OBJECT_SYNC spec #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d10e385 to
3336d98
Compare
36792eb to
cdb9d58
Compare
cdb9d58 to
f4dd5d4
Compare
f4dd5d4 to
b4d8e4d
Compare
b4d8e4d to
948ad9b
Compare
bebe5fa to
5a6893a
Compare
948ad9b to
39111b8
Compare
39111b8 to
8a7c38e
Compare
8a7c38e to
2335a1d
Compare
Based on [1] at dd25dca. The development approach here was roughly the following: I decided the internal types and interfaces that I wanted to exist, and some of the implementation details (e.g. the mutations that DefaultRealtimeObjects performs during a sync sequence). I then asked Cursor to fill in the most of the implementation and tests (providing it with the relevant spec text). I have done some tweaking of the code that Cursor generated, but that doesn't mean that all of the code that's here is exactly the same as code that I would write, and I'm going to leave it that way. I have looked through all (and changed some) of the tests that Cursor generated, and in doing so also fixed the spec conformance tags (because it got those very wrong), so I am _fairly_ confident that the tests are testing what they claim to test. But I have not looked at them in minute detail (in particular, the exact data created by the canned data factories that it created). I also think that tests are one of those things where there's not always one right way to write them, so I'm not going to get hung up on "could it have done this test differently?" or "are the tests consistent with each other?" at the moment. There are some outstanding questions on the spec PR at the moment; have implemented to best of my understanding, and will update later once those are answered. Not yet implemented: - Checking of channel modes before performing operations (RTO2 and the points that refer to it); there's an outstanding question about this at [2] [1] ably/specification#333 [2] https://github.com/ably/specification/pull/333/files#r2152297442
Based on [1] at dd25dca. The development approach here was roughly the following: I decided the internal types and interfaces that I wanted to exist, and some of the implementation details (e.g. the mutations that DefaultRealtimeObjects performs during a sync sequence). I then asked Cursor to fill in the most of the implementation and tests (providing it with the relevant spec text). I have done some tweaking of the code that Cursor generated, but that doesn't mean that all of the code that's here is exactly the same as code that I would write, and I'm going to leave it that way. I have looked through all (and changed some) of the tests that Cursor generated, and in doing so also fixed the spec conformance tags (because it got those very wrong), so I am _fairly_ confident that the tests are testing what they claim to test. But I have not looked at them in minute detail (in particular, the exact data created by the canned data factories that it created). I also think that tests are one of those things where there's not always one right way to write them, so I'm not going to get hung up on "could it have done this test differently?" or "are the tests consistent with each other?" at the moment. There are some outstanding questions on the spec PR at the moment; have implemented to best of my understanding, and will update later once those are answered. Not yet implemented: - Checking of channel modes before performing operations (RTO2 and the points that refer to it); there's an outstanding question about this at [2] A note re the MutableState pattern that I've used here — done this so that the class can essentially have mutating instance methods which can call each other without having to worry about whose responsibility it is to acquire the mutex. [1] ably/specification#333 [2] https://github.com/ably/specification/pull/333/files#r2152297442
| **** @(RTO5c1a)@ If an object with @ObjectState.objectId@ exists in the internal @ObjectsPool@: | ||
| ***** @(RTO5c1a1)@ Override the internal data for the object as per "RTLC6":#RTLC6, "RTLM6":#RTLM6 | ||
| **** @(RTO5c1b)@ If an object with @ObjectState.objectId@ does not exist in the internal @ObjectsPool@: | ||
| ***** @(RTO5c1b1)@ Create a new @LiveObject@ using the data from @ObjectState@ and add it to the internal @ObjectsPool@: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we already have objectState.objectId, why can't we create zero-value liveobject from the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like an alternative and duplicate of RTO6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time, RTO6 allow us to validate ObjectId properly, which is missing in current case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTO5c1b1 makes sense as the object creation here depends on ObjectState to have a correct field (ObjectState.counter for a counter and ObjectState.map for a map) to decide what object to create. If SDK can't find a recognizable field, the SDK can't process such ObjectState message and logs a warning instead (RTO5c1b1c).
Also, since ObjectState.objectId and ObjectState.counter/ObjectState.map are part of the same ObjectState object, the spec expects that realtime has sent a correct objectId that matches data from ObjectState.counter/ObjectState.map.
| ** @(RTO3a)@ @ObjectsPool@ is a @Dict<String, LiveObject>@ - a map of @LiveObject@s keyed by "@objectId@":../features#OST2a string | ||
| ** @(RTO3b)@ It must always contain a @LiveMap@ object with id @root@ | ||
| * @(RTO4)@ When a channel @ATTACHED@ @ProtocolMessage@ is received, the @ProtocolMessage@ may contain a @HAS_OBJECTS@ bit flag indicating that it will perform an objects sync, see "TR3":../features#TR3 . Note that this does not imply that objects are definitely present on the channel, only that there may be; the @OBJECT_SYNC@ message may be empty | ||
| ** @(RTO4a)@ If the @HAS_OBJECTS@ flag is 1, the server will shortly perform an @OBJECT_SYNC@ sequence as described in "RTO5":#RTO5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ably-js, we check if objects state is INITIALIZED, so irrespective of hasObjects, we always call startNewSync in such a case, can we include this case here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check in ably-js you mentioned is only required so that library emits "syncing" -> "synced" events (in that order) for lifecycle events even if HAS_OBJECTS flag was false.
The lifecycle events are not part of this spec and will be added in the later PR, which will also address this spec point. Will work on the lifecycle events PR as soon as we finish addressing the outstanding comments on existing PRs.
In relation to this PR, it's fine for the spec to only check HAS_OBJECTS flag
dd25dca to
fc46aed
Compare
bfcf05b to
b3e06dc
Compare
sacOO7
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ( You can recheck all review comments in case )
This is the spec equivalent of the object sync sequence handling in ably-js https://github.com/ably/ably-js/blob/main/src/plugins/objects/objects.ts#L215
Resolves PUB-1041