Skip to content

Commit

Permalink
Deduplicate new puzzles by URL.
Browse files Browse the repository at this point in the history
When adding a puzzle, we start a transaction to atomically check if a
puzzle already exists with that URL and only add it if either no such
puzzle exists or if the client indicates that duplicate URLs are
allowed. In the event of a transaction failure, we delete the Google
document that was created optimistically before saving the puzzle.

On the client side, we add a new checkbox to allow duplicate URLs,
unchecked by default. If we encounter a duplicate URL error, we show an
error indicating that the puzzle may be a dupe and telling the user what
to check if the duplicate URLs are intentional.
  • Loading branch information
jpd236 committed Dec 1, 2024
1 parent 0a8dbc0 commit 4f3e4c0
Show file tree
Hide file tree
Showing 7 changed files with 131 additions and 8 deletions.
54 changes: 52 additions & 2 deletions imports/client/components/PuzzleModalForm.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Meteor } from "meteor/meteor";
import React, {
Suspense,
useCallback,
Expand All @@ -9,6 +10,7 @@ import React, {
} from "react";
import Alert from "react-bootstrap/Alert";
import Col from "react-bootstrap/Col";
import FormCheck from "react-bootstrap/FormCheck";
import type { FormControlProps } from "react-bootstrap/FormControl";
import FormControl from "react-bootstrap/FormControl";
import FormGroup from "react-bootstrap/FormGroup";
Expand Down Expand Up @@ -38,6 +40,7 @@ export interface PuzzleModalFormSubmitPayload {
tags: string[];
docType?: GdriveMimeTypesType;
expectedAnswerCount: number;
allowDuplicateUrls?: boolean;
}

enum PuzzleModalFormSubmitState {
Expand Down Expand Up @@ -93,6 +96,9 @@ const PuzzleModalForm = React.forwardRef(
const [expectedAnswerCount, setExpectedAnswerCount] = useState<number>(
puzzle ? puzzle.expectedAnswerCount : 1,
);
const [allowDuplicateUrls, setAllowDuplicateUrls] = useState<
boolean | undefined
>(puzzle ? undefined : false);
const [submitState, setSubmitState] = useState<PuzzleModalFormSubmitState>(
PuzzleModalFormSubmitState.IDLE,
);
Expand Down Expand Up @@ -157,6 +163,13 @@ const PuzzleModalForm = React.forwardRef(
setExpectedAnswerCountDirty(true);
}, []);

const onAllowDuplicateUrlsChange = useCallback(
(event: React.ChangeEvent<HTMLInputElement>) => {
setAllowDuplicateUrls(event.currentTarget.checked);
},
[],
);

const onFormSubmit = useCallback(
(callback: () => void) => {
setSubmitState(PuzzleModalFormSubmitState.SUBMITTING);
Expand All @@ -170,9 +183,24 @@ const PuzzleModalForm = React.forwardRef(
if (docType) {
payload.docType = docType;
}
if (allowDuplicateUrls) {
payload.allowDuplicateUrls = allowDuplicateUrls;
}
onSubmit(payload, (error) => {
if (error) {
setErrorMessage(error.message);
if (
error instanceof Meteor.Error &&
typeof error.error === "number" &&
error.error === 409
) {
setErrorMessage(
"A puzzle already exists with this URL - did someone else already add this" +
' puzzle? To force creation anyway, check the "Allow puzzles with identical' +
' URLs" box above and try again.',
);
} else {
setErrorMessage(error.message);
}
setSubmitState(PuzzleModalFormSubmitState.FAILED);
} else {
setSubmitState(PuzzleModalFormSubmitState.IDLE);
Expand All @@ -181,11 +209,21 @@ const PuzzleModalForm = React.forwardRef(
setUrlDirty(false);
setTagsDirty(false);
setExpectedAnswerCountDirty(false);
setAllowDuplicateUrls(false);
callback();
}
});
},
[onSubmit, huntId, title, url, tags, expectedAnswerCount, docType],
[
onSubmit,
huntId,
title,
url,
tags,
expectedAnswerCount,
docType,
allowDuplicateUrls,
],
);

const show = useCallback(() => {
Expand Down Expand Up @@ -278,6 +316,17 @@ const PuzzleModalForm = React.forwardRef(
</FormGroup>
) : null;

const allowDuplicateUrlsCheckbox =
!puzzle && typeof allowDuplicateUrls === "boolean" ? (
<FormCheck
label="Allow puzzles with identical URLs"
type="checkbox"
disabled={disableForm}
onChange={onAllowDuplicateUrlsChange}
className="mt-1"
/>
) : null;

return (
<Suspense
fallback={
Expand Down Expand Up @@ -320,6 +369,7 @@ const PuzzleModalForm = React.forwardRef(
onChange={onUrlChange}
value={currentUrl}
/>
{allowDuplicateUrlsCheckbox}
</Col>
</FormGroup>

Expand Down
9 changes: 6 additions & 3 deletions imports/lib/models/Model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type {
IndexDirection,
IndexSpecification,
CreateIndexesOptions,
ClientSession,
} from "mongodb";
import { z } from "zod";
import { IsInsert, IsUpdate, IsUpsert, stringId } from "./customTypes";
Expand Down Expand Up @@ -447,6 +448,7 @@ class Model<
doc: z.input<this["schema"]>,
options: {
bypassSchema?: boolean | undefined;
session?: ClientSession | undefined;
} = {},
): Promise<z.output<IdSchema>> {
const { bypassSchema } = options;
Expand All @@ -456,9 +458,10 @@ class Model<
raw = { ...doc, _id: this.collection._makeNewID() };
}
try {
await this.collection
.rawCollection()
.insertOne(raw, { bypassDocumentValidation: true });
await this.collection.rawCollection().insertOne(raw, {
bypassDocumentValidation: true,
session: options.session,
});
return raw._id;
} catch (e) {
formatValidationError(e);
Expand Down
1 change: 1 addition & 0 deletions imports/methods/createPuzzle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export default new TypedMethod<
tags: string[];
expectedAnswerCount: number;
docType: GdriveMimeTypesType;
allowDuplicateUrls?: boolean;
},
string
>("Puzzles.methods.create");
1 change: 1 addition & 0 deletions imports/methods/updatePuzzle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export default new TypedMethod<
url?: string;
tags: string[];
expectedAnswerCount: number;
allowDuplicateUrls?: boolean;
},
void
>("Puzzles.methods.update");
23 changes: 23 additions & 0 deletions imports/server/gdrive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,16 @@ async function createDocument(
return fileId;
}

async function deleteDocument(id: string) {
await checkClientOk();
if (!GoogleClient.drive)
throw new Meteor.Error(500, "Google integration is disabled");

await GoogleClient.drive.files.delete({
fileId: id,
});
}

export async function moveDocument(id: string, newParentId: string) {
await checkClientOk();
if (!GoogleClient.drive)
Expand Down Expand Up @@ -301,3 +311,16 @@ export async function ensureDocument(

return doc!;
}

export async function deleteUnusedDocument(puzzle: { _id: string }) {
const doc = await Documents.findOneAsync({ puzzle: puzzle._id });
if (!doc) {
return;
}

await checkClientOk();
await withLock(`puzzle:${puzzle._id}:documents`, async () => {
await deleteDocument(doc.value.id);
await Documents.removeAsync(doc._id);
});
}
48 changes: 45 additions & 3 deletions imports/server/methods/createPuzzle.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { check, Match } from "meteor/check";
import { Meteor } from "meteor/meteor";
import { MongoInternals } from "meteor/mongo";
import { Random } from "meteor/random";
import Flags from "../../Flags";
import Logger from "../../Logger";
Expand All @@ -11,7 +12,7 @@ import Puzzles from "../../lib/models/Puzzles";
import { userMayWritePuzzlesForHunt } from "../../lib/permission_stubs";
import createPuzzle from "../../methods/createPuzzle";
import GlobalHooks from "../GlobalHooks";
import { ensureDocument } from "../gdrive";
import { deleteUnusedDocument, ensureDocument } from "../gdrive";
import getOrCreateTagByName from "../getOrCreateTagByName";
import GoogleClient from "../googleClientRefresher";
import defineMethod from "./defineMethod";
Expand All @@ -27,11 +28,20 @@ defineMethod(createPuzzle, {
docType: Match.OneOf(
...(Object.keys(GdriveMimeTypes) as GdriveMimeTypesType[]),
),
allowDuplicateUrls: Match.Optional(Boolean),
});
return arg;
},

async run({ huntId, title, tags, expectedAnswerCount, docType, url }) {
async run({
huntId,
title,
tags,
expectedAnswerCount,
docType,
url,
allowDuplicateUrls,
}) {
check(this.userId, String);

const hunt = await Hunts.findOneAsync(huntId);
Expand Down Expand Up @@ -81,7 +91,39 @@ defineMethod(createPuzzle, {
await ensureDocument(fullPuzzle, docType);
}

await Puzzles.insertAsync(fullPuzzle);
// In a transaction, look for a puzzle with the same URL. If present, we
// reject the insertion unless the client overrides it.
const client = MongoInternals.defaultRemoteCollectionDriver().mongo.client;
const session = client.startSession();
try {
await session.withTransaction(async () => {
if (url) {
const existingPuzzleWithUrl = await Puzzles.collection
.rawCollection()
.findOne({ hunt: huntId, url }, { session });
if (existingPuzzleWithUrl && !allowDuplicateUrls) {
throw new Meteor.Error(
409,
`Puzzle with URL ${url} already exists`,
);
}
}
await Puzzles.insertAsync(fullPuzzle, { session });
});
} catch (error) {
// In the case of any error, try to delete the document we created before the transaction.
// If that fails too, let the original error propagate.
try {
await deleteUnusedDocument(fullPuzzle);
} catch (deleteError) {
Logger.warn("Unable to clean up document on failed puzzle creation", {
error: deleteError,
});
}
throw error;
} finally {
await session.endSession();
}

// Run any puzzle-creation hooks, like creating a default document
// attachment or announcing the puzzle to Slack.
Expand Down
3 changes: 3 additions & 0 deletions imports/server/methods/updatePuzzle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ defineMethod(updatePuzzle, {
url: Match.Optional(String),
tags: [String],
expectedAnswerCount: Number,
// We accept this argument since it's provided by the form, but it's not checked here - only
// during puzzle creation, to avoid duplicates when creating new puzzles.
allowDuplicateUrls: Match.Optional(Boolean),
});

return arg;
Expand Down

0 comments on commit 4f3e4c0

Please sign in to comment.