Page MenuHomeSoftware Heritage
Differential D6281

converters: Recompute hashes and check they match the originals
ClosedPublic
Actions

Authored by vlorentz on Sep 16 2021, 2:03 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T399: (Re-)Compute data checksums before insertion
T3135: Improve integrity of ingested content
Commits
rDLDG6d7a998b1093: converters: Recompute hashes and check they match the originals
Summary

This makes sure we don't write corrupt objects to the storage,
like the examples in T75.

Diff Detail

Repository
rDLDG Git loader
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23678
Build 36961: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 36960: arc lint + arc unit

Event Timeline

vlorentz created this revision.Sep 16 2021, 2:03 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 16 2021, 2:03 PM
swh-public-ci added a comment.Sep 16 2021, 2:04 PM

Build has FAILED

Patch application report for D6281 (id=22747)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..3558c5f
Fast-forward
 requirements.txt                        |   1 +
 swh/loader/git/converters.py            |  74 ++++++++++++++++----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 3 files changed, 164 insertions(+), 26 deletions(-)
Changes applied before test
commit 3558c5f5aaf1ba9f36e1544cb1719ebeb848aada
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 16 2021, 2:04 PM
Harbormaster failed remote builds in B23678: Diff 22747!
vlorentz mentioned this in D6282: HashableObject: Add type annotation for 'id' attribute.Sep 16 2021, 2:09 PM
vlorentz requested review of this revision.Sep 16 2021, 2:09 PM
vlorentz added a parent revision: D6282: HashableObject: Add type annotation for 'id' attribute.
vlorentz edited the test plan for this revision. (Show Details)
vlorentz updated this revision to Diff 22749.Sep 16 2021, 2:12 PM

fix typeann

swh-public-ci added a comment.Sep 16 2021, 2:13 PM

Build has FAILED

Patch application report for D6281 (id=22749)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..4f17deb
Fast-forward
 requirements.txt                        |   1 +
 swh/loader/git/converters.py            |  74 ++++++++++++++++----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 3 files changed, 164 insertions(+), 26 deletions(-)
Changes applied before test
commit 4f17deb0037c7108c3e56344331014882b60475a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

Harbormaster failed remote builds in B23681: Diff 22749!Sep 16 2021, 2:13 PM
swh-public-ci added a comment.Sep 16 2021, 2:28 PM

Build has FAILED

Patch application report for D6281 (id=22749)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..4f17deb
Fast-forward
 requirements.txt                        |   1 +
 swh/loader/git/converters.py            |  74 ++++++++++++++++----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 3 files changed, 164 insertions(+), 26 deletions(-)
Changes applied before test
commit 4f17deb0037c7108c3e56344331014882b60475a
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

vlorentz updated this revision to Diff 22752.Sep 16 2021, 2:29 PM

bump swh-model version

swh-public-ci added a comment.Sep 16 2021, 2:31 PM

Build is green

Patch application report for D6281 (id=22752)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..a34cbe5
Fast-forward
 requirements-swh.txt                    |   2 +-
 requirements.txt                        |   1 +
 swh/loader/git/converters.py            |  74 ++++++++++++++++----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 4 files changed, 165 insertions(+), 27 deletions(-)
Changes applied before test
commit a34cbe5196eab14b98d311ae4eda3ab590c18675
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

Harbormaster completed remote builds in B23684: Diff 22752.Sep 16 2021, 2:31 PM
vlorentz edited the test plan for this revision. (Show Details)Sep 16 2021, 2:36 PM
ardumont accepted this revision.Sep 16 2021, 5:09 PM
ardumont added a subscriber: ardumont.

lgtm, a couple of questions/remarks inline.

swh/loader/git/converters.py
44

I have a hard time reading this ^ chunk (line 37 to 45).

Does that mean something like, all functions decorated with check_ids must respect the signature (self, obj: ShaFile, log=None) -> _THashable when called upon?

(That plus what the decorator actually does, checking for mismatched checksums).

