Item.fromJSON(): Dedupe invalid-for-type fields when storing in Extra

3de54455f6 removed redundant base-mapped fields when a valid-for-type
field was also set, but that still left duplicate fields in Extra when a
valid field wasn't set. This will happen until translators (most notably
Embedded Metadata) are fixed to stop setting redundant fields.

https://forums.zotero.org/discussion/81262/translator-error-sage-lots-of-extra-data-in-extra

chrome/content/zotero/xpcom/data/item.js

@@ -4362,7 +4362,7 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 	// try to save multiple versions of base-mapped fields, which they shouldn't need to do.
 	//
 	// https://github.com/zotero/zotero/issues/1504#issuecomment-572415083
-	if (extraFields.size) {
+	if (!strict && extraFields.size) {
 		for (let field of setFields.keys()) {
 			let baseField;
 			if (Zotero.ItemFields.isBaseField(field)) {
@@ -4385,6 +4385,52 @@ Zotero.Item.prototype.fromJSON = function (json, options = {}) {
 				}
 			}
 		}
+		
+		//
+		// Deduplicate remaining Extra fields
+		//
+		// For each invalid-for-type base field, remove any mapped fields with the same value
+		let baseFields = [];
+		for (let field of extraFields.keys()) {
+			if (Zotero.ItemFields.getID(field) && Zotero.ItemFields.isBaseField(field)) {
+				baseFields.push(field);
+			}
+		}
+		for (let baseField of baseFields) {
+			let value = extraFields.get(baseField);
+			let mappedFieldNames = Zotero.ItemFields.getTypeFieldsFromBase(baseField, true);
+			for (let mappedField of mappedFieldNames) {
+				if (extraFields.has(mappedField) && extraFields.get(mappedField) === value) {
+					Zotero.warn(`Removing redundant Extra field '${mappedField}' for item `
+						+ this.libraryKey);
+					extraFields.delete(mappedField);
+				}
+			}
+		}
+		
+		// Special handling for Type fields, since Type isn't a base field and therefore isn't set
+		// in translators that are doing this
+		if (!extraFields.has('type')) {
+			let typeFieldNames = Zotero.ItemFields.getTypeFieldsFromBase('type', true)
+				// This is actually 'medium' but as of 2/2020 the Embedded Metadata translator
+				// assigns it along with the other 'type' fields.
+				.concat('audioFileType');
+			let typeValue = false;
+			for (let typeFieldName of typeFieldNames) {
+				let value = extraFields.get(typeFieldName);
+				if (value !== undefined) {
+					if (typeValue === false) {
+						typeValue = value;
+						extraFields.set('type', value);
+					}
+					if (typeValue == value) {
+						Zotero.warn(`Removing redundant Extra field '${typeFieldName}' for item `
+							+ this.libraryKey);
+						extraFields.delete(typeFieldName);
+					}
+				}
+			}
+		}
 	}
 	
 	this.setField('extra', Zotero.Utilities.Internal.combineExtraFields(extra, extraFields));

test/tests/itemTest.js

@@ -1885,6 +1885,78 @@ describe("Zotero.Item", function () {
 				assert.equal(item.getField('label'), 'B');
 				assert.equal(item.getField('extra'), 'Foo: D');
 			});
+			
+			it("should remove invalid-for-type base-mapped fields with same values and use base field if not present when storing in Extra", function () {
+				var json = {
+					itemType: "artwork",
+					publisher: "Foo", // Invalid base field
+					company: "Foo", // Invalid base-mapped field
+					label: "Foo" // Invaid base-mapped field
+				};
+				var item = new Zotero.Item;
+				item.fromJSON(json);
+				assert.equal(item.getField('extra'), 'Publisher: Foo');
+			});
+			
+			it("should deduplicate invalid-for-type base-mapped Type fields with same values and keep Type when storing in Extra", function () {
+				var json = {
+					itemType: "document",
+					type: "Foo", // Invalid base field
+					reportType: "Foo", // Invalid base-mapped field
+					websiteType: "Foo" // Invaid base-mapped field
+				};
+				// Confirm that 'type' is still invalid for 'document', in case this changes
+				assert.isFalse(Zotero.ItemFields.isValidForType(
+					Zotero.ItemFields.getID('type'),
+					Zotero.ItemTypes.getID('document')
+				));
+				var item = new Zotero.Item;
+				item.fromJSON(json);
+				assert.equal(item.getField('extra'), 'Type: Foo');
+			});
+			
+			it("should remove invalid-for-type base-mapped Type fields with same values and use Type if not present when storing in Extra", function () {
+				var json = {
+					itemType: "document",
+					reportType: "Foo", // Invalid base-mapped field
+					websiteType: "Foo" // Invaid base-mapped field
+				};
+				// Confirm that 'type' is still invalid for 'document', in case this changes
+				assert.isFalse(Zotero.ItemFields.isValidForType(
+					Zotero.ItemFields.getID('type'),
+					Zotero.ItemTypes.getID('document')
+				));
+				var item = new Zotero.Item;
+				item.fromJSON(json);
+				assert.equal(item.getField('extra'), 'Type: Foo');
+			});
+			
+			it("shouldn't deduplicate invalid-for-type base-mapped Type fields with different values when storing in Extra", function () {
+				var json = {
+					itemType: "document",
+					reportType: "Foo", // Invalid base-mapped field
+					type: "Foo", // Invalid base field
+					websiteType: "Bar" // Invaid base-mapped field with different value
+				};
+				var item = new Zotero.Item;
+				item.fromJSON(json);
+				assert.equal(item.getField('extra'), 'Type: Foo\nWebsite Type: Bar');
+			});
+			
+			it("shouldn't deduplicate invalid-for-type base-mapped Type fields with different values when storing in Extra", function () {
+				var json = {
+					itemType: "artwork",
+					publisher: "Foo", // Invalid base field
+					company: "Foo", // Invalid base-mapped field
+					label: "Bar" // Invaid base-mapped field with different value
+				};
+				var item = new Zotero.Item;
+				item.fromJSON(json);
+				var parts = item.getField('extra').split('\n');
+				assert.lengthOf(parts, 2);
+				assert.include(parts, 'Publisher: Foo');
+				assert.include(parts, 'Label: Bar');
+			});
 		});
 		
 		describe("strict mode", function () {