Including namespace of Coordinate in SW360Component name #380

bs-ondem · 2019-12-16T08:38:02Z

Fixes #371
Creating the name of the SW360Component with the namespace and name of
the Coordinate fact of the artifact using a '/' as the delimiter
identical to the PURLs.

The SW360ComponentAdapterUtils::createComponentName method tries to
extract the component name from the ArtifactCoordinate fact first. If
it doesn't exist, it extracts it from the ArtifactFilename fact.
Otherwise it returns an empty string.

Request Reviewer

@blaumeiser-at-bosch @neubs-bsi

Type of Change

improvements

How Has This Been Tested?

A new parameterised test added for SW360ComponentAdapterUtils class.

Checklist

Must:

All related issues are referenced in commit messages

Optional:

I have provided tests for the changes (if there are changes that need additional tests)

blaumeiser-at-bosch · 2019-12-16T08:47:12Z

...es/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ComponentAdapterUtils.java

+                .orElse(artifact.askFor(ArtifactFilename.class)
+                        .flatMap(ArtifactFilename::getBestFilenameEntryGuess)
+                        .map(ArtifactFilenameEntry::getFilename)
+                        .orElse(""));


We could use something like a UUID for the name, an empty name is perhaps not the right option

neubs-bsi · 2019-12-16T08:47:54Z

...es/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ComponentAdapterUtils.java

+                .orElse(artifact.askFor(ArtifactFilename.class)
+                        .flatMap(ArtifactFilename::getBestFilenameEntryGuess)
+                        .map(ArtifactFilenameEntry::getFilename)
+                        .orElse(""));


I am not really happy with an empty string. This is the name that will stand in SW360 and there shouldn't be multiple ones with empty string. Additionally by this name the release with be searched, making wrong assignments to releases with the same version and also an empty string name very likely.

I think, if there is no name here, we maybe shouldn't have this in SW360 at all.
@blaumeiser-at-bosch what do you think?

That was my though as well, if we do not have enough meat to at least have a simple name, we should inform the user that there is something strange here, and we should not push arbitrary data to SW360. At least a filename should be there.

Yes the user should be informed, but not in the adapter utils class? The component client will fail in creating a component with an empty string and will log an error.

Do you mean when the request is made?
I feel like we should try to avoid making requests we know will fail to save resources.

We are trying to avoid it with the CoordinatesValidator

I totally agree that here is not the point of informing the user here. This is a helper who does something useful. The question is, whether no information at all should return an error aka an exception instead of an empty string?

Just for clarity, is that the flow you want?

if AritfactCoordinate then return <component name extracted from Coordinates> else if ArtifactFilename then return <component name extracted from ArtifactFilenameEntry> else throw new ExecutionException(..);

Yes, I think this makes sense. This exception can be caught and it is easier to distinguish between make the right thing and do something exceptional

neubs-bsi · 2019-12-16T15:26:20Z

Just as a question. Does this PR also coincidentally solve this issue: #348 ?
Since it is also related to the method your PR is changing

bs-ondem · 2019-12-16T16:38:02Z

@neubs-bsi I'm not sure, but I will have a look at it after improving my changes based on your comments.

blaumeiser-at-bosch · 2019-12-17T12:31:01Z

...es/sw360/src/main/java/org/eclipse/sw360/antenna/sw360/utils/SW360ComponentAdapterUtils.java

+                .orElseGet(() -> artifact.askFor(ArtifactFilename.class)
+                        .flatMap(ArtifactFilename::getBestFilenameEntryGuess)
+                        .map(ArtifactFilenameEntry::getFilename)
+                        .orElseThrow(() -> new ExecutionException("No suitable component name found.")));


Error message is misleading. Should be something like artifact does not have enough information to build a name + artifact.toString() or something like this, Enough information that allows us to understand what is going on, if a customer comes back to us with this problem!

blaumeiser-at-bosch · 2019-12-17T12:32:50Z

...sw360/src/main/java/org/eclipse/sw360/antenna/sw360/adapter/SW360ComponentClientAdapter.java

+            String componentName = SW360ComponentAdapterUtils.createComponentName(artifact);
+            return getComponentByName(componentName, header);
+        } catch (ExecutionException e) {
+            LOGGER.debug("No component found for {}. Reason: {}", artifact, e.getMessage());


Always log the exception, especially debug messages are meant for us to understand what is going on, if a user comes back to us, so don't be shy and print out whatever you think could help us in identifying the problem.

blaumeiser-at-bosch · 2019-12-17T12:34:34Z

...w360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/generators/SW360UpdaterImpl.java

                }
