Differential D353

swh.deposit.parsers: Do not lose information during parsing
ClosedPublic
Actions

Authored by ardumont on Jul 5 2018, 6:55 PM.

Tags

Subscribers

None

Details

Reviewers

Maniphest Tasks

T1131: Fix the deposit XML parser to have multiple authors in json output

Commits

rDDEPaeaab0284ff3: swh.deposit.parsers: Simplify current xml parsing
rDDEPe989e3b18847: swh.deposit.parsers: Update docstring
rDDEPd9870db239d2: swh.deposit.parsers: Do not lose information during parsing

Summary

The current behavior of the internal libraries that parses the xml into dict merges entries with the same name.

In effect, we are losing information (e.g author).
That's not a correct behavior (e.g author, license, ...) and the diff fixes that.

Close T1131

Test Plan

Tests ok

Diff Detail

Repository

rDDEP Push deposit

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Jul 5 2018, 6:55 PM

Harbormaster completed remote builds in B1274: Diff 1132.Jul 5 2018, 6:55 PM

ardumont added inline comments.Jul 5 2018, 6:58 PM

swh/deposit/parsers.py
41	I don't like that much but i think that will do for the moment.
swh/deposit/tests/loader/test_loader.py
277	We apparently miss the data loss here.

ardumont added inline comments.Jul 5 2018, 7:03 PM

swh/deposit/tests/loader/test_loader.py
277	Indeed, the source is ok with me ;)

ardumont edited the summary of this revision. (Show Details)Jul 6 2018, 9:24 AM

ardumont added a project: SWORD deposit.

swh.deposit.parsers: Update docstring

Harbormaster completed remote builds in B1275: Diff 1133.Jul 6 2018, 9:44 AM

moranegg added inline comments.Jul 6 2018, 12:09 PM

swh/deposit/parsers.py
41	I don't like it either, There are many more entries that can be lists and I don't know exactly which.. If you want I can check, but I feel like doing so we will always have the risk of loosing other data. And sometime there is only one entry per tag that will be forced into a list element.

ardumont added inline comments.Jul 6 2018, 12:23 PM

swh/deposit/parsers.py
41	Yes, that's one of the reason i don't like it. losing other data... affiliation comes to mind for example. For information, I'm currently working on another implementation that drops the XMLParser altogether. It takes care of the current limitation. It also permits to drop the extra repetitive namespace in keys. I'll update the diff as soon possible.

Move away from django_restframework_xml

Improve implementation to be generic and complete

swh.deposit.parsers: Update docstring
swh.deposit.parsers: Simplify current xml parsing

Harbormaster completed remote builds in B1276: Diff 1134.Jul 6 2018, 12:38 PM

ardumont added inline comments.Jul 6 2018, 12:41 PM

swh/deposit/tests/api/test_parser.py
42	Namespaces are not lost, they are just new key entries.

moranegg accepted this revision.Jul 6 2018, 3:00 PM

This revision is now accepted and ready to land.Jul 6 2018, 3:00 PM

Closed by commit rDDEPd9870db239d2: swh.deposit.parsers: Do not lose information during parsing (authored by ardumont). · Explain WhyJul 6 2018, 3:09 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

swh/

deposit/

62 lines

tests/

api/

116 lines

loader/

24 lines

Diff 1135

swh/deposit/parsers.py

Loading...

swh/deposit/tests/api/test_parser.py

Loading...

swh/deposit/tests/loader/test_loader.py

Loading...