feat(model): Add Snippet additionalData #7022

nnobelis · 2023-05-22T05:50:16Z

Please have a look at the individual commit messages for the details.

model/src/main/kotlin/utils/Snippet.kt

fviernau · 2023-05-22T06:06:10Z

scanner/src/main/kotlin/scanners/fossid/FossId.kt

        internal val SERVER_URL_KEY = "serverurl"

        @JvmStatic
        internal val PROJECT_REVISION_LABEL = "projectVcsRevision"


commit-msg: Please explain why the added data is needed. E.g. what are the use cases?

codecov · 2023-05-22T06:07:48Z

Codecov Report

Patch coverage: 90.90% and project coverage change: +0.01 🎉

Comparison is base (f4f59dc) 64.28% compared to head (d00f658) 64.30%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7022      +/-   ##
============================================
+ Coverage     64.28%   64.30%   +0.01%     
  Complexity     1976     1976              
============================================
  Files           332      332              
  Lines         16734    16744      +10     
  Branches       2382     2383       +1     
============================================
+ Hits          10758    10767       +9     
  Misses         4937     4937              
- Partials       1039     1040       +1

Flag	Coverage Δ
funTest-docker	`58.97% <ø> (ø)`
test	`40.30% <90.90%> (+0.08%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
model/src/main/kotlin/utils/SortedSetConverters.kt	`0.00% <ø> (ø)`
...in/kotlin/provenance/NestedProvenanceScanResult.kt	`83.16% <ø> (ø)`
...ain/kotlin/scanners/scanoss/ScanOssResultParser.kt	`82.27% <ø> (ø)`
scanner/src/main/kotlin/scanners/fossid/FossId.kt	`75.53% <90.90%> (+0.35%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

model/src/main/kotlin/utils/Snippet.kt

scanner/src/test/kotlin/scanners/fossid/FossIdTest.kt

nnobelis · 2023-05-22T06:40:57Z

@sschuberth

Additionally, it would be interesting to know if SCANOSS has something similar, and if so, such an ID should probably become a first-class property.

Unfortunately id does not:

{
  "src/main/java/com/vdurmont/semver4j/RequirementForScanOSS.java":[
  {
    "id": "snippet",
    "lines": "1-710",
    "oss_lines": "1-710",
    "matched": "98%",
    "file_hash": "6ff2427335b985212c9b79dfa795799f",
    "source_hash": "bd4bff27f540f4f2c9de012acc4b48a3",
    "file_url": "https://osskb.org/api/file_contents/6ff2427335b985212c9b79dfa795799f",
    "purl": [
      "pkg:github/vdurmont/semver4j"
    ],
    "vendor": "vdurmont",
    "component": "semver4j",
    "version": "3.1.0",
    "latest": "3.1.0",
    "url": "https://github.com/vdurmont/semver4j",
    "status": "pending",
    "release_date": "2019-09-13",
    "file": "src/main/java/com/vdurmont/semver4j/Requirement.java",
    "url_hash": "b92cd7e4d588747588d1b5fe1f8c664c",
    "licenses": [
      {
        "name": "MIT",
        "patent_hints": "no",
        "copyleft": "no",
        "checklist_url": "https://www.osadl.org/fileadmin/checklists/unreflicenses/MIT.txt",
        "osadl_updated": "2023-03-26T02:11:00+00:00",
        "source": "component_declared",
        "url": "https://spdx.org/licenses/MIT.html"
      }
    ],
    "server": {
      "version": "5.2.5",
      "kb_version": {
        "monthly": "23.03",
        "daily": "23.03.19"
      }
    }
  }
]
}

id behaves a bit like FossID MatchType: it is either file or snippet. file would be FossID MatchType.FULL and snippet would be MatchType.PARTIAL. But then you see the difference mentioned above that ScanOSS does not consider a full match a snippet, while FossID does,
And no, there is no numeric identified in ScanOSS.

My concerns were only about the use of the custom data, but no use is introduced in this PR.

sschuberth · 2023-05-22T12:45:05Z

model/src/main/kotlin/utils/Snippet.kt

+     * be made available in the scan Summary.
+     */
+    @JsonInclude(JsonInclude.Include.NON_EMPTY)
+    val additionalData: Map<String, String> = emptyMap()


It just occurred to me that we already have additionalData in ScanResult. So do we really need this at this place?

I took inspiration of ScanResult.additionalData 😃

Yes we need it at the snippet level, to carry snippet specific data such as the ID.
How would you put this information in the additionalData of ScanResult ?
Knowing it's a Map<String, String> and you can't have structured data in it ?

Knowing it's a Map<String, String> and you can't have structured data in it ?

Well you could (e.g. by applying a naming convention), but on a second thought I agree it's not very feasible.

sschuberth

Not strictly related to this PR, but it also just occurred to me that the SnippetFinding class is located in the utils package. Unless there's a good reason for that, I'd prefer it to go along the LicenseFinding and CopyrightFinding classes in the root model package. Could you mind adding a preceding commit to this PR that addressed that?

sschuberth · 2023-05-22T13:05:31Z

model/src/main/kotlin/utils/Snippet.kt

+
+    /**
+     * A map for scanner specific snippet data that cannot be mapped into any generalized property, but still needs to
+     * be made available in the scan Summary.


This should say "scan summary" or "[ScanSummary]".

sschuberth · 2023-05-22T14:04:24Z

model/src/main/kotlin/SnippetFinding.kt

+package org.ossreviewtoolkit.model

-import org.ossreviewtoolkit.model.TextLocation
+import org.ossreviewtoolkit.model.utils.Snippet


Argh, sorry, IMO Snippet should be moved as well, as it's not a utility to work on model classes, but a model class itself.

mnonnenmacher · 2023-05-22T20:16:55Z

model/src/main/kotlin/Snippet.kt

 */

-package org.ossreviewtoolkit.model.utils
+package org.ossreviewtoolkit.model


Commit message nit: The breaking api change should be indicated by an exclamation mark: "fix(model)!: ..."

This allows to align with `LicenseFinding` and `CopyrightFinding`. Signed-off-by: Nicolas Nobelis <[email protected]>

Some scanner-specific snippet information is not captured by the current snippet model. For instance, FossId delivers an id for each snippet. This commit adds a new property `additionalData` in the `Snippet` class to store this information. Signed-off-by: Nicolas Nobelis <[email protected]>

This information will be accessed in a future commit to fill a FossID snippet report which will be added to ORT. Additionally, the `id` will be used to fetch the matched lines of a snippet using the function `FossIdRestService.listMatchedLines`. Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis requested a review from a team as a code owner May 22, 2023 05:50

fviernau previously requested changes May 22, 2023

View reviewed changes

sschuberth requested changes May 22, 2023

View reviewed changes

model/src/main/kotlin/utils/Snippet.kt Show resolved Hide resolved

model/src/main/kotlin/utils/Snippet.kt Show resolved Hide resolved

scanner/src/test/kotlin/scanners/fossid/FossIdTest.kt Outdated Show resolved Hide resolved

nnobelis force-pushed the nnobelis/snippet_additional_data branch 2 times, most recently from 7b1d764 to 14ccf85 Compare May 22, 2023 06:31

nnobelis requested review from fviernau and sschuberth May 22, 2023 06:41

This was referenced May 22, 2023

FossID API for matched line needs to be improved #7028

Open

Provide in ORT a (FossID) snippet report #7030

Closed

sschuberth requested changes May 22, 2023

View reviewed changes

nnobelis force-pushed the nnobelis/snippet_additional_data branch from 14ccf85 to 3aa7ce6 Compare May 22, 2023 13:43

nnobelis requested a review from sschuberth May 22, 2023 13:44

sschuberth reviewed May 22, 2023

View reviewed changes

nnobelis force-pushed the nnobelis/snippet_additional_data branch from 3aa7ce6 to 52952c1 Compare May 22, 2023 16:07

nnobelis requested a review from sschuberth May 22, 2023 16:08

sschuberth previously approved these changes May 22, 2023

View reviewed changes

nnobelis dismissed sschuberth’s stale review via 22c1541 May 22, 2023 20:14

nnobelis force-pushed the nnobelis/snippet_additional_data branch from 52952c1 to 22c1541 Compare May 22, 2023 20:14

nnobelis requested a review from sschuberth May 22, 2023 20:15

mnonnenmacher reviewed May 22, 2023

View reviewed changes

mnonnenmacher added the breaking API change label May 22, 2023

nnobelis added 3 commits May 23, 2023 07:52

fix(model)!: Move SnippetFinding and Snippet to the model package

2676fb9

This allows to align with `LicenseFinding` and `CopyrightFinding`. Signed-off-by: Nicolas Nobelis <[email protected]>

nnobelis force-pushed the nnobelis/snippet_additional_data branch from 22c1541 to d00f658 Compare May 23, 2023 05:52

nnobelis requested a review from mnonnenmacher May 23, 2023 05:52

sschuberth approved these changes May 23, 2023

View reviewed changes

sschuberth enabled auto-merge (rebase) May 23, 2023 06:36

fviernau approved these changes May 23, 2023

View reviewed changes

sschuberth merged commit f861bb0 into oss-review-toolkit:main May 23, 2023

nnobelis deleted the nnobelis/snippet_additional_data branch May 24, 2023 05:56

sschuberth added the release notes label May 25, 2023

feat(model): Add Snippet additionalData #7022

feat(model): Add Snippet additionalData #7022

Uh oh!

Conversation

nnobelis commented May 22, 2023

Uh oh!

Uh oh!

fviernau May 22, 2023

Choose a reason for hiding this comment

Uh oh!

codecov bot commented May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nnobelis commented May 22, 2023

Uh oh!

sschuberth May 22, 2023

Choose a reason for hiding this comment

Uh oh!

nnobelis May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sschuberth May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sschuberth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sschuberth May 22, 2023

Choose a reason for hiding this comment

Uh oh!

sschuberth May 22, 2023

Choose a reason for hiding this comment

Uh oh!

mnonnenmacher May 22, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 22, 2023 •

edited

Loading

nnobelis May 22, 2023 •

edited

Loading

sschuberth May 22, 2023 •

edited

Loading

sschuberth left a comment •

edited

Loading