fix: prevent multiple, duplicate build-system entries

[jeremylong]

PR #1349, for some projects, ends up creating multiple duplicate build-system external references. The fix is to ensure we have not already added an external reference of type: build-system.

With the current implementation, I've seen the plugin produce records like:

"externalReferences": [
        {
          "url": "https://some.build.system.internal/job/88",
          "type": "build-system",
          "comment": "as declared via cyclonedx-webpack-plugin config \"rootComponentBuildSystem\""
        },
        {
          "url": "https://some.internal.vcs/org/repo",
          "type": "vcs",
          "comment": "as declared via cyclonedx-webpack-plugin config \"rootComponentVCS\""
        },
        {
          "url": "https://some.build.system.internal/job/88",
          "type": "build-system",
          "comment": "as declared via cyclonedx-webpack-plugin config \"rootComponentBuildSystem\""
        },
        {
          "url": "https://some.build.system.internal/job/88",
          "type": "build-system",
          "comment": "as declared via cyclonedx-webpack-plugin config \"rootComponentBuildSystem\""
        },
        {
          "url": "https://some.build.system.internal/job/88",
          "type": "build-system",
          "comment": "as declared via cyclonedx-webpack-plugin config \"rootComponentBuildSystem\""
        }
      ]

fixes #1356

[jkowalleck]

so you propose to add the setting only, if no build system was present?

have you thought about the alternatives?
I'd recommend creating an issue where we can discuss the situation, instead of dropping a out-of-nowhere PR.

Therefore, I'll mark this PR as "draft" fdor now

[jeremylong]

Sorry - I always assume code is easier to use as a discussion point. This PR is using the same check as is already being done for VCS so I didn't think there would be a concern.

[jkowalleck]

This PR is using the same check as is already being done for VCS so I didn't think there would be a concern.

exactly. and i fond this debatable.

• The docs do nowhere say that they would only apply if no such thing existed before.
• an alternative would be to override an existing thing.

[jeremylong]

Instead of overriding an existing value, the code should not add duplicate entries. See the proposal above.

[jkowalleck]

Instead of overriding an existing value, the code should not add duplicate entries. See the proposal above.

and exactly this thing is debatable. see #1355 (comment)

I mean, the original code came from you, now you've changed your mind about the implementation, or did no think it trough in the first place.
What if you changed you mind in a month, wanna PR another "bugfix"?
From a maintainers perspective, I was sceptical about this feature before, but now I am even more sceptical about it.
In #1344, my original concern was, that this BuildSystem-URL feature already existed, and was taking data from the package manifest. And you tried to get in an additional feature to set this value dynamically (which is an understandable request).
And now you realize that you have the data twice, in the package manifest and in the dynamic setup at the same time. I told you so.

I am tending to tell you, that you should manage your manifest properly, to solve your use case, instead of changing code.
How does this sound to you?

[jkowalleck]

Reminder for self: regardless whether the BuildSystem implementation becomes an "override" or a "conditional", this change would be a breaking change.
not a stopper, just a remark/justification

[jeremylong]

I think you are conflating the VCS and Build-System. The build-system only originates from the rootComponentBuildSystem configuration of the plugin. The VCS could come from the package.json or the rootComponentVCS configuration.

The only thing I didn't think through is that the call to addRootComponentExtRefs could occur more than once on the root component.

[jeremylong]

I'd prefer this not to be a breaking change - and just not add duplicate entries.

[jkowalleck]

I think you are conflating the VCS and Build-System. The build-system only originates from the rootComponentBuildSystem configuration of the plugin. The VCS could come from the package.json or the rootComponentVCS configuration.

The only thing I didn't think through is that the call to addRootComponentExtRefs could occur more than once on the root component.

you are right; i was wrong.
I need to thing this trough, again.

[jkowalleck]

could you provide a reproducible test, that causes multiple BuildSystem entries, and after fixing the bug there is just one?

[jeremylong]

@jkowalleck I've been trying to figure out how to easily create a reproducible test for the problem I've encountered. Unfortunately, the only thing I've been able to come up with will be complicated. It would require installing a ServiceNow CLI, configuring the CLI to work with a Now instance, etc.

If you are really going to require a reproducer it will likely take a LOT of work - when the fix is to just prevent the addition of the exact same build-system URL. I'd suggest merging my suggestion above and squash and merge the PR. However, I do understand wanting processes to be followed around reproducers to prevent regressions, etc.

Thoughts?

[jkowalleck]

Thoughts?

agree. Lets merge this in.

Tomorrow I'm on a long train journey, so I might have time to create a regression test for this case ;-)

[jkowalleck]

fix was released via https://github.com/CycloneDX/cyclonedx-webpack-plugin/releases/tag/v4.0.1

[jeremylong]

Thank you!