sw360(purl): replacing coordinates with purl in externalId #291

neubs-bsi · 2019-09-16T13:41:05Z

Added purl to sw360 external Ids and removed coordinates

neubs-bsi · 2019-09-16T13:41:32Z

@blaumeiser-at-bosch is that how you imagined it with your issue?

blaumeiser-at-bosch

In principle, what I planned, but see the only remark in the code

blaumeiser-at-bosch · 2019-09-16T15:38:43Z

modules/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ReleaseAdapterUtils.java

-        release.setCoordinates(getMapOfCoordinates(artifact));
-    }
+    private static void setPackageUrl(SW360Release sw360Release, Artifact artifact) {
+        Optional<PackageURL> packageURL = artifact.askForAll(ArtifactCoordinates.class)


Here, I think this findAny is not the adequate solution. In ArtifactUtils there is a default preference for coordinates which returns the coordinates according to priority, this seems to be the right thing to do.

If I remember right, we discussed that we want to store all known purls for the object. In the same way as the hash codes, i.e., with an _ suffix.

I had a call yesterday with @sschuberth. In our discussion we came to the conclusion that per artifact there should only be one purl. Did we get that wrong?

This is the principle discussion whether we have one data entry for a component with different established distributions or several. E.g. a repackaged maven component, which is provided in a p2 repository has two different coordinates with the same content.

One can argue that these are two components and therefore two instances with different purls, but effectively the metadata for the component are the same, as the sources from which the component has been build are the same.

So, my preferred solution is to have all known instances of a component managed in one data object. That means multiple purls pointing to the same data object

A similar case is in nuget, where we have the original source in nuget.org, but the same component, differently packaged from Microsoft, or with alternative package managers in other languages. In SW360, we should be able to manage multiple instances, whereas I think the way we did it in Antenna with multiple coordinates might be wrong because we look at a given set of instances, not the overall data of a component.

Adding @sschuberth into the discussion

the metadata for the component are the same, as the sources from which the component has been build are the same.

IMO we can and should use PURL to make exactly that distinction: To differentiate between different packagings of the same component or source code (e.g. .tar.gz vs. .zip), and to differentiate between different locations from which the same package (e.g. Maven Central vs. JCenter).

Linking some (semi-)related issue discussions here:

"separate type from provider": package-url/purl-spec#33
"Source of repackaged code": clearlydefined/curated-data#25

on the SW360 we do not distinguish between an artifact, its sources, or whether it was pulled from maven or from github. This means that in SW360 all the corresponding PURLs should be added to one release.

I think two artifacts with the same name and version are the same item in SW360.

@maxhbr what format would you suggest for the case: two purls from the same type?
(like started in this conversation: #291 (comment))

Going over the discussion between you @sschuberth and Jeff, it seems that I have a different understanding of what a purl references. From what I see, a purl references a unique id/coordinates for a component release unifying the different ways of how component coordinates are represented between technologies.

That means, that for one technology (e.g. maven) I would expect exactly one purl that references a version of a component. Having said that, a technology means basically a packaging type or a package manager.

There are several consequences:

A component release available in different package manager technologies has multiple purls, each of which uniquely identifies the component release.

I do not care about the provider or source of the component, if it is the content, it has the corresponding purl of the package manager technology, since both the instances are the same thing, i.e., built from the same source code.

If the content is different, then it is a different release, i.e., it has a different purl

Whether I trust that the release has the content the purl indicates is not a matter of purl but a matter of do I trust the source of this release.

The source of the release is something I would like to know but this is out of scope of the purl spec.

The purl type indentifies the technology with which I use the component in the build, aka as the package manager technology. I still can download the component from the source and build it on my own, but even then the normal way would be to build the corresponding library and use it as if I would download it directly from the internet: e.g., Build a Jar, deploy it to a maven repo manager (e.g. Nexus) and reference it with the repo manager as provider in my pom.xml.

If I do not use it as described in 6, e.g., if I download the source and directly build it together with my sources, this still is the same content, but from my point, I would need to use a different purl, e.g., with type github, because the way I use it is different. I do not use the component as Maven component anymore, but as bunch of sources, so different purl.

I think that covers my line of thinking.

Specifically for this PR and how to implement the solution, what is the implication of your line of thinking for this implementation?

We have multiple purls per release

The purls should be requested as proposed in the ArtifactUtils default sequence, not findAny or findFirst

Since I assume only one purl per technology, I would follow Max comment from below that the key in the external id should be the type used in the purl as redundant element

Purls are used in the Release object with PackageURL class not string and only when trnaslated into the JSON the string representation is used.

neubs-bsi · 2019-09-19T06:23:45Z

Waiting for #292 before continuing, since necessary for Enricher

blaumeiser-at-bosch · 2019-09-19T14:09:41Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

    @JsonIgnore
-    public Map<String, String> getCoordinates() {
+    public Set<String> getPurls() {


I think it would be more appropriate to use the PackageURL class instead of String to handle the object and only use the String representation when transfering it to SW360.

I think that the SW360Release should be as near as possible to the representation which will be transferred and therefore I think that it is perfectly fine to have them as string. Especially since they are stored in the externalId map.

blaumeiser-at-bosch · 2019-09-19T14:10:40Z

modules/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ReleaseAdapterUtils.java

+    private static void setPackageUrls(SW360Release sw360Release, Artifact artifact) {
+        Set<String> packageURLS = artifact.askForAll(ArtifactCoordinates.class).stream()
+                .map(ArtifactCoordinates::getPurl)
+                .map(PackageURL::toString)


I am not sure if toString is the right method, the "official" method for getting the String representation of the PURL is canonicalize().

While toString() indeed just calls canonicalize() I'd also prefer to be explicit and directly call canonicalize() here.

maxhbr · 2019-09-19T15:39:34Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

+        int i = 1;
+        for (String purl : purls) {
+            if (purl != null && !purl.isEmpty()) {
+                externalIds.put(PURL_PREFIX + i, purl);


why are the keys just the purls with enumeration, we have a better option. In my opinion it would be better to use the old prefix, something like:

'mvn': 'pkg:maven/org.apache.xmlgraphics/batik-anim@1.9.1'

This also allows for better searches via the SW360 rest interface.

Or maybe just use the scheme as key. (yes, there can be multiple purls of the same scheme for the same artifact. But I do not think that this is a problem here)

@blaumeiser-at-bosch what do you think?

Interesting point, disadvantage here is that I have to know which kind of component that is so searching is easier on the one hand side, because I do not have to search until the index ends, but it is also harder, because I have to find out which types of purls are there.

In general, we should use the same type as key, not anything else, so in the example case, "maven" instead of "mvn"

With scheme you mean the type, right?

Yes, I probably mean type.

maxhbr · 2019-09-19T15:43:26Z

...360/src/test/java/org/eclipse/sw360/antenna/sw360/workflow/processors/SW360EnricherTest.java

+                .withNamespace("test")
+                .withVersion("1.2.3")
+                .build()
+                .toString()));


same as https://github.com/eclipse/antenna/pull/291/files#r326196146, use canonicalize().

maxhbr · 2019-09-19T15:43:49Z

...360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/processors/SW360EnricherImpl.java

@@ -70,8 +70,8 @@ private void addClearingStateIfAvailable(Artifact artifact, SW360Release release
    }

    private void mapExternalIdsOnSW360Release(SW360Release sw360Release, Artifact artifact) {
-        mapCoordinates(sw360Release)
-                .forEach(artifact::addFact);
+        mapCoordinatesFromPurls(sw360Release).


usually we place the dot in the next line

blaumeiser-at-bosch

One small thing

blaumeiser-at-bosch · 2019-09-23T12:51:36Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

-    private static final String PURL_PREFIX = "purl_";
+    private static final Set<String> PURL_TYPE = Stream.of(
+            "bitbucket", "composer", "deb", "docker",
+            "gem", "generic", "github", "golang",


We have type p2 as well as custom type for p2 repository links

blaumeiser-at-bosch · 2019-09-24T08:28:08Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

        return externalIds.entrySet().stream()
-                .filter(e -> SW360CoordinateKeysToArtifactCoordinates.getValues().contains(e.getKey()))
+                .filter(e ->PURL_TYPE.stream()


Should be a space behind the arrow

blaumeiser-at-bosch · 2019-09-24T08:33:41Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

        return externalIds.entrySet().stream()
-                .filter(e -> SW360CoordinateKeysToArtifactCoordinates.getValues().contains(e.getKey()))
+                .filter(e ->PURL_TYPE.stream()
+                .anyMatch(purlType -> e.getKey().equals(purlType)))


Wrong indentation, that is part of the above filter, perhaps do not do it in a new line or do it as PURL_TYPE.contains(e.getKey())

blaumeiser-at-bosch · 2019-09-24T08:34:11Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

+    private void dropAllPurls() {
+        externalIds.keySet().stream()
+                .filter(e -> PURL_TYPE.stream()
+                        .anyMatch(e::startsWith))


Why startsWith? Isn't that redundant code to my comment above, should then be a method, I think

blaumeiser-at-bosch · 2019-09-24T08:35:10Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

+    public SW360Release setPurls(Map<String, String> purls) {
+        dropAllPurls();
+
+        purls.forEach(externalIds::put);


Why not putAll?

blaumeiser-at-bosch · 2019-09-24T08:35:36Z

modules/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ReleaseAdapterUtils.java

@@ -34,7 +37,7 @@ public static SW360Release prepareRelease(SW360Component component, Set<String>
        release.setComponentId(componentId);
        release.setMainLicenseIds(sw360LicenseIds);

-        SW360ReleaseAdapterUtils.setCoordinates(release, artifact);
+        SW360ReleaseAdapterUtils.setPackageUrls(release,artifact);


Space missing after ,

maxhbr · 2019-09-24T08:54:28Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

+    private static final Set<String> PURL_TYPE = Stream.of(
+            "bitbucket", "composer", "deb", "docker",
+            "gem", "generic", "github", "golang", "maven",
+             "npm", "nuget", "p2", "pypi", "rpm").collect(Collectors.toSet());


Suggested change

"npm", "nuget", "p2", "pypi", "rpm").collect(Collectors.toSet());

"npm", "nuget", "p2", "pypi", "rpm").collect(Collectors.toSet());

maxhbr · 2019-09-24T09:04:13Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

        return this;
    }

+    private void dropAllPurls() {


this drops the docker purl or other unsupported purls, without replacing it later? that should not happen.

I think PURLs, which we do not handle, should be preserved in the release.

Just for clarification: you suggest to overwrite the externalIds entries with the entries we get when setPurls is called? (which would happen automatically due to it being a map when using putAll)

I am unsure, maybe one wants do drop the p2 coordinates (and other supported types), if

they are explicitly not provided or

if they should be contained in the antenna artifact, if they were present previously.

maybe best solution is: just remove dropAllPurls

Good, point @maxhbr
Actually, the question is, what setPurls does, Is it resetting the object or simply adding the purls given. Currently it completely resets the object concerning purls. Could be the right thing to do. How often will the case happen, that there are already purls inside the object and we add more?

When I read this, I was also unsure, if the deletion should be done, but after rethinking it now, I would propose to not delete the purls and check the concrete use cases first, before we lose data.

Currently setPurls() is only called in Antenna when creating a new release from artifact data.
Hence, currently there would never be any purls already safed in the release. The mergeWith function of the release merges all externalIds overall.
Hence, currently the dropAllPurls function is redundant.

maxhbr

There are some unclear points, since there is not a 1:1 mapping between PURLs and ArtifactCoordinates

maxhbr · 2019-09-24T10:47:31Z

...sw360/src/test/java/org/eclipse/sw360/antenna/sw360/workflow/generator/SW360UpdaterTest.java

@@ -56,9 +56,8 @@ public void artifactIsMappedToSw360ReleaseCorrectly() {
            assertThat(false).isTrue();
        }

-        assertThat(release.getCoordinates()).containsKeys("mvn");
-        assertThat(release.getCoordinates()).hasSize(1);
-        assertThat(release.getCoordinates()).containsValue("org.group.id:artifactId(test1):1.2.3");


Why isn't there a replacement for this unit test?

I added the test in the updater test

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

...360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/processors/SW360EnricherImpl.java

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

blaumeiser-at-bosch · 2019-09-24T14:24:34Z

That is the problem with the key as the type of the corresponding coordinates. Perhaps the better solution is to check for the value of the Entry to be a purl, i.e., being a url with protocol "pkg"? This way we get every purl even those we do not know as of now?

If we just get every purl, we should first start supporting all features of purls and all types. Until then it is a good idea to restrict on keys which we control. This is a kind of namespacing and we should not try to understand everything every other tool does.

And there is also the problem that, if there are multiple maven purls, we end up in a nondeterministic state, where we do not know which purl is taken.

This open up a different problem, if we allow to have multiple purls with the same type, we need also a change in the storage model, because currently we can only hold one purl per type in SW360. So we cannot handle the case of multiple purls of the same type anyhow. This is a restriction for now. We would end up with _x again to handle this, but then we run into trouble within the Coordinates as well. This becomes too big for this change.

If we can identify all purls, we should return it, the SW360 module does not have to mind the model gaps from the internal Antenna model, if we can easily find out any purl stored, we can filter at another place, e.g., when we create the coordinates objects.

blaumeiser-at-bosch · 2019-09-24T15:22:59Z

On hold: @maxhbr implements a change in ArtifactCoordinates reducing the different types to one type which influences the way this PR needs to be implemented

neubs-bsi · 2019-10-02T12:18:03Z

@blaumeiser-at-bosch && @maxhbr

Hello,
after our planning today, I started cleaning up here.
The commit integrates most cleared points now.
Comments that are not addressed yet are the following:

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java

...360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/processors/SW360EnricherImpl.java

blaumeiser-at-bosch · 2019-10-03T12:53:07Z

@neubs-bsi Please rebase

outdated and partially moved to other PR

maxhbr

Looks good

Coordinates have been removed from the sw360 data model and purls have been added * removed coordinates to coordinates key mapping * added purls in sw360 release * represented by a map of string, string * keys are the antenna supported types of purl * test have been updated * documentation has been updated Signed-off-by: Stephanie Neubauer <Stephanie.Neubauer@bosch-si.com>

neubs-bsi requested a review from blaumeiser-at-bosch September 16, 2019 13:41

blaumeiser-at-bosch suggested changes Sep 16, 2019

View reviewed changes

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from 0abb9e5 to 62a055a Compare September 19, 2019 13:30

neubs-bsi changed the title ~~Neubs bsi/sw360/purl/#828~~ sw360(purl): replacing coordinates with purl in externalId Sep 19, 2019

neubs-bsi marked this pull request as ready for review September 19, 2019 13:33

neubs-bsi requested a review from blaumeiser-at-bosch September 19, 2019 13:33

blaumeiser-at-bosch suggested changes Sep 19, 2019

View reviewed changes

maxhbr suggested changes Sep 19, 2019

View reviewed changes

blaumeiser-at-bosch suggested changes Sep 23, 2019

View reviewed changes

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from e04db78 to eb7f7b7 Compare September 23, 2019 13:01

neubs-bsi requested review from maxhbr and blaumeiser-at-bosch September 24, 2019 07:16

blaumeiser-at-bosch suggested changes Sep 24, 2019

View reviewed changes

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from eb7f7b7 to c18c7ad Compare September 24, 2019 08:50

blaumeiser-at-bosch approved these changes Sep 24, 2019

View reviewed changes

maxhbr suggested changes Sep 24, 2019

View reviewed changes

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from c18c7ad to 778219e Compare September 24, 2019 09:36

maxhbr previously requested changes Sep 24, 2019

View reviewed changes

blaumeiser-at-bosch suggested changes Sep 24, 2019

View reviewed changes

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java Show resolved Hide resolved

neubs-bsi added the depends on something label Sep 25, 2019

neubs-bsi added wontfix This will not be worked on and removed depends on something wontfix This will not be worked on labels Oct 2, 2019

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch 2 times, most recently from d4c6585 to afcba26 Compare October 2, 2019 12:13

blaumeiser-at-bosch approved these changes Oct 3, 2019

View reviewed changes

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/rest/resource/releases/SW360Release.java Show resolved Hide resolved

...360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/processors/SW360EnricherImpl.java Show resolved Hide resolved

maxhbr added the needs rebase label Oct 3, 2019

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from afcba26 to c36fda7 Compare October 7, 2019 06:42

neubs-bsi removed the needs rebase label Oct 7, 2019

neubs-bsi requested a review from maxhbr October 7, 2019 06:44

maxhbr approved these changes Oct 7, 2019

View reviewed changes

neubs-bsi force-pushed the neubs-bsi/sw360/purl/#828 branch from c36fda7 to dc842ee Compare October 7, 2019 14:36

neubs-bsi merged commit f7dc570 into eclipse-archived:master Oct 8, 2019

neubs-bsi deleted the neubs-bsi/sw360/purl/#828 branch October 8, 2019 06:15

	"npm", "nuget", "p2", "pypi", "rpm").collect(Collectors.toSet());
	"npm", "nuget", "p2", "pypi", "rpm").collect(Collectors.toSet());

sw360(purl): replacing coordinates with purl in externalId #291

sw360(purl): replacing coordinates with purl in externalId #291

Conversation

neubs-bsi commented Sep 16, 2019

neubs-bsi commented Sep 16, 2019

blaumeiser-at-bosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaumeiser-at-bosch Sep 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neubs-bsi commented Sep 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaumeiser-at-bosch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxhbr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaumeiser-at-bosch commented Sep 24, 2019

blaumeiser-at-bosch commented Sep 24, 2019

neubs-bsi commented Oct 2, 2019

blaumeiser-at-bosch commented Oct 3, 2019

maxhbr left a comment

Choose a reason for hiding this comment

blaumeiser-at-bosch Sep 20, 2019 •

edited

Loading

neubs-bsi commented Sep 19, 2019 •

edited

Loading