Fix UnsupportedOperationException consistency in immutable graph classes#8096
Fix UnsupportedOperationException consistency in immutable graph classes#8096stevearmstrong-dev wants to merge 2 commits intogoogle:masterfrom
Conversation
This commit addresses issue google#3909 by ensuring that all Set-returning methods in ImmutableGraph, ImmutableValueGraph, and ImmutableNetwork consistently throw UnsupportedOperationException when modification is attempted, regardless of whether the collections are empty or not. Changes: - Wrapped all Set-returning methods (nodes(), edges(), adjacentNodes(), predecessors(), successors(), incidentEdges()) with Collections.unmodifiableSet() in all three immutable graph classes - Added comprehensive tests demonstrating the fix works for both empty and non-empty graphs - Applied changes to both JRE and Android versions Fixes google#3909
Enhancements: - Created ImmutableGraphTest (was missing) with 22 test methods - Added tests for clear() operations on all Set-returning methods - Added tests for Iterator.remove() operations - Enhanced existing ImmutableValueGraphTest and ImmutableNetworkTest All tests verify UnsupportedOperationException is thrown consistently for various mutation attempts (add, remove, clear, iterator.remove). Applied to both JRE and Android test modules.
|
See my comment on the original issue for my thoughts on this. |
|
@jrtom Got it - since all graph implementations rely on unmodifiable view Sets to maintain internal consistency, the fix needs to extend beyond just |
|
@stevearmstrong-dev I should clarify, before you invest more time in this: someone at Google (first candidates: @netdpb or @cpovirk) should sign off on this proposed solution. While I'm certainly an interested party in That said: while it would add a small bit of overhead, I don't think that it would harm anything to make this change across the board, and it might keep someone from making an unfortunate error down the line. So I would approve it if it were up to me. :) I'll also note that this behavior is arguably implied by this statement in GraphsExplained:
|
|
Thankyou @jrtom for the heads up. I'll keep track of this for further updates from the team. |
|
This does seem reasonable. I forget if even the I am fairly swamped right now but will put this on The List. |
cpovirk
added
type=enhancement
3 participants
Summary
Fixes #3909 by ensuring all Set-returning methods in ImmutableGraph, ImmutableValueGraph, and ImmutableNetwork consistently throw UnsupportedOperationException when modification is attempted, even for empty collections.
Changes
Wrapped all Set-returning methods with
Collections.unmodifiableSet():nodes()edges()adjacentNodes(N node)predecessors(N node)successors(N node)incidentEdges(N node)Applied changes to:
Added comprehensive tests demonstrating the fix works for both empty and non-empty graphs
Test Plan