Page MenuHomeSoftware Heritage

deposit.loader: Fix committer date appropriately
ClosedPublic

Authored by ardumont on Apr 21 2020, 1:42 PM.

Details

Summary

Thix fixes the loader deposit to use the correct field date sent from the
deposit read api.

This adds another test scenario where committer date and author date are
different. This is to further check the dates are set correctly in the
revision. In the previous test data, the committer date and author were the
same so no hash change were detected.

Related to T2368

Test Plan

tox

Diff Detail

Repository
rDLDBASE Generic VCS/Package Loader
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build is green

Patch application report for D3039 (id=10795)

Rebasing onto 0b9eab71f4...

Current branch diff-target is up to date.
Changes applied before test
commit b82c6e61574966b69fefca6900ecdf38820d0c0d
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 21 10:54:57 2020 +0200

    deposit.loader: Fix committer date appropriately
    
    This adds another test scenario where committer and date are different. This is
    to further check the dates are set correctly in the revision. In the previous
    test data, the committer date and author were the same so no hash change were
    detected.
    
    Related to T2368

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

What is the semantics of committer_date for the deposit, and can you correct the existing DB records to give them the correct value?

What is the semantics of committer_date for the deposit, and can you correct the existing DB records to give them the correct value?

Decoding the test data and squinting a bit, it's date = codemeta:dateCreated, and committer_date = codemeta:datePublished.

It'd be nice for the tests to be explicit about this, rather than only check that the output revision id is what we expect.

douardda added a subscriber: douardda.

Fine for me. I agree the semantic of this date in the context of a deposit could be documented somehow (a simple comment may be enough).

This revision is now accepted and ready to land.Apr 21 2020, 3:35 PM

What is the semantics of committer_date for the deposit

if transmitted by the deposit client, the committer date should be the date of the publication.

It's not the loader to decide though, it's done deposit side.

The main description declared in the diff description mentions this...

can you correct the existing DB records to give them the correct value?

No, we do not modifies back previous data.

Decoding at the test data and squinting a bit, it's date = codemeta:dateCreated, and committer_date = codemeta:datePublished.

yes, except that it's already done by the api/1/private/meta/.

So here the code just take the revision['committer_date'], thus why the assertion is done with this.

Decoding at the test data and squinting a bit, it's date = codemeta:dateCreated, and committer_date = codemeta:datePublished.

yes, except that it's already done by the api/1/private/meta/.

So here the code just take the revision['committer_date'], thus why the assertion is done with this.

It's done deposit side at [1]

[1] https://forge.softwareheritage.org/source/swh-deposit/browse/master/swh/deposit/api/private/deposit_read.py$0-135

Which we already discussed with @douardda that it'd need to be evolved.
But i'm not doing that right now.

Decoding at the test data and squinting a bit, it's date = codemeta:dateCreated, and committer_date = codemeta:datePublished.

yes, except that it's already done by the api/1/private/meta/.

So here the code just take the revision['committer_date'], thus why the assertion is done with this.

It's done deposit side at [1]

[1] https://forge.softwareheritage.org/source/swh-deposit/browse/master/swh/deposit/api/private/deposit_read.py$0-135

Which we already discussed with @douardda that it'd need to be evolved.
But i'm not doing that right now.

Right, I guess that's fine for now, just figured I'd mention it while I looked at it.

Right, I guess that's fine for now, just figured I'd mention it while I looked at it.

sure thing ;)

Add a note about date fields provenance

Build is green

Patch application report for D3039 (id=10803)

Rebasing onto 0b9eab71f4...

Current branch diff-target is up to date.
Changes applied before test
commit 6bc9a1c63f3f980a6ce1e1e79ebeb5b6d609024c
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Tue Apr 21 10:54:57 2020 +0200

    qdeposit.loader: Fix committer date appropriately
    
    This adds another test scenario where committer and date are different. This is
    to further check the dates are set correctly in the revision. In the previous
    test data, the committer date and author were the same so no hash change were
    detected.
    
    This also adds a note about the date provenance. The ones used to fill in the
    revision date fields.
    
    Related to T2368

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