+                releases.add(sw360ReleaseFinal);
+            } catch(Exception e) {


Never catch a generic Exception, you also catch all RuntimeExceptions and the like, which should not be caught.

blaumeiser-at-bosch · 2019-12-17T12:40:11Z

...w360/src/main/java/org/eclipse/sw360/antenna/sw360/workflow/generators/SW360UpdaterImpl.java

                }
+                releases.add(sw360ReleaseFinal);
+            } catch(Exception e) {
+                LOGGER.warn("Release for {} will not be created in SW360 due to missing facts. Reason: {}",


Also log the exception as debug entry! Actually I think this should even be logged as an error because the user might ignore warnings, but with this issue here, he will never get an approval for the project, he has to do something and this is necessary to address with him

Actually, the log message should contain a hint about what needs to be done. Without the component in SW360, the compliance office will not work on the component. What can he do to solve this? Good question, isn't it, can he do something in the config file?

I have to admit, I miss the situation as of now, where this case can happen at all because we will detect either a coordinate because we get information from a package manager, or we detect a file so we have at least a filename. Is there any way, in which we can detect a dependency without any information? I have no idea how. Since we do not expect that, perhaps it is even feasible to break completely, so no caught exception here. If something happens, that cannot be, it is best to not react at all, so that the user is coming to us in order to solve the issue.

Yes, when a component is added via the antennaconf.xml this can happen.
Especially then it is important that the user adds enough information so this component can be used correctly.

THen we should break at reading in the config file if we cannot detect enough informationen for a coordinate.

Follow up issue for config.xml #384

Fixes eclipse-archived#371 Creating the name of the SW360Component with the namespace and name of the Coordinate fact of the artifact using a '/' as the delimiter identical to the PURLs. The SW360ComponentAdapterUtils::createComponentName method tries to extract the component name from the ArtifactCoordinate fact first. If it doesn't exist, it extracts it from the ArtifactFilename fact. Otherwise it will throw an ExecutionException. Signed-off-by: Onur Demirci <Onur.Demirci@Bosch-si.com>

bs-ondem force-pushed the bs-ondem/fix/better_component_name branch from 637c1b4 to c3d7f5f Compare December 16, 2019 08:39

blaumeiser-at-bosch suggested changes Dec 16, 2019

View reviewed changes

neubs-bsi reviewed Dec 16, 2019

View reviewed changes

bs-ondem force-pushed the bs-ondem/fix/better_component_name branch from debfda2 to 3b5aef2 Compare December 17, 2019 11:53

bs-ondem requested review from blaumeiser-at-bosch and neubs-bsi December 17, 2019 11:53

blaumeiser-at-bosch suggested changes Dec 17, 2019

View reviewed changes

bs-ondem requested a review from blaumeiser-at-bosch December 18, 2019 12:42

blaumeiser-at-bosch approved these changes Dec 18, 2019

View reviewed changes

bs-ondem force-pushed the bs-ondem/fix/better_component_name branch 2 times, most recently from 52388d1 to 5bd9d75 Compare December 19, 2019 09:30

bs-ondem force-pushed the bs-ondem/fix/better_component_name branch from 5bd9d75 to 46e3bdd Compare December 19, 2019 11:11

bs-ondem merged commit 6f9b9f3 into eclipse-archived:master Dec 19, 2019

blaumeiser-at-bosch deleted the bs-ondem/fix/better_component_name branch January 2, 2020 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Including namespace of Coordinate in SW360Component name #380

Including namespace of Coordinate in SW360Component name #380

bs-ondem commented Dec 16, 2019

blaumeiser-at-bosch Dec 16, 2019

neubs-bsi Dec 16, 2019

neubs-bsi Dec 16, 2019

blaumeiser-at-bosch Dec 16, 2019

bs-ondem Dec 16, 2019

neubs-bsi Dec 16, 2019

bs-ondem Dec 16, 2019

blaumeiser-at-bosch Dec 16, 2019

bs-ondem Dec 16, 2019

blaumeiser-at-bosch Dec 16, 2019

neubs-bsi commented Dec 16, 2019

bs-ondem commented Dec 16, 2019

blaumeiser-at-bosch Dec 17, 2019

blaumeiser-at-bosch Dec 17, 2019

blaumeiser-at-bosch Dec 17, 2019

blaumeiser-at-bosch Dec 17, 2019

neubs-bsi Dec 17, 2019

blaumeiser-at-bosch Dec 17, 2019

bs-ondem Dec 18, 2019

Including namespace of Coordinate in SW360Component name #380

Including namespace of Coordinate in SW360Component name #380

Conversation

bs-ondem commented Dec 16, 2019

Request Reviewer

Type of Change

How Has This Been Tested?

Checklist

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

neubs-bsi commented Dec 16, 2019

bs-ondem commented Dec 16, 2019

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