Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Provide a deep config copy from ConfigHelper.use() #701

Merged
merged 3 commits into from
May 15, 2019
+75 −1
Merged

Provide a deep config copy from ConfigHelper.use() #701

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a7fd3e
Provide a deep config copy from ConfigHelper.use()
marcusholl May 10, 2019
a0649aa
Explain what 'deepCopy' means
marcusholl May 15, 2019
f38729c
Merge remote-tracking branch 'github/master' into HEAD
marcusholl May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/com/sap/piper/ConfigurationHelper.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,11 @@ class ConfigurationHelper implements Serializable {
handleValidationFailures()
MapUtils.traverse(config, { v -> (v instanceof GString) ? v.toString() : v })
if(config.verbose) step.echo "[${name}] Configuration: ${config}"
return config
return MapUtils.deepCopy(config)
}



/* private */ def getConfigPropertyNested(key) {
return getConfigPropertyNested(config, key)
}
Expand Down
34 changes: 34 additions & 0 deletions src/com/sap/piper/MapUtils.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,38 @@ class MapUtils implements Serializable {
}
m.putAll(updates)
}

static deepCopy(Map original) {
Map copy = [:]
for (def e : original.entrySet()) {
if(e.value == null) {
copy.put(e.key, e.value)
} else {
copy.put(e.key, deepCopy(e.value))
}
}
copy
}

static deepCopy(Set original) {
Set copy = []
for(def e : original)
copy << deepCopy(e)
copy
}

static deepCopy(List original) {
List copy = []
for(def e : original)
copy << deepCopy(e)
copy
}

/*
* In fact not a copy, but a catch all for everything not matching
* with the other signatures
*/
static deepCopy(def original) {
original
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the types there is a description available on that site: http://groovy-lang.org/json.html.

We have to deal with:

  • java.lang.String
  • java.lang.BigDecimal, java.lang.Integer
  • java.lang.Boolean, maybe the corresponding primitive
  • java.util.Date
  • and of course java.util.LinkedHashMap and java.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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@benhei benhei May 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
}
31 changes: 31 additions & 0 deletions test/groovy/com/sap/piper/MapUtilsTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,35 @@ class MapUtilsTest {
MapUtils.traverse(m, { s -> (s.startsWith('x')) ? "replaced" : s})
assert m == [a: 'replaced', m: [b: 'replaced', c: 'otherString']]
}

@Test
void testDeepCopy() {

List l = ['a', 'b', 'c']

def original = [
list: l,
set: (Set)['1', '2'],
nextLevel: [
list: ['x', 'y'],
duplicate: l,
set: (Set)[9, 8, 7]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would [9, 8, 7] as Set work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this would also work here. So we have several options to get a Set here rather than a list.

]
]

def copy = MapUtils.deepCopy(original)

assert ! copy.is(original)
assert ! copy.list.is(original.list)
assert ! copy.set.is(original.set)
assert ! copy.nextLevel.list.is(original.nextLevel.list)
assert ! copy.nextLevel.set.is(original.nextLevel.set)
assert ! copy.nextLevel.duplicate.is(original.nextLevel.duplicate)

// Within the original identical list is used twice, but the
// assuption is that there are different lists in the copy.
assert ! copy.nextLevel.duplicate.is(copy.list)

assert copy == original
}
}