search

Netflix / archaius

Library for configuration management API
Apache License 2.0
2.46k stars 485 forks source link

Disallow mutation of internal config state via keys Iterator #645

Closed kilink closed 1 year ago

kilink commented 1 year ago

Config implementations exposed internal mutable state by returning keySet iterators for their underlying Maps because the default Iterator for HashMap / LinkedHashMap implements remove. Wrap all of the maps with Collections.unmodifiableMap to avoid the issue.

rgallardo-netflix commented 1 year ago

I like the change, but help me think through why we don't care about breaking the behavior for anyone who is currently relying on this. My gut feeling is that they'd be causing weird trouble for themselves anyway and so no one is doing it, but I want to be convinced before merging.

kilink commented 1 year ago

The Javadoc for Config states that the API is read-only, so in my mind it is not backwards incompatible at all. Mutating the internals of the config in this manner wouldn't even be safe since most implementations use HashMap, and Config itself is intended to be accessed from multiple threads. I think in the unlikely case that someone is using the Iterator in this fashion they are probably shooting themselves in the foot.

rgallardo-netflix commented 1 year ago

Well, I wasn't thinking about published API, more about https://imgs.xkcd.com/comics/workflow.png But your point about corruption is good enough, and it also made me realize that mutating internal state in this manner would skip firing notifications to listeners, so definitely broken and whoever is doing it deserves to have their code fail.

I'll merge.