Page MenuHomeSoftware Heritage
Differential D6231

Rework `ProvenanceStorageInterface` to have a single add method per entity
ClosedPublic
Actions

Authored by aeviso on Sep 9 2021, 4:45 PM.

Details

Reviewers
jayeshv
Group Reviewers
Reviewers
Commits
rDPROV657226a87bbe: Rework `ProvenanceStorageInterface` to have a single add method per entity
Summary

All entities now have a single <entity>_add method in the interface, with optional
associated date if applicable. A new location_add is also added to the interface.
Extend all backend implementations to support the new methods. Add pre-condition to
relation_add (relying on the new methods) which simplifies its different implementations.

Diff Detail

Repository
rDPROV Provenance database
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aeviso created this revision.Sep 9 2021, 4:45 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptSep 9 2021, 4:45 PM
swh-public-ci added a comment.Sep 9 2021, 4:48 PM

Build has FAILED

Patch application report for D6231 (id=22543)

Rebasing onto 4c2b0907a9...

Current branch diff-target is up to date.
Changes applied before test
commit a4a41ce4ac8b21f170a509ada464cfedeb4f9d05
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Thu Sep 9 16:40:33 2021 +0200

    Extend `ProvenanceStorageInterface` with new methods
    
    Add new methods `entity_add` and `location_add` to the interface.
    Extend all backend implementations to support the new methods.
    Add pre-condition to `relation_add` (relying on the new methods)
    which simplifies its different implementations.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 9 2021, 4:48 PM
Harbormaster failed remote builds in B23488: Diff 22543!
aeviso updated this revision to Diff 22544.Sep 9 2021, 5:00 PM

Fix rebasing issue

swh-public-ci added a comment.Sep 9 2021, 5:04 PM

Build is green

Patch application report for D6231 (id=22544)

Rebasing onto 4c2b0907a9...

Current branch diff-target is up to date.
Changes applied before test
commit e04501ce8fad122bfb22239041b636dbb37b7e85
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Thu Sep 9 16:40:33 2021 +0200

    Extend `ProvenanceStorageInterface` with new methods
    
    Add new methods `entity_add` and `location_add` to the interface.
    Extend all backend implementations to support the new methods.
    Add pre-condition to `relation_add` (relying on the new methods)
    which simplifies its different implementations.

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

Harbormaster completed remote builds in B23489: Diff 22544.Sep 9 2021, 5:04 PM
aeviso requested review of this revision.Sep 9 2021, 5:04 PM
aeviso updated this revision to Diff 22575.Sep 10 2021, 4:28 PM

rebase

aeviso retitled this revision from Extend `ProvenanceStorageInterface` with new methods to Rework `ProvenanceStorageInterface` to have a single add method per entity.Sep 10 2021, 4:31 PM
aeviso edited the summary of this revision. (Show Details)
swh-public-ci added a comment.Sep 10 2021, 4:32 PM

Build is green

Patch application report for D6231 (id=22575)

Rebasing onto 4c2b0907a9...

Current branch diff-target is up to date.
Changes applied before test
commit 03d156bcb7fa12b544ec385bc50cad57f426a661
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Thu Sep 9 16:40:33 2021 +0200

    Rework `ProvenanceStorageInterface` to have a single add method per entity
    
    All entities now have a single `<entity>_add` method in the interface, with optional
    associated date if applicable. A new `location_add` is also added to the interface.
    Extend all backend implementations to support the new methods. Add pre-condition to
    `relation_add` (relying on the new methods) which simplifies its different implementations.

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

Harbormaster completed remote builds in B23509: Diff 22575.Sep 10 2021, 4:32 PM
aeviso added a child revision: D6243: Change relation methods in `ProvenanceStorageInterface` to take/return denormalized data.Sep 13 2021, 10:41 AM
jayeshv requested changes to this revision.Sep 13 2021, 12:17 PM
jayeshv added a subscriber: jayeshv.
jayeshv added inline comments.
swh/provenance/mongo/backend.py
34

This logic can be slightly improved with upsert=True, but that improvement will be implemented in the on-going mongo refactoring. So, it is fine like this here.
This is true for all entirty_add methods.
https://docs.mongodb.com/manual/reference/method/db.collection.update/#std-label-upsert-behavior

swh/provenance/postgresql/provenance.py
63

Since we are actually adding a new object, set_date name looks a bit strange here. Should we rename the entity_set_date to entity_add?
Just a suggestion, not a blocker.

111

Catching a bare exception is generally considered as a bad idea. If you don't know the exact exception, I suggest using the base exception from psycopg2.

136

bare exception. An easier alternative is to add a method like 'execute query and handle these exceptions there'

This revision now requires changes to proceed.Sep 13 2021, 12:17 PM
aeviso marked 4 inline comments as done.Sep 13 2021, 12:24 PM
aeviso added inline comments.
swh/provenance/mongo/backend.py
34

Ok, but its not the aim of this refactoring.

swh/provenance/postgresql/provenance.py
63

This internal methods handles the query for inserting entities that only have an (optional) associated date (ie. content and directory).Hence its name.

111

I don't fully agree, we want to guaranty that in production the storage don't tiers down a client. If anything happens a log will be saved. This try/catch logic needs to be abstracted though, since it is used several time in this class. Anyway, it's not in the scope of this refactoring.

136

Ack

jayeshv accepted this revision.Sep 13 2021, 1:07 PM
This revision is now accepted and ready to land.Sep 13 2021, 1:07 PM
aeviso updated this revision to Diff 22597.Sep 13 2021, 1:57 PM
aeviso marked 4 inline comments as done.

rebase

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

Build is green

Patch application report for D6231 (id=22597)

Rebasing onto 4c2b0907a9...

Current branch diff-target is up to date.
Changes applied before test
commit 657226a87bbe211e9e963a701a960aff58636e08
Author: Andres Ezequiel Viso <aeviso@softwareheritage.org>
Date:   Thu Sep 9 16:40:33 2021 +0200

    Rework `ProvenanceStorageInterface` to have a single add method per entity
    
    All entities now have a single `<entity>_add` method in the interface, with optional
    associated date if applicable. A new `location_add` is also added to the interface.
    Extend all backend implementations to support the new methods. Add pre-condition to
    `relation_add` (relying on the new methods) which simplifies its different implementations.

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

Harbormaster completed remote builds in B23531: Diff 22597.Sep 13 2021, 2:01 PM
Closed by commit rDPROV657226a87bbe: Rework `ProvenanceStorageInterface` to have a single add method per entity (authored by aeviso). · Explain WhySep 13 2021, 2:11 PM
This revision was automatically updated to reflect the committed changes.
aeviso added a commit: rDPROV657226a87bbe: Rework `ProvenanceStorageInterface` to have a single add method per entity.

Revision Contents
Changeset List

PathSize
swh/
provenance/
77 lines
mongo/
265 lines
postgresql/
107 lines
100 lines
sql/
10 lines
tests/
459 lines
2 lines

Diff 22600

View Options

swh/provenance/interface.py

Loading...
View Options

swh/provenance/mongo/backend.py

Loading...
View Options

swh/provenance/postgresql/provenance.py

Loading...
View Options

swh/provenance/provenance.py

Loading...
View Options

swh/provenance/sql/40-funcs.sql

Loading...
View Options

swh/provenance/tests/test_provenance_storage.py

Loading...
View Options

swh/provenance/tests/test_revision_content_layer.py

Loading...
About · Developer information · Legal