Skip to content

Commit

Permalink
bugfixes to sync
Browse files Browse the repository at this point in the history
  • Loading branch information
zadam committed Dec 29, 2023
1 parent 8dbc592 commit f704cac
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 10 deletions.
14 changes: 14 additions & 0 deletions db/migrations/0228__fix_blobIds.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
UPDATE blobs SET blobId = REPLACE(blobId, '+', 'X');
UPDATE blobs SET blobId = REPLACE(blobId, '/', 'Y');

UPDATE notes SET blobId = REPLACE(blobId, '+', 'X');
UPDATE notes SET blobId = REPLACE(blobId, '/', 'Y');

UPDATE attachments SET blobId = REPLACE(blobId, '+', 'X');
UPDATE attachments SET blobId = REPLACE(blobId, '/', 'Y');

UPDATE revisions SET blobId = REPLACE(blobId, '+', 'X');
UPDATE revisions SET blobId = REPLACE(blobId, '/', 'Y');

UPDATE entity_changes SET entityId = REPLACE(entityId, '+', 'X') WHERE entityName = 'blobs';
UPDATE entity_changes SET entityId = REPLACE(entityId, '/', 'Y') WHERE entityName = 'blobs';
4 changes: 2 additions & 2 deletions src/services/app_info.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ const build = require('./build.js');
const packageJson = require('../../package.json');
const {TRILIUM_DATA_DIR} = require('./data_dir.js');

const APP_DB_VERSION = 227;
const SYNC_VERSION = 31;
const APP_DB_VERSION = 228;
const SYNC_VERSION = 32;
const CLIPPER_PROTOCOL_VERSION = "1.0";

module.exports = {
Expand Down
4 changes: 3 additions & 1 deletion src/services/blob.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,14 @@ const utils = require('./utils.js');

function getBlobPojo(entityName, entityId) {
const entity = becca.getEntity(entityName, entityId);

if (!entity) {
throw new NotFoundError(`Entity ${entityName} '${entityId}' was not found.`);
}

const blob = becca.getBlob(entity);
if (!blob) {
throw new NotFoundError(`Blob ${entity.blobId} for ${entityName} '${entityId}' was not found.`);
}

const pojo = blob.getPojo();

Expand Down
3 changes: 3 additions & 0 deletions src/services/consistency_checks.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ const {sanitizeAttributeName} = require('./sanitize_attribute_name.js');
const noteTypes = require('../services/note_types.js').getNoteTypeNames();

class ConsistencyChecks {
/**
* @param autoFix - automatically fix all encountered problems. False is only for debugging during development (fail fast)
*/
constructor(autoFix) {
this.autoFix = autoFix;
this.unrecoveredConsistencyErrors = false;
Expand Down
2 changes: 2 additions & 0 deletions src/services/erase.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ function eraseNotes(noteIdsToErase) {
function setEntityChangesAsErased(entityChanges) {
for (const ec of entityChanges) {
ec.isErased = true;
// we're not changing hash here, not sure if good or not
// content hash check takes isErased flag into account, though
ec.utcDateChanged = dateUtils.utcNowDateTime();

entityChangesService.putEntityChangeWithForcedChange(ec);
Expand Down
7 changes: 5 additions & 2 deletions src/services/migration.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@ async function migrate() {
}
});

log.info("VACUUMing database, this might take a while ...");
sql.execute("VACUUM");
if (currentDbVersion === 214) {
// special VACUUM after the big migration
log.info("VACUUMing database, this might take a while ...");
sql.execute("VACUUM");
}
}

function executeMigration(mig) {
Expand Down
10 changes: 7 additions & 3 deletions src/services/sync_update.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,16 @@ function updateNormalEntity(remoteEC, remoteEntityRow, instanceId, updateContext
updateContext.updated[remoteEC.entityName].push(remoteEC.entityId);
}

if (!localEC || localEC.utcDateChanged < remoteEC.utcDateChanged || localEC.hash !== remoteEC.hash) {
if (!localEC || localEC.utcDateChanged < remoteEC.utcDateChanged
|| localEC.hash !== remoteEC.hash
|| localEC.isErased !== remoteEC.isErased
) {
entityChangesService.putEntityChangeWithInstanceId(remoteEC, instanceId);
}

return true;
} else if (localEC.hash !== remoteEC.hash && localEC.utcDateChanged > remoteEC.utcDateChanged) {
} else if ((localEC.hash !== remoteEC.hash || localEC.isErased !== remoteEC.isErased)
&& localEC.utcDateChanged > remoteEC.utcDateChanged) {
// the change on our side is newer than on the other side, so the other side should update
entityChangesService.putEntityChangeForOtherInstances(localEC);

Expand Down Expand Up @@ -148,7 +152,7 @@ function eraseEntity(entityChange) {
];

if (!entityNames.includes(entityName)) {
log.error(`Cannot erase entity '${entityName}', id '${entityId}'.`);
log.error(`Cannot erase ${entityName} '${entityId}'.`);
return;
}

Expand Down
4 changes: 2 additions & 2 deletions src/services/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ function hashedBlobId(content) {

// we don't want such + and / in the IDs
const kindaBase62Hash = base64Hash
.replace('+', 'X')
.replace('/', 'Y');
.replaceAll('+', 'X')
.replaceAll('/', 'Y');

// 20 characters of base62 gives us ~120 bit of entropy which is plenty enough
return kindaBase62Hash.substr(0, 20);
Expand Down

3 comments on commit f704cac

@u1735067
Copy link

Choose a reason for hiding this comment

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

Shouldn't the base64url encoding (https://en.wikipedia.org/wiki/Base64 with - and _ instead of + and / ; or another one) be used here, instead of performing manual replacement?
For example, if the sha512 digest contains the following sequence (in hex) abcfffe5589, it's q8//5VgJ in base64, and it would become q8YY5VgJ after replacement.
But it's the same base64 value as the sequence (in hex) abc618e5589: q8YY5VgJ

@zadam
Copy link
Owner Author

@zadam zadam commented on f704cac Jan 9, 2024

Choose a reason for hiding this comment

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

Basically I'm trying to avoid any non-alphanumeric characters and this is a stupid way to do it.

This hash is not security sensitive, so I believe it's OK.

@u1735067
Copy link

Choose a reason for hiding this comment

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

Maybe use https://github.com/cryptocoinjs/base-x (or b62/base32, but base-x seems to be faster according to https://stackoverflow.com/questions/30468292/how-to-generate-base62-uuids-in-node-js / https://gist.github.com/dmarcelino/879d4da2a0e0c32f7d74)?
But while it weaken the hash / increase probability of collisions, I won't insist, it's just it looked weird to me (and I found this because I'm still on an old version and I've seen reports of corruptions in newer versions).

Please sign in to comment.