Repository: Wikidata/Wikidata-Toolkit (java)

Issue:
Deserializing a property with `localMedia` datatype fails
When trying to deserialize a property with `localMedia` dataype, such as this one:

```json
        {
            "pageid": 6,
            "ns": 122,
            "title": "Property:P3",
            "lastrevid": 9,
            "modified": "2022-08-07T21:40:47Z",
            "type": "property",
            "datatype": "localMedia",
            "id": "P3",
            "labels": {
                "en": {
                    "language": "en",
                    "value": "media file"
                }
            },
            "descriptions": {},
            "aliases": {},
            "claims": {}
        }
```

We fail because the deserialization of the datatype fails, because the JSON encoding of datatypes is supposedly not allowed to contain capital letters: it should match some regex:
https://github.com/Wikidata/Wikidata-Toolkit/blob/e99f19683099beae2b0f8e6c22f094495357f951/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImpl.java#L110

This regex does not make much sense to me because it fails even for some datatypes that Wikidata-Toolkit is already aware of, such as `commonsMedia`.

I see various ways forward:
* Add the `localMedia` datatype to WDTK, just like #701 did it for EDTF. While that solves this exact issue, we are not solving the underlying problem and the code will fail again, should any other such datatype be introduced in the future. Datatypes are extensible so we cannot assume we know of all the datatypes in use in Wikibase instances.
* Allowing capital letters in the regex that validates JSON datatypes. While that also works, this breaks the current assumption that the two transformations between JSON datatypes and datatype URIs are inverse of each other: `localMedia` (JSON datatype) would be converted to `http://wikiba.se/ontology#LocalMedia` by `DatatypeIdImpl.getDatatypeIriFromJsonDatatype`, and then that value would be converted back to `local-media` (not `localMedia`) by `DatatypeIdImpl.getJsonDatatypeFromDatatypeIri`. Because WDTK stores datatypes internally as URIs and not as JSON strings, this means that serializing back the property above will result in an invalid datatype. I would be tempted to have WDTK store the original JSON datatype as well, to make sure it is preserved.

Pull request:
Fix deserialization of unknown property datatypes
Closes #716.

Because the translation between the IRI and JSON datatypes is unreliable, we need
to remember the JSON datatype as well as the IRI, so that we are able to reliably
deserialize and re-serialize properties with custom datatypes. See more discussion in #716.

Patch:
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/Equality.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/Equality.java
index 01b6e551..1db188fc 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/Equality.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/Equality.java
@@ -86,7 +86,8 @@ public class Equality {
 			return true;
 		}
 		return o2 instanceof DatatypeIdValue
-			&& o1.getIri().equals(((DatatypeIdValue) o2).getIri());
+			&& o1.getIri().equals(((DatatypeIdValue) o2).getIri())
+			&& o1.getJsonString().equals(((DatatypeIdValue) o2).getJsonString());
 	}
 
 	/**
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/ToString.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/ToString.java
index 07b93d1c..63097cd0 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/ToString.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/helpers/ToString.java
@@ -122,7 +122,7 @@ public class ToString {
 	 * @return a string representation of the object
 	 */
 	public static String toString(DatatypeIdValue o) {
-		return o.getIri();
+		return o.getIri() + " (" + o.getJsonString() + ")";
 	}
 
 	/**
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImpl.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImpl.java
index ee3287be..77fc28fd 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImpl.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImpl.java
@@ -112,6 +112,11 @@ public class DataObjectFactoryImpl implements DataObjectFactory {
 		return new DatatypeIdImpl(id);
 	}
 
+	@Override
+	public DatatypeIdValue getDatatypeIdValue(String id, String jsonString) {
+		return new DatatypeIdImpl(id, jsonString);
+	}
+
 	@Override
 	public TimeValue getTimeValue(long year, byte month, byte day, byte hour,
 			byte minute, byte second, byte precision, int beforeTolerance,
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImpl.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImpl.java
index 7131e696..6887af5d 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImpl.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImpl.java
@@ -107,7 +107,7 @@ public class DatatypeIdImpl implements DatatypeIdValue {
 	 */
 	public static final String JSON_DT_EDTF = "edtf";
 
