Skip to content
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

Add support for compensating writes #5599

Merged
merged 22 commits into from
Mar 23, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* None

### Enhancements
* Added new error class `CompensatingWriteError`, that that indicates that one or more object changes have been reverted by the server.
papafe marked this conversation as resolved.
Show resolved Hide resolved
elle-j marked this conversation as resolved.
Show resolved Hide resolved
This can happen when the client creates/updates objects that do not match any subscription, or performs writes on an object it didn't have permission to ([#5599](https://github.com/realm/realm-js/pull/5599)).
papafe marked this conversation as resolved.
Show resolved Hide resolved
* Added configuration option `SyncConfiguration.cancelWaitsOnNonFatalError`. Set to true, all async operations (such as opening the Realm with `Realm.open`) will fail when a non-fatal error, such as a timeout, occurs.
* Added an overload to `Object.linkingObjects` method that takes type of the linking object as an input instead of its string name ([#5326](https://github.com/realm/realm-js/issues/5326))
Example usage:
Expand Down
87 changes: 86 additions & 1 deletion integration-tests/tests/src/tests/sync/flexible.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,17 @@ import {
BSON,
ClientResetMode,
ConfigurationWithSync,
ErrorCallback,
FlexibleSyncConfiguration,
Realm,
SessionStopPolicy,
CompensatingWriteError,
CompensatingWriteErrorInfo,
SyncConfiguration,
} from "realm";

import { authenticateUserBefore, importAppBefore, openRealmBeforeEach } from "../../hooks";
import { DogSchema, IPerson, PersonSchema } from "../../schemas/person-and-dog-with-object-ids";
import { DogSchema, IPerson, PersonSchema, IDog } from "../../schemas/person-and-dog-with-object-ids";
import { closeRealm } from "../../utils/close-realm";
import { expectClientResetError } from "../../utils/expect-sync-error";
import { createSyncConfig } from "../../utils/open-realm";
Expand Down Expand Up @@ -396,6 +399,88 @@ describe.skipIf(environment.missingServer, "Flexible sync", function () {
});
});