Now that i said it, it actually makes sense to me...

swh/loader/git/tests/test_converters.py
116–120

why not ^ ?

or even simpler:

def sha(self):
    return lambda: self._sha

?

This revision is now accepted and ready to land.Sep 16 2021, 5:09 PM
vlorentz added inline comments.Sep 16 2021, 5:22 PM
swh/loader/git/tests/test_converters.py
116–120

because that's not what dulwich expects.

vlorentz updated this revision to Diff 22763.Sep 16 2021, 5:23 PM

rewrite without the very readable annotation

vlorentz updated this revision to Diff 22764.Sep 16 2021, 5:23 PM

remove dep on typing-extensions

vlorentz requested review of this revision.Sep 16 2021, 5:24 PM
swh-public-ci added a comment.Sep 16 2021, 5:25 PM

Build is green

Patch application report for D6281 (id=22763)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..61e2d2c
Fast-forward
 requirements-swh.txt                    |   2 +-
 requirements.txt                        |   1 +
 swh/loader/git/converters.py            |  64 +++++++++++++-----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 4 files changed, 152 insertions(+), 30 deletions(-)
Changes applied before test
commit 61e2d2c1f1ce043e6bed9b9b22a4b065150835b4
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

Harbormaster completed remote builds in B23693: Diff 22763.Sep 16 2021, 5:25 PM
swh-public-ci added a comment.Sep 16 2021, 5:25 PM

Build is green

Patch application report for D6281 (id=22764)

Could not rebase; Attempt merge onto 85eb54015c...

Updating 85eb540..6d7a998
Fast-forward
 requirements-swh.txt                    |   2 +-
 swh/loader/git/converters.py            |  64 +++++++++++++-----
 swh/loader/git/tests/test_converters.py | 115 ++++++++++++++++++++++++++++----
 3 files changed, 151 insertions(+), 30 deletions(-)
Changes applied before test
commit 6d7a998b1093aaba83d0660188fbe48c0ed5b269
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 14:01:20 2021 +0200

    converters: Recompute hashes and check they match the originals
    
    This makes sure we don't write corrupt objects to the storage,
    like the examples in T75.

commit f413e171b6e0ffd703613089bdfbee24dc048bda
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Sep 16 13:39:59 2021 +0200

    converters: Add typing

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

Harbormaster completed remote builds in B23694: Diff 22764.Sep 16 2021, 5:25 PM
douardda added a subscriber: douardda.Sep 16 2021, 5:26 PM
douardda added inline comments.
swh/loader/git/converters.py
36

why not make check_id return the object?
so you can write:

return check_id(Directory(id=tree.sha().digest(), entries=tuple(entries),))
vlorentz added inline comments.Sep 16 2021, 5:35 PM
swh/loader/git/converters.py
36

because it's weird and forces line wrapping when constructing Revision

ardumont added inline comments.Sep 16 2021, 5:41 PM
swh/loader/git/tests/test_converters.py
116–120

ah!

ardumont accepted this revision.Sep 16 2021, 6:06 PM
This revision is now accepted and ready to land.Sep 16 2021, 6:06 PM
Closed by commit rDLDG6d7a998b1093: converters: Recompute hashes and check they match the originals (authored by vlorentz). · Explain WhySep 17 2021, 12:27 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDG6d7a998b1093: converters: Recompute hashes and check they match the originals.
vlorentz added a task: T3135: Improve integrity of ingested content.Sep 20 2021, 11:05 AM
vlorentz added a task: T399: (Re-)Compute data checksums before insertion.Nov 26 2021, 3:52 PM

Revision Contents
Changeset List

PathSize
1 line
swh/
loader/
git/
46 lines
tests/
115 lines

Diff 22747

View Options

requirements.txt

Loading...
View Options

swh/loader/git/converters.py

Loading...
View Options

swh/loader/git/tests/test_converters.py

Loading...
About · Developer information · Legal