SLING-11878 | Update ModifiableValueMap javadoc to cover missing methods inherited from java.util.Map #48

toniedzwiedz · 2023-05-15T13:25:10Z

The original javadoc for ModifiableValueMap was written before the addition of new methods to java.util.Map as of Java 8. This commit updates the javadoc to mention the replace and replaceAll methods that can be used to modify resources underlying a ModifiableValueMap instance.

Refer to https://issues.apache.org/jira/browse/SLING-11878 for context.

toniedzwiedz · 2023-05-15T13:28:06Z

@stefanseifert I've got a follow-up question. Should we also cover computeIfAbsent and computeIfPresent? These were added in Java 8 as well but I'm not sure if the Javadoc should recommend for or against using them (or stay neutral)

stefanseifert · 2023-05-15T13:43:59Z

@toniedzwiedz yes, i think we can include them as well - please update the PR if possible

toniedzwiedz · 2023-05-15T14:01:16Z

@stefanseifert compute, computeIfPresent and computeIfAbsent have been added to the javadoc.

stefanseifert · 2023-05-15T14:06:31Z

@toniedzwiedz just stumbled over the merge method which was also added in java 8 - i think we should add it as well?

toniedzwiedz · 2023-05-15T14:14:12Z

@stefanseifert good spot. Could be, assuming that implementations actually support it correctly. I think this snowballed a little, to a point where I'm not 100% sure I've covered everything in my quick experiments with the Groovy console. Are there any tests we can fall back on, to ensure these recommendations actually hold water between releases?

stefanseifert · 2023-05-15T14:25:21Z

i'm quite sure all these method added in 1.8 still use the default JDK implementation and thus should not be affected by Sling module updates (looking e.g. at https://github.com/apache/sling-org-apache-sling-jcr-resource/blob/master/src/main/java/org/apache/sling/jcr/resource/internal/JcrModifiableValueMap.java). so i think it should be safe to rely on the documentation on those methods in the JDK javadocs, and according to them all compute and merge methods may modify the map.

stefanseifert · 2023-05-15T14:26:18Z

and to my knowledge there are no test cases that actually cover this - as they are default JDK methods not implemented directly.

inherited from java.util.Map The original javadoc for `ModifiableValueMap` was written before the addition of new methods to `java.util.Map` as of Java 8. This commit updates the javadoc to mention the following methods that can can be used to modify resources underlying a `ModifiableValueMap`: - `replace` - `replaceAll` - `compute` - `computeIfAbsent` - `computeIfPresent` - `merge` Refer to https://issues.apache.org/jira/browse/SLING-11878 for context.

toniedzwiedz · 2023-05-15T14:38:21Z

@stefanseifert okay, I'm not familiar with how ModifiableValueMap implementations (or the default one, most notably) translate these map operations to resource modification internally, and the Javadoc explicitly calls out some methods as potentially dangerous, so it felt better to ask.

I guess if all those methods do is eventually call put or some such internally, after evaluating a function passed by the caller, then we should be fine.

Anyway, I've added merge to the list.