Repository: OpenRefine/OpenRefine (java)

Issue:
wikibase: Bug introduced by property label resolution in QA issues when new entities are created
As mentioned by @sunilnatraj  in https://github.com/OpenRefine/OpenRefine/pull/7068#discussion_r1914734982:

> For the new entity case, setting self as the reference is incorrect. 
> 
> The value of new entity is this non-existent Q2852607907487331030. 
> 
> As part of https://github.com/OpenRefine/OpenRefine/pull/6895 the code resolves the entity to a proper label, this code fails in this scenario.
> 
> `java.util.concurrent.ExecutionException: org.wikidata.wdtk.wikibaseapi.apierrors.NoSuchEntityErrorException: [no-such-entity] Could not find an entity with the ID "Q2852607907487331030".
> 	at com.google.common.cache.LocalCache.loadAll(LocalCache.java:4136)
> 	at com.google.common.cache.LocalCache.getAll(LocalCache.java:4087)
> 	at com.google.common.cache.LocalCache$LocalLoadingCache.getAll(LocalCache.java:5004)
> 	at org.openrefine.wikibase.utils.EntityCache.getMultipleDocuments(EntityCache.java:117)
> 	at org.openrefine.wikibase.qa.EditInspector.resolveWarningPropertyLabels(EditInspector.java:235)`

So the changes in https://github.com/OpenRefine/OpenRefine/pull/6895 introduced a bug that should be fixed.

Pull request:
wikibase: Fix label fetching in QA issues for new entities
Closes #7089.

This clarifies the filtering logic to select which entity ids should be fetched to get their labels, to avoid fetching them for entity ids generated via reconciliation. Those already have associated labels (existing label for existing entities, cell values for new entities).

Patch:
diff --git a/extensions/wikibase/src/org/openrefine/wikibase/qa/EditInspector.java b/extensions/wikibase/src/org/openrefine/wikibase/qa/EditInspector.java
index 6567a5844..6256e54b6 100644
--- a/extensions/wikibase/src/org/openrefine/wikibase/qa/EditInspector.java
+++ b/extensions/wikibase/src/org/openrefine/wikibase/qa/EditInspector.java
@@ -68,6 +68,7 @@ import org.openrefine.wikibase.qa.scrutinizers.UnsourcedScrutinizer;
 import org.openrefine.wikibase.qa.scrutinizers.UseAsQualifierScrutinizer;
 import org.openrefine.wikibase.qa.scrutinizers.WhitespaceScrutinizer;
 import org.openrefine.wikibase.schema.WikibaseSchema;
+import org.openrefine.wikibase.schema.entityvalues.ReconEntityIdValue;
 import org.openrefine.wikibase.schema.entityvalues.SuggestedEntityIdValue;
 import org.openrefine.wikibase.schema.entityvalues.SuggestedItemIdValue;
 import org.openrefine.wikibase.schema.entityvalues.SuggestedPropertyIdValue;
@@ -217,17 +218,23 @@ public class EditInspector {
         }
     }
 
-    private void resolveWarningPropertyLabels() throws ExecutionException {
+    protected void resolveWarningPropertyLabels() throws ExecutionException {
         if (entityCache != null) {
             List<EntityIdValue> propertyIdValues = warningStore.getWarnings().stream()
                     .flatMap(warning -> warning.getProperties().entrySet().stream()
-                            .filter(entry -> entry.getValue() instanceof EntityIdValue)
                             .filter(entry -> {
-                                if (entry instanceof SuggestedEntityIdValue) {
-                                    String label = ((SuggestedEntityIdValue) entry.getValue()).getLabel();
+                                Object value = entry.getValue();
+                                if (!(value instanceof PropertyIdValue || value instanceof ItemIdValue)) {
+                                    return false;
+                                }
+                                if (value instanceof SuggestedEntityIdValue) {
+                                    String label = ((SuggestedEntityIdValue) value).getLabel();
                                     return label == null || label.isEmpty();
+                                } else if (value instanceof ReconEntityIdValue) {
+                                    return false; // already contains labels via the recon object
+                                } else {
+                                    return true; // no label associated with the value, so we fetch it
                                 }
-                                return true;
                             })
                             .map(entry -> (EntityIdValue) entry.getValue()))
                     .collect(Collectors.toList());
@@ -237,7 +244,7 @@ public class EditInspector {
             warningStore.getWarnings().stream()
                     .forEach(warning -> {
                         warning.getProperties().forEach((key, value) -> {
-                            if (value instanceof PropertyIdValue || value instanceof ItemIdValue) {
+                            if (value instanceof EntityIdValue && propertyIdValues.contains(value)) {
                                 EntityIdValue entityIdValue = (EntityIdValue) value;
                                 String label = "";
 


Test patch:
diff --git a/extensions/wikibase/tests/src/org/openrefine/wikibase/qa/EditInspectorTest.java b/extensions/wikibase/tests/src/org/openrefine/wikibase/qa/EditInspectorTest.java
index b952607fa..2d8fd1ac3 100644
--- a/extensions/wikibase/tests/src/org/openrefine/wikibase/qa/EditInspectorTest.java
+++ b/extensions/wikibase/tests/src/org/openrefine/wikibase/qa/EditInspectorTest.java
@@ -1,12 +1,17 @@
 
 package org.openrefine.wikibase.qa;
 
+import static org.mockito.Mockito.mock;
 import static org.testng.Assert.assertEquals;
 
 import org.testng.annotations.Test;
 
+import com.google.refine.model.Recon;
+
 import org.openrefine.wikibase.manifests.Manifest;
 import org.openrefine.wikibase.manifests.ManifestParser;
+import org.openrefine.wikibase.qa.QAWarning.Severity;
+import org.openrefine.wikibase.schema.entityvalues.ReconItemIdValue;
 import org.openrefine.wikibase.testing.TestingData;
 
 public class EditInspectorTest {
@@ -37,4 +42,24 @@ public class EditInspectorTest {
         EditInspector editInspector = new EditInspector(new QAWarningStore(), manifest, false);
         assertEquals(editInspector.scrutinizers.size(), scrutinizerNotDependingOnPropertyConstraintCount);
     }
+
+    @Test
+    public void testLabelFetchingIgnoresReconciledEntityIds() throws Exception {
+        // regression test for https://github.com/OpenRefine/OpenRefine/issues/7089
+        String manifestJson = TestingData.jsonFromFile("manifest/wikidata-manifest-v1.0.json");
+        Manifest manifest = ManifestParser.parse(manifestJson);
+        QAWarningStore qaWarningStore = new QAWarningStore();
+        QAWarning qaWarning1 = new QAWarning("my-warning", "my-warning", Severity.WARNING, 1);
+        Recon recon = mock(Recon.class);
+        recon.judgment = Recon.Judgment.New;
+        ReconItemIdValue reconItemIdValue = new ReconItemIdValue(recon, "cell value");
+        qaWarning1.setProperty("example_entity", reconItemIdValue);
+        qaWarning1.setProperty("some_string", "foo");
+        qaWarningStore.addWarning(qaWarning1);
+
+        EditInspector editInspector = new EditInspector(qaWarningStore, manifest, false);
+        editInspector.resolveWarningPropertyLabels();
+
+        assertEquals(qaWarning1.getProperties().get("example_entity"), reconItemIdValue);
+    }
 }