Provide a deep config copy from ConfigHelper.use() #701
Conversation
marcusholl
requested review from
OliverNocon,
benhei,
CCFenner and
daniel-kurzynski
| * with the other signatures | ||
| */ | ||
| static deepCopy(def original) { | ||
| original |
There was a problem hiding this comment.
Which types do we here in practice?
I would expect numbers and Strings. Both are to my knownledge immutable (so no problem).
However, can we codify this assumption and fail if it is violated?
There was a problem hiding this comment.
Regarding the types there is a description available on that site: http://groovy-lang.org/json.html.
We have to deal with:
java.lang.Stringjava.lang.BigDecimal,java.lang.Integerjava.lang.Boolean, maybe the corresponding primitivejava.util.Date- and of course
java.util.LinkedHashMapandjava.util.ArrayList
The only issue we have wrt immutability is the Date. Currently we do not use this type in our config.
However, can we codify this assumption and fail if it is violated?
That a good approach. In the "sister" pull request #642 we have already such a type check. Since Date is not used up to now this type is omitted there. I will introduce this check also here.
There was a problem hiding this comment.
However, can we codify this assumption and fail if it is violated?
First I though that would in principle a good idea. In the meantime I played around with it and now I think we are in a better shape when we do not enforce immutability here.
Another question is: should the default config be immutable. IMO the answer to that question is: it should be immutable. But for that we have PR #642, which is not superseded by this PR here.
With this PR we ensure that the collections and the maps are copied (... deep). These collections are not imutable (... if they would some code manipulating the config map would not not work anymore), so we do not focus on immutability here for the maps and the collections.
In the test cases we have also examples where we hand-over instances into the steps which are mutable (basically mocks for e.g. utils). But I can also imagine use-cases where this is reasonable for real-world use cases.
@benhei : having this in mind you might re-think about the proposal of ensuring immutability for the types contained in the maps/collections. Again: the default configs immutability is addressed by #642.
An example how dealing with immutable values contained somewhere in the config might look like is provided by marcusholl#4. But as outlined above I would like to suggest not to go this way.
Other question (should be IMO handled in another PR in case we would like to have this): How should we deal with the parameters handed over via step-signature?
There was a problem hiding this comment.
I think we have a miss-understanding here. My question was whether we can ensure the contract "either a object is mutable and is copied or its immutable". Lets discuss tomorrow.
There was a problem hiding this comment.
@benhei : I checked the details.
we have to deal with the following mutable classes when running the tests (each occurance decorated by me with a comment explaining more details):
ArtifactSetVersionTest // anonymous class or closure
CheckChangeInDevelopmentTest // anonymous class or closure
CloudFoundryDeployTest // anonymous class or closure
SOLMAN/com.sap.piper.cm.BackendType. // real issue, the enum is expected to be provided also in real life from pipelines (or config
TransportRequestCreateTest // anonymous class or closure
TransportRequestReleaseTest// anonymous class or closure
TransportRequestUploadFileTest// anonymous class or closure
artifactSetVersion// anonymous class or closure
com.sap.piper.DescriptorUtils // looks like mocking
com.sap.piper.JenkinsUtils // looks like mocking
com.sap.piper.Utils // looks like mocking
com.sap.piper.integration.WhitesourceOrgAdminRepository // real issue. This type is expected to be provided from a pipeline
com.sap.piper.integration.WhitesourceRepository // real issue. This type is expected to be provided from a pipeline
So we have real use cases where we can hand over (via signature) mutable instances. I cannot say much about the whitesource, but in general I can think about reasonable cases. Regarding the enums: to me it looks like there is nothing wrong with providing enums via signature. This is better than strings. And enums are not immutabe by default. I mean most enums in reality will be imutable, but there is no contract requiring that ...
I will add comments to the corresponding methods creating defensive copies or the Maps/Collections saying that only the Maps/Collections are copied.
That is not a perfect solution, but it helps us for the problems we see currently.
| nextLevel: [ | ||
| list: ['x', 'y'], | ||
| duplicate: l, | ||
| set: (Set)[9, 8, 7] |
There was a problem hiding this comment.
Would [9, 8, 7] as Set work here?
There was a problem hiding this comment.
Yes, this would also work here. So we have several options to get a Set here rather than a list.
The config map prepared by ConfigHelper is a mix from several configuration levels. The lowest config level (DefaultValueCache) is shared between several ConfigHelper invocations. In case a Map or Collection which is inherited from the DefaultValueCache level gets modified, this is also visible for all subsequent steps. This causes trouble and situation which are hard to debug. With this change here each invocation of ConfigHelper.use() provides a deep defensive copy. With that we can ensure that there is no configuration update from one step to another.
marcusholl
force-pushed
the
pr/defensiveCopy
branch
from
02b37ae
to
7a7fd3e
Compare
The config map prepared by
ConfigHelperis a mix from several configuration levels. The lowest config level (DefaultValueCache) is shared between severalConfigHelper/step invocations. In case a Map or Collection which is inherited from theDefaultValueCachelevel gets modified, this is also visible for all subsequent steps. This causes trouble and situations which are hard to debug (See e.g. SAP-archive/jenkins-pipelines#16).With this change here each invocation of ConfigHelper.use() provides a deep defensive copy. With that we can ensure that there is no configuration update from one step to another.
There is another related pull request making the DefaultValueCache immutable (#642). This PR is not superseded by thisone. This two PRs complements each other.
Currently #642 fails since there is an update performed the the config map from one of the steps (hitting finally an unmodifiable Map). With is resolved with the PR here since the immutable map is replaced by another (mutable) map when creating the defensive copy.