From c086fd6317824c6ff41b581e12c20077c32cf083 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Tue, 15 Jul 2025 15:18:09 +0100 Subject: [PATCH] add remove-elements Update Xobj.java debug test add element partial revert of add changes extra add methods try to use add again Update XmlComplexContentImpl.java revert some code to fix compile test Update XmlComplexContentImpl.java fix remove Update Xobj.java refactor Update XmlComplexContentImpl.java refactor to do some bulk finds another refactor to improve perf see if I can remove complicated and inefficient code Revert "see if I can remove complicated and inefficient code" This reverts commit e8779ad0f986aa387fcb702640b5c6d467c3a06c. avoid find_element_user Update XmlComplexContentImpl.java version Update XmlComplexContentImpl.java --- build.xml | 2 +- gradle.properties | 4 +- .../org/apache/xmlbeans/impl/store/Cur.java | 2 +- .../org/apache/xmlbeans/impl/store/Xobj.java | 211 ++++++++++++++++- .../xmlbeans/impl/values/TypeStore.java | 61 ++++- .../impl/values/XmlComplexContentImpl.java | 212 +++++++++++++++--- src/test/java/common/Common.java | 16 ++ .../scomp/detailed/SchemaCompilerTests.java | 3 +- 8 files changed, 459 insertions(+), 52 deletions(-) diff --git a/build.xml b/build.xml index 66587bb65..e2ac90804 100644 --- a/build.xml +++ b/build.xml @@ -21,7 +21,7 @@ - + diff --git a/gradle.properties b/gradle.properties index 1ff4209cf..03001ee23 100644 --- a/gradle.properties +++ b/gradle.properties @@ -14,8 +14,8 @@ # comment-out, because of naming problems with the distribution plugin -#project.version=5.3.1-SNAPSHOT -XMLBeansVersion=5.3.1-SNAPSHOT +#project.version=5.4.0-SNAPSHOT +XMLBeansVersion=5.4.0-SNAPSHOT # Specifies the JVM arguments used for the daemon process. # The setting is particularly useful for tweaking memory settings. diff --git a/src/main/java/org/apache/xmlbeans/impl/store/Cur.java b/src/main/java/org/apache/xmlbeans/impl/store/Cur.java index 241508f6f..5b7d12eb4 100755 --- a/src/main/java/org/apache/xmlbeans/impl/store/Cur.java +++ b/src/main/java/org/apache/xmlbeans/impl/store/Cur.java @@ -343,7 +343,7 @@ static Xobj createElementXobj(Locale l, QName name, QName parentName) { private void createHelper(Xobj x) { assert x._locale == _locale; - // insert the new Xobj into an exisiting tree. + // insert the new Xobj into an existing tree. if (isPositioned()) { Cur from = tempCur(x, 0); diff --git a/src/main/java/org/apache/xmlbeans/impl/store/Xobj.java b/src/main/java/org/apache/xmlbeans/impl/store/Xobj.java index 55391fe7d..7e42ebf50 100644 --- a/src/main/java/org/apache/xmlbeans/impl/store/Xobj.java +++ b/src/main/java/org/apache/xmlbeans/impl/store/Xobj.java @@ -1992,6 +1992,19 @@ public void find_all_element_users(QName name, List fil } } + @SuppressWarnings("unchecked") + @Override + public void find_multiple_element_users(final QName name, final List fillMeUp, + final int maxCount) { + int count = 0; + for (Xobj x = _firstChild; x != null && count < maxCount; x = x._nextSibling) { + if (x.isElem() && x._name.equals(name)) { + fillMeUp.add((T) x.getUser()); + count++; + } + } + } + @SuppressWarnings("unchecked") @Override public void find_all_element_users(QNameSet names, List fillMeUp) { @@ -2002,6 +2015,19 @@ public void find_all_element_users(QNameSet names, List } } + @SuppressWarnings("unchecked") + @Override + public void find_multiple_element_users(final QNameSet names, final List fillMeUp, + final int maxCount) { + int count = 0; + for (Xobj x = _firstChild; x != null && count < maxCount; x = x._nextSibling) { + if (x.isElem() && names.contains(x._name)) { + fillMeUp.add((T) x.getUser()); + count++; + } + } + } + private static TypeStoreUser insertElement(QName name, Xobj x, int pos) { x._locale.enter(); @@ -2017,6 +2043,26 @@ private static TypeStoreUser insertElement(QName name, Xobj x, int pos) { } } + private static TypeStoreUser[] insertElements(final QName name, final Xobj x, + final int pos, final int count) { + x._locale.enter(); + + TypeStoreUser[] users = new TypeStoreUser[count]; + try { + Cur c = x._locale.tempCur(); + c.moveTo(x, pos); + for (int i = count - 1; i >= 0; i--) { + c.createElement(name); + users[i] = c.getUser(); + } + c.release(); + } finally { + x._locale.exit(); + } + return users; + } + + @Override public TypeStoreUser insert_element_user(QName name, int i) { if (i < 0) { throw new IndexOutOfBoundsException(); @@ -2039,6 +2085,35 @@ public TypeStoreUser insert_element_user(QName name, int i) { return insertElement(name, x, 0); } + @Override + public TypeStoreUser[] insert_elements_users(final QName name, final int i, + final int count) { + if (i < 0) { + throw new IndexOutOfBoundsException(); + } + + if (!isContainer()) { + throw new IllegalStateException(); + } + + if (count <= 0) { + return new TypeStoreUser[0]; + } + + Xobj x = _locale.findNthChildElem(this, name, null, i); + + if (x == null) { + if (i > _locale.count(this, name, null) + 1) { + throw new IndexOutOfBoundsException(); + } + + return add_elements_users(name, count); + } + + return insertElements(name, x, 0, count); + } + + @Override public TypeStoreUser insert_element_user(QNameSet names, QName name, int i) { if (i < 0) { throw new IndexOutOfBoundsException(); @@ -2061,6 +2136,35 @@ public TypeStoreUser insert_element_user(QNameSet names, QName name, int i) { return insertElement(name, x, 0); } + @Override + public TypeStoreUser[] insert_elements_users(final QNameSet names, final QName name, + final int i, final int count) { + if (i < 0) { + throw new IndexOutOfBoundsException(); + } + + if (!isContainer()) { + throw new IllegalStateException(); + } + + if (count <= 0) { + return new TypeStoreUser[0]; + } + + Xobj x = _locale.findNthChildElem(this, null, names, i); + + if (x == null) { + if (i > _locale.count(this, null, names) + 1) { + throw new IndexOutOfBoundsException(); + } + + return add_elements_users(name, count); + } + + return insertElements(name, x, 0, count); + } + + @Override public TypeStoreUser add_element_user(QName name) { if (!isContainer()) { throw new IllegalStateException(); @@ -2094,6 +2198,51 @@ public TypeStoreUser add_element_user(QName name) { : insertElement(name, candidate, 0); } + @Override + public TypeStoreUser[] add_elements_users(final QName name, final int count) { + if (!isContainer()) { + throw new IllegalStateException(); + } + + if (count <= 0) { + return new TypeStoreUser[0]; + } + + QNameSet endSet = null; + boolean gotEndSet = false; + + Xobj candidate = null; + + for (Xobj x = _lastChild; x != null; x = x._prevSibling) { + if (x.isContainer()) { + if (x._name.equals(name)) { + break; + } + + if (!gotEndSet) { + endSet = _user.get_element_ending_delimiters(name); + gotEndSet = true; + } + + if (endSet == null || endSet.contains(x._name)) { + candidate = x; + } + } + } + + final TypeStoreUser[] users; + if (candidate == null) { + // If there is no candidate, then I need to insert at the end of this container + // and create a new element for each of the count + users = insertElements(name, this, END_POS, count); + } else { + // If I have a candidate, then I need to insert at the candidate and create + // a new element for each of the count + users = insertElements(name, candidate, 0, count); + } + return users; + } + private static void removeElement(Xobj x) { if (x == null) { throw new IndexOutOfBoundsException(); @@ -2110,6 +2259,7 @@ private static void removeElement(Xobj x) { } } + @Override public void remove_element(QName name, int i) { if (i < 0) { throw new IndexOutOfBoundsException(); @@ -2130,6 +2280,30 @@ public void remove_element(QName name, int i) { removeElement(x); } + @Override + public void remove_elements_after(QName name, int i) { + if (i < 0) { + throw new IndexOutOfBoundsException(); + } + + if (!isContainer()) { + throw new IllegalStateException(); + } + + ArrayList toRemove = new ArrayList<>(); + Xobj x; + for (x = _firstChild; x != null; x = x._nextSibling) { + if (x.isElem() && x._name.equals(name) && --i < 0) { + toRemove.add(x); + } + } + final int size = toRemove.size(); + for (int j = size - 1; j >= 0; j--) { + removeElement(toRemove.get(j)); + } + } + + @Override public void remove_element(QNameSet names, int i) { if (i < 0) { throw new IndexOutOfBoundsException(); @@ -2150,6 +2324,25 @@ public void remove_element(QNameSet names, int i) { removeElement(x); } + @Override + public void remove_elements_after(QNameSet names, int i) { + if (i < 0) { + throw new IndexOutOfBoundsException(); + } + + if (!isContainer()) { + throw new IllegalStateException(); + } + + Xobj x; + + for (x = _firstChild; x != null; x = x._nextSibling) { + if (x.isElem() && names.contains(x._name) && --i < 0) { + removeElement(x); + } + } + } + public TypeStoreUser find_attribute_user(QName name) { Xobj a = getAttr(name); @@ -2294,7 +2487,7 @@ public void array_setter(XmlObject[] sources, QName elementName) { try { // TODO - this is the quick and dirty implementation, make this faster - int m = sources.length; + final int m = sources.length; List copies = new ArrayList<>(); List types = new ArrayList<>(); @@ -2326,18 +2519,14 @@ public void array_setter(XmlObject[] sources, QName elementName) { } } - int n = count_elements(elementName); + final int n = count_elements(elementName); - for (; n > m; n--) { - remove_element(elementName, m); + if (n > m) { + remove_elements_after(elementName, m); + } else if (n < m) { + add_elements_users(elementName, m - n); } - for (; m > n; n++) { - add_element_user(elementName); - } - - assert m == n; - List elementsUser = new ArrayList<>(); find_all_element_users(elementName, elementsUser); @@ -2348,8 +2537,6 @@ public void array_setter(XmlObject[] sources, QName elementName) { .map(x -> (Xobj) x) .collect(Collectors.toList()); - assert elements.size() == n; - Cur c = tempCur(); for (int i = 0; i < n; i++) { diff --git a/src/main/java/org/apache/xmlbeans/impl/values/TypeStore.java b/src/main/java/org/apache/xmlbeans/impl/values/TypeStore.java index 876fbf7c8..5e404bf14 100644 --- a/src/main/java/org/apache/xmlbeans/impl/values/TypeStore.java +++ b/src/main/java/org/apache/xmlbeans/impl/values/TypeStore.java @@ -183,7 +183,19 @@ public interface TypeStore extends NamespaceManager * Returns all TypeStoreUsers corresponding to elements with one * of the names is the QNameSet. */ - void find_all_element_users(QNameSet name, List fillMeUp); + void find_all_element_users(QNameSet names, List fillMeUp); + + /** + * @since 5.4.0 + */ + void find_multiple_element_users(QName name, List fillMeUp, + int maxCount); + + /** + * @since 5.4.0 + */ + void find_multiple_element_users(QNameSet names, List fillMeUp, + int maxCount); /** * Inserts a new element at the position that will make it @@ -202,11 +214,35 @@ public interface TypeStore extends NamespaceManager TypeStoreUser insert_element_user(QName name, int i); /** - * Like the above method, except that it inserts an element named + * Inserts new elements at the position that will make the first one + * the ith element with the given name owned by this textstore, + * and returns a TypeStoreUser array with all new elements. + * + * Note that if there are no existing elements of the given + * name, you may need to call back to discover the proper + * ordering to use to insert the first one. Otherwise, + * it should be inserted adjacent to existing elements with + * the same name. + * + * Should throw an IndexOutOfBoundsException if i < 0 + * or if i > # of elts + * @since 5.4.0 + */ + TypeStoreUser[] insert_elements_users(QName name, int i, int count); + + /** + * Like the {@link #insert_element_user(QName, int)} method, except that it inserts an element named * name, after the ith member of set. */ TypeStoreUser insert_element_user(QNameSet set, QName name, int i); + /** + * Like the {@link #insert_elements_users(QName, int, int)} method, except that it inserts elements named + * name, after the ith member of set. + * @since 5.4.0 + */ + TypeStoreUser[] insert_elements_users(QNameSet set, QName name, int i, int count); + /** * Adds a new element at the last position adjacent to existing * elements of the same name. @@ -216,6 +252,16 @@ public interface TypeStore extends NamespaceManager */ TypeStoreUser add_element_user(QName name); + /** + * Adds elements at the last position adjacent to existing + * elements of the same name. + * + * Note that if there are no existing elements of the given + * name, the same comment applies as with insert_element_user. + * @since 5.4.0 + */ + TypeStoreUser[] add_elements_users(QName name, int count); + /** * Removes the ith element with the given name. * @@ -229,6 +275,17 @@ public interface TypeStore extends NamespaceManager */ void remove_element(QNameSet names, int i); + /** + * Removes all elements after the i-th element with the given name. + * @since 5.4.0 + */ + void remove_elements_after(QName name, int i); + + /** + * Removes all elements after the i-th element with the given name. + * @since 5.4.0 + */ + void remove_elements_after(QNameSet name, int i); /** * Returns the TypeStoreUser underneath the attribute with the given diff --git a/src/main/java/org/apache/xmlbeans/impl/values/XmlComplexContentImpl.java b/src/main/java/org/apache/xmlbeans/impl/values/XmlComplexContentImpl.java index 2407c785e..b4ad6a119 100644 --- a/src/main/java/org/apache/xmlbeans/impl/values/XmlComplexContentImpl.java +++ b/src/main/java/org/apache/xmlbeans/impl/values/XmlComplexContentImpl.java @@ -22,6 +22,7 @@ import javax.xml.namespace.QName; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.ArrayList; import java.util.Calendar; import java.util.Date; import java.util.List; @@ -315,13 +316,10 @@ protected void arraySetterHelper(XmlObject[] sources, QName elemName, QNameSet s TypeStore store = get_store(); if (sources == null || sources.length == 0) { - int m = (set == null) ? store.count_elements(elemName) : store.count_elements(set); - for (; m > 0; m--) { - if (set == null) { - store.remove_element(elemName, 0); - } else { - store.remove_element(set, 0); - } + if (set == null) { + store.remove_elements_after(elemName, 0); + } else { + store.remove_elements_after(set, 0); } return; } @@ -428,6 +426,140 @@ protected void arraySetterHelper(XmlObject[] sources, QName elemName, QNameSet s // get_store().array_setter( sources, elemName ); } + protected void arraySetterHelper2(XmlObject[] sources, QName elemName, QNameSet set) { + TypeStore store = get_store(); + + if (sources == null || sources.length == 0) { + if (set == null) { + store.remove_elements_after(elemName, 0); + } else { + store.remove_elements_after(set, 0); + } + return; + } + + // Verify if the sources contain children of this node + int i; + // how many elements in the original array + int m = (set == null) ? store.count_elements(elemName) : store.count_elements(set); + int startSrc = 0, startDest = 0; + for (i = 0; i < sources.length; i++) { + if (sources[i].isImmutable()) { + continue; + } + try (XmlCursor c = sources[i].newCursor()) { + if (c.toParent() && c.getObject() == this) { + break; + } + } + } + if (i < sources.length) { + TypeStoreUser current = (set == null) ? store.find_element_user(elemName, 0) : store.find_element_user(set, 0); + if (current == sources[i]) { + // The new object matches what already exists in the array + // Heuristic: we optimize for the case where the new elements + // in the array are the same as the existing elements with + // potentially new elements inserted + + // First insert the new element in the array at position 0 + int j = 0; + if (i > 0) { + TypeStoreUser[] users; + if (set == null) { + users = store.insert_elements_users(elemName, 0, i); + } else { + users = store.insert_elements_users(set, elemName, 0, i); + } + for (TypeStoreUser user : users) { + ((XmlObjectBase) user).set(sources[j++]); + } + } + final ArrayList users = new ArrayList<>(sources.length); + if (set == null) { + store.find_all_element_users(elemName, users); + } else { + store.find_all_element_users(set, users); + } + final int usersLen = users.size(); + int inserted = 0; + for (i++, j++; i < sources.length; i++, j++) { + // Cursor is implicitly closed + XmlCursor c = sources[i].isImmutable() ? null : sources[i].newCursor(); + if (c != null && c.toParent() && c.getObject() == this) { + c.close(); + final int pos = j + inserted; + current = pos < usersLen ? users.get(pos) : null; + if (current != sources[i]) { + // Fall back to the general case + break; + } + } else { + if (c != null) { + c.close(); + } + // Insert before the current element + TypeStoreUser user = (set == null) ? store.insert_element_user(elemName, j) : store.insert_element_user(set, elemName, j); + ((XmlObjectBase) user).set(sources[i]); + inserted++; + } + } + startDest = j; + startSrc = i; + m = store.count_elements(elemName); + } + // Fall through + } else { + // All of the elements in the existing array are to + // be deleted and replaced with elements from the + // sources array + } + + // The general case: we assume that some of the elements + // in the new array already exist, but at different indexes + + // Starting with position i in the sources array, copy the remaining elements + // to the end of the original array... + if (i < sources.length) { + TypeStoreUser[] users = store.add_elements_users(elemName, sources.length - i); + int j = i; + for (TypeStoreUser user : users) { + ((XmlObjectBase) user).set(sources[j++]); + } + } + + // ... then come back and insert the elements starting with startSource + // up to i from the sources array into the current array, starting with + // startDest + final int n = i; + + if (set == null) { + store.remove_elements_after(elemName, n - startSrc + startDest); + } else { + store.remove_elements_after(set, n - startSrc + startDest); + } + + + final int size = Math.min(m - startDest, sources.length - startSrc); + ArrayList users = new ArrayList<>(size); + if (set == null) { + store.find_multiple_element_users(elemName, users, size); + } else { + store.find_multiple_element_users(set, users, size); + } + i = startSrc; + for (XmlObjectBase user : users) { + user.set(sources[i++]); + } + + for (TypeStoreUser u : store.add_elements_users(elemName, n - i)) { + ((XmlObjectBase) u).set(sources[i++]); + } + + // We can't just delegate to array_setter because we need + // synchronization on the sources (potentially each element + // in the array on a different lock) + // get_store().array_setter( sources, elemName ); + } private void commonSetterHelper(QName elemName, QNameSet set, T[] sources, BiConsumer fun) { commonSetterHelper(elemName, set, (sources == null) ? 0 : sources.length, fun); @@ -438,54 +570,68 @@ private void commonSetterHelper(QName elemName, QNameSet set, int n, BiConsumer< int m = (set == null) ? store.count_elements(elemName) : store.count_elements(set); - for (; m > n; m--) { + if (m > n) { if (set == null) { - store.remove_element(elemName, m - 1); + store.remove_elements_after(elemName, n); } else { - store.remove_element(set, m - 1); + store.remove_elements_after(set, n); } + m = n; } - for (int i = 0; i < n; i++) { - TypeStoreUser user; + ArrayList users = new ArrayList<>(m); + if (set == null) { + store.find_all_element_users(elemName, users); + } else { + store.find_all_element_users(set, users); + } + int i = 0; + for (XmlObjectBase user : users) { + fun.accept(user, i++); + } - if (i >= m) { - user = store.add_element_user(elemName); - } else if (set == null) { - user = store.find_element_user(elemName, i); - } else { - user = store.find_element_user(set, i); - } - fun.accept((XmlObjectBase) user, i); + for (TypeStoreUser u : store.add_elements_users(elemName, n - m)) { + fun.accept((XmlObjectBase) u, i++); } } private void commonSetterHelper2(QName elemName, QNameSet set, T[] sources, BiConsumer c) { - int n = (sources == null) ? 0 : sources.length; + final int n = (sources == null) ? 0 : sources.length; TypeStore store = get_store(); int m = (set == null) ? store.count_elements(elemName) : store.count_elements(set); - for (; m > n; m--) { + if (m > n) { + // If the existing array is longer than the new one, we need to remove + // the extra elements if (set == null) { - store.remove_element(elemName, m - 1); + store.remove_elements_after(elemName, n); } else { - store.remove_element(set, m - 1); + store.remove_elements_after(set, n); } + m = n; } - for (int i = 0; i < n; i++) { - TypeStoreUser user; + ArrayList users = new ArrayList<>(m); + if (set == null) { + store.find_all_element_users(elemName, users); + } else { + store.find_all_element_users(set, users); + } + int i = 0; + for (XmlObjectBase user : users) { + if (i >= n) { + break; + } + c.accept(user, sources[i++]); + } - if (i >= m) { - user = store.add_element_user(elemName); - } else if (set == null) { - user = store.find_element_user(elemName, i); - } else { - user = store.find_element_user(set, i); + for (TypeStoreUser u : store.add_elements_users(elemName, n - m)) { + if (i >= n) { + break; } - c.accept((XmlObjectBase) user, sources[i]); + c.accept((XmlObjectBase) u, sources[i++]); } } } diff --git a/src/test/java/common/Common.java b/src/test/java/common/Common.java index 8569433fa..e34bb3081 100644 --- a/src/test/java/common/Common.java +++ b/src/test/java/common/Common.java @@ -119,6 +119,22 @@ public static boolean hasSevereError(List errors) { return errFound; } + public static String collateSevereErrors(List errors) { + boolean errFound = errors.stream().anyMatch(e -> e.getSeverity() == XmlError.SEVERITY_ERROR); + String errorTxt = ""; + if (errFound) { + StringBuilder errorBuilder = new StringBuilder("Errors found:\n"); + for (XmlError xmlError : errors) { + if (xmlError.getSeverity() == XmlError.SEVERITY_ERROR) { + errorBuilder.append(xmlError).append("\n"); + } + } + errorTxt = errorBuilder.toString(); + } + errors.clear(); + return errorTxt; + } + /** * Validate schemas to instance based on the docType */ diff --git a/src/test/java/compile/scomp/detailed/SchemaCompilerTests.java b/src/test/java/compile/scomp/detailed/SchemaCompilerTests.java index f5d13bbea..a825103d8 100644 --- a/src/test/java/compile/scomp/detailed/SchemaCompilerTests.java +++ b/src/test/java/compile/scomp/detailed/SchemaCompilerTests.java @@ -24,6 +24,7 @@ import java.util.List; import static common.Common.*; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; /** @@ -144,7 +145,7 @@ void testNoExt() { params.setNoExt(true); SchemaCompiler.compile(params); - assertFalse(hasSevereError(errors), "testNoExt(): failure when executing scomp"); + assertEquals("", collateSevereErrors(errors)); } @Test