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

Use a pull request flow instead of an update-file-contents flow #3

Open
domenic opened this issue Dec 10, 2017 · 0 comments
Open

Use a pull request flow instead of an update-file-contents flow #3

domenic opened this issue Dec 10, 2017 · 0 comments

Comments

@domenic
Copy link
Member

domenic commented Dec 10, 2017

This code, and its counterpart below for entities, is bad:

const [privateFile, publicFile] = await Promise.all([
jsonGitHubDatabase.get("individual-private"),
jsonGitHubDatabase.get("individual-public")
]);
const { gitHubID } = publicData.info;
const { email } = privateData.info;
for (const existingPrivateEntry of privateFile.content) {
if (existingPrivateEntry.info.email === email) {
throw new Conflict(`Email ${email} is already present in the system.`);
}
}
for (const existingPublicEntry of publicFile.content) {
if (existingPublicEntry.info.gitHubID === gitHubID) {
throw new Conflict(`GitHub ID ${gitHubID} is already present in the system.`);
}
}
privateFile.content.push(privateData);
publicFile.content.push(publicData);
const commitMessage = `Add data for ${publicData.info.name}`;
// Use sequential await, and not Promise.all, as it's important that we don't commit the public
// data unless we have the corresponding private data.
await jsonGitHubDatabase.update(
"individual-private", commitMessage, privateFile.sha, privateFile.content);
await jsonGitHubDatabase.update(
"individual-public", commitMessage, publicFile.sha, publicFile.content);

In particular, if a modification occurs to the file contents between them being fetched and them being updated (between lines 6/7 and lines 32/35), those modifications will be overwritten. (There is no "lock" on the "database".)

Fortunately this is not the end of the world as if someone signs the form, but then it gets overridden, this problem will be easily seen in Git history, and can be corrected by humans. And unlike many production databases, we don't expect this to have high QPS, so it's unlikely to happen anyway.

Still, this code is bad and I feel bad for writing it.

The correct fix here is to use Git's conflict resolution mechanism. That is, instead of using the update-file-contents part of the GitHub API, use the submit-and-merge-a-pull-request part of the GitHub API. This would still be done with no human intervention, but it would go through the merge process, thus guaranteeing either a merge conflict or a successful merge.

This has the added benefit that you can watch the participant-data repository to see new entities and individuals get added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant