Page MenuHomeSoftware Heritage
Differential D7469

npm: Include package version id in ExtID manifest
ClosedPublic
Actions

Authored by vlorentz on Mar 30 2022, 11:33 AM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Commits
rDLDBASE62270989a254: npm: Add all fields we use to the ExtID manifest
rDLDBASE290018832527: npm: Include package version id in ExtID manifest
Summary

In at least one occurence, replicate.npmjs.com shows the exact same
tarball for two different versions.

As we only used the shasum to identify a release (formerly revision)
when loading incrementally, this causes two ExtIDs to be created for the
same target, which causes a crash when re-loading incrementally
(formerly, this caused duplicate branches' revision to be replaced by
the other one).

For example, https://archive.softwareheritage.org/swh:1:snp:9fe3ba7515a9b567ab7f66e20d1f4ff7f9f5cf23;origin=https://www.npmjs.com/package/koreanbots
has a branch named "releases/3.0.0-alpha3" which points to a revision
with the right date and directory, but "3.0.0-beta" as commit message.
This may have been caused by both revisions having the same directory.

This seems to have confused the next load of the loader, whose result
can be seen at https://archive.softwareheritage.org/swh:1:snp:e9dca2ee671f8d12a0163fe4cacfc61eabf7d8fe;origin=https://www.npmjs.com/package/koreanbots
as it used the revision in the "releases/3.0.0-alpha3" branch as the
target of the "releases/3.0.0-beta" branch.

Diff Detail

Event Timeline

vlorentz created this revision.Mar 30 2022, 11:33 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 30 2022, 11:33 AM
swh-public-ci added a comment.Mar 30 2022, 11:34 AM

Build has FAILED

Patch application report for D7469 (id=27083)

Rebasing onto 4409e4b341...

Current branch diff-target is up to date.
Changes applied before test
commit 290018832527c854be90deb4f54e6220e1f19fd8
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Mar 30 10:47:25 2022 +0200

    npm: Include package version id in ExtID manifest
    
    In at least one occurence, replicate.npmjs.com shows the exact same
    tarball for two different versions.
    
    As we only used the shasum to identify a release (formerly revision)
    when loading incrementally, this causes two ExtIDs to be created for the
    same target, which causes a crash when re-loading incrementally
    (formerly, this caused duplicate branches' revision to be replaced by
    the other one).
    
    For example, https://archive.softwareheritage.org/swh:1:snp:9fe3ba7515a9b567ab7f66e20d1f4ff7f9f5cf23;origin=https://www.npmjs.com/package/koreanbots
    has a branch named "releases/3.0.0-alpha3" which points to a revision
    with the right date and directory, but "3.0.0-beta" as commit message.
    This may have been caused by both revisions having the same directory.
    
    This seems to have confused the next load of the loader, whose result
    can be seen at https://archive.softwareheritage.org/swh:1:snp:e9dca2ee671f8d12a0163fe4cacfc61eabf7d8fe;origin=https://www.npmjs.com/package/koreanbots
    as it used the revision in the "releases/3.0.0-alpha3" branch as the
    target of the "releases/3.0.0-beta" branch.

Link to build: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/703/
See console output for more information: https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/703/console

Harbormaster returned this revision to the author for changes because remote builds failed.Mar 30 2022, 11:35 AM
Harbormaster failed remote builds in B28000: Diff 27083!
anlambert added a subscriber: anlambert.Mar 30 2022, 11:57 AM

Looks good to me but we got hit again by the race condition between mypy and flake8 on Jeninks so the diff is still in draft state.

swh-public-ci added a comment.Mar 30 2022, 12:01 PM

Build is green

Patch application report for D7469 (id=27083)

Rebasing onto 4409e4b341...

