Skip to content

new version of the plugin (1.5) #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

new version of the plugin (1.5) #8

wants to merge 5 commits into from

Conversation

vdel26
Copy link
Contributor

@vdel26 vdel26 commented Aug 30, 2013

With the previous version of the plugin, you created an API client object
by passing provider_key AND the corresponding application keys.

Now you only pass the provider_key when creating the API client object. Application authorization keys are passed
when the method is called (authrep, authorize or report). Therefore you don't need to create a new client for each incoming request. Also, that's how the Ruby or Java plugins work.

It is backwards compatible with the older version. You can check it by
running the old test suite (tests.py) over the new plugin version.

Other changes:

  • new suite of tests to cover the new way of using the plugin
    (tests-v2.py)
  • added test coverage for the user_key auth pattern
  • new examples and extended documentation
  • available to install as pip package (pip install ThreeScalePY)

vdel26 added 4 commits July 8, 2013 18:53
With the previous version of the plugin you created an API client object
by passing provider_key AND the corresponding application keys.
Now you only pass the provider_key and the application keys are passed
when the method is invokated (authrep, authorize or report). That's how
the Ruby or Java plugins work.

It is backwards compatible with the older version. You can check it by
running the old test suite (tests.py) over the new plugin version.

Other changes:
- new suite of tests to cover the new way of using the plugin
  (tests-v2.py)
- added test coverage for the user_key auth pattern
- new examples and extended documentation
@@ -85,11 +88,10 @@ def testAuthRepWithInvalidMetric(self):
"""test authrep API with invalid metric"""
authrep = self.ThreeScaleAuthRep(self.provider_key, self.app_id, self.app_key)

self.assertFalse(authrep.authrep({"invalid_metric":1}))
self.assertFalse(authrep.authrep(usage={"invalid_metric":1})) #expects (None, None, {...}) or (usage={...})
Copy link
Contributor

Choose a reason for hiding this comment

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

won't this break compatibility when upgrading? can't we do some type checking in the authrep method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, but at that moment using named parameters was the only way I came up with to be able to pass the app keys in the authrep call (not in the client object) while maintaining compatibility with the old way. I will document that point a bit more. If it is ok, I will correct with smth more elegant it in the new update.

Copy link
Contributor

Choose a reason for hiding this comment

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

there should be huge warning when upgrading that it breaks calls when reporting metrics in authrep call (and maybe others)
isn't there in python something like "key".isString? or checking for dict type or checking for number of params (1 - usage, 2 - app_key, app_id, but then what about user_key?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying now doing this check in authrep and seems to work so far:
if isinstance(app_id, dict):
usage = app_id
app_id = ""

@mikz
Copy link
Contributor

mikz commented Aug 30, 2013

good work @vdel26 !

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.

2 participants