-	private static final Pattern JSON_DATATYPE_PATTERN = Pattern.compile("^[a-z\\-]+$");
+	private static final Pattern JSON_DATATYPE_PATTERN = Pattern.compile("^[a-zA-Z\\-]+$");
 	private static final Pattern DATATYPE_ID_PATTERN = Pattern.compile("^http://wikiba\\.se/ontology#([a-zA-Z]+)$");
 
 	/**
@@ -115,6 +115,14 @@ public class DatatypeIdImpl implements DatatypeIdValue {
 	 */
 	private final String iri;
 
+	/**
+	 * JSON representation of the datatype. We store this as well
+	 * because the conversion mechanism between JSON datatypes and
+	 * datatype URIs is sadly not perfect: see
+	 * <a href="https://github.com/Wikidata/Wikidata-Toolkit/issues/716">issue #716</a>.
+	 */
+	private final String jsonString;
+
 	/**
 	 * Returns the WDTK datatype IRI for the property datatype as represented by
 	 * the given JSON datatype string.
@@ -216,12 +224,14 @@ public class DatatypeIdImpl implements DatatypeIdValue {
 	 */
 	public DatatypeIdImpl(DatatypeIdValue other) {
 		this.iri = other.getIri();
+		this.jsonString = other.getJsonString();
 	}
 
 	/**
-	 * Constructs an object representing the datatype id from a string denoting
-	 * the datatype. It also sets the correct IRI for the datatype. This constructor
-	 * is meant to be used for JSON deserialization.
+	 * Constructs an object representing the datatype id from a IRI denoting
+	 * the datatype. It also tries to determine the JSON datatype based on this
+	 * IRI, based on a buggy heuristic. If you also happen to have the JSON datatype
+	 * at hand, better use {@link DatatypeIdImpl(String, String)}.
 	 *
 	 * @param iri
 	 *             the WDTK IRI for the datatype
@@ -233,14 +243,36 @@ public class DatatypeIdImpl implements DatatypeIdValue {
 			throws IllegalArgumentException {
 		Validate.notNull(iri, "An IRI must be provided to create a DatatypeIdValue");
 		this.iri = iri;
+		// the JSON datatype is not supplied, so we fall back on our buggy heuristic
+		// to guess how it should be represented in JSON.
+		this.jsonString = getJsonDatatypeFromDatatypeIri(this.iri);
+	}
+
+	/**
+	 * Constructs an object representing the datatype id from an IRI denoting the datatype,
+	 * as well as a string corresponding to its JSON serialization. This constructor
+	 * is meant to be used for JSON deserialization.
+	 *
+	 * @param iri
+	 *             the WDTK IRI for the datatype
+	 * @throws IllegalArgumentException
+	 *             if the given datatype string could not be matched to a known
+	 *             datatype or was null
+	 */
+	public DatatypeIdImpl(String iri, String jsonString)
+			throws IllegalArgumentException {
+		Validate.notNull(iri, "An IRI must be provided to create a DatatypeIdValue");
+		this.iri = iri;
+		Validate.notNull(jsonString, "A JSON representation of the datatype must be provided to create a DatatypeIdValue");
+		this.jsonString = jsonString;
 	}
 	
 	/**
 	 * Returns the string used to represent this datatype in JSON.
-	 * @return
 	 */
+	@Override
 	public String getJsonString() {
-		return getJsonDatatypeFromDatatypeIri(this.iri);
+		return this.jsonString;
 	}
 
 	@Override
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImpl.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImpl.java
index 88055f25..8fee287d 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImpl.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImpl.java
@@ -108,7 +108,7 @@ public class PropertyDocumentImpl extends TermedStatementDocumentImpl
 			@JsonProperty("lastrevid") long revisionId,
 			@JacksonInject("siteIri") String siteIri) {
         super(jsonId, labels, descriptions, aliases, claims, revisionId, siteIri);
-        this.datatype = new DatatypeIdImpl(DatatypeIdImpl.getDatatypeIriFromJsonDatatype(datatype));
+        this.datatype = new DatatypeIdImpl(DatatypeIdImpl.getDatatypeIriFromJsonDatatype(datatype), datatype);
     }
 
     /**
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DataObjectFactory.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DataObjectFactory.java
index 7f7acfb9..b5dabb14 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DataObjectFactory.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DataObjectFactory.java
@@ -141,6 +141,20 @@ public interface DataObjectFactory {
 	 */
 	DatatypeIdValue getDatatypeIdValue(String id);
 
+	/**
+	 * Creates a {@link DatatypeIdValue}. The datatype IRI is usually one of the
+	 * constants defined in {@link DatatypeIdValue}, but this is not enforced,
+	 * since there might be extensions that provide additional types. The JSON
+	 * string is its representation in the JSON serialization of properties.
+	 *
+	 * @param id
+	 *            the IRI string that identifies the datatype
+	 * @param jsonString
+	 * 			  the JSON representation of this datatype
+	 * @return a {@link DatatypeIdValue} corresponding to the input
+	 */
+	DatatypeIdValue getDatatypeIdValue(String id, String jsonString);
+
 	/**
 	 * Creates a {@link TimeValue}.
 	 *
diff --git a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DatatypeIdValue.java b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DatatypeIdValue.java
index 2792dcd0..d03420cd 100644
--- a/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DatatypeIdValue.java
+++ b/wdtk-datamodel/src/main/java/org/wikidata/wdtk/datamodel/interfaces/DatatypeIdValue.java
@@ -108,4 +108,9 @@ public interface DatatypeIdValue {
 	 * @return String with the IRI
 	 */
 	String getIri();
+
+	/**
+	 * The string identifying this datatype in the JSON serialization of a property.
+	 */
+    String getJsonString();
 }


Test patch:
diff --git a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImplTest.java b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImplTest.java
index 6905b7df..e4b2bdb5 100644
--- a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImplTest.java
+++ b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DataObjectFactoryImplTest.java
@@ -81,6 +81,13 @@ public class DataObjectFactoryImplTest {
 		assertEquals(o1, o2);
 	}
 
+	@Test
+	public final void testGetDatatypeIdWithJsonString() {
+		DatatypeIdValue o1 = new DatatypeIdImpl(DatatypeIdValue.DT_STRING, DatatypeIdImpl.JSON_DT_STRING);
+		DatatypeIdValue o2 = factory.getDatatypeIdValue(DatatypeIdValue.DT_STRING, DatatypeIdImpl.JSON_DT_STRING);
+		assertEquals(o1, o2);
+	}
+
 	@Test
 	public final void testGetTimeValue() {
 		TimeValue o1 = new TimeValueImpl(2007, (byte) 5, (byte) 12,
diff --git a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImplTest.java b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImplTest.java
index dd7cd424..93636de1 100644
--- a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImplTest.java
+++ b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/DatatypeIdImplTest.java
@@ -30,6 +30,7 @@ public class DatatypeIdImplTest {
 	private final DatatypeIdImpl d1 = new DatatypeIdImpl(DatatypeIdValue.DT_ITEM);
 	private final DatatypeIdImpl d2 = new DatatypeIdImpl("http://wikiba.se/ontology#WikibaseItem");
 	private final DatatypeIdImpl d3 = new DatatypeIdImpl(DatatypeIdValue.DT_TIME);
+	private final DatatypeIdImpl d4 = new DatatypeIdImpl("http://wikiba.se/ontology#SomeUnknownDatatype", "some-unknownDatatype");
 
 	@Test(expected = NullPointerException.class)
 	public void datatypeIdNotNull() {
@@ -50,6 +51,19 @@ public class DatatypeIdImplTest {
 		assertEquals(d1.hashCode(), d2.hashCode());
 	}
 
+	@Test
+	public void doNotChokeOnUnknownDatatypes() {
+		// for issue https://github.com/Wikidata/Wikidata-Toolkit/issues/716
+		assertEquals("some-unknownDatatype", d4.getJsonString());
+		assertEquals("http://wikiba.se/ontology#SomeUnknownDatatype", d4.getIri());
+	}
+
+	@Test
+	public void testDeserializeUnknownJsonDatatype() {
+		// for issue https://github.com/Wikidata/Wikidata-Toolkit/issues/716
+		assertEquals("http://wikiba.se/ontology#LocalMedia", DatatypeIdImpl.getDatatypeIriFromJsonDatatype("localMedia"));
+	}
+
 	@Test
 	public void testIriForItem() {
 		assertEquals(
diff --git a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImplTest.java b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImplTest.java
index 3eddad3f..77af1f00 100644
--- a/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImplTest.java
+++ b/wdtk-datamodel/src/test/java/org/wikidata/wdtk/datamodel/implementation/PropertyDocumentImplTest.java
@@ -61,6 +61,7 @@ public class PropertyDocumentImplTest {
 			statementGroups, datatypeId, 1234);
 
 	private final String JSON_PROPERTY = "{\"type\":\"property\",\"id\":\"P2\",\"labels\":{\"en\":{\"language\":\"en\",\"value\":\"label\"}},\"descriptions\":{\"fr\":{\"language\":\"fr\",\"value\":\"des\"}},\"aliases\":{\"de\":[{\"language\":\"de\",\"value\":\"alias\"}]},\"claims\":{\"P42\":[{\"rank\":\"normal\",\"id\":\"MyId\",\"mainsnak\":{\"property\":\"P42\",\"snaktype\":\"somevalue\"},\"type\":\"statement\"}]},\"datatype\":\"wikibase-item\",\"lastrevid\":1234}";
+	private final String JSON_PROPERTY_WITH_UNKNOWN_DATATYPE = "{\"type\":\"property\",\"id\":\"P2\",\"labels\":{\"en\":{\"language\":\"en\",\"value\":\"label\"}},\"descriptions\":{},\"aliases\":{},\"claims\":{},\"datatype\":\"some-unknownDatatype\",\"lastrevid\":1234}";
 
 	@Test
 	public void fieldsAreCorrect() {
@@ -107,7 +108,7 @@ public class PropertyDocumentImplTest {
 				Collections.emptyList(), datatypeId, 1234);
 		PropertyDocument pdDiffDatatype = new PropertyDocumentImpl(pid,
 				labelList, descList, aliasList,
-				statementGroups, new DatatypeIdImpl("string"), 1234);
+				statementGroups, new DatatypeIdImpl(DatatypeIdValue.DT_STRING), 1234);
 		PropertyDocument pdDiffRevisions = new PropertyDocumentImpl(pid,
 				labelList, descList, aliasList,
 				statementGroups, datatypeId, 1235);
@@ -247,6 +248,12 @@ public class PropertyDocumentImplTest {
 	public void testPropertyToJava() throws IOException {
 		assertEquals(pd1, mapper.readValue(JSON_PROPERTY, PropertyDocumentImpl.class));
 	}
+
+	@Test
+	public void testPropertyToJavaWithUnknownDatatype() throws JsonProcessingException {
+		PropertyDocumentImpl pd = mapper.readValue(JSON_PROPERTY_WITH_UNKNOWN_DATATYPE, PropertyDocumentImpl.class);
+		assertEquals("some-unknownDatatype", pd.getJsonDatatype());
+	}
         
         @Test
 	public void testWithOverridenDescription() {