google
Fix UnsupportedOperationException consistency in immutable graph classes
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:
- ImmutableGraph (JRE and Android)
- ImmutableValueGraph (JRE and Android)
- ImmutableNetwork (JRE and Android)
-
Added comprehensive tests demonstrating the fix works for both empty and non-empty graphs
Test Plan
- Added 8 new test methods to ImmutableValueGraphTest
- Added 8 new test methods to ImmutableNetworkTest
- Tests verify UOE is thrown consistently for all Set-returning methods
- All changes applied to both JRE and Android test modules
@jrtom Got it - since all graph implementations rely on unmodifiable view Sets to maintain internal consistency, the fix needs to extend beyond just Immutable* classes. I'll update
StandardMutable*, Forwarding*, and abstract base classes to ensure consistent UOE behavior throughout. Appreciate the clarification!
@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 common.graph (having originated it), and I expect David and Chris to give my opinions regarding it some weight, I no longer have the authority to approve changes to it, as I'm no longer a member of the GitHub "Google" organization.
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:
Accessors which return collections return unmodifiable views of the graph
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 Collections.unmodifiable* collections are as good about always throwing UOE for mutation operations (e.g., if you call removeAll(emptyCollection)), though that may have improved in recent years. Even if they're not perfect, I'm not sure we'd want to go to the effort to do something perfect.
I am fairly swamped right now but will put this on The List.
