Skip to content

Commit 3b0c9ba

Browse files
tglmantglman
committed
fix: corrected update of backup config, ported from @odysseaspenta PR
1 parent 1afbf10 commit 3b0c9ba

3 files changed

Lines changed: 210 additions & 4 deletions

File tree

‎agent/src/main/java/com/orientechnologies/agent/services/backup/OBackupConfig.java‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -183,10 +183,22 @@ protected void validateBackup(final ODocument doc) {
183183
}
184184

185185
public ODocument changeBackup(String uuid, ODocument doc) {
186-
validateBackup(doc);
187-
removeBackup(uuid);
188-
pushBackup(doc);
189-
return doc;
186+
synchronized (this) {
187+
doc.field(ID, uuid);
188+
validateBackup(doc);
189+
final Collection<ODocument> backups = configuration.field(BACKUPS);
190+
ODocument toRemove = null;
191+
for (ODocument backup : backups) {
192+
if (backup.field(ID).equals(uuid)) {
193+
toRemove = backup;
194+
}
195+
}
196+
if (toRemove != null) {
197+
backups.remove(toRemove);
198+
}
199+
pushBackup(doc);
200+
return doc;
201+
}
190202
}
191203

192204
public ODocument removeBackup(final String uuid) {

‎agent/src/main/java/com/orientechnologies/agent/services/backup/log/OBackupDBLogger.java‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,24 @@ private void initLogger() {
7777

7878
@Override
7979
public OBackupLog log(final OBackupLog log) {
80+
// Log to local file only for error operations
81+
OBackupLogType op = log.getType();
82+
if (op == OBackupLogType.BACKUP_ERROR && log instanceof OBackupErrorLog) {
83+
OBackupErrorLog errorLog = (OBackupErrorLog) log;
84+
logger.error(
85+
"Backup operation: %s, UUID: %s, UnitId: %d, TxId: %d, DbName: %s, Mode: %s, Timestamp:"
86+
+ " %s, Message: %s",
87+
null,
88+
op,
89+
log.getUuid(),
90+
log.getUnitId(),
91+
log.getTxId(),
92+
log.getDbName(),
93+
log.getMode(),
94+
log.getTimestamp(),
95+
errorLog.getMessage());
96+
}
97+
8098
return getDatabase()
8199
.executeWithDB(
82100
session -> {

‎agent/src/test/java/com/orientechnologies/agent/services/backup/OBackupConfigTest.java‎

Lines changed: 176 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,176 @@
1+
/*
2+
* Copyright 2016 OrientDB LTD (info(at)orientdb.com)
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*
16+
* For more information: http://www.orientdb.com
17+
*/
18+
19+
package com.orientechnologies.agent.services.backup;
20+
21+
import static org.junit.Assert.*;
22+
23+
import com.orientechnologies.common.io.OIOUtils;
24+
import com.orientechnologies.orient.core.record.impl.ODocument;
25+
import java.io.File;
26+
import java.io.IOException;
27+
import java.nio.file.Files;
28+
import java.util.Collection;
29+
import org.junit.After;
30+
import org.junit.Before;
31+
import org.junit.Test;
32+
33+
/**
34+
* Unit tests for OBackupConfig focusing on the changeBackup UUID preservation bug.
35+
*
36+
* <p>Bug: OBackupConfig.changeBackup(uuid, doc) removes the old backup entry by UUID and then
37+
* pushes the new doc without writing the UUID back onto the doc. This means any client that does
38+
* not include the "uuid" field in the PUT request body will produce a backup entry with no UUID in
39+
* both the in-memory configuration and the persisted backups.json file.
40+
*/
41+
public class OBackupConfigTest {
42+
43+
private File tempDir;
44+
private OBackupConfig config;
45+
46+
@Before
47+
public void setUp() throws IOException {
48+
tempDir = Files.createTempDirectory("backup-config-test").toFile();
49+
new File(tempDir, "config").mkdirs();
50+
// OBackupConfig.load() resolves "${ORIENTDB_HOME}/config/backups.json"
51+
System.setProperty("ORIENTDB_HOME", tempDir.getAbsolutePath());
52+
config = new OBackupConfig().load();
53+
}
54+
55+
@After
56+
public void tearDown() {
57+
System.clearProperty("ORIENTDB_HOME");
58+
deleteRecursively(tempDir);
59+
}
60+
61+
// ---------------------------------------------------------------------------
62+
// Helpers
63+
// ---------------------------------------------------------------------------
64+
65+
private ODocument buildBackupDoc() {
66+
ODocument modes = new ODocument();
67+
ODocument fullMode = new ODocument();
68+
fullMode.field("when", "0 0/1 * * * ?");
69+
modes.field("FULL_BACKUP", fullMode);
70+
71+
ODocument doc = new ODocument();
72+
doc.field("dbName", "testDb");
73+
doc.field("directory", "/tmp/test-backup");
74+
doc.field("modes", modes);
75+
return doc;
76+
}
77+
78+
private ODocument readBackupJsonFromDisk() throws IOException {
79+
File f = new File(tempDir, "config/backups.json");
80+
return new ODocument().fromJSON(OIOUtils.readFileAsString(f), "noMap");
81+
}
82+
83+
private void deleteRecursively(File f) {
84+
if (f.isDirectory()) {
85+
for (File child : f.listFiles()) deleteRecursively(child);
86+
}
87+
f.delete();
88+
}
89+
90+
// ---------------------------------------------------------------------------
91+
// Tests
92+
// ---------------------------------------------------------------------------
93+
94+
/**
95+
* Baseline: addAndPushBackup must assign a UUID and persist it to disk. This passes and
96+
* establishes the expected contract.
97+
*/
98+
@Test
99+
public void testAddBackupPersistsUUID() throws IOException {
100+
ODocument doc = buildBackupDoc();
101+
ODocument added = config.addAndPushBackup(doc);
102+
103+
String uuid = added.field(OBackupConfig.ID);
104+
assertNotNull("addAndPushBackup must generate a UUID", uuid);
105+
106+
// Verify in-memory
107+
Collection<ODocument> backups = config.getConfig().field(OBackupConfig.BACKUPS);
108+
assertEquals(1, backups.size());
109+
assertEquals(uuid, backups.iterator().next().field(OBackupConfig.ID));
110+
111+
// Verify on disk
112+
Collection<ODocument> diskBackups = readBackupJsonFromDisk().field(OBackupConfig.BACKUPS);
113+
assertEquals(1, diskBackups.size());
114+
assertEquals(uuid, diskBackups.iterator().next().field(OBackupConfig.ID));
115+
}
116+
117+
/**
118+
* Demonstrates the bug: changeBackup does not copy the UUID parameter onto the replacement
119+
* document before persisting it.
120+
*
121+
* <p>When a PUT /backupManager/{uuid} request arrives, OServerCommandBackupManager parses the raw
122+
* request body into a fresh ODocument and passes it straight to changeBackup. If the client does
123+
* not include the "uuid" field in the body (which is a reasonable expectation given that the UUID
124+
* is already in the URL), the stored backup entry ends up with no UUID. This makes the entry
125+
* unreachable by any subsequent GET, PUT, or DELETE that keys on UUID.
126+
*/
127+
@Test
128+
public void testChangeBackupPreservesUUID() throws IOException {
129+
// 1. Add a backup and capture the generated UUID.
130+
ODocument added = config.addAndPushBackup(buildBackupDoc());
131+
String uuid = added.field(OBackupConfig.ID);
132+
assertNotNull(uuid);
133+
134+
// 2. Build an updated document that intentionally omits the UUID field,
135+
// exactly as the HTTP handler does when it parses the raw PUT body.
136+
ODocument updatedDoc = buildBackupDoc(); // no "uuid" field
137+
assertNull(
138+
"Pre-condition: the update doc must not contain a UUID",
139+
updatedDoc.field(OBackupConfig.ID));
140+
141+
// 3. Call changeBackup — the UUID is supplied via the url path parameter.
142+
config.changeBackup(uuid, updatedDoc);
143+
144+
// 4. The in-memory configuration must still carry the original UUID.
145+
Collection<ODocument> memBackups = config.getConfig().field(OBackupConfig.BACKUPS);
146+
assertEquals("Backup count must remain 1 after a change", 1, memBackups.size());
147+
String memUuid = memBackups.iterator().next().field(OBackupConfig.ID);
148+
// BUG: memUuid is null because changeBackup never calls doc.field(ID, uuid)
149+
assertEquals(
150+
"UUID in in-memory configuration must match the original UUID after changeBackup",
151+
uuid,
152+
memUuid);
153+
}
154+
155+
/**
156+
* Same bug expressed at the persistence layer: after changeBackup the UUID must be present in
157+
* backups.json, otherwise a server restart will reload entries with no UUID and all scheduler
158+
* lookups by UUID will silently fail.
159+
*/
160+
@Test
161+
public void testChangeBackupPersistsUUIDToDisk() throws IOException {
162+
ODocument added = config.addAndPushBackup(buildBackupDoc());
163+
String uuid = added.field(OBackupConfig.ID);
164+
165+
ODocument updatedDoc = buildBackupDoc(); // no "uuid" field
166+
config.changeBackup(uuid, updatedDoc);
167+
168+
Collection<ODocument> diskBackups = readBackupJsonFromDisk().field(OBackupConfig.BACKUPS);
169+
assertEquals(
170+
"Backup count in backups.json must remain 1 after a change", 1, diskBackups.size());
171+
String diskUuid = diskBackups.iterator().next().field(OBackupConfig.ID);
172+
// BUG: diskUuid is null for the same reason
173+
assertEquals(
174+
"UUID in backups.json must match the original UUID after changeBackup", uuid, diskUuid);
175+
}
176+
}

0 commit comments

Comments
 (0)