Repository: OpenRefine/OpenRefine (java)

Issue:
Commons upload: newly uploaded cells should contain `File:` prefix in cell.recon.name
According to @sunilnatraj's analysis in https://github.com/OpenRefine/CommonsExtension/pull/153, newly-uploaded files get represented by matched reconciled cells whose `.name` field does not contain a `File:` prefix, contrary to matches returned by the reconciliation service (for existing files). It would be worth unifying those two.
If that's indeed the case, the simplest would be (I think) to change our upload code so that matched cells do contain the `File:` prefix. It could be that this breaks some workflows, if folks explicitly rely on those filenames not having the `File:` prefix, but I would expect downstream uses of uploaded files generally go through the Mids instead, so I think it would be fine to introduce such a change.


Pull request:
wikibase: Improve recon name for created entities
Fixes #6996 
Changes proposed in this pull request:
- Extended NewEntityLibrary to also hold label for new entities 
- Updated EditBatchProcessor performEdit to set label from the value set in NewEntityLibrary
- Updated test cases 


Patch:
diff --git a/extensions/wikibase/src/org/openrefine/wikibase/editing/EditBatchProcessor.java b/extensions/wikibase/src/org/openrefine/wikibase/editing/EditBatchProcessor.java
index 4a227b8a6..a027f7104 100644
--- a/extensions/wikibase/src/org/openrefine/wikibase/editing/EditBatchProcessor.java
+++ b/extensions/wikibase/src/org/openrefine/wikibase/editing/EditBatchProcessor.java
@@ -29,6 +29,7 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.OptionalLong;
 import java.util.Set;
@@ -39,6 +40,8 @@ import org.slf4j.LoggerFactory;
 import org.wikidata.wdtk.datamodel.interfaces.EntityDocument;
 import org.wikidata.wdtk.datamodel.interfaces.EntityIdValue;
 import org.wikidata.wdtk.datamodel.interfaces.EntityUpdate;
+import org.wikidata.wdtk.datamodel.interfaces.LabeledDocument;
+import org.wikidata.wdtk.datamodel.interfaces.MonolingualTextValue;
 import org.wikidata.wdtk.wikibaseapi.ApiConnection;
 import org.wikidata.wdtk.wikibaseapi.EditingResult;
 import org.wikidata.wdtk.wikibaseapi.WikibaseDataEditor;
@@ -183,14 +186,19 @@ public class EditBatchProcessor {
                 // New entities
                 ReconEntityIdValue newCell = (ReconEntityIdValue) update.getEntityId();
                 EntityIdValue createdDocId;
+                String name = "";
                 if (update instanceof MediaInfoEdit) {
                     MediaFileUtils mediaFileUtils = new MediaFileUtils(connection);
                     createdDocId = ((MediaInfoEdit) update).uploadNewFile(editor, mediaFileUtils, summary, tags, filePageWaitTime,
                             filePageMaxWaitTime);
+                    name = "File:".concat(((MediaInfoEdit) update).getFileName());
                 } else {
-                    createdDocId = editor.createEntityDocument(update.toNewEntity(), summary, tags).getEntityId();
+                    LabeledDocument labeledDocument = (LabeledDocument) editor.createEntityDocument(update.toNewEntity(), summary, tags);
+                    createdDocId = labeledDocument.getEntityId();
+                    name = getDocumentLabel(labeledDocument.getLabels(), createdDocId.getId());
                 }
                 library.setId(newCell.getReconInternalId(), createdDocId.getId());
+                library.setName(newCell.getReconInternalId(), name);
                 newEntityUrl = createdDocId.getSiteIri() + createdDocId.getId();
             } else {
                 // Existing entities
@@ -372,4 +380,22 @@ public class EditBatchProcessor {
         batchCursor = 0;
     }
 
+    private String getDocumentLabel(Map<String, MonolingualTextValue> labels, String defaultName) {
+        if (labels == null || labels.isEmpty()) {
+            return defaultName;
+        }
+        String docLabel = null;
+        String defaultLanguage = Locale.getDefault().getLanguage();
+        for (Map.Entry<String, MonolingualTextValue> entry : labels.entrySet()) {
+            String language = entry.getKey();
+            MonolingualTextValue labelValue = entry.getValue();
+            if (defaultLanguage.equals(language)) {
+                return labelValue.getText();
+            }
+            if (docLabel == null) {
+                docLabel = labelValue.getText();
+            }
+        }
+        return docLabel == null ? defaultName : docLabel;
+    }
 }
diff --git a/extensions/wikibase/src/org/openrefine/wikibase/editing/NewEntityLibrary.java b/extensions/wikibase/src/org/openrefine/wikibase/editing/NewEntityLibrary.java
index 696b1df6c..020a3087b 100644
--- a/extensions/wikibase/src/org/openrefine/wikibase/editing/NewEntityLibrary.java
+++ b/extensions/wikibase/src/org/openrefine/wikibase/editing/NewEntityLibrary.java
@@ -50,15 +50,19 @@ import com.google.refine.model.recon.StandardReconConfig;
  */
 public class NewEntityLibrary {
 
-    private Map<Long, String> map;
+    private Map<Long, String> reconToEntityId;
+    private Map<Long, String> reconToName;
 
     public NewEntityLibrary() {
-        map = new HashMap<>();
+        reconToEntityId = new HashMap<>();
+        reconToName = new HashMap<>();
     }
 
     @JsonCreator
-    public NewEntityLibrary(@JsonProperty("qidMap") Map<Long, String> map) {
-        this.map = map;
+    public NewEntityLibrary(@JsonProperty("qidMap") Map<Long, String> reconToEntityId,
+            @JsonProperty("nameMap") Map<Long, String> reconToName) {
+        this.reconToEntityId = reconToEntityId;
+        this.reconToName = reconToName;
     }
 
     /**
@@ -69,7 +73,11 @@ public class NewEntityLibrary {
      * @return the id (or null if unallocated yet)
      */
     public String getId(long id) {
-        return map.get(id);
+        return reconToEntityId.get(id);
+    }
+
+    public String getName(long id) {
+        return reconToName.get(id);
     }
 
     /**
@@ -81,7 +89,11 @@ public class NewEntityLibrary {
      *            : the associated id returned by Wikibase
      */
     public void setId(long id, String returnedId) {
-        map.put(id, returnedId);
+        reconToEntityId.put(id, returnedId);
+    }
+
+    public void setName(long id, String name) {
+        reconToName.put(id, name);
     }
 
     /**
@@ -110,15 +122,17 @@ public class NewEntityLibrary {
                 Recon recon = cell.recon;
                 boolean changed = false;
                 if (Recon.Judgment.New.equals(recon.judgment) && !reset
-                        && map.containsKey(recon.id)) {
+                        && reconToEntityId.containsKey(recon.id)) {
                     recon.judgment = Recon.Judgment.Matched;
-                    recon.match = new ReconCandidate(map.get(recon.id), cell.value.toString(),
+                    recon.match = new ReconCandidate(reconToEntityId.get(recon.id).toString(),
+                            reconToName != null && reconToName.get(recon.id) != null ? reconToName.get(recon.id).toString()
+                                    : cell.value.toString(),
                             new String[0], 100);
                     recon.addCandidate(recon.match);
                     changed = true;
 
                 } else if (Recon.Judgment.Matched.equals(recon.judgment) && reset
-                        && map.containsKey(recon.id)) {
+                        && reconToEntityId.containsKey(recon.id)) {
                     recon.judgment = Recon.Judgment.New;
                     if (recon.candidates != null) {
                         recon.candidates.remove(recon.candidates.size() - 1);
@@ -155,7 +169,12 @@ public class NewEntityLibrary {
      */
     @JsonProperty("qidMap")
     public Map<Long, String> getIdMap() {
-        return map;
+        return reconToEntityId;
+    }
+
+    @JsonProperty("nameMap")
+    public Map<Long, String> getNameMap() {
+        return reconToName;
     }
 
     @Override
@@ -164,16 +183,19 @@ public class NewEntityLibrary {
             return false;
         }
         NewEntityLibrary otherLibrary = (NewEntityLibrary) other;
-        return map.equals(otherLibrary.getIdMap());
+        return reconToEntityId.equals(otherLibrary.getIdMap()) && (reconToName != null && reconToName.equals(otherLibrary.getNameMap()));
     }
 
     @Override
     public int hashCode() {
-        return map.hashCode();
+        return reconToEntityId.hashCode() + (reconToName != null ? reconToName.hashCode() : 0);
     }
 
     @Override
     public String toString() {
-        return map.toString();
+        return "{" +
+                "reconToEntityId=" + reconToEntityId +
+                ", reconToName=" + reconToName +
+                '}';
     }
 }
diff --git a/extensions/wikibase/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperation.java b/extensions/wikibase/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperation.java
index 2bdb3fd83..c8f0e4b17 100644
--- a/extensions/wikibase/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperation.java
+++ b/extensions/wikibase/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperation.java
@@ -210,6 +210,7 @@ public class PerformWikibaseEditsOperation extends EngineDependentOperation {
             if (newEntityLibrary != null) {
                 writer.write("newItems=");
                 ObjectMapper mapper = new ObjectMapper();
+                mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL);
                 writer.write(mapper.writeValueAsString(newEntityLibrary) + "\n");
             }
             writer.write("/ec/\n"); // end of change
@@ -287,6 +288,7 @@ public class PerformWikibaseEditsOperation extends EngineDependentOperation {
                 onDone.add(createFacetAction);
 
             }
+
         }
 
         @Override
diff --git a/extensions/wikibase/src/org/openrefine/wikibase/updates/MediaInfoEdit.java b/extensions/wikibase/src/org/openrefine/wikibase/updates/MediaInfoEdit.java
index dede650c5..a930b21fa 100644
--- a/extensions/wikibase/src/org/openrefine/wikibase/updates/MediaInfoEdit.java
+++ b/extensions/wikibase/src/org/openrefine/wikibase/updates/MediaInfoEdit.java
@@ -314,7 +314,9 @@ public class MediaInfoEdit extends LabeledStatementEntityEdit {
         ReconEntityIdValue reconEntityIdValue = (ReconEntityIdValue) id;
         MediaInfoIdValue mid = response.getMid(mediaFileUtils.getApiConnection(), reconEntityIdValue.getRecon().identifierSpace);
         NewEntityLibrary library = new NewEntityLibrary();
+        String label = this.fileName.startsWith("File:") ? this.fileName : "File:".concat(this.fileName);
         library.setId(reconEntityIdValue.getReconInternalId(), mid.getId());
+        library.setName(reconEntityIdValue.getReconInternalId(), label);
         ReconEntityRewriter rewriter = new ReconEntityRewriter(library, id);
         try {
             MediaInfoEdit rewritten = (MediaInfoEdit) rewriter.rewrite(this);


Test patch:
diff --git a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/EditBatchProcessorTest.java b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/EditBatchProcessorTest.java
index 26d959216..e3aa9e7b3 100644
--- a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/EditBatchProcessorTest.java
+++ b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/EditBatchProcessorTest.java
@@ -143,6 +143,7 @@ public class EditBatchProcessorTest extends WikidataRefineTest {
 
         NewEntityLibrary expectedLibrary = new NewEntityLibrary();
         expectedLibrary.setId(1234L, "Q1234");
+        expectedLibrary.setName(1234L, "better label");
         assertEquals(expectedLibrary, library);
     }
 
diff --git a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/NewEntityLibraryTest.java b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/NewEntityLibraryTest.java
index b25f029fe..3edcbf01a 100644
--- a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/NewEntityLibraryTest.java
+++ b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/NewEntityLibraryTest.java
@@ -49,7 +49,9 @@ public class NewEntityLibraryTest extends WikidataRefineTest {
     public void setUp() {
         library = new NewEntityLibrary();
         library.setId(1234L, "Q345");
+        library.setName(1234L, "new uni");
         library.setId(3289L, "Q384");
+        library.setName(3289L, "University of Ljubljana");
     }
 
     @Test
@@ -87,7 +89,7 @@ public class NewEntityLibraryTest extends WikidataRefineTest {
     @Test
     public void testSerialize() {
         JacksonSerializationTest.canonicalSerialization(NewEntityLibrary.class, library,
-                "{\"qidMap\":{\"1234\":\"Q345\",\"3289\":\"Q384\"}}");
+                "{\"qidMap\":{\"1234\":\"Q345\",\"3289\":\"Q384\"},\"nameMap\":{\"1234\":\"new uni\",\"3289\":\"University of Ljubljana\"}}");
     }
 
     private void isMatchedTo(String qid, Cell cell) {
diff --git a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/ReconEntityRewriterTest.java b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/ReconEntityRewriterTest.java
index cfbaae0d3..5808289f7 100644
--- a/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/ReconEntityRewriterTest.java
+++ b/extensions/wikibase/tests/src/org/openrefine/wikibase/editing/ReconEntityRewriterTest.java
@@ -61,6 +61,7 @@ public class ReconEntityRewriterTest {
     public void testSuccessfulRewrite() {
         rewriter = new ReconEntityRewriter(library, TestingData.newIdA);
         library.setId(4567L, "Q1234");
+        library.setName(4567L, "Q1234");
         assertEquals(newlyCreated, rewriter.copy(TestingData.newIdB));
     }
 
@@ -75,6 +76,7 @@ public class ReconEntityRewriterTest {
     public void testSubjectRewritten() {
         ItemIdValue subject = TestingData.newIdB;
         library.setId(4567L, "Q1234");
+        library.setName(4567L, "Q1234");
         rewriter = new ReconEntityRewriter(library, subject);
         assertEquals(newlyCreated, rewriter.copy(subject));
     }
@@ -90,6 +92,7 @@ public class ReconEntityRewriterTest {
         ItemIdValue subject = TestingData.newIdA;
         rewriter = new ReconEntityRewriter(library, subject);
         library.setId(4567L, "Q1234");
+        library.setName(4567L, "label");
         TermedStatementEntityEdit update = new ItemEditBuilder(subject)
                 .addStatement(TestingData.generateStatementAddition(subject, TestingData.newIdB))
                 .addStatement(TestingData.generateStatementDeletion(subject, TestingData.existingId))
@@ -115,6 +118,7 @@ public class ReconEntityRewriterTest {
         ItemIdValue subject = TestingData.newIdA;
         rewriter = new ReconEntityRewriter(library, subject);
         library.setId(4567L, "Q1234");
+        library.setName(4567L, "Q1234");
         TermedStatementEntityEdit update = new ItemEditBuilder(TestingData.newIdB)
                 .addDescription(Datamodel.makeMonolingualTextValue("beschreibung", "de"), false)
                 .addAlias(Datamodel.makeMonolingualTextValue("darstellung", "de"))
@@ -134,6 +138,7 @@ public class ReconEntityRewriterTest {
         ItemIdValue subject = TestingData.matchedId;
         rewriter = new ReconEntityRewriter(library, subject);
         library.setId(4567L, "Q1234");
+        library.setName(4567L, "label");
         TermedStatementEntityEdit update = new ItemEditBuilder(subject)
                 .addStatement(TestingData.generateStatementAddition(subject, TestingData.newIdB))
                 .addStatement(TestingData.generateStatementDeletion(subject, TestingData.existingId))
@@ -159,6 +164,7 @@ public class ReconEntityRewriterTest {
         ItemIdValue subject = TestingData.existingId;
         rewriter = new ReconEntityRewriter(library, subject);
         library.setId(7654L, "P1234");
+        library.setName(7654L, "label");
         TermedStatementEntityEdit update = new ItemEditBuilder(subject)
                 .addStatement(TestingData.generateStatementAddition(subject, TestingData.newPropertyIdB))
                 .addStatement(TestingData.generateStatementDeletion(subject, TestingData.existingPropertyId))
diff --git a/extensions/wikibase/tests/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperationTest.java b/extensions/wikibase/tests/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperationTest.java
index 5f35fd64a..83f1e59fc 100644
--- a/extensions/wikibase/tests/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperationTest.java
+++ b/extensions/wikibase/tests/src/org/openrefine/wikibase/operations/PerformWikibaseEditsOperationTest.java
@@ -95,4 +95,25 @@ public class PerformWikibaseEditsOperationTest extends OperationTest {
         assertEquals(changeString, saveChange(change));
     }
 
+    @Test
+    public void testLoadChange_v2()
+            throws Exception {
+        String changeString = "newItems={\"qidMap\":{\"1234\":\"Q789\"},\"nameMap\":{\"1234\":\"Non-existent entity\"}}\n" + "/ec/\n";
+        LineNumberReader reader = makeReader(changeString);
+        Change change = PerformWikibaseEditsOperation.PerformWikibaseEditsChange.load(reader, pool);
+
+        project.rows.get(0).cells.set(0, TestingData.makeNewItemCell(1234L, "my new item"));
+
+        change.apply(project);
+
+        assertEquals(Recon.Judgment.Matched, project.rows.get(0).cells.get(0).recon.judgment);
+        assertEquals("Q789", project.rows.get(0).cells.get(0).recon.match.id);
+        assertEquals("Non-existent entity", project.rows.get(0).cells.get(0).recon.match.name);
+
+        change.revert(project);
+
+        assertEquals(Recon.Judgment.New, project.rows.get(0).cells.get(0).recon.judgment);
+
+        assertEquals(changeString, saveChange(change));
+    }
 }