search

jacek99 / corepost

REST micro-framework for Python Twisted with multi-core support
http://jacek99.github.com/corepost/
Other
47 stars 15 forks source link

Some minor features added, a few tweaks and a bug corrected #5

Closed kaosat-dev closed 12 years ago

kaosat-dev commented 12 years ago

Hello!

I have been using Corepost for a bit now, and required a few additional features:

So I have added these if that is ok/ of any interest for you. I also:

Thanks again for your great work, I have found corepost very nice and easy to use! Cheers. Mark

jacek99 commented 12 years ago

Hi Mark, thank you so much for your contribution. I will look at merging it very soon and doing a new release.

Do your patches include the BDDs for unit tests as well?

jacek99 commented 12 years ago

1) some issues: I don't like the way jsonpickle serializes classes. It includes private fields, whereas I exclude them. Is there any way to customize that? Otherwise I will remove it.

2) can you explain the _registerRouters() bug with an example? I am not sure I can understand it. Pls attach some sample that can reproduce it, even if it is just a small class. I can add the BDD to reproduce it.

Thanks!

kaosat-dev commented 12 years ago

No problem , I am glad if any of this can help, as Corepost helps me a lot!

Sorry about that, but since I have never used this kind of BDD before, I did not commit my more basic units test, I'll add some as soon as I am a bit more familiar with the syntax , or I can post the basic (standard twisted ones) if you want?

1) Normally it is customized by overriding an object's getstate and setstate methods (setstate can be left blank) , just like basic python pickling : ie for example :

def __getstate__(self):
    state = self.__dict__.copy()
    del state["_someprivatestuff"]
    return state

This could be considered clunky, but the huge advantage of jsonpickle , is that it can serialize pretty much anything you throw at it ( with the default corepost implementation , I could not easily get the json representation of a datetime object, as the json module is really limited as soon as you try to serialize anything but basic types). But I can understand if you want to remove it.

2)I will post the tests that showed this problem tonight when I get home! :)

jacek99 commented 12 years ago

OK, no problem. I will try to switch to using jsonpickle then as the default implementation and probably remove my home-grown solution altogether (no need to reinvent the wheel).

Will wait for (2) so I can write a unit test to reproduce it

Thanks!

kaosat-dev commented 12 years ago

Ok, here goes, sorry if it is a bit long and "dirty": in this test for example, the call to test_get_ops2 will fail , and in debug mode I was able to identify the fact that since @routes don't get reinitialized between tests, rq.url gets "altered" after each passing through __registerRouters in the RequestRouter class

from twisted.trial import unittest
from twisted.web.server import Site
from twisted.internet import reactor, defer
from corepost.web import RESTResource
import jsonpickle
from pollapli.core.utils.wait import wait
from pollapli.core.interface.rest.resources_v3.server import ServerResource
from pollapli.core.interface.rest.tests.rest_caller import RestCaller

class TestServerResource(unittest.TestCase):

def setUp(self):
    self.url = 'http://localhost'
    self.port = 8000
    self.full_url = "%s:%d" % (self.url, self.port)
    app = RESTResource((ServerResource(),))
    self.factory = Site(app)
    self.server = reactor.listenTCP(self.port, self.factory)

    @defer.inlineCallbacks
    def tearDown(self):
        self.factory.stopFactory()
        yield self.server.stopListening()
    self.factory = None

@defer.inlineCallbacks
def test_get_ops(self):
    exp_status = 200
    exp_response_body = {}

    response = yield RestCaller().get("%s/server/ops" % self.full_url, {'Content-Type': ['application/json']})
    obs_status = response.code
    obs_response_body = jsonpickle.decode(response.body)
    self.assertEquals(obs_status, exp_status)
    self.assertEquals(obs_response_body, exp_response_body)

@defer.inlineCallbacks
def test_get_ops2(self):
    exp_status = 200
    exp_response_body = {}

    response = yield RestCaller().get("%s/server/ops" % self.full_url, {'Content-Type': ['application/json']})
    obs_status = response.code
    obs_response_body = jsonpickle.decode(response.body)
    self.assertEquals(obs_status, exp_status)
    self.assertEquals(obs_response_body, exp_response_body)
jacek99 commented 12 years ago

Sorry, it's taking me a long time to merge this one, it breaks multiple tests in many places.

I will need to look at the code changes and maybe merge it manually :-(

On Thu, May 31, 2012 at 7:14 PM, Mark Moissette < reply@reply.github.com

wrote:

Hello!

I have been using Corepost for a bit now, and required a few additional features:

  • handling of some other (standard) http verbs (OPTIONS, HEAD, PATCH)
  • automatic 501 error handling in case of existing URL but with an unimplemented verb in the request

So I have added these if that is ok/ of any interest for you. I also:

  • added support for simplejson for more powerfull json serialization
  • found a bug in the registerRouters method , that would mangle the urls when registerRouters gets called more than once (this was visible in unit test I did for a rest api I created, as any tests except for the first, for the same url/verb would fail).
  • did a few tweaks here and there (see commit messages)

Thanks again for your great work, I have found corepost very nice and easy to use! Cheers. Mark

You can merge this Pull Request by running:

git pull https://github.com/kaosat-dev/corepost master

Or you can view, comment on it, or merge it online at:

https://github.com/jacek99/corepost/pull/5

-- Commit Summary --

  • - added support for OPTIONS, HEAD , and PATCH http verbs
  • - corrected json import bug added in previous commit

-- File Changes --

M corepost/convert.py (55) M corepost/enums.py (8) M corepost/routing.py (81) M corepost/utils.py (12)

-- Patch Links --

https://github.com/jacek99/corepost/pull/5.patch https://github.com/jacek99/corepost/pull/5.diff

Reply to this email directly or view it on GitHub: https://github.com/jacek99/corepost/pull/5

kaosat-dev commented 12 years ago

Hello Jacek. No problem at all, I should have been more careful with the tests too. If you want, I have a bit of time this weekend, and I have checked your tests more closely, so I can add modifications to the tests as well as more tests to cover the things I added and modified, so you won't have to do a manual merge?

jacek99 commented 12 years ago

Hi Mark,

I've finally completed the merge of your changes, but with many differences

a) jsonpickle

