Make default value cache immutable #642
Conversation
| * | ||
| * All other Objects we expect here (... basically from parsing json) are expected to be | ||
| * immutable: | ||
| * - java.lang.String (in the unlikely case we see other CharSequences we convert to String) |
There was a problem hiding this comment.
What about GStringImpl? Do we need that also?
There was a problem hiding this comment.
We do not get GStrings when parsing json files. See e.g. here.
| private static def immutable(Collection _in, Collection _out) { | ||
|
|
||
| for(def e : _in) { | ||
| if(e in List || e in Set || e in Map) { |
There was a problem hiding this comment.
Do we need to support List and Set? How is this defined in yaml?
There was a problem hiding this comment.
Set we actually do not need, I agree. It seems "out of the box" not possible to get sets when parsing json. --> Set support has been erased.
| @@ -18,7 +31,9 @@ class DefaultValueCache implements Serializable { | |||
| } | |||
|
|
|||
| static createInstance(Map defaultValues){ | |||
| instance = new DefaultValueCache(defaultValues) | |||
| instance = new DefaultValueCache( | |||
| immutable(defaultValues) | |||
There was a problem hiding this comment.
Hm. I found that we are manipulating the default values at multiple places:
https://github.com/SAP/cloud-s4-sdk-pipeline-lib/blob/4a77b8bcb236c808bd80f78eade769e4336d7f2b/vars/setupDownloadCache.groovy#L17
https://github.com/SAP/cloud-s4-sdk-pipeline-lib/blob/4a77b8bcb236c808bd80f78eade769e4336d7f2b/vars/setupDownloadCache.groovy#L23
The reason is that we have default values that depend on the configuration of the Jenkins.
There was a problem hiding this comment.
Does the second example above really affect the default values?
The funny thing is that the first example is annotated with a fixme comment (Here we misuse the defaultConfiguration ...)
The point is: default values are shared over several steps. Having the defaults mutable opens a huge area for introducing bugs. These bugs are not obvious and hard to troubleshoot, as we already saw with the phantom stash in neoDeploy. I think the normal mind set when developing is "Default Values? Well, they are stable ...". But with the current approach they can be manipulated by error. See our issue with stashing and neoDeploy ... Having our ConfigMerger approach it is fully intransparent that certain mutable collections etc. comes without defensive copy from the default values.
We can discuss if we would like to continue with mutable default values. My personal opinion is: we will have less trouble if they are immutable. This change is not provided for fun or for the sake of sticking to some academical coding principles.
@OliverNocon already suggested to provide a deep defensive copy of the configuration when calling ConfigurationHelper#use(). That copy can be - of course - mutable. With that we avoid updating default values from one step so that update is visible to other steps. That would be IMO also an improvement, also reducing the risk of unplanned config updates. But again: in my personal opinion our life is less complicated if the default values are immutable. I will provide another PR creating defensive copies in ConfigurationHelper#use(./.)
There was a problem hiding this comment.
I think as we can still create a new instance, I am fine with the change.
marcusholl
force-pushed
the
pr/immutableDefaultValues
branch
from
9e1ecd1
to
2879c48
Compare
|
@daniel-kurzynski : regarding your comment above from April 12th. Do you think it would be possible to resolve your concerns by having a syntax like e.g. The second occurance injects a constant from |
|
Too old, maybe obsolute with upcoming go implementation |
OliverNocon
added
invalid
and removed
config
REVIEW
The default values provided by us via
resources/default_pipeline_environment.ymlmust not be modifiable by pipelines / pipeline steps. If these properties are modifiable we get in trouble like here: SAP-archive/jenkins-pipelines#18 and here: SAP-archive/jenkins-pipelines#16.This is "fixing the root cause" for this type of issue. With this PR we ensure that the default values cannot be updated by pipelines / pipeline steps.
The unit tests fails until the fix(es) for the issue above are not merged. The corresponding pull requests are:
#632 (especially this one),
#631 and
#614