Add support for arbitrary deserializers

[csaroff]

🎩 What?

Allow custom deserializers other than the default json deserializer.

🤔 Why?

Some of GCP's pub/sub notifications have messages that are simple strings which can't be deserialized as json.

For example, notifications that a record was inserted into the dicom store comes through with a string indicating the path to that record and nothing more.

🔗 #229

[csaroff]

I'm slightly concerned about catching arbitrary exceptions here. Feedback welcome

[csaroff]

@andrewgy8 Any other thoughts?

[jonasae]

What do you think about changing the proposal to rely on an interface?

This would provide some benefits like:

• Anyone could provide a particular deserializer implementation
• deserializer.deserialize() could rise a DeserializingException and then avoid catching a generic exception
• Avoid passing a lambda as a default argument
• As we will be calling a method, we don't need to check if its a callable anymore

class DeserializingException(Exception)

class JsonDeserializer:
  def deserialize(message): 
    try:
      return json.loads(x.data.decode("utf-8"))
    except json.JSONDecodeError as e:
      raise DeserializingException(e) 

Then in the subscription's constructor would look like this:

def __init__(
        self,
        func,
        topic,
        prefix="",
        suffix="",
        filter_by=None,
        backend_filter_by=None,
        deserializer=JsonDeserializer,
    ):
        ...
        self._deserializer = deserializer
        ...

And finally Callback.__call__:

def __call__(self, message):
       ...
        try:
            data = self._subscription._deserializer.deserialize(message)
        except DeseralizingException as e:
          ...

[csaroff]

I like the exception handling where we catch a DeserializationException internally and ask users to raise that exception within their deserializer.

I don't really see the point of wrapping it in a class though. Seems slightly more complicated and I don't see what it adds.

[jonasae]

I like the exception handling where we catch a DeserializationException internally and ask users to raise that exception within their deserializer.

I don't really see the point of wrapping it in a class though. Seems slightly more complicated and I don't see what it adds.

Setting aside the benefits of using interfaces when injecting dependencies that might have different implementations and must fulfil a contract, It is slightly more complicated.

At least we are removing the responsibility of checking if the passed argument is a callable (the _init_deserializer method won't be needed).

In terms of lines of code, both solutions are similar ;-)

[andrewgy8]

Id say both solutions are identical. In the end they are callables. You could imagine passing a function, class or lambda in there would be perfectly feasible. And they would all work.

The other argument to keeping it a generic callable rather than a specific interface is the parity with the filter_by argument in the sub decorator. Which can also be any callable. https://mercadonarele.readthedocs.io/en/latest/guides/filters.html#filter-by-parameter

[csaroff]

At least we are removing the responsibility of checking if the passed argument is a callable (the _init_deserializer method won't be needed).

Maybe I'm missing something, but are we?

If someone passed in a string or an int as a deserializer, wouldn't they just get an exception when we try to call their deserialize method?

>>> deserializer = 1
>>> deserializer.deserialize()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'int' object has no attribute 'deserialize'

[jonasae]

Id say both solutions are identical. In the end they are callables. You could imagine passing a function, class or lambda in there would be perfectly feasible. And they would all work.

The other argument to keeping it a generic callable rather than a specific interface is the parity with the filter_by argument in the sub decorator. Which can also be any callable. https://mercadonarele.readthedocs.io/en/latest/guides/filters.html#filter-by-parameter

Good point. For consistency sake, I'm buying it.
But please, don't pass a lambda directly in the definition as a default callable.

[jonasae]

Maybe I'm missing something, but are we?

If someone passed in a string or an int as a deserializer, wouldn't they just get an exception when we try to call their deserialize method?

No, you are not. In fact, it's me who missed the parenthesis in the proposal. I meant to pass an instance, not a class name.

[csaroff]

Looks like master received a force-push last month. I rebased my branch so I think the changes should be good to go!