Page MenuHomeSoftware Heritage
Differential D4118

storage: check uniqueness based on all relevant columns, not just id and indexer_configuration_id.
ClosedPublic
Actions

Authored by vlorentz on Oct 1 2020, 6:14 PM.

Details

Reviewers
anlambert
Group Reviewers
Reviewers
Commits
rDCIDX07821f0da7c4: storage: check uniqueness based on all relevant columns, not just id and…
Summary

ctags and fossology can legitimately add multiple objects with the same
id and indexer_configuration_id at the same time, but different values
for other columns.

Before this commit, it would immediately crash the 'check_id_duplicates'
checks.

As a side-effect, this commit also remove the 'add_merge' logic in the
in-memory storage, and stores it like proper rows like the pg does instead
of working around this broken deduplication.

As an other side-effect, removing this broken logic removes an
inconsistency between the in-mem and pg storage (see the added tests
named test_add_empty for Ctags and Fossology), which Fossology indexer
tests relied on, so this commit updates these tests as well.

Sorry, this is quite a large diff; I intended it as a small refactoring but
it uncovered a bunch of bugs that I needed to fix too

Diff Detail

Repository
rDCIDX Metadata indexer
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Oct 1 2020, 6:14 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptOct 1 2020, 6:14 PM
swh-public-ci added a comment.Oct 1 2020, 6:16 PM

Build has FAILED

Patch application report for D4118 (id=14513)

Rebasing onto cd650f102d...

Current branch diff-target is up to date.
Changes applied before test
commit 83a02daaba15a087f5f09095754ec734e86643b3
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 1 18:13:40 2020 +0200

    storage: check uniqueness based on all relevant columns, not just id and indexer_configuration_id.
    
    ctags and fossology can legitimately add multiple objects with the same
    id and indexer_configuration_id at the same time, but different values
    for other columns.
    
    Before this commit, it would immediately crash the 'check_id_duplicates'
    checks.
    
    As a side-effect, this commit also remove the 'add_merge' logic in the
    in-memory storage, and stores it like proper rows like the pg does instead
    of working around this broken deduplication.

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

Harbormaster failed remote builds in B15780: Diff 14513!Oct 1 2020, 6:16 PM
vlorentz planned changes to this revision.Oct 1 2020, 6:18 PM
vlorentz updated this revision to Diff 14550.Oct 2 2020, 12:18 PM

fix tests (see fourth paragraph of the description of this diff)

vlorentz edited the summary of this revision. (Show Details)Oct 2 2020, 12:18 PM
swh-public-ci added a comment.Oct 2 2020, 12:20 PM

Build is green

Patch application report for D4118 (id=14550)

Rebasing onto cd650f102d...

Current branch diff-target is up to date.
Changes applied before test
commit f006cd254963d2a38f2384fb3640a684a4c06cc1
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 1 18:13:40 2020 +0200

    storage: check uniqueness based on all relevant columns, not just id and indexer_configuration_id.
    
    ctags and fossology can legitimately add multiple objects with the same
    id and indexer_configuration_id at the same time, but different values
    for other columns.
    
    Before this commit, it would immediately crash the 'check_id_duplicates'
    checks.
    
    As a side-effect, this commit also remove the 'add_merge' logic in the
    in-memory storage, and stores it like proper rows like the pg does instead
    of working around this broken deduplication.
    
    As an other side-effect, removing this broken logic removes an
    inconsistency between the in-mem and pg storage (see the added tests
    named `test_add_empty` for Ctags and Fossology), which Fossology indexer
    tests relied on, so this commit updates these tests as well.

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

Harbormaster completed remote builds in B15812: Diff 14550.Oct 2 2020, 12:20 PM
vlorentz edited the summary of this revision. (Show Details)Oct 2 2020, 12:20 PM
anlambert requested changes to this revision.Oct 2 2020, 1:37 PM
anlambert added a subscriber: anlambert.

I get the idea and did not notice any particular issue in the diff.

Nevertheless, these changes break swh-web tests (P797).

I did not analyze in details the issues but my guess is that a regression is introduced in the indexer memory storage.

swh/indexer/storage/in_memory.py
322

Could you add types to that method signature ?

349

same here

This revision now requires changes to proceed.Oct 2 2020, 1:37 PM
vlorentz updated this revision to Diff 14553.Oct 2 2020, 2:04 PM

fix fossology test, which didn't test the right endpoint; then fix a bug the test missed, which caused the issues in swh-web.

swh-public-ci added a comment.Oct 2 2020, 2:05 PM

Build is green

Patch application report for D4118 (id=14553)

Rebasing onto cd650f102d...

Current branch diff-target is up to date.
Changes applied before test
commit 07821f0da7c4a6e693244193b8bd2ee83b0c9613
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Thu Oct 1 18:13:40 2020 +0200

    storage: check uniqueness based on all relevant columns, not just id and indexer_configuration_id.
    
    ctags and fossology can legitimately add multiple objects with the same
    id and indexer_configuration_id at the same time, but different values
    for other columns.
    
    Before this commit, it would immediately crash the 'check_id_duplicates'
    checks.
    
    As a side-effect, this commit also remove the 'add_merge' logic in the
    in-memory storage, and stores it like proper rows like the pg does instead
    of working around this broken deduplication.
    
    As an other side-effect, removing this broken logic removes an
    inconsistency between the in-mem and pg storage (see the added tests
    named `test_add_empty` for Ctags and Fossology), which Fossology indexer
    tests relied on, so this commit updates these tests as well.

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

Harbormaster completed remote builds in B15815: Diff 14553.Oct 2 2020, 2:05 PM
vlorentz added inline comments.Oct 2 2020, 2:05 PM
swh/indexer/storage/in_memory.py
322

That's on my todo-list for next couple of weeks.

anlambert accepted this revision.Oct 2 2020, 2:18 PM
This revision is now accepted and ready to land.Oct 2 2020, 2:18 PM
Closed by commit rDCIDX07821f0da7c4: storage: check uniqueness based on all relevant columns, not just id and… (authored by vlorentz). · Explain WhyOct 2 2020, 3:16 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDCIDX07821f0da7c4: storage: check uniqueness based on all relevant columns, not just id and….

Revision Contents
Changeset List

PathSize
swh/
indexer/
storage/
67 lines
12 lines
159 lines
51 lines
tests/
storage/
55 lines
2 lines
13 lines

Diff 14561

View Options

swh/indexer/storage/__init__.py

Loading...
View Options

swh/indexer/storage/converters.py

Loading...
View Options

swh/indexer/storage/in_memory.py

Loading...
View Options

swh/indexer/storage/model.py

Loading...
View Options

swh/indexer/tests/storage/test_storage.py

Loading...
View Options

swh/indexer/tests/test_fossology_license.py

Loading...
View Options

swh/indexer/tests/utils.py

Loading...
About · Developer information · Legal