I removed it altogether. I just couldn't make it work consistently across json, yaml and XML. Besides I really disliked the way it serialized some objects (the whole /py/object thing). Can we handle it like this:

b) HEAD, OPTIONS, PATCH support

I merged it and added BDD to test it. But only HEAD seems to work, Twisted seems to kick out OPTIONS and PATCH with a 501 error before it even gets to Corepost. How did you get it to work?

c) fixing breakage

Some of your changes broke existing functionality (passing raw Strings to the JSON encoder instead of returning as plain text, double reading of request.content.read() that caused PUT logic to fail, etc). I fixed those.

In order to make it easier for you to ensure any changes you do do not introduce any regressions, I've included a test.sh in the root. Just do "easy_install freshen" and then run "./test.sh" if you make any change. That will re-run all the BDDs and ensure no regressions are present.

For now I've just committed the code, but have not released it officially yet. Waiting for your feedback on these issues...

Cheers Jacek

On Fri, Jun 15, 2012 at 5:55 PM, Mark Moissette < reply@reply.github.com

wrote:

Hello Jacek. No problem at all, I should have been more careful with the tests too. If you want, I have a bit of time this weekend, and I have checked your tests more closely, so I can add modifications to the tests as well as more tests to cover the things I added and modified, so you won't have to do a manual merge?

Reply to this email directly or view it on GitHub: https://github.com/jacek99/corepost/pull/5#issuecomment-6367561

kaosat-dev commented 12 years ago

Hi Jacek ! Sorry for the late reply. Thanks a lot for the merge !

a) I can understand that, the output from jsonpickle is very different

b) My bad, it seems there were some changes I did to the RESTResource class that I failed to commit correctly: normally this can be solved by adding the various render_XXX methods that are missing. But to be sure I'll double check when I get home, and will run your new tests against my local version

c) sorry about that , will be more carefull and run the test more regularly in the future (although the PUT breakage is weird, I use PUT extensively, and have specific tests for those in the app that uses corepost, and had no issues).

Raw strings to the JSON encoder : did you mean the part where Corepost's default behaviour is to set the content-type header to text/plain ?

-General questions on some design aspects of Corepost (as I have worked a lot on it, in an yet to be committed experimental branch):

jacek99 commented 12 years ago

Hi Mark,

yes, for the plain test I meant setting the response as text-plain.

For your other questions:

a) I think I would prefer to keep Corepost the way it is for decorators. It is very similar in terms of design to the Java JAX-RS APIs I use a work and we have HUGE apps with tons of REST resources, no issues in maintenance. So I am not interested really in adopting a different API. I personally feel it scales nicely to both small and large apps.

b) yeah, I will get to the multi-core...as usual with open source, it's time, time and time that are missing.

Cheers Jacek

P.S. I will commit an enhancement next week that will externalize the whole handling of responses to an external interface. So you can cleanly replace the default CorePost functionality with a custom implementation that can do whatever you need. Should be a cleaner approach than the ResponseFilter

On Fri, Jul 6, 2012 at 8:40 AM, Mark Moissette < reply@reply.github.com

wrote:

Hi Jacek ! Sorry for the late reply. Thanks a lot for the merge !

a) I can understand that, the output from jsonpickle is very different

  • I'll try to implement it using a ResponseFilter and use jsonpickle and see how it goes
  • In the meantime I have tried some alternate approach to use jsonpickle AND have cleaner , more manageable serialization, but I did not commit it as it involved a few weird-ish workarounds

b) My bad, it seems there were some changes I did to the RESTResource class that I failed to commit correctly: normally this can be solved by adding the various render_XXX methods that are missing. But to be sure I'll double check when I get home, and will run your new tests against my local version

c) sorry about that , will be more carefull and run the test more regularly in the future (although the PUT breakage is weird, I use PUT extensively, and have specific tests for those in the app that uses corepost, and had no issues).

Raw strings to the JSON encoder : did you mean the part where Corepost's default behaviour is to set the content-type header to text/plain ?

-General questions on some design aspects of Corepost (as I have worked a lot on it, in an yet to be committed experimental branch):

  • was there some consideration at some point to use a Routes/Ror like central route map ? I love the decorator based approach of Corepost, but beyond a certain complexity threshold, it becomes hard to manage routes in a decentralized manner. Perhaps this is out of the scope of what Corepost is intended for?
    • what is the direction you want to take Corepost into in the future: more as tool to quickly generate restfull apis, not really meant for too complex/ heavyweight stuff ? (Either is good, it is just for clarity's sake)
    • Will you still work on the multi-core aspect as originally planned?

Reply to this email directly or view it on GitHub: https://github.com/jacek99/corepost/pull/5#issuecomment-6803887