Page MenuHomeSoftware Heritage
Differential D2627

In case of errors, return a simple dictionary instead of pickled exception.
ClosedPublic
Actions

Authored by vlorentz on Feb 5 2020, 5:12 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Maniphest Tasks
T2239: storage: kafka issue: Can't pickle <class 'cimpl.KafkaException'>: import of module 'cimpl' failed
Commits
rDCORE08289ce5c33e: In case of errors, return a simple dictionary instead of pickled exception.
Summary

Pros:

  • Language-independent
  • Works with unpickleable exceptions (eg. cimpl.KafkaException, and RLock objects in exceptions raised by tenacity)

Con:

  • Shows a less nice error on the client side (if the client is in Python)

Resolves T2239.

Test Plan

tox passes this repo, but I'm going to try tests with RPC servers that use it.

Diff Detail

Repository
rDCORE Foundations and core functionalities
Branch
error-no-pickle
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 10588
Build 15811: tox-on-jenkinsJenkins
Build 15810: arc lint + arc unit

Event Timeline

vlorentz created this revision.Feb 5 2020, 5:12 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptFeb 5 2020, 5:12 PM
vlorentz added a comment.Feb 5 2020, 5:14 PM

but I'm going to try tests with RPC servers that use it.

Meh. Of course it doesn't.

swh-public-ci added a comment.Feb 5 2020, 5:14 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/399/ for more details.

Harbormaster completed remote builds in B10453: Diff 9393.Feb 5 2020, 5:14 PM
ardumont added a subscriber: ardumont.Feb 5 2020, 6:06 PM

starts to look, better ;)

swh/core/api/__init__.py
97

is self.args systematically a list?

284

unsure about removing that statement.
i guess that's the price for not having tests on that part ;)

ardumont accepted this revision.Feb 6 2020, 12:07 PM
This revision is now accepted and ready to land.Feb 6 2020, 12:07 PM
vlorentz added inline comments.Feb 6 2020, 12:09 PM
swh/core/api/__init__.py
97

It's a tuple, but yes

284

It's a bad idea to return None in case an endpoint doesn't exist, anyway.

vlorentz added a comment.Feb 6 2020, 1:21 PM

Note: this diff will stay open for a little while, @ardumont and I have other changes to make before it can be merged

vlorentz added a child revision: D2630: Re-raise StorageArgumentException through API calls..Feb 6 2020, 2:22 PM
vlorentz updated this revision to Diff 9477.Feb 11 2020, 3:46 PM

Update RPCClient to re-raise whitelisted exceptions types.

swh-public-ci added a comment.Feb 11 2020, 3:48 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/405/ for more details.

Harbormaster completed remote builds in B10540: Diff 9477.Feb 11 2020, 3:48 PM
vlorentz updated this revision to Diff 9527.Feb 14 2020, 12:21 PM

rebase

This revision was landed with ongoing or failed builds.Feb 14 2020, 12:22 PM
Closed by commit rDCORE08289ce5c33e: In case of errors, return a simple dictionary instead of pickled exception. (authored by vlorentz). · Explain Why
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDCORE08289ce5c33e: In case of errors, return a simple dictionary instead of pickled exception..
swh-public-ci added a comment.Feb 14 2020, 12:22 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/407/ for more details.

Harbormaster completed remote builds in B10588: Diff 9527.Feb 14 2020, 12:22 PM

Revision Contents
Changeset List

PathSize
swh/
core/
api/
69 lines
tests/
11 lines

Diff 9527

View Options

swh/core/api/__init__.py

# Copyright (C) 2015-2018 The Software Heritage developers # Copyright (C) 2015-2018 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import binascii import binascii
import datetime import datetime
import pytest
import unittest import unittest
from swh.model import hashutil, identifiers from swh.model import hashutil, identifiers
from swh.model.exceptions import ValidationError from swh.model.exceptions import ValidationError
from swh.model.hashutil import hash_to_bytes as _x from swh.model.hashutil import hash_to_bytes as _x
from swh.model.identifiers import ( from swh.model.identifiers import (
CONTENT, CONTENT,
DIRECTORY, DIRECTORY,
RELEASE, RELEASE,
REVISION, REVISION,
SNAPSHOT, SNAPSHOT,
PersistentId, PersistentId,
normalize_timestamp,
) )
class UtilityFunctionsIdentifier(unittest.TestCase): class UtilityFunctionsIdentifier(unittest.TestCase):
def setUp(self): def setUp(self):
self.str_id = "c2e41aae41ac17bd4a650770d6ee77f62e52235b" self.str_id = "c2e41aae41ac17bd4a650770d6ee77f62e52235b"
self.bytes_id = binascii.unhexlify(self.str_id) self.bytes_id = binascii.unhexlify(self.str_id)
self.bad_type_id = object() self.bad_type_id = object()
▲ Show 20 Lines • Show All 930 Lines • ▼ Show 20 Lines def setUp(self):
"url": "https://github.com/torvalds/linux", "url": "https://github.com/torvalds/linux",
} }
def test_content_identifier(self): def test_content_identifier(self):
self.assertEqual( self.assertEqual(
identifiers.origin_identifier(self.origin), identifiers.origin_identifier(self.origin),
"b63a575fe3faab7692c9f38fb09d4bb45651bb0f", "b63a575fe3faab7692c9f38fb09d4bb45651bb0f",
) )
TS_DICTS = [
(
{"timestamp": 12345, "offset": 0},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": False},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": False},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": {"seconds": 12345}, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": None,
},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{
"timestamp": {"seconds": 12345, "microseconds": 100},
"offset": 0,
"negative_utc": None,
},
{
"timestamp": {"seconds": 12345, "microseconds": 100},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": True},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": True,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
]
@pytest.mark.parametrize("dict_input,expected", TS_DICTS)
def test_normalize_timestamp_dict(dict_input, expected):
assert normalize_timestamp(dict_input) == expected
TS_DICTS_INVALID_TIMESTAMP = [
{"timestamp": 1.2, "offset": 0},
{"timestamp": "1", "offset": 0},
# these below should really also trigger a ValueError...
# {"timestamp": {"seconds": "1"}, "offset": 0},
# {"timestamp": {"seconds": 1.2}, "offset": 0},
# {"timestamp": {"seconds": 1.2}, "offset": 0},
]
@pytest.mark.parametrize("dict_input", TS_DICTS_INVALID_TIMESTAMP)
def test_normalize_timestamp_dict_invalid_timestamp(dict_input):
with pytest.raises(ValueError, match="non-integer timestamp"):
normalize_timestamp(dict_input)
View Options

