Page MenuHomeSoftware Heritage
Differential D346

identifiers: Make invalid persistent identifier parsing raise error
AbandonedPublic
Actions

Authored by ardumont on Jun 20 2018, 10:24 AM.

Details

Reviewers
anlambert
zack
Group Reviewers
Reviewers
Summary

So far we accept any kinds of persistent identifier, including invalid
ones. We should raise instead.

Related T1104

Before, for example:

$ python3
In [1]: from swh.model.identifiers import parse_persistent_identifier
In [2]: parse_persistent_identifier('foo')
Out[2]: {'namespace': 'foo', 'scheme_version': {}}

Now:

$ python3
Python 3.6.6rc1 (default, Jun 13 2018, 06:59:48)
[GCC 8.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from swh.model.identifiers import parse_persistent_identifier
>>> parse_persistent_identifier('foo')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tony/work/inria/repo/swh/swh-environment/swh-model/swh/model/identifiers.py", line 677, in parse_persistent_identifier
    'Wrong format: There should be 4 mandatory parameters')
swh.model.identifiers.SWHMalformedIdentifierException: Wrong format: There should be 4 mandatory parameters
Test Plan

Tests should still pass

Diff Detail

Repository
rDMOD Data model
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1260
Build 1604: arc lint + arc unit

Event Timeline

ardumont created this revision.Jun 20 2018, 10:24 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 20 2018, 10:24 AM
Harbormaster completed remote builds in B1254: Diff 1108.Jun 20 2018, 10:24 AM
ardumont added inline comments.Jun 20 2018, 10:27 AM
swh/model/tests/test_identifiers.py
865–866

Do we also make those raise? Think so.

This optional part is not checked and the actual code silently skips failure.

anlambert added inline comments.Jun 20 2018, 10:57 AM
swh/model/tests/test_identifiers.py
865–866

Yes I think so. The optional parts must be in the form 'key=value'. I handled malformed cases but did not raise any exception in the associated code.

ardumont updated this revision to Diff 1109.Jun 20 2018, 11:08 AM
  • identifiers: Raise when invalid contextual data in persistent id
Harbormaster completed remote builds in B1255: Diff 1109.Jun 20 2018, 11:08 AM
ardumont marked 2 inline comments as done.Jun 20 2018, 11:08 AM
ardumont added inline comments.
swh/model/tests/test_identifiers.py
865–866

sold.

ardumont marked 2 inline comments as done.Jun 20 2018, 11:09 AM
ardumont updated this revision to Diff 1110.Jun 20 2018, 12:14 PM

As per discussion with anlambert, converge towards the existing swh-web utils code [1]

[1] https://forge.softwareheritage.org/source/swh-web/browse/master/swh/web/common/utils.py$254

  • identifiers: Also validate the hash is correct
  • identifiers: Fix wrong type in docstring
Harbormaster completed remote builds in B1256: Diff 1110.Jun 20 2018, 12:14 PM
ardumont updated this revision to Diff 1112.Jun 21 2018, 10:16 AM
  • identifiers: Permit to pass directly the object's id to persistent_identifier api
Harbormaster completed remote builds in B1258: Diff 1112.Jun 21 2018, 10:16 AM
ardumont added a comment.Jun 21 2018, 10:30 AM

As per discussion with anlambert, converge towards the existing swh-web utils code [1]

Clarifying, the webapp's utils code is a 'duplicate' that will possibly become an adapter of the api here.
Probably delegating the parsing to the swh.model function, and possibly transforming any exceptions raised during parsing into the needed swh.web ones.
So that, everything continues working as expected in the swh.web module.

swh/model/identifiers.py
646

@anlambert Following our irc discussion, I found a middle ground. We can now pass directly the identifier instead of a dict.
That will help for the client using this api (as they might not be aware of what's the right key for the identifier).
Still, i kept the dict resolution as our internal representation uses it, for example.

ardumont updated this revision to Diff 1113.Jun 21 2018, 10:49 AM
  • swh.model.cli: Catch specific exception during identifiers check
Harbormaster completed remote builds in B1259: Diff 1113.Jun 21 2018, 10:49 AM
ardumont added inline comments.Jun 21 2018, 10:51 AM
swh/model/cli.py
22

And todo fixed!

anlambert added inline comments.Jun 21 2018, 10:59 AM
swh/model/identifiers.py
646

That's great! It is really more convenient to work directly with ids in swh-web instead of creating a dict with only one entry. Thanks!

anlambert accepted this revision.Jun 21 2018, 11:20 AM
This revision is now accepted and ready to land.Jun 21 2018, 11:20 AM
zack requested changes to this revision.Jun 21 2018, 11:29 AM
zack added inline comments.
swh/model/identifiers.py
666

naming, the hardest thing ever in programming!

can we please call this exception just "InvalidIdentifier" ?

rationale:

  • "invalid" is a more clear and direct term than "malformed"
  • "SWH" is redundant, it's our code and it's clear from the module path
  • "Exception" is redundant, the class derives from that class anyway
This revision now requires changes to proceed.Jun 21 2018, 11:29 AM
ardumont updated this revision to Diff 1114.Jun 21 2018, 11:55 AM
  • identifiers: Validate that inputs are correct
  • identifiers: Reuse ValidationError exception + update docstring
Harbormaster completed remote builds in B1260: Diff 1114.Jun 21 2018, 11:55 AM
ardumont added inline comments.Jun 21 2018, 12:04 PM
swh/model/identifiers.py
666

Did not see this prior to update the diff.

naming, the hardest thing ever in programming!

could not agree more!

rationale:

Love your rationale, thx.

can we please call this exception just "InvalidIdentifier" ?

I just removed this class altogether and use directly the main ValidationError class (used in model).

My rationale was that we are checking for validation of input, be that type, identifier, persistent identifier.
When failing those, we raise a ValidationError with the error detailed in the message.
Seems reasonable enough since we fail a validation.

zack added inline comments.Jun 21 2018, 12:08 PM
swh/model/identifiers.py
666

I just removed this class altogether and use directly the main ValidationError class (used in model).

even better :-)

ardumont mentioned this in D348: swh.web.common.utils: Adapt persistent identifier computations.Jun 21 2018, 1:31 PM
anlambert accepted this revision.Jun 21 2018, 1:44 PM
ardumont mentioned this in rDMODbd22f277c309: identifiers: Reuse ValidationError exception + update docstring.Jun 21 2018, 1:45 PM
ardumont abandoned this revision.Jul 17 2018, 6:41 PM

All concerns have been addressed.
This is already merged and improved through:

bd22f277c309
f2422d65c2b4
dfb128e9210b
b6073e27611e
79d57ac7b788
7779c573e9c7
0d3a05141b81
8a0cc22a73f9

Revision Contents
Changeset List

PathSize
swh/
model/
9 lines
77 lines
tests/
82 lines

Diff 1114

View Options

swh/model/cli.py

Loading...
View Options

swh/model/identifiers.py

Loading...
View Options

swh/model/tests/test_identifiers.py

Loading...
About · Developer information · Legal