Current branch diff-target is up to date.
Changes applied before test
commit 290018832527c854be90deb4f54e6220e1f19fd8
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Mar 30 10:47:25 2022 +0200

    npm: Include package version id in ExtID manifest
    
    In at least one occurence, replicate.npmjs.com shows the exact same
    tarball for two different versions.
    
    As we only used the shasum to identify a release (formerly revision)
    when loading incrementally, this causes two ExtIDs to be created for the
    same target, which causes a crash when re-loading incrementally
    (formerly, this caused duplicate branches' revision to be replaced by
    the other one).
    
    For example, https://archive.softwareheritage.org/swh:1:snp:9fe3ba7515a9b567ab7f66e20d1f4ff7f9f5cf23;origin=https://www.npmjs.com/package/koreanbots
    has a branch named "releases/3.0.0-alpha3" which points to a revision
    with the right date and directory, but "3.0.0-beta" as commit message.
    This may have been caused by both revisions having the same directory.
    
    This seems to have confused the next load of the loader, whose result
    can be seen at https://archive.softwareheritage.org/swh:1:snp:e9dca2ee671f8d12a0163fe4cacfc61eabf7d8fe;origin=https://www.npmjs.com/package/koreanbots
    as it used the revision in the "releases/3.0.0-alpha3" branch as the
    target of the "releases/3.0.0-beta" branch.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/704/ for more details.

swh-public-ci added a comment.Mar 30 2022, 12:04 PM

Build is green

Patch application report for D7469 (id=27083)

Rebasing onto 4409e4b341...

Current branch diff-target is up to date.
Changes applied before test
commit 290018832527c854be90deb4f54e6220e1f19fd8
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Mar 30 10:47:25 2022 +0200

    npm: Include package version id in ExtID manifest
    
    In at least one occurence, replicate.npmjs.com shows the exact same
    tarball for two different versions.
    
    As we only used the shasum to identify a release (formerly revision)
    when loading incrementally, this causes two ExtIDs to be created for the
    same target, which causes a crash when re-loading incrementally
    (formerly, this caused duplicate branches' revision to be replaced by
    the other one).
    
    For example, https://archive.softwareheritage.org/swh:1:snp:9fe3ba7515a9b567ab7f66e20d1f4ff7f9f5cf23;origin=https://www.npmjs.com/package/koreanbots
    has a branch named "releases/3.0.0-alpha3" which points to a revision
    with the right date and directory, but "3.0.0-beta" as commit message.
    This may have been caused by both revisions having the same directory.
    
    This seems to have confused the next load of the loader, whose result
    can be seen at https://archive.softwareheritage.org/swh:1:snp:e9dca2ee671f8d12a0163fe4cacfc61eabf7d8fe;origin=https://www.npmjs.com/package/koreanbots
    as it used the revision in the "releases/3.0.0-alpha3" branch as the
    target of the "releases/3.0.0-beta" branch.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/705/ for more details.

Harbormaster completed remote builds in B28000: Diff 27083.Mar 30 2022, 12:04 PM
vlorentz requested review of this revision.Mar 30 2022, 12:04 PM
olasd added a subscriber: olasd.Mar 30 2022, 5:34 PM

So, from what I understand, we're currently generating releases combining intrinsic data (the target directory hash) and extrinsic data provided by the package index (release name and message).

Shouldn't we just ensure that the content of the generated release only uses intrinsic metadata parsed from the contents of the tarball?

vlorentz planned changes to this revision.Apr 4 2022, 10:12 AM
vlorentz added a comment.Apr 5 2022, 10:00 AM
In D7469#195352, @olasd wrote:

So, from what I understand, we're currently generating releases combining intrinsic data (the target directory hash) and extrinsic data provided by the package index (release name and message).

Correct.

Shouldn't we just ensure that the content of the generated release only uses intrinsic metadata parsed from the contents of the tarball?

Then we would lose information provided by NPM, but not included in the tarball (package name and version).

The right fix is to make all fields used in the release part of the ExtID.

vlorentz updated this revision to Diff 27212.Apr 5 2022, 10:35 AM
  • npm: Add all fields we use to the ExtID manifest
