Skip to content

Content-Type support other than application/json#49

Merged
mission-liao merged 1 commit intopyopenapi:masterfrom
WangJiannan:mime
Feb 15, 2016
Merged

Content-Type support other than application/json#49
mission-liao merged 1 commit intopyopenapi:masterfrom
WangJiannan:mime

Conversation

@WangJiannan
Copy link
Copy Markdown
Contributor

pyswagger requires swagger operation consume and produce in application/json format. To support other content-type (application/xml, application/x-protobuf, etc.), I draft a commit.

Here is my usage:

class XmlCodec:
    def marshal(self, _type, _format, value):
        return '<?xml version="1.0" encoding="UTF-8" ?>' + dicttoxml.dicttoxml(value, root=False, attr_type=False)

    def unmarshal(self, _type, _format, data):
        return xmltodict.parse(data)


def _create_mime_codec():
    mime_codec = MimeCodec()
    xmlCodec = XmlCodec()
    for _type in ['application', 'text']:
        mime_codec.register('%s/xml' % _type, xmlCodec)
    return mime_codec


_mime_codec = _create_mime_codec()

app = SwaggerApp.load(filepath, mime_codec=_mime_codec)
client = Client()
op = app.op['op']
pet_Tom=dict(id=1, name='Tom', photoUrls=['http://test'])
# use default/first consume/produce
client.request(op(body=pet_Tom))
# consume 'application/xml'
client.request(op('application/xml', body=pet_Tom))
# consume 'application/xml', produce 'application/json'
client.request(op('application/xml', 'application/json', body=pet_Tom))

Comment thread pyswagger/io.py Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is tricky, I don't sure how to handle it in a better way. Actually, I find prim_factory and mime_codec have some similar work. I'm now just leave prim_factory unchanged.

@WangJiannan
Copy link
Copy Markdown
Contributor Author

This commit is still a draft, I'll add test case after review feedback.

@mission-liao
Copy link
Copy Markdown
Member

Yep, there is no way for users to send/receive in MIME format other than 'application/json'. To make it works, lots of things need to be done.

Here is my review and suggestion of your works:

  • I would like to keep Operation.call accepting **kwargs only, because when every time Swagger has a new version, this part would be reworked. Keep it simple make rework easier. To pass 'preferred' MIME format, you could add a property for SwaggerRequest. The usage would be similar to this:
pet_Tom=dict(id=1, name='Tom', photoUrls=['http://test'])
req, resp = op(body=pet_Tom)

# [0] my sending format, [1] my preferred response format in response
req.mime = ('application/xml', 'application/json') 
client.request((req, resp),)
  • The 'MimeCodec' could be patched to Operation object (check pyswagger.scanner.v2_0.patch_obj, I patched prim_factory to each Operation object), so SwaggerRequest could access it via self.__op.
  • SwaggerResponse.apply_with need to take MIME format in header section into consideration, too.

@mission-liao
Copy link
Copy Markdown
Member

I would stay away from internet for a few days, if you have any PR, I will review them next Friday (or later).

@WangJiannan
Copy link
Copy Markdown
Contributor Author

Got it, thanks for telling me this.

@WangJiannan WangJiannan force-pushed the mime branch 2 times, most recently from 4171f80 to b508a8c Compare December 2, 2015 02:49
@WangJiannan
Copy link
Copy Markdown
Contributor Author

Pull request is updated.

  • keep Operation.call accepting **kwargs only
    • Done
  • The 'MimeCodec' could be patched to Operation object
    • This is exactly what I did in original pull request
  • SwaggerResponse.apply_with need to take MIME format in header section into consideration, too
    • I think it's already done in my original pull request

@mission-liao
Copy link
Copy Markdown
Member

Thanks for your works.

The relation between formats of data types:
user-input -> prim_factory -> primitives for pyswagger (abbreviated as pyprim) -> marshal -> json/xml

