This is an automated email from the ASF dual-hosted git repository. tballison pushed a commit to branch TIKA-4809-metadata-key-api in repository https://gitbox.apache.org/repos/asf/tika.git
commit 7816e83bfa6389298f8199c44fa4ef9c4e888b76 Author: tallison <[email protected]> AuthorDate: Tue Aug 11 11:21:24 2026 -0400 TIKA-4809 metadata-key stage 1: Property registration fix --- .../java/org/apache/tika/metadata/Property.java | 46 ++++++++- .../org/apache/tika/metadata/PropertyTest.java | 113 +++++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-) diff --git a/tika-core/src/main/java/org/apache/tika/metadata/Property.java b/tika-core/src/main/java/org/apache/tika/metadata/Property.java index 3d67141414..aa3df83307 100644 --- a/tika-core/src/main/java/org/apache/tika/metadata/Property.java +++ b/tika-core/src/main/java/org/apache/tika/metadata/Property.java @@ -25,6 +25,9 @@ import java.util.SortedSet; import java.util.TreeSet; import java.util.concurrent.ConcurrentHashMap; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** * XMP property definition. Each instance of this class defines a single * metadata property like "dc:format". In addition to the property name, @@ -36,6 +39,7 @@ import java.util.concurrent.ConcurrentHashMap; */ public final class Property implements Comparable<Property> { + private static final Logger LOG = LoggerFactory.getLogger(Property.class); private static final Map<String, Property> PROPERTIES = new ConcurrentHashMap<>(); private final String name; private final boolean internal; @@ -51,6 +55,20 @@ public final class Property implements Comparable<Property> { private Property(String name, boolean internal, PropertyType propertyType, ValueType valueType, String[] choices, Property primaryProperty, Property[] secondaryExtractProperties) { + this(name, internal, propertyType, valueType, choices, primaryProperty, + secondaryExtractProperties, true); + } + + /** + * @param register whether to intern this Property in the static registry. Composites + * (non-null primaryProperty) never register, regardless. Non-composite + * registration is first-wins: a name collision keeps the + * earlier-registered Property and this one is not stored; a + * same-name-different-shape collision also logs a WARN. + */ + private Property(String name, boolean internal, PropertyType propertyType, ValueType valueType, + String[] choices, Property primaryProperty, + Property[] secondaryExtractProperties, boolean register) { this.name = name; this.internal = internal; this.propertyType = propertyType; @@ -70,8 +88,20 @@ public final class Property implements Comparable<Property> { this.secondaryExtractProperties = null; // Only store primary properties for lookup, not composites - synchronized (PROPERTIES) { - PROPERTIES.put(name, this); + if (register) { + synchronized (PROPERTIES) { + Property incumbent = PROPERTIES.putIfAbsent(name, this); + // same-shape re-mints (e.g. class re-init) stay quiet; a shape mismatch + // is a real bug (two differently-typed Properties claim the same name) + if (incumbent != null && (incumbent.propertyType != propertyType + || incumbent.valueType != valueType)) { + LOG.warn( + "Property registration collision for '{}': keeping {}/{}, " + + "dropping {}/{}", + name, incumbent.propertyType, incumbent.valueType, propertyType, + valueType); + } + } } } } @@ -94,6 +124,18 @@ public final class Property implements Comparable<Property> { this(name, internal, propertyType, valueType, null); } + /** + * Package-private, non-registering, lock-free minting path: skips {@code PROPERTIES} + * entirely, no interning and no lock. For call sites that construct Properties per-call + * from runtime/document-derived names, where interning would grow the static registry + * without bound (e.g. future {@code KeyPrefix} mints, a digest-template factory). + * Never composite. + */ + static Property mintUnregistered(String name, boolean internal, PropertyType propertyType, + ValueType valueType, String[] choices) { + return new Property(name, internal, propertyType, valueType, choices, null, null, false); + } + /** * Get the type of a property * diff --git a/tika-core/src/test/java/org/apache/tika/metadata/PropertyTest.java b/tika-core/src/test/java/org/apache/tika/metadata/PropertyTest.java new file mode 100644 index 0000000000..74d24439af --- /dev/null +++ b/tika-core/src/test/java/org/apache/tika/metadata/PropertyTest.java @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tika.metadata; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +/** Registration semantics of {@link Property}: first-wins interning, and the non-registering mint path. */ +public class PropertyTest { + + @Test + public void testFactoryRegistrationIsFirstWins() { + String name = "prop-test:collide-" + System.nanoTime(); + Property first = Property.internalText(name); + Property second = Property.internalInteger(name); + + // collision must not overwrite: the earlier-registered Property stays authoritative + assertSame(first, Property.get(name)); + assertEquals(Property.ValueType.TEXT, Property.get(name).getValueType()); + assertEquals(Property.ValueType.INTEGER, second.getValueType(), + "the losing Property object itself is still a valid, independent instance"); + } + + @Test + public void testShapeMismatchedCollisionStillFirstWins() { + // exercises the WARN branch (differing propertyType AND valueType); no log-capture + // idiom exists in this module, so this only asserts the first-wins outcome + String name = "prop-test:shape-mismatch-" + System.nanoTime(); + Property first = Property.internalText(name); + Property second = Property.internalDateBag(name); + + assertSame(first, Property.get(name)); + assertEquals(Property.PropertyType.SIMPLE, Property.get(name).getPropertyType()); + assertEquals(Property.PropertyType.BAG, second.getPropertyType()); + } + + @Test + public void testSameShapeReregistrationStaysQuietAndFirstWins() { + // exercises the non-WARN branch: identical shape, so no mismatch to report + String name = "prop-test:same-shape-" + System.nanoTime(); + Property first = Property.internalText(name); + Property second = Property.internalText(name); + + assertSame(first, Property.get(name)); + assertFalse(first == second); + } + + @Test + public void testUnregisteredPropertyIsFunctionalButNotInRegistry() { + String name = "prop-test:unregistered-" + System.nanoTime(); + assertNull(Property.get(name), "precondition: name must not already be registered"); + + Property minted = Property.mintUnregistered(name, false, Property.PropertyType.SIMPLE, + Property.ValueType.TEXT, null); + + assertEquals(name, minted.getName()); + assertEquals(Property.PropertyType.SIMPLE, minted.getPropertyType()); + assertEquals(Property.ValueType.TEXT, minted.getValueType()); + assertFalse(minted.isInternal()); + assertTrue(minted.isExternal()); + assertSame(minted, minted.getPrimaryProperty()); + + // never interned: absent from both single lookup and prefix lookup + assertNull(Property.get(name)); + assertFalse(Property.getProperties("prop-test").contains(minted)); + } + + @Test + public void testUnregisteredPropertyDoesNotBlockOrGetBlockedByFactoryRegistration() { + String name = "prop-test:coexist-" + System.nanoTime(); + + Property minted = Property.mintUnregistered(name, true, Property.PropertyType.SIMPLE, + Property.ValueType.TEXT, null); + assertNull(Property.get(name)); + + // a later factory call for the same name registers normally: the unregistered mint + // left no trace in PROPERTIES to collide with + Property registered = Property.internalText(name); + assertSame(registered, Property.get(name)); + assertFalse(minted == registered); + } + + @Test + public void testUnregisteredPropertyWithChoicesAndBagType() { + String name = "prop-test:bag-" + System.nanoTime(); + Property minted = Property.mintUnregistered(name, true, Property.PropertyType.BAG, + Property.ValueType.CLOSED_CHOICE, new String[] {"a", "b"}); + + assertEquals(Property.PropertyType.BAG, minted.getPropertyType()); + assertTrue(minted.isMultiValuePermitted()); + assertTrue(minted.getChoices().contains("a")); + assertNull(Property.get(name)); + } +}
