seznam / fastrpc

FastRPC library
http://seznam.github.io/frpc
GNU Lesser General Public License v2.1
46 stars 46 forks source link

python3-fastrpc and xmlrpc.client compatibility #33

Open sejvlond opened 8 years ago

sejvlond commented 8 years ago
python3-fastrpc (7.0.3)
xmlrpc.client.__version__ == '3.4'

fastrpc does not return tuple, when method name is not provided

In [6]: fastrpc.loads(fastrpc.dumps(("test",)))
Out[6]: ('test', None)

In [7]: xmlrpc.client.loads(xmlrpc.client.dumps(("test",)))
Out[7]: (('test',), None)

it is ok when method name is set

In [8]: fastrpc.loads(fastrpc.dumps(("test",), "xxx"))
Out[8]: (('test',), 'xxx')

In [9]: xmlrpc.client.loads(xmlrpc.client.dumps(("test",), "xxx"))
Out[9]: (('test',), 'xxx')

fastrpc raises RuntimeError: Parser error:< Empty document > but only on strings more than 3 chars long:

In [10]: fastrpc.loads("asda")
RuntimeError: Parser error:< Empty document >

Otherwise no error occures

In [12]: fastrpc.loads("asd")
Out[12]: (None, None)
In [13]: xmlrpc.client.loads("asd")
ExpatError: syntax error: line 1, column 0

So there is no chance how to recognize parse error, or validly returned None with no method name..

# OK
In [52]: fastrpc.loads(fastrpc.dumps((None,), "xxx"))
Out[52]: ((None,), 'xxx')

# Validly returned None
In [53]: fastrpc.loads(fastrpc.dumps((None,), None))
Out[53]: (None, None)

# Invalid, but same as valid None
In [63]: fastrpc.loads("asd")
Out[63]: (None, None)

# OK
In [58]: xmlrpc.client.loads(xmlrpc.client.dumps((None,), "xxx", allow_none=True))
Out[58]: ((None,), 'xxx')

# OK
In [57]: xmlrpc.client.loads(xmlrpc.client.dumps((None,), None, allow_none=True))
Out[57]: ((None,), None)

# OK
In [64]: xmlrpc.client.loads("asd")
ExpatError: syntax error: line 1, column 0

Loading xmlrpc.client.Fault with fastrpc fails to set faultCode - that means it is not possible to use fastrpc on xml documents generated with xmlrpc lib

In [49]: fastrpc.loads(xmlrpc.client.dumps(xmlrpc.client.Fault(504, "Err")))
Fault: <fastrpc.Fault: 0, Err>
In [51]: fastrpc.loads(fastrpc.dumps(fastrpc.Fault(504, "Err")))
Fault: <fastrpc.Fault: 504, Err>
volca02 commented 8 years ago

The way loads handles data is dependent on fastRPC library itself, which is trying to be smart, deciding whether it should use Binary or XML based unmarshaller by looking at the stream length and first 2 bytes.

You can craft arbitary length sequences that will pass as valid data, for example <sdfadsfasdfa

I don't currently see a way to enforce Binary unmarshaller to force the validation, I'll see what I can do here.

volca02 commented 8 years ago

I found the reason the faultCode is sometimes unset - there is no guarantee the faultCode will be the first element in the xml stream:

<?xml version='1.0'?>\n<methodResponse>\n<fault>\n<value><struct>\n<member>\n<name>faultCode</name>\n<value><int>504</int></value>\n</member>\n<member>\n<name>fau ltString</name>\n<value><string>Err</string>...

or

<?xml version='1.0'?>\n<methodResponse>\n<fault>\n<value><struct>\n<member>\n<name>faultString</name>\n<value><string>Err</string></value>\n</member>\n<member>\n< name>faultCode</name>\n<value><int>504</int>...

The code that parses the fault is not able to handle the faultCode if it comes as a second value.

sejvlond commented 8 years ago

faultCode

ok, but it's a bug, right? and should be fixed in major 8 version?

fastRPC library itself, which is trying to be smart

would be nice to have some parameter in fastrpc.loads where I can specify content-type, for example: fastrpc.loads(data, use_binary=True/False/None) where None should be default and it means, try to load it at your discretion (backward compatible) and True and False means use fastrpc or use xmlrpc. Because I know, what data I've got from content-type header.

volca02 commented 8 years ago

Yep, it's a bug. I'll fix it in frpc8.

I'll look into the kwargs for loads, it seems possible to do.

volca02 commented 8 years ago

We decided not to change the behavior of loads (tuple/non-tuple parameters) as it would likely break existing usage and would be hard to fix. We may implement an opt-in change later on, but we don't like the idea of postponing the frpc8 release to implement this.

The useBinary for loads will probably make it into the release.

volca02 commented 8 years ago

useBinary in loads was implemented in 01f566c7628f9b1acbb20a35b745c12d95f368ad

sejvlond commented 8 years ago

We decided not to change the behavior of loads (tuple/non-tuple parameters)

and the behavior is: When method is not set, returns non-tuple, when method is set, returns tuple?

volca02 commented 8 years ago

Yes. There is unknown amount of code that relies on the current behavior.

sejvlond commented 8 years ago

ok, I will wrap this for myself as well