swh/core/api/tests/test_rpc_client_server.py

# Copyright (C) 2015-2018 The Software Heritage developers # Copyright (C) 2015-2018 The Software Heritage developers
# See the AUTHORS file at the top-level directory of this distribution # See the AUTHORS file at the top-level directory of this distribution
# License: GNU General Public License version 3, or any later version # License: GNU General Public License version 3, or any later version
# See top-level LICENSE file for more information # See top-level LICENSE file for more information
import binascii import binascii
import datetime import datetime
import pytest
import unittest import unittest
from swh.model import hashutil, identifiers from swh.model import hashutil, identifiers
from swh.model.exceptions import ValidationError from swh.model.exceptions import ValidationError
from swh.model.hashutil import hash_to_bytes as _x from swh.model.hashutil import hash_to_bytes as _x
from swh.model.identifiers import ( from swh.model.identifiers import (
CONTENT, CONTENT,
DIRECTORY, DIRECTORY,
RELEASE, RELEASE,
REVISION, REVISION,
SNAPSHOT, SNAPSHOT,
PersistentId, PersistentId,
normalize_timestamp,
) )
class UtilityFunctionsIdentifier(unittest.TestCase): class UtilityFunctionsIdentifier(unittest.TestCase):
def setUp(self): def setUp(self):
self.str_id = "c2e41aae41ac17bd4a650770d6ee77f62e52235b" self.str_id = "c2e41aae41ac17bd4a650770d6ee77f62e52235b"
self.bytes_id = binascii.unhexlify(self.str_id) self.bytes_id = binascii.unhexlify(self.str_id)
self.bad_type_id = object() self.bad_type_id = object()
▲ Show 20 Lines • Show All 930 Lines • ▼ Show 20 Lines def setUp(self):
"url": "https://github.com/torvalds/linux", "url": "https://github.com/torvalds/linux",
} }
def test_content_identifier(self): def test_content_identifier(self):
self.assertEqual( self.assertEqual(
identifiers.origin_identifier(self.origin), identifiers.origin_identifier(self.origin),
"b63a575fe3faab7692c9f38fb09d4bb45651bb0f", "b63a575fe3faab7692c9f38fb09d4bb45651bb0f",
) )
TS_DICTS = [
(
{"timestamp": 12345, "offset": 0},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": False},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": False},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": {"seconds": 12345}, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": None,
},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
(
{
"timestamp": {"seconds": 12345, "microseconds": 100},
"offset": 0,
"negative_utc": None,
},
{
"timestamp": {"seconds": 12345, "microseconds": 100},
"offset": 0,
"negative_utc": False,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": True},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": True,
},
),
(
{"timestamp": 12345, "offset": 0, "negative_utc": None},
{
"timestamp": {"seconds": 12345, "microseconds": 0},
"offset": 0,
"negative_utc": False,
},
),
]
@pytest.mark.parametrize("dict_input,expected", TS_DICTS)
def test_normalize_timestamp_dict(dict_input, expected):
assert normalize_timestamp(dict_input) == expected
TS_DICTS_INVALID_TIMESTAMP = [
{"timestamp": 1.2, "offset": 0},
{"timestamp": "1", "offset": 0},
# these below should really also trigger a ValueError...
# {"timestamp": {"seconds": "1"}, "offset": 0},
# {"timestamp": {"seconds": 1.2}, "offset": 0},
# {"timestamp": {"seconds": 1.2}, "offset": 0},
]
@pytest.mark.parametrize("dict_input", TS_DICTS_INVALID_TIMESTAMP)
def test_normalize_timestamp_dict_invalid_timestamp(dict_input):
with pytest.raises(ValueError, match="non-integer timestamp"):
normalize_timestamp(dict_input)
About · Developer information · Legal