Keep OrderedProperties keySet and values in sync with the map

[rootvector2]

OrderedProperties.keySet() hands out orderedKeys itself, the private LinkedHashSet that tracks insertion order, so mutations through the returned set never reach the backing Hashtable. keySet().remove(k) reports success and drops the key from keys(), entrySet() and store() while getProperty(k) still returns the value, and keySet().add(k) succeeds where a key set view has to reject it. values() has the mirror problem: it is not overridden, so the inherited view deletes the mapping and leaves a stale key behind, which makes toString() throw.

Both now return small views whose removals route through the map, so the order tracker and the store move together. Keeping it in the views is what makes the plain Properties idioms work here, rather than callers having to know that remove(k) is the only safe way to delete a key. Found by running SpotBugs over the module, which flags the keySet() return as exposed internal state.

• Read the contribution guidelines for this project.
• Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
• I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
• Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
• Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
• Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
• Each commit in the pull request should have a meaningful subject line and body.

[Copilot]

Pull request overview

This PR fixes OrderedProperties view consistency so keySet() and values() behave like proper map-backed views: removals through the views update both the backing Properties/Hashtable mappings and the internal insertion-order tracker (orderedKeys), avoiding stale keys/values and failures like toString() throwing.

Changes:

• Replace the exposed internal orderedKeys return from keySet() with a dedicated key-set view that routes removals/clear through the map.
• Override values() to return an insertion-ordered values view whose removals also remove the corresponding key/mapping.
• Add unit tests (including parameterized coverage) for removals via key/value views and for rejecting keySet().add(...).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
src/main/java/org/apache/commons/collections4/properties/OrderedProperties.java	Adds key/value view implementations and an ordered-key iterator to keep orderedKeys and the backing map in sync during view mutations.
src/test/java/org/apache/commons/collections4/properties/OrderedPropertiesTest.java	Adds tests validating key/value view removal behavior and that keySet().add(...) is rejected.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[garydgregory]

@rootvector2 see copilot comments. If you ran copilot on your own branch before creating a PR, we could save some time here.

[rootvector2]

Addressed the copilot comment, the iterator's remove() now takes the OrderedProperties monitor before touching orderedKeys. Fair point on process, I'll run a copilot pass on the branch before opening the next one.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/main/java/org/apache/commons/collections4/properties/OrderedProperties.java:200

• Returning a new view instance on every keySet() call can cause avoidable allocations in common call patterns (and differs from the usual JDK behavior of reusing a single view instance). Consider caching the view in a transient field (lazy init) and returning the cached instance; same recommendation applies to values().

    @Override
    public Set<Object> keySet() {
        return new KeySet();
    }

src/main/java/org/apache/commons/collections4/properties/OrderedProperties.java:246

• The new views/iterators inherit Hashtable-style concurrency expectations (external synchronization required during iteration), but orderedKeysIterator() mixes unsynchronized traversal (hasNext/next) with synchronized remove(). Please document the expected synchronization contract for iterating these views (e.g., callers should synchronized (this) while iterating), so users don’t assume the views provide thread-safe iteration.

    /**
     * Creates an iterator over the keys in insertion order whose {@link Iterator#remove()} also removes the mapping.
     *
     * @return A new iterator.
     */
    private Iterator<Object> orderedKeysIterator() {
        final Iterator<Object> iterator = orderedKeys.iterator();
        return new Iterator<Object>() {

            private Object last;

            @Override
            public boolean hasNext() {
                return iterator.hasNext();
            }

            @Override
            public Object next() {
                last = iterator.next();
                return last;
            }

            @Override
            public void remove() {
                // All orderedKeys writes happen under the OrderedProperties monitor.
                synchronized (OrderedProperties.this) {
                    // Not remove(Object), which would edit orderedKeys while this iterator walks it.
                    iterator.remove();
                    OrderedProperties.super.remove(last);
                }
            }
        };
    }

src/main/java/org/apache/commons/collections4/properties/OrderedProperties.java:55

• New behavior is introduced for view-driven clear() (and similarly in Values.clear()), but the added tests only cover removals and keySet().add(...). Please add unit test coverage that keySet().clear() and values().clear() remove all mappings and leave the order tracker/store consistent (e.g., size 0, empty toString(), and no stale keys).

        @Override
        public void clear() {
            OrderedProperties.this.clear();
        }

[garydgregory]

@rootvector2 Merged 🚀 , ty. Note that there was a missing test for addAll() on the key set. I added one.

[rootvector2]

thanks for merging, and good catch on the missing addAll() test.