Added Support For Apache JSON and Binary Data #139

JonnoFTW · 2020-10-01T03:43:05Z

#13 Added support for apache json, so that apache thrift server/clients can talk to thriftpy2 server/clients.
#136 Added support for binary data fields, I'm not sure what ramifications this would have on python2.

…ng converted properly back from json deserialisation

codecov · 2020-10-01T03:46:00Z

Codecov Report

Merging #139 into master will increase coverage by 0.91%.
The diff coverage is 88.49%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   83.14%   84.05%   +0.91%     
==========================================
  Files          43       44       +1     
  Lines        3921     4133     +212     
==========================================
+ Hits         3260     3474     +214     
+ Misses        661      659       -2

Impacted Files	Coverage Δ
thriftpy2/contrib/aio/rpc.py	`84.44% <ø> (ø)`
thriftpy2/contrib/aio/protocol/binary.py	`55.73% <21.42%> (-2.07%)`	⬇️
thriftpy2/contrib/aio/protocol/compact.py	`60.17% <50.00%> (-0.82%)`	⬇️
thriftpy2/protocol/binary.py	`89.25% <76.47%> (-1.06%)`	⬇️
thriftpy2/protocol/compact.py	`84.39% <89.47%> (+2.13%)`	⬆️
thriftpy2/protocol/apache_json.py	`97.51% <97.51%> (ø)`
thriftpy2/protocol/__init__.py	`87.50% <100.00%> (+0.83%)`	⬆️
thriftpy2/protocol/json.py	`96.63% <100.00%> (+13.88%)`	⬆️
thriftpy2/thrift.py	`88.88% <100.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17a66b5...89c4cb1. Read the comment docs.

ethe · 2020-10-09T02:43:24Z

Some CI has failed, could you please fix it?

Fixed flake8 in test

JonnoFTW · 2020-10-09T04:06:43Z

@ethe don't merge yet, I have some problems in my own codebase where TApacheJSONProtocol does not work with THttpClient

…gs, added six dependency

JonnoFTW · 2020-10-13T00:54:08Z

@ethe feel free to check it out now

thriftpy2/__init__.py

thriftpy2/protocol/__init__.py

thriftpy2/thrift.py

This reverts commit 18dd0fb

aisk · 2020-10-22T05:45:19Z

Hi @JonnoFTW I approved this PR, thanks!

JonnoFTW · 2020-10-23T02:31:34Z

@ethe are you happy to merge now? All tests pass along with those in my interoperability repo.

…ated calls to `self.trans.read(1)` takes a long time with large requests

ethe · 2020-10-26T08:20:12Z

Merged, thank you for your patience and contribution.

Added TType.Binary support Relevant to this pull request to add binary support to thriftpy2: Thriftpy/thriftpy2#139

dmulter · 2021-01-20T15:16:16Z

I've been using Thriftpy2 with a private Thrift server for a long time now, and this latest addition has broken things for me. Sorry I can't share the Thrift files, but here's the error when making an RPC call:

.virtualenv/lib/python3.8/site-packages/thriftpy2/thrift.py:219: in _req
    return self._recv(_api)
.virtualenv/lib/python3.8/site-packages/thriftpy2/thrift.py:231: in _recv
    fname, mtype, rseqid = self._iprot.read_message_begin()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

>   ???
E   cybin.ProtocolError: No protocol version header

thriftpy2/protocol/cybin/cybin.pyx:454: ProtocolError

JonnoFTW · 2021-01-20T21:25:03Z

@dmulter are you able to provide a test case to recreate this? Also did you install from pypi or from git?

dmulter · 2021-01-20T21:44:11Z

Normally I would provide a test case, but I'm not sure I can in this case. I'm not responsible for the server, it's complex, and under NDA. It uses TCompactProtocol and TMemoryBuffer transport. I will see what I can do. It just started failing when I did the update.

I install your package from PyPI. I do a pretty standard make_client and the first Thrift function I call fails with this error. I've seen this error is typical when the transport is wrong somehow. I'm sure you hear this all the time, but nothing else has changed and I've produced numerous production releases of my client for many years.

JonnoFTW · 2021-01-20T21:45:42Z

Can you try installing from git?

dmulter · 2021-01-20T22:04:04Z

Fails the same way.

JonnoFTW · 2021-01-21T02:27:22Z

@dmulter can you please give a minimum working example or just tell me exactly what combination of client/server protocols and transports you are using on each end. I'll add it to the tests and see if I can recreate the problem.

ethe · 2021-01-21T07:39:24Z

@dmulter could you please provide a sample IDL file? we need to reproduce it.

JonnoFTW · 2021-01-25T06:35:54Z

@dmulter I'm unable to replicate the problem you are having, here's my test case (it still passes if I use TCompactProtocol):

Spec:

struct BinarySpec {
    1: map<string, binary> str2bin,
    2: map<string, string> str2str,
}

service BinaryService {
    BinarySpec run(1: BinarySpec b_spec);
}

import time
from thriftpy2 import load
from thriftpy2.transport.memory import TCyMemoryBuffer
from thriftpy2.protocol import cybin
from thriftpy2.thrift import TType
from thriftpy2.rpc import make_client, make_server
from multiprocessing import Process

def test_tcompact_memory_buffer():
    spec = load("binary_test.thrift")

    factory = cybin.TCyBinaryProtocolFactory()
    obj = spec.BinarySpec(
        str2bin={"key": b"Binary Value"},
        str2str={"key": "String Value"}
    )

    class Handler:
        def run(self, b_spec):
            print("\nServer received:")
            print(b_spec)
            return b_spec

    def run_server():
        server = make_server(
            service=spec.BinaryService,
            handler=Handler(),
            proto_factory=factory,
        )
        server.serve()

    proc = Process(target=run_server)
    try:
        proc.start()
        time.sleep(0.5)
        client = make_client(spec.BinaryService, proto_factory=factory)
        res = client.run(obj)
        assert res
        print("Server sent:")
        print(res)
    finally:
        proc.kill()
    time.sleep(0.2)

Gives the output:

Server received:
BinarySpec(str2bin={'key': b'Binary Value'}, str2str={'key': 'String Value'})
Server sent:
BinarySpec(str2bin={'key': b'Binary Value'}, str2str={'key': 'String Value'})

If you could modify this test so that it fails in the same way your code does, that would be very useful in fixing the issue.

dmulter · 2021-01-25T18:47:50Z

Some additional status:

I thought I had installed from a Git clone, but just tried as part of my testing and I get the following error during build: clang: error: no such file or directory: 'thriftpy2/transport/cybase.c'
I verified that the test you created above works fine. I had to change things a bit to avoid multiprocessing errors that are also included in the next bullet item code.
I created a version that makes the specific service call and it also works fine. See code at the end.
Seems clear to me that the issue requires the actual server implementation since local tests are not exposing the problem. My strategy now is trying to identify why the Thriftpy2-0.4.13 release broke when talking to the server. I'm trying to build a local version that I can use for debugging the returned bytes to help identify what the code is choking on. I first have to solve the first item above. Note that I can't give access to the real server since it is not ours and is both private and protected via a layer of complex authentication that must be performed first. I experimented with various other transports and was not able to find a way to make the test pass.

xxx.thrift:

enum ApiVersion
{
    kApiVersion1 = 1;
    kApiVersion2;
    kApiVersion3;
}
const ApiVersion kServerApiVersion_Current = ApiVersion.kApiVersion2;

service PrivateService
{
    void openSession
        (
        1: string sessionId,
        2: ApiVersion clientVersion
        )
}

test_xxx.py:

import time
import sys
from thriftpy2 import load
from thriftpy2.rpc import make_client, make_server
from multiprocessing import Process

class Handler:
    def openSession(self, sessionId, clientVersion):
        print(f"\nServer received: sessionId={sessionId} clientVersion={clientVersion}")
        sys.stdout.flush()

def run_server():
    print("Starting server...")
    server = make_server(
        service=spec.PrivateService,
        handler=Handler(),
    )
    server.serve()

spec = load("xxx.thrift")

def test_private_service():
    proc = Process(target=run_server)
    try:
        proc.start()
        time.sleep(0.5)

        print("Calling openSession...")
        client = make_client(service=spec.PrivateService)
        session_id = "xxx"
        client.openSession(session_id, spec.kServerApiVersion_Current)
        assert False, "tests actually worked" # so we can see print output
    finally:
        proc.kill()
    time.sleep(0.2)

dmulter · 2021-01-25T22:01:37Z

Installed Cython and built from source. Currently experimenting with what's returned from the server. Not sure if it helps, but the size value it expects for the protocol version header has a value of 3c68746d. Trying to get the full server response...

dmulter · 2021-01-25T22:32:59Z

Found the problem and it was not this PR, but a different change on 0.4.13. The protocol error was because HTML for an error page was being returned. The breaking change was due to an inserted path separator after port as I'm passing a parsed URL as follows:

    from urllib.parse import urlparse

    url = urlparse(full_url)
    client = make_client(
        service=spec.TestService,
        scheme=url.scheme,
        host=url.netloc,
        path=url.path,
        port="443"
    )

And the change was faf1055

JonnoFTW · 2021-01-27T05:16:42Z

@ethe considering that the @dmulter 's issue didn't come from this pull request, can we merge it back in? The slash problem can be fixed separately.

ethe · 2021-01-27T09:26:27Z

@JonnoFTW yeah your merge does not break compact protocol, however, there is another problem, see #156

JonnoFTW added 5 commits October 1, 2020 11:52

added binary data fields

bb4ea29

Added apache json transport

925ff4d

Added apache json transport and test case, fixed some types not getti…

1ef611d

…ng converted properly back from json deserialisation

Fixed imports

1d5c395

Fixed imports

8d19a98

JonnoFTW mentioned this pull request Oct 1, 2020

Question about JSON protocol #13

Closed

Fixed style problems

e8394fd

JonnoFTW added 3 commits October 9, 2020 14:31

Fixed backwards compatibility issue with json.loads in test

234ebec

Fixed flake8 in test

Removed redundant getvalue() call

ce0dd12

switched to python2.7 friendly super() call

fd71b90

JonnoFTW added 6 commits October 9, 2020 14:53

added future import for json

aa0822e

added tests for make_server from http and rpc modules

205dc7b

refactored req to _req and data loading

0e74710

Fixed up socket timeouts

1d772b1

Fixed test, added in string to try and trip up json parser

b21b114

kill => terminate

77c1736

ethe changed the title ~~Added Support For Apache JSON and Binary Data~~ [WIP] Added Support For Apache JSON and Binary Data Oct 12, 2020

JonnoFTW added 5 commits October 12, 2020 17:59

fix flake errors

d007379

Added extra test case for boolean map keys

162ad2b

Fixed test failing in python2 because json.loads return unicode strin…

891ba28

…gs, added six dependency

Fixed up type coercion when testing

f71bf9d

removed redundant string check

29f19be

JonnoFTW changed the title ~~[WIP] Added Support For Apache JSON and Binary Data~~ Added Support For Apache JSON and Binary Data Oct 13, 2020

Version bump

18dd0fb

ethe reviewed Oct 13, 2020

View reviewed changes

thriftpy2/__init__.py Outdated Show resolved Hide resolved

thriftpy2/protocol/__init__.py Outdated Show resolved Hide resolved

thriftpy2/thrift.py Show resolved Hide resolved

Revert "Version bump"

8bb4ed6

This reverts commit 18dd0fb

JonnoFTW added 3 commits October 21, 2020 12:23

Made cybin work with string/binary

e1e9a53

Fixed docstrings and some edge cases with binary specs

e32de23

Added more test cases

5a4fcbb

aisk approved these changes Oct 22, 2020

View reviewed changes

JonnoFTW mentioned this pull request Oct 26, 2020

Update utils.py unmade/thrift-pyi#19

Merged

JonnoFTW added 3 commits October 26, 2020 12:05

Added more test cases to check for exception raising

7945d78

Wait a bit for the process to die before the next test starts

a77ded3

Use getvalue if available to read the contents of the transport, repe…

89c4cb1

…ated calls to `self.trans.read(1)` takes a long time with large requests

ethe merged commit 1e61072 into Thriftpy:master Oct 26, 2020

unmade added a commit to unmade/thrift-pyi that referenced this pull request Oct 28, 2020

Merge pull request #19 from JonnoFTW/master

a17a5ea

Added TType.Binary support Relevant to this pull request to add binary support to thriftpy2: Thriftpy/thriftpy2#139

ethe mentioned this pull request Oct 29, 2020

mixed up binary and string types #136

Closed

This was referenced Jan 21, 2021

v0.4.13 serialization break #156

Open

Revert "Added Support For Apache JSON and Binary Data" #157

Merged

unmade mentioned this pull request Sep 9, 2021

thrift binary type converted into pyi string type unmade/thrift-pyi#25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Support For Apache JSON and Binary Data #139

Added Support For Apache JSON and Binary Data #139

JonnoFTW commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

ethe commented Oct 9, 2020 •

edited

Loading

JonnoFTW commented Oct 9, 2020

JonnoFTW commented Oct 13, 2020

aisk commented Oct 22, 2020

JonnoFTW commented Oct 23, 2020

ethe commented Oct 26, 2020

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 20, 2021

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 20, 2021

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 21, 2021

ethe commented Jan 21, 2021

JonnoFTW commented Jan 25, 2021 •

edited

Loading

dmulter commented Jan 25, 2021 •

edited

Loading

dmulter commented Jan 25, 2021

dmulter commented Jan 25, 2021

JonnoFTW commented Jan 27, 2021

ethe commented Jan 27, 2021

Added Support For Apache JSON and Binary Data #139

Added Support For Apache JSON and Binary Data #139

Conversation

JonnoFTW commented Oct 1, 2020 • edited Loading

codecov bot commented Oct 1, 2020 • edited Loading

Codecov Report

ethe commented Oct 9, 2020 • edited Loading

JonnoFTW commented Oct 9, 2020

JonnoFTW commented Oct 13, 2020

aisk commented Oct 22, 2020

JonnoFTW commented Oct 23, 2020

ethe commented Oct 26, 2020

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 20, 2021

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 20, 2021

dmulter commented Jan 20, 2021

JonnoFTW commented Jan 21, 2021

ethe commented Jan 21, 2021

JonnoFTW commented Jan 25, 2021 • edited Loading

dmulter commented Jan 25, 2021 • edited Loading

dmulter commented Jan 25, 2021

dmulter commented Jan 25, 2021

JonnoFTW commented Jan 27, 2021

ethe commented Jan 27, 2021

JonnoFTW commented Oct 1, 2020 •

edited

Loading

codecov bot commented Oct 1, 2020 •

edited

Loading

ethe commented Oct 9, 2020 •

edited

Loading

JonnoFTW commented Jan 25, 2021 •

edited

Loading

dmulter commented Jan 25, 2021 •

edited

Loading