Skip to content

Commit 070501a

Browse files
john-hilljohn-hill
authored
Merge pull request #5520 from john-hill/PLFM-9481
handle case where a user updated a cell on a column that is deleted i…
2 parents ec153ba + 84b779a commit 070501a

File tree

7 files changed

+95
-30
lines changed

7 files changed

+95
-30
lines changed

‎services/repository-managers/src/main/java/org/sagebionetworks/repo/manager/grid/synch/row/CellCopyImpl.java‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.sagebionetworks.repo.manager.grid.synch.row;
22

3+
import java.util.List;
34
import java.util.Map;
45
import java.util.Set;
56
import java.util.stream.Collectors;
@@ -11,20 +12,20 @@
1112

1213
public class CellCopyImpl implements Copy<CellCopyItem, CellSourceItem> {
1314

14-
private final RowCopyItem copyItem;
15+
private final List<CellCopyItem> cells;
1516
private final Map<String, ConValue> mergeCells;
1617
private final Set<String> userDeletedCells;
1718

18-
public CellCopyImpl(RowCopyItem copyItem) {
19-
this.copyItem = copyItem;
20-
mergeCells = copyItem.getCells().stream()
21-
.collect(Collectors.toMap(CellCopyItem::getName, CellCopyItem::getValue));
19+
public CellCopyImpl(RowCopyItem copyItem, Set<String> finalColumnNames) {
20+
this.cells = copyItem.getCells().stream().filter(c -> finalColumnNames.contains(c.getName()))
21+
.collect(Collectors.toList());
22+
mergeCells = cells.stream().collect(Collectors.toMap(CellCopyItem::getName, CellCopyItem::getValue));
2223
this.userDeletedCells = getUserDeletedCells(copyItem);
2324
}
2425

2526
@Override
2627
public Stream<CellCopyItem> streamItems() {
27-
return copyItem.getCells().stream();
28+
return cells.stream();
2829
}
2930

3031
@Override
@@ -52,8 +53,8 @@ static Set<String> getUserDeletedCells(RowCopyItem copyItem) {
5253
|| ConType.NULL.equals(cell.getValue().getType())))
5354
.map(CellCopyItem::getName).collect(Collectors.toSet());
5455
}
55-
56-
public Map<String, ConValue> getMergedCells(){
56+
57+
public Map<String, ConValue> getMergedCells() {
5758
return mergeCells;
5859
}
5960

‎services/repository-managers/src/main/java/org/sagebionetworks/repo/manager/grid/synch/row/RowMergeImpl.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ public RowMergeImpl(SynchronizationLogic logic, SourceHandler sourceHandler,
127127
public void merge(String rowKey, RowCopyItem copyItem, RowSourceItemReference sourceItemRef) {
128128

129129
RowSourceItem sourceItem = sourceItemRef.fetchRow();
130-
CellCopyImpl cellCopy = new CellCopyImpl(copyItem);
130+
CellCopyImpl cellCopy = new CellCopyImpl(copyItem, columnNameMap.keySet());
131131
CellSourceImpl cellSource = new CellSourceImpl(sourceItem);
132132

133133
// Synchronize cells using nested application of SynchronizationLogic
@@ -185,8 +185,10 @@ void resetCopyRow(LogicalTimestamp vectorNodeId, Map<String, ConValue> mergeCell
185185
List<Integer> valueIndex = new ArrayList<>();
186186
for (Entry<String, ConValue> e : mergeCells.entrySet()) {
187187
Column column = columnNameMap.get(e.getKey());
188-
values.add(e.getValue());
189-
valueIndex.add(column.getVectorIndex());
188+
if(column != null) {
189+
values.add(e.getValue());
190+
valueIndex.add(column.getVectorIndex());
191+
}
190192
}
191193
intendedChangePublisher.publish(new UpdateRowChange(vectorNodeId, values, valueIndex.toArray(Integer[]::new),
192194
metadataNodeId, synapseRowConValue));

‎services/repository-managers/src/test/java/org/sagebionetworks/repo/manager/grid/synch/row/RowMergeImplTest.java‎

Lines changed: 52 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,6 @@
2727
import org.sagebionetworks.repo.manager.grid.synch.handler.SourceHandler;
2828
import org.sagebionetworks.repo.manager.grid.synch.io.RowSourceItem;
2929
import org.sagebionetworks.repo.manager.grid.synch.io.RowSourceItemReference;
30-
import org.sagebionetworks.repo.manager.grid.synch.row.CellCopyItem;
31-
import org.sagebionetworks.repo.manager.grid.synch.row.RowCopyItemImpl;
32-
import org.sagebionetworks.repo.manager.grid.synch.row.RowMergeImpl;
3330
import org.sagebionetworks.repo.model.UnauthorizedException;
3431
import org.sagebionetworks.repo.model.grid.patch.ConType;
3532
import org.sagebionetworks.repo.model.grid.patch.ConValue;
@@ -190,7 +187,7 @@ public void testMergeWithCellUpdatedInCopyAndApplyCellsThrowsIllegalArgumentExce
190187

191188
verifyNoMoreInteractionsWithAllMocks();
192189
}
193-
190+
194191
@Test
195192
public void testMergeWithCellUpdatedInCopyAndApplyCellsNotFoundException() {
196193

@@ -218,7 +215,7 @@ public void testMergeWithCellUpdatedInCopyAndApplyCellsNotFoundException() {
218215

219216
verifyNoMoreInteractionsWithAllMocks();
220217
}
221-
218+
222219
@Test
223220
public void testMergeWithCellUpdatedInCopyAndApplyCellsUnauthorizedException() {
224221

@@ -300,6 +297,56 @@ public void testMergeWithCellAddedToSource() {
300297
verifyNoMoreInteractionsWithAllMocks();
301298
}
302299

300+
@Test
301+
public void testMergeWithCellUpdedAndNotInFinalSchema() {
302+
finalSchema = List.of(new Column().setName("a").setVectorIndex(1), new Column().setName("c").setVectorIndex(2));
303+
RowMergeImpl merge = setupRowMerge();
304+
305+
RowSourceItem sourceItem = new RowSourceItem(new TreeMap<>(Map.of("a", c1, "b", c2, "c", c3)), rowKey, synRow);
306+
307+
RowCopyItemImpl copyItem = new RowCopyItemImpl().setVectorNodeId(rowVectorId).setRgaNodeId(rgaNodeId)
308+
.setMetadataNodeId(metadataNodeId)
309+
.setCells(List.of(new CellCopyItem().setName("a").setValue(c1).setWasChangedByUser(false),
310+
new CellCopyItem().setName("b").setValue(new ConValue(ConType.STRING, "deleted"))
311+
.setWasChangedByUser(true)))
312+
.setSynapseRow(synRow);
313+
314+
when(mockRowSourceItemReference.fetchRow()).thenReturn(sourceItem);
315+
316+
// call under test
317+
merge.merge(rowKey, copyItem, mockRowSourceItemReference);
318+
319+
verify(mockIntendedChangePublisher).publish(new UpdateRowChange(rowVectorId, List.of(c1, c3),
320+
new Integer[] { 1, 2 }, metadataNodeId, synRow.toConValue()));
321+
322+
verifyNoMoreInteractionsWithAllMocks();
323+
}
324+
325+
@Test
326+
public void testMergeWithCellUpdedAndNotInFinalSchemaAndMissingFromSource() {
327+
finalSchema = List.of(new Column().setName("a").setVectorIndex(1), new Column().setName("c").setVectorIndex(2));
328+
RowMergeImpl merge = setupRowMerge();
329+
330+
RowSourceItem sourceItem = new RowSourceItem(new TreeMap<>(Map.of("a", c1, "c", c3)), rowKey, synRow);
331+
332+
RowCopyItemImpl copyItem = new RowCopyItemImpl().setVectorNodeId(rowVectorId).setRgaNodeId(rgaNodeId)
333+
.setMetadataNodeId(metadataNodeId)
334+
.setCells(List.of(new CellCopyItem().setName("a").setValue(c1).setWasChangedByUser(false),
335+
new CellCopyItem().setName("b").setValue(new ConValue(ConType.STRING, "deleted"))
336+
.setWasChangedByUser(true)))
337+
.setSynapseRow(synRow);
338+
339+
when(mockRowSourceItemReference.fetchRow()).thenReturn(sourceItem);
340+
341+
// call under test
342+
merge.merge(rowKey, copyItem, mockRowSourceItemReference);
343+
344+
verify(mockIntendedChangePublisher).publish(new UpdateRowChange(rowVectorId, List.of(c1, c3),
345+
new Integer[] { 1, 2 }, metadataNodeId, synRow.toConValue()));
346+
347+
verifyNoMoreInteractionsWithAllMocks();
348+
}
349+
303350
@Test
304351
public void testMergeWithCellDeletedFromSource() {
305352
finalSchema = List.of(new Column().setName("a").setVectorIndex(1), new Column().setName("b").setVectorIndex(0));
@@ -325,7 +372,6 @@ public void testMergeWithCellDeletedFromSource() {
325372
verifyNoMoreInteractionsWithAllMocks();
326373
}
327374

328-
329375
@Test
330376
public void testMergeWithNoSynapseRow() {
331377

‎services/workers/src/test/java/org/sagebionetworks/agent/worker/AgentChatWorkerIntegrationTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
import org.junit.jupiter.api.AfterEach;
1616
import org.junit.jupiter.api.BeforeEach;
17+
import org.junit.jupiter.api.Disabled;
1718
import org.junit.jupiter.api.Test;
1819
import org.junit.jupiter.api.extension.ExtendWith;
1920
import org.sagebionetworks.AsynchronousJobWorkerHelper;
@@ -162,6 +163,7 @@ public void testGetEntityMetadataHandler() throws AssertionError, AsynchJobFaile
162163
}
163164

164165
@Test
166+
@Disabled // unstable test, see: PLFM-9486
165167
public void testGetEntityChildrenHandler() throws AssertionError, AsynchJobFailedException {
166168
Project project = entityService.createEntity(admin.getId(), new Project().setName(UUID.randomUUID().toString()),
167169
null);
@@ -192,6 +194,7 @@ public void testGetEntityChildrenHandler() throws AssertionError, AsynchJobFaile
192194
assertNotNull(response);
193195
assertEquals(session.getSessionId(), response.getSessionId());
194196
assertNotNull(response.getResponseText());
197+
195198
assertTrue(response.getResponseText().contains(f1.getName()));
196199
assertTrue(response.getResponseText().contains(f2.getName()));
197200
assertTrue(response.getResponseText().contains(f1.getId()));

‎services/workers/src/test/java/org/sagebionetworks/agent/worker/GridAgentChatWorkerIntegrationTest.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import org.json.JSONObject;
1919
import org.junit.jupiter.api.AfterEach;
2020
import org.junit.jupiter.api.BeforeEach;
21+
import org.junit.jupiter.api.Disabled;
2122
import org.junit.jupiter.api.Test;
2223
import org.junit.jupiter.api.extension.ExtendWith;
2324
import org.sagebionetworks.AsynchronousJobWorkerHelper;
@@ -423,6 +424,7 @@ public void testViewWithSchemaAndAgentChat() throws Exception {
423424
}
424425

425426
@Test
427+
@Disabled // Unstable test, see: PLFM-9487.
426428
public void testRegularExpression() throws AssertionError, Exception {
427429
createGridSessionFromCsv(
428430
new String[] { "firstName", "lastName", "phone", "formattedName", "cleanPhone" },

‎services/workers/src/test/java/org/sagebionetworks/grid/workers/GridSynchronizationIntegrationTest.java‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -198,10 +198,18 @@ public void testSynchronizeEntityView() throws Exception {
198198
// update the first row in the grid
199199
Patch patch = new Patch()
200200
.setPatchId(new LogicalTimestamp().setReplicaId(userReplica.getReplicaId()).setSequenceNumber(101L));
201-
LogicalTimestamp conId = patch
202-
.addNewOperation(Operations.newConstant().setValue(new ConValue(ConType.STRING, "oneUpdated")));
203-
patch.addNewOperation(Operations.insertVector()
204-
.setVectorId(fetchedRows.get(0).getRowObject().getData().getVectorId()).setMap(Map.of(0, conId)));
201+
202+
patch.addNewOperation(
203+
Operations.insertVector().setVectorId(fetchedRows.get(0).getRowObject().getData().getVectorId())
204+
.setMap(Map.of(
205+
// 0
206+
0, patch.addNewOperation(
207+
Operations.newConstant().setValue(new ConValue(ConType.STRING, "oneUpdated"))),
208+
// 2
209+
2, patch.addNewOperation(
210+
Operations.newConstant().setValue(new ConValue(ConType.STRING, "1.3")))
211+
212+
)));
205213
JsonRxMessage message = new JsonRxMessage(JsonRxMessageType.RequestData).setId(102).setMethod("patch")
206214
.setBody(PatchCompactSerializable.serialize(patch));
207215
websoceket.send(message.toJson());
@@ -221,7 +229,7 @@ public void testSynchronizeEntityView() throws Exception {
221229
System.out.println(results);
222230
Set<String> expected = Set.of(
223231
//
224-
"{\"theString\":\"oneUpdated\",\"theId\":111,\"toRemove\":\"1.1\"}",
232+
"{\"theString\":\"oneUpdated\",\"theId\":111,\"toRemove\":\"1.3\"}",
225233
//
226234
"{\"theString\":\"two\",\"toRemove\":\"2.2\"}",
227235
//
@@ -241,8 +249,8 @@ public void testSynchronizeEntityView() throws Exception {
241249
.put(c3Name, "1.2").put(c4Name, "oneUpdatedInSource"));
242250
setFileJSON(newfile.getId(),
243251
new JSONObject().put(c1Name, "four").put(c2Name, 444L).put(c3Name, "4.4").put(c4Name, "newlyAdded"));
244-
setFileJSON(files.get(1).getId(),
245-
new JSONObject().put(c1Name, "two").put(c2Name, 222L).put(c3Name, "2.2").put(c4Name, "updatedInSource"));
252+
setFileJSON(files.get(1).getId(), new JSONObject().put(c1Name, "two").put(c2Name, 222L).put(c3Name, "2.2")
253+
.put(c4Name, "updatedInSource"));
246254
waitForFilesToReplicat(files);
247255

248256
boolean newVersion = false;
@@ -286,21 +294,21 @@ public void testSynchronizeEntityView() throws Exception {
286294
.put(c3Name, "1.2").put(c4Name, "oneUpdatedInSource"));
287295
verifyExpectedAnnotations(files.get(1).getId(), new JSONObject().put(c1Name, "two").put(c2Name, 222L)
288296
.put(c3Name, "2.2").put(c4Name, "updatedInSource"));
289-
verifyExpectedAnnotations(files.get(2).getId(), new JSONObject().put(c1Name, "four").put(c2Name, 444L)
290-
.put(c3Name, "4.4").put(c4Name, "newlyAdded"));
297+
verifyExpectedAnnotations(files.get(2).getId(),
298+
new JSONObject().put(c1Name, "four").put(c2Name, 444L).put(c3Name, "4.4").put(c4Name, "newlyAdded"));
291299

292300
}
293301

294302
void verifyExpectedAnnotations(String fileId, JSONObject expected) {
295303
boolean includeDerived = false;
296304
JSONObject fetched = entityService.getEntityJson(admin.getId(), fileId, includeDerived);
297-
System.out.println("FileId: "+fileId);
298-
System.out.println("Fetched: "+fetched.toString());
299-
System.out.println("Passed: "+expected.toString());
305+
System.out.println("FileId: " + fileId);
306+
System.out.println("Fetched: " + fetched.toString());
307+
System.out.println("Passed: " + expected.toString());
300308
Iterator<String> it = expected.keys();
301309
while (it.hasNext()) {
302310
String key = it.next();
303-
assertEquals(expected.get(key),fetched.get(key));
311+
assertEquals(expected.get(key), fetched.get(key));
304312
}
305313
}
306314

‎services/workers/src/test/java/org/sagebionetworks/table/worker/TableWorkerIntegrationTest.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import org.json.JSONObject;
3838
import org.junit.jupiter.api.AfterEach;
3939
import org.junit.jupiter.api.BeforeEach;
40+
import org.junit.jupiter.api.Disabled;
4041
import org.junit.jupiter.api.Test;
4142
import org.junit.jupiter.api.extension.ExtendWith;
4243
import org.mockito.Mockito;
@@ -773,6 +774,7 @@ public void testColumnOrderWithQueries() throws Exception {
773774
* Test if things work if the table index is not being build, which can happen for example after a migration
774775
*/
775776
@Test
777+
@Disabled // unstable test, see: PLFM-9485
776778
public void testRoundTripAfterMigrate() throws Exception {
777779
createSchemaOneOfEachType();
778780
createTableWithSchema();
@@ -3184,6 +3186,7 @@ public void testPLFM_5639() throws Exception {
31843186
* @throws Exception
31853187
*/
31863188
@Test
3189+
@Disabled // Unstable test, see: PLFM-9484
31873190
public void testPLFM_6305() throws Exception {
31883191
ColumnModel columnOne = new ColumnModel();
31893192
columnOne.setColumnType(ColumnType.INTEGER);

0 commit comments

Comments
 (0)