The correct procedure of unmarshal should be:
xml/json -> unmarshal -> user-input -> prim_factory -> pyprim

However, I didn’t follow this “expected” design and just make pyprim accept json as string-buffer.
ex.
pyswagger.primtives._model.Model.apply_with
pyswagger.primtives._model.Array.apply_with

Therefore, it works in pyswagger.io.SwaggerResponse.apply_with, because pyprim accept json string. But it’s not a correct design.

Here is the right design:

  • those ‘json.load’ in Array/Model should be removed
  • in pyswagger.io.SwaggerResponse.apply_with, when we encounter a ‘body’, this string should go through MIME codec, generate a correct input for prim_factory, and then send them to prim if necessary.

it seems really messy in io-related code, and I can’t predict what would go wrong when reworking. So these are the follow-up options:

  • option#1 is I merge your works to master, and I will follow your design to make things right.
  • option#2 is let you finish these rework, and I will peer review your work.

What do you think? and thanks for your work again.

@WangJiannan
Copy link
Copy Markdown
Contributor Author

Thanks for your feedback. I would prefer option#2, since I just start learning Python and I need some practice :)

@mission-liao
Copy link
Copy Markdown
Member

Ok, let's fulfill missing pieces of this project.

@mission-liao
Copy link
Copy Markdown
Member

Hi, here is an issue related to this PR, I would start fix that issue next Monday, if you have any progress on this case, please let me know.

@WangJiannan
Copy link
Copy Markdown
Contributor Author

Sorry for the late response, a series of things happened and I'm on vacation now, will update the pull request around Feb. 2.

@mission-liao
Copy link
Copy Markdown
Member

Hi, I'm very appreciate for your work, it's really nice to see things still going on. Before you proceeding, make sure to rebase to current code base (maybe resubmit a PR if possible)

@mission-liao mission-liao added this to the v0.8.18 milestone Feb 1, 2016
@WangJiannan WangJiannan force-pushed the mime branch 3 times, most recently from b052f8d to 8dafbd9 Compare February 3, 2016 09:45
@WangJiannan
Copy link
Copy Markdown
Contributor Author

Hi liao, I've updated the pull request (PR is reused since the update is base on original code change), please help to review.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use StringIO instead of text in case the response is binary format, e.g. protobuf

@mission-liao
Copy link
Copy Markdown
Member

Hi, it looks great at the first sight, I will review it more carefully later today, well done and thanks.

Comment thread pyswagger/primitives/codec.py Outdated
self._codecs = {}
self.register('text/plain', PlainCodec())
jsonCodec = JsonCodec()
for _type in ['application', 'text']:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, keywords are better kept in one string than separated into parts. Could you rewrite this part in this form?

self.register('application/json', jsonCodec)
self.register('text/json', jsonCodec)

In this form, it's easier to trace code for new comers.

@WangJiannan
Copy link
Copy Markdown
Contributor Author

fairly reasonable, code is updated.

@WangJiannan
Copy link
Copy Markdown
Contributor Author

Hey, I saw Taiwan earthquake news today, hope everything goes well with you.

mission-liao added a commit that referenced this pull request Feb 15, 2016
Content-Type support other than application/json
@mission-liao mission-liao merged commit 5f92783 into pyopenapi:master Feb 15, 2016
mission-liao added a commit that referenced this pull request Feb 16, 2016
- customised codec: #49
- support for no SSL certificate
#60
- renders failed on windows
#63
@mission-liao
Copy link
Copy Markdown
Member

For the encoding problem, I have to confess that I take the easiest way to deal with it. And it should be a good timing to take care of it now. I''ll open an issue for discussion and a proposal for dealing this later.

mission-liao added a commit to pyopenapi/pyopenapi that referenced this pull request Aug 12, 2017
- customised codec: pyopenapi/pyswagger#49
- support for no SSL certificate
pyopenapi/pyswagger#60
- renders failed on windows
pyopenapi/pyswagger#63
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants