Page MenuHomeSoftware Heritage
Differential D8036

Check CFF Value Types
ClosedPublic
Actions

Authored by VickyMerzOwn on Jun 24 2022, 8:01 PM.

Details

Reviewers
vlorentz
Maniphest Tasks
T4275: CffMapping: Add checks for value types
Commits
rDCIDX1be4e184d4b9: cff: Add checks for value types
Summary

Fixes T4275

Test Plan

Test with:

  • Empty values to certain fields (eg. name)
  • Values of invalid types to some fields (eg. if name set to integer, the name field should be skipped)
  • Values that do not conform to regular expression of the field should be skipped (eg. email, date)

Diff Detail

Repository
rDCIDX Metadata indexer
Branch
CFFBugs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 30053
Build 46981: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46980: arc lint + arc unit

Event Timeline

VickyMerzOwn created this revision.Jun 24 2022, 8:01 PM
swh-public-ci added a comment.Jun 24 2022, 8:06 PM

Build has FAILED

Patch application report for D8036 (id=28937)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit b47215aa548e38983979bdd689eacb0f93df9099
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Fri Jun 24 23:20:41 2022 +0530

    cff: Add checks for value types

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

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 24 2022, 8:06 PM
Harbormaster failed remote builds in B30030: Diff 28937!
VickyMerzOwn updated this revision to Diff 28939.Jun 24 2022, 9:05 PM

Fix introduced type errors

  • Check that content_dict is a dict prior to checking field-value-types
  • Verify presence of key in dictionary prior to checking value of the key
swh-public-ci added a comment.Jun 24 2022, 9:11 PM

Build is green

Patch application report for D8036 (id=28939)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit a1dccdde953ed3169bf95717f33cc4a8cf6924be
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30032: Diff 28939.Jun 24 2022, 9:11 PM
VickyMerzOwn requested review of this revision.Jun 24 2022, 9:11 PM
vlorentz requested changes to this revision.Jun 25 2022, 7:18 AM

you can move the inner for loop in normalize_authors and combine the conditionals with the existing ones. This will considerably simplify your code

swh/indexer/metadata_dictionary/cff.py
63

bool() is redundant, and you forgot to check it is a string before running the regexp.

too restrictive; this rejects valid addresses like foo+bar@example.com. Validating email addresses with regexps (or even recursive parsers) is generally a lost cause, and in this particular case we don't really need to reject invalid email addresses anyway (as long as they are strings)

This revision now requires changes to proceed.Jun 25 2022, 7:18 AM
vlorentz added inline comments.Jun 25 2022, 7:21 AM
swh/indexer/metadata_dictionary/cff.py
69–75

you are changing the behavior here, by allowing arbitrary fieldes in the output; this would add a new source of potentially invalid data. And even when valid, CFF field names are never valid in expanded JSON-LD, so they would most likely be discarded anyway.

VickyMerzOwn updated this revision to Diff 28965.Jun 28 2022, 10:22 AM

Checks value types in normalize methods
Deletes the extra function added for type checking

swh-public-ci added a comment.Jun 28 2022, 10:28 AM

Build is green

Patch application report for D8036 (id=28965)

Could not rebase; Attempt merge onto bbf9dc8047...

Updating bbf9dc8..9d2f1c9
Fast-forward
 swh/indexer/metadata_dictionary/cff.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Changes applied before test
commit 9d2f1c9888475ccbd8b95779c429a25e578ba36a
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Tue Jun 28 13:49:57 2022 +0530

    cff: Check value types in normalize methods

commit a1dccdde953ed3169bf95717f33cc4a8cf6924be
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30053: Diff 28965.Jun 28 2022, 10:28 AM
VickyMerzOwn updated this revision to Diff 28967.Jun 28 2022, 10:49 AM

Checks value types in normalize methods
Deletes the extra function added for type checking

swh-public-ci added a comment.Jun 28 2022, 10:55 AM

Build is green

Patch application report for D8036 (id=28967)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit 9d2f1c9888475ccbd8b95779c429a25e578ba36a
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Tue Jun 28 13:49:57 2022 +0530

    cff: Check value types in normalize methods

commit a1dccdde953ed3169bf95717f33cc4a8cf6924be
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30055: Diff 28967.Jun 28 2022, 10:55 AM
VickyMerzOwn updated this revision to Diff 28969.Jun 28 2022, 11:47 AM

Adds test for empty fields in CFFMapping

swh-public-ci added a comment.Jun 28 2022, 11:54 AM

Build is green

Patch application report for D8036 (id=28969)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit 9a281c41d93ad8e092a68bbaea7ca55c14fd6421
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Tue Jun 28 15:15:38 2022 +0530

    Test cffmapping when empty fields are present

commit 9d2f1c9888475ccbd8b95779c429a25e578ba36a
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Tue Jun 28 13:49:57 2022 +0530

    cff: Check value types in normalize methods

commit a1dccdde953ed3169bf95717f33cc4a8cf6924be
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30057: Diff 28969.Jun 28 2022, 11:54 AM
VickyMerzOwn updated this revision to Diff 28973.Jun 28 2022, 4:31 PM

Squash commits

swh-public-ci added a comment.Jun 28 2022, 4:37 PM

Build is green

Patch application report for D8036 (id=28973)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit 4d6798c766a5fe5d9250cfbf1ee71192b55f5420
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30061: Diff 28973.Jun 28 2022, 4:37 PM
VickyMerzOwn updated this revision to Diff 28979.Jun 29 2022, 9:47 AM

Adds test for invalid value for a field.
If a list(or any other type) is provided where a string is expected, the field is simply ignored.

swh-public-ci added a comment.Jun 29 2022, 9:53 AM

Build is green

Patch application report for D8036 (id=28979)

Rebasing onto bbf9dc8047...

Current branch diff-target is up to date.
Changes applied before test
commit e564baf9504df744afc03fad21164c8a095bec67
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30066: Diff 28979.Jun 29 2022, 9:53 AM
vlorentz accepted this revision.Jun 29 2022, 10:23 AM

thanks!

This revision is now accepted and ready to land.Jun 29 2022, 10:23 AM
VickyMerzOwn updated this revision to Diff 28982.Jun 29 2022, 10:46 AM

Applied commit on master (using cherry-pick)

swh-public-ci added a comment.Jun 29 2022, 10:52 AM

Build is green

Patch application report for D8036 (id=28982)

Rebasing onto 70c7e91fa8...

Current branch diff-target is up to date.
Changes applied before test
commit 1be4e184d4b9425b10715e17b783d103c852015e
Author: Satvik Vemuganti <vemugantisesha@iitbhilai.ac.in>
Date:   Sat Jun 25 00:16:23 2022 +0530

    cff: Add checks for value types

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

Harbormaster completed remote builds in B30069: Diff 28982.Jun 29 2022, 10:52 AM
vlorentz accepted this revision.Jun 29 2022, 11:00 AM
Closed by commit rDCIDX1be4e184d4b9: cff: Add checks for value types (authored by VickyMerzOwn). · Explain WhyJun 29 2022, 11:01 AM
This revision was automatically updated to reflect the committed changes.
VickyMerzOwn added a commit: rDCIDX1be4e184d4b9: cff: Add checks for value types.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
indexer/
metadata_dictionary/
60 lines100%
Loading...

Diff 28965

View Options

swh/indexer/metadata_dictionary/cff.py

Loading...
About · Developer information · Legal