swh-public-ci added a comment.Apr 5 2022, 10:38 AM

Build is green

Patch application report for D7469 (id=27212)

Rebasing onto 4409e4b341...

Current branch diff-target is up to date.
Changes applied before test
commit 9c158d615d4955411a1e221a821a33025604e5db
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Tue Apr 5 10:35:15 2022 +0200

    npm: Add all fields we use to the ExtID manifest
    
    Just in case they also vary despite using the same id.

commit 290018832527c854be90deb4f54e6220e1f19fd8
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Mar 30 10:47:25 2022 +0200

    npm: Include package version id in ExtID manifest
    
    In at least one occurence, replicate.npmjs.com shows the exact same
    tarball for two different versions.
    
    As we only used the shasum to identify a release (formerly revision)
    when loading incrementally, this causes two ExtIDs to be created for the
    same target, which causes a crash when re-loading incrementally
    (formerly, this caused duplicate branches' revision to be replaced by
    the other one).
    
    For example, https://archive.softwareheritage.org/swh:1:snp:9fe3ba7515a9b567ab7f66e20d1f4ff7f9f5cf23;origin=https://www.npmjs.com/package/koreanbots
    has a branch named "releases/3.0.0-alpha3" which points to a revision
    with the right date and directory, but "3.0.0-beta" as commit message.
    This may have been caused by both revisions having the same directory.
    
    This seems to have confused the next load of the loader, whose result
    can be seen at https://archive.softwareheritage.org/swh:1:snp:e9dca2ee671f8d12a0163fe4cacfc61eabf7d8fe;origin=https://www.npmjs.com/package/koreanbots
    as it used the revision in the "releases/3.0.0-alpha3" branch as the
    target of the "releases/3.0.0-beta" branch.

See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/707/ for more details.

Harbormaster completed remote builds in B28112: Diff 27212.Apr 5 2022, 10:38 AM
anlambert accepted this revision.Apr 5 2022, 3:53 PM
anlambert added inline comments.
swh/loader/package/npm/loader.py
56

looks like you can avoid string concatenation here

157

To avoid mixing up releases, we every field used to build the

This revision is now accepted and ready to land.Apr 5 2022, 3:53 PM
vlorentz updated this revision to Diff 27274.Apr 7 2022, 10:17 AM

typos

This revision was landed with ongoing or failed builds.Apr 7 2022, 10:17 AM
Closed by commit rDLDBASE290018832527: npm: Include package version id in ExtID manifest (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDBASE290018832527: npm: Include package version id in ExtID manifest.
vlorentz added a commit: rDLDBASE62270989a254: npm: Add all fields we use to the ExtID manifest.
swh-public-ci added a comment.Apr 7 2022, 10:20 AM

Build is green

Patch application report for D7469 (id=27274)

Rebasing onto 62270989a2...

First, rewinding head to replay your work on top of it...
Fast-forwarded diff-target to base-revision-709-D7469.
Changes applied before test



See https://jenkins.softwareheritage.org/job/DLDBASE/job/tests-on-diff/709/ for more details.
Harbormaster completed remote builds in B28203: Diff 27274.Apr 7 2022, 10:20 AM
Revision Contents
Changeset List
PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
package/
npm/
33 lines90%
Loading...
tests/
data/
https_registry.npmjs.org/
1 line--
https_replicate.npmjs.com/
141 lines--
96 lines98%
Loading...
Diff 27212
View Options
swh/loader/package/npm/loader.py
Loading...
View Options
swh/loader/package/npm/tests/data/https_registry.npmjs.org/org_-_org-0.0.3-beta.tgz
Loading...
View Options
swh/loader/package/npm/tests/data/https_replicate.npmjs.com/org_version_mismatch
Loading...
View Options
swh/loader/package/npm/tests/test_npm.py
Loading...
About · Developer information · Legal