describe("Errors", () => {
papafe marked this conversation as resolved.
Show resolved Hide resolved
it("raises compensating writes errors", async function () {
const person1Id = new BSON.ObjectId("0000002a9a7969d24bea4cf5");
const person2Id = new BSON.ObjectId("0000002a9a7969d24bea4cf6");
const dogId = new BSON.ObjectId("0000002a9a7969d24bea4cf7");
papafe marked this conversation as resolved.
Show resolved Hide resolved

const promise = new Promise<void>((resolve, _) => {
(async () => {
papafe marked this conversation as resolved.
Show resolved Hide resolved
const errorCallback: ErrorCallback = (_, error) => {
expect(error.code).to.equal(231);
expect(error.isFatal).to.be.false;
expect(error.message).to.contain(
"Client attempted a write that is outside of permissions or query filters; it has been reverted",
);

if (!(error instanceof CompensatingWriteError)) {
throw new Error("Expected a CompensatingWriteError");
}

expect(error.compensatingWrites.length).to.equal(3);

const compensatingWrites = error.compensatingWrites.sort((a, b) =>
(a.primaryKey as BSON.ObjectId).toString().localeCompare((b.primaryKey as BSON.ObjectId).toString()),
);

expect((compensatingWrites[0].primaryKey as BSON.ObjectId).equals(person1Id)).to.be.true;
expect((compensatingWrites[1].primaryKey as BSON.ObjectId).equals(person2Id)).to.be.true;
expect((compensatingWrites[2].primaryKey as BSON.ObjectId).equals(dogId)).to.be.true;

expect(compensatingWrites[0].objectName).to.equal(FlexiblePersonSchema.name);
expect(compensatingWrites[1].objectName).to.equal(FlexiblePersonSchema.name);
expect(compensatingWrites[2].objectName).to.equal(DogSchema.name);

expect(compensatingWrites[0].reason).to.contain("object is outside of the current query view");
expect(compensatingWrites[1].reason).to.contain("object is outside of the current query view");
expect(compensatingWrites[2].reason).to.contain("object is outside of the current query view");

resolve();
};

const realm = await Realm.open({
schema: [FlexiblePersonSchema, DogSchema],
sync: {
flexible: true,
user: this.user,
onError: errorCallback,
},
});

await realm.subscriptions.update((mutableSubs) => {
mutableSubs.add(realm.objects(FlexiblePersonSchema.name).filtered("age < 30"));
mutableSubs.add(realm.objects(DogSchema.name).filtered("age > 5"));
});

realm.write(() => {
//Outside subscriptions
const tom = realm.create<IPerson>(FlexiblePersonSchema.name, {
_id: person1Id,
name: "Tom",
age: 36,
});
realm.create<IPerson>(FlexiblePersonSchema.name, { _id: person2Id, name: "Maria", age: 44 });
realm.create<IDog>(DogSchema.name, { _id: dogId, name: "Puppy", age: 1, owner: tom });

//Inside subscriptions
const luigi = realm.create<IPerson>(FlexiblePersonSchema.name, {
_id: new BSON.ObjectId(),
name: "Luigi",
age: 20,
});
realm.create<IPerson>(FlexiblePersonSchema.name, { _id: new BSON.ObjectId(), name: "Mario", age: 22 });
realm.create<IDog>(DogSchema.name, { _id: new BSON.ObjectId(), name: "Oldy", age: 6, owner: luigi });
});

await realm?.syncSession?.uploadAllLocalChanges();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?all?these?question?marks? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're fun to use 😁

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it should only be needed after syncSession.

Copy link
Contributor

@takameyer takameyer Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional chaining! It replaces this code:

if( realm && realm.syncSession && realm.syncSession.uploadAllLocalChanges){
  await realm.syncSession.uploadAllLocalChanges();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe we don't even need them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just the second one, I think it's fine :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need either one. If the syncSession is undefined/null, I'd rather that this throws than silently does nothing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript will be defensive about this and have a type error if you do not have a check.

Copy link
Member

@nirinchev nirinchev Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, why is Realm.syncSession nullable but Realm.subscriptions not? In any case, this should use ! then to express that we know this is not null.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that subscriptions can just return an empty array, even if sync isn't enabled. syncSession should return null if sync isn't configured

})();
});

await promise;
});
});

describe("With realm opened before", function () {
openRealmBeforeEach({
schema: [FlexiblePersonSchema, DogSchema],
Expand Down
10 changes: 8 additions & 2 deletions packages/bindgen/spec.yml
Original file line number Diff line number Diff line change
Expand Up @@ -546,8 +546,7 @@ records:
# server_requests_action:
# type: sync::ProtocolErrorInfo::Action
# default: sync::ProtocolErrorInfo::Action::NoAction
# compensating_writes_info: std::vector<sync::CompensatingWriteErrorInfo>

compensating_writes_info: std::vector<CompensatingWriteErrorInfo>

Request:
cppName: app::Request
Expand Down Expand Up @@ -591,6 +590,13 @@ records:
default_request_timeout_ms: util::Optional<uint64_t>
device_info: DeviceInfo

CompensatingWriteErrorInfo:
cppName: sync::CompensatingWriteErrorInfo
fields:
object_name: std::string
reason: std::string
primary_key: Mixed

opaqueTypes:
- Schema
- Group
Expand Down
2 changes: 2 additions & 0 deletions packages/realm/src/Types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import { BSON, Dictionary, List, RealmSet, Results } from "./internal";

export type Mixed = unknown;
papafe marked this conversation as resolved.
Show resolved Hide resolved

const RealmDictionary = Dictionary;
type RealmDictionary<T> = Dictionary<T>;

Expand Down
48 changes: 46 additions & 2 deletions packages/realm/src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
// eslint-disable-next-line @typescript-eslint/no-unused-vars -- Used by TS docs
ClientResetMode,
Configuration,
Mixed,
binding,
} from "./internal";

Expand Down Expand Up @@ -92,7 +93,9 @@ export class TimeoutError extends Error {

/** @internal */
export function fromBindingSyncError(error: binding.SyncError) {
if (error.isClientResetRequested) {
if (error.systemError.code == 231) {
papafe marked this conversation as resolved.
Show resolved Hide resolved
return new CompensatingWriteError(error);
} else if (error.isClientResetRequested) {
return new ClientResetError(error);
} else {
return new SyncError(error);
Expand All @@ -117,7 +120,6 @@ export class SyncError extends Error {
}
}

const ORIGINAL_FILE_PATH_KEY = "ORIGINAL_FILE_PATH";
const RECOVERY_FILE_PATH_KEY = "RECOVERY_FILE_PATH";

/**
Expand All @@ -138,3 +140,45 @@ export class ClientResetError extends SyncError {
};
}
}

/**
* An error class that indicates that one or more object changes have been reverted by the server.
papafe marked this conversation as resolved.
Show resolved Hide resolved
* This can happen when the client creates/updates objects that do not match any subscription, or performs writes on
* an object it didn't have permission to.
*/
export class CompensatingWriteError extends SyncError {
/**
* The array of compensating writes performed by the server.
papafe marked this conversation as resolved.
Show resolved Hide resolved
takameyer marked this conversation as resolved.
Show resolved Hide resolved
*/
public infos: CompensatingWriteInfo[] = [];
papafe marked this conversation as resolved.
Show resolved Hide resolved
papafe marked this conversation as resolved.
Show resolved Hide resolved

/** @internal */
constructor(error: binding.SyncError) {
super(error);
if (error.compensatingWritesInfo) {
papafe marked this conversation as resolved.
Show resolved Hide resolved
error.compensatingWritesInfo.forEach((element) => {
this.infos.push({ objectName: element.objectName, reason: element.reason, primaryKey: element.primaryKey });
});
papafe marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

/**
* The details of a compensating write performed by the server.
*/
export type CompensatingWriteInfo = {
/**
* The type of the object affected by the compensating write.
papafe marked this conversation as resolved.
Show resolved Hide resolved
*/
objectName: string;

/**
* The reason for the compensating write.
*/
reason: string;

/**
* The primary key of the object affected by the compensating write.
papafe marked this conversation as resolved.
Show resolved Hide resolved
*/
primaryKey: Mixed;
};
2 changes: 2 additions & 0 deletions packages/realm/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ export {
CollectionChangeCallback,
CollectionChangeSet,
CollectionPropertyTypeName,
CompensatingWriteError,
CompensatingWriteInfo,
Configuration,
ConfigurationWithoutSync,
ConfigurationWithSync,
Expand Down