Skip to content
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

adds new_session_client method #100

Merged
merged 4 commits into from
Nov 7, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 80 additions & 20 deletions faunadb/client.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,35 @@
from time import time
# pylint: disable=redefined-builtin
from builtins import object
import threading

from requests import Request, Session
from requests.auth import HTTPBasicAuth
from requests.adapters import HTTPAdapter

from faunadb.errors import _get_or_raise, FaunaError, UnexpectedError
from faunadb.query import _wrap
from faunadb.request_result import RequestResult
from faunadb._json import parse_json_or_none, to_json

class _Counter(object):
def __init__(self, init_value=0):
self.lock = threading.Lock()
self.counter = init_value

def __str__(self):
return "Counter(%s)" % self.counter

def get_and_increment(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still a race condition in here:

  • Thread 1 calls decrement: decrements counter to 0 and close the session
  • Thread 2 calls get_and_increment: increments the counter to 1, returns 0, therefore fail to create a new session client
  • Thread 3 calls get_and_increment: increments the counter to 2, returns 1, allowing a new session client to be create with a closed session.

The knowledge of a live session must be composed by its ref count plus a flag that tells you if the session was closed already.

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 understand you reasoning but, if decrement returns 0 means that no other client is holding this session anymore, also, at this point, if __del__ gets called means that this client is being garbage collected, hence, no client could possibly call new_session_client.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Thread 1 would not decrement to zero if thread 2 and 3 are holding a client. Ignore me. :)

with self.lock:
counter = self.counter
self.counter += 1
return counter

def decrement(self):
with self.lock:
self.counter -= 1
return self.counter

class FaunaClient(object):
"""
Expand All @@ -27,13 +48,18 @@ class FaunaClient(object):
# pylint: disable=too-many-arguments, too-many-instance-attributes
def __init__(
self,
secret,
domain="rest.faunadb.com",
scheme="https",
port=None,
timeout=60,
secret=None,
observer=None):
observer=None,
pool_connections=10,
pool_maxsize=10,
**kwargs):
"""
:param secret:
Auth token for the FaunaDB server.
:param domain:
Base URL for the FaunaDB server.
:param scheme:
Expand All @@ -42,36 +68,45 @@ def __init__(
Port of the FaunaDB server.
:param timeout:
Read timeout in seconds.
:param secret:
Auth token for the FaunaDB server.
:param observer:
Callback that will be passed a :any:`RequestResult` after every completed request.
:param pool_connections:
The number of connection pools to cache.
:param pool_maxsize:
The maximum number of connections to save in the pool.
"""

self.domain = domain
self.scheme = scheme
self.port = (443 if scheme == "https" else 80) if port is None else port

self.session = Session()
if secret is not None:
self.session.auth = (secret, "")

self.session.headers.update({
"Accept-Encoding": "gzip",
"Content-Type": "application/json;charset=utf-8"
})
self.session.timeout = timeout

self.auth = HTTPBasicAuth(secret, "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm: is this base64 encrypted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

self.base_url = "%s://%s:%s" % (self.scheme, self.domain, self.port)

self.observer = observer

self.pool_connections = pool_connections
self.pool_maxsize = pool_maxsize

if ('session' not in kwargs) or ('counter' not in kwargs):
self.session = Session()
self.session.mount('https://', HTTPAdapter(pool_connections=pool_connections,
pool_maxsize=pool_maxsize))
self.session.mount('http://', HTTPAdapter(pool_connections=pool_connections,
pool_maxsize=pool_maxsize))
self.counter = _Counter(1)

self.session.headers.update({
"Accept-Encoding": "gzip",
"Content-Type": "application/json;charset=utf-8"
})
self.session.timeout = timeout
else:
self.session = kwargs['session']
self.counter = kwargs['counter']

def __del__(self):
# pylint: disable=bare-except
try:
if self.counter.decrement() == 0:
self.session.close()
except:
pass

def query(self, expression):
"""
Expand All @@ -89,6 +124,31 @@ def ping(self, scope=None, timeout=None):
"""
return self._execute("GET", "ping", query={"scope": scope, "timeout": timeout})

def new_session_client(self, secret, observer=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this is kind of a clunky title, and it's also not documented what this does or is for, so from both of these points, the user of this library will probably find this method a mystery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If anyone oppose, I'll use ruby driver as a reference to this

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me, though I'm biased 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm, with_secret still a good name for this method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so, yes.

"""
Create a new client from the existing config with a given secret.
The returned client share its parent underlying resources.

:param secret:
Credentials to use when sending requests.
:param observer:
Callback that will be passed a :any:`RequestResult` after every completed request.
:return:
"""
if self.counter.get_and_increment() > 0:
return FaunaClient(secret=secret,
domain=self.domain,
scheme=self.scheme,
port=self.port,
timeout=self.session.timeout,
observer=observer or self.observer,
session=self.session,
counter=self.counter,
pool_connections=self.pool_connections,
pool_maxsize=self.pool_maxsize)
else:
raise UnexpectedError("Cannnot create a session client from a closed session", None)

def _execute(self, action, path, data=None, query=None):
"""Performs an HTTP action, logs it, and looks for errors."""
if query is not None:
Expand Down Expand Up @@ -118,5 +178,5 @@ def _execute(self, action, path, data=None, query=None):
def _perform_request(self, action, path, data, query):
"""Performs an HTTP action."""
url = self.base_url + "/" + path
req = Request(action, url, params=query, data=to_json(data))
req = Request(action, url, params=query, data=to_json(data), auth=self.auth)
return self.session.send(self.session.prepare_request(req))
15 changes: 6 additions & 9 deletions tests/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def setUpClass(cls):
# Turn off annoying logging about reset connections.
getLogger("requests").setLevel(WARNING)

cls.root_client = cls.get_client(secret=_FAUNA_ROOT_KEY)
cls.root_client = cls._get_client()

rnd = ''.join(random.choice(string.ascii_lowercase) for _ in range(10))
db_name = "faunadb-python-test" + rnd
Expand All @@ -41,11 +41,11 @@ def setUpClass(cls):

cls.server_key = cls.root_client.query(
query.create(Ref("keys"), {"database": cls.db_ref, "role": "server"}))["secret"]
cls.client = cls.get_client()
cls.client = cls.root_client.new_session_client(secret=cls.server_key)

cls.admin_key = cls.root_client.query(
query.create(Ref("keys"), {"database": cls.db_ref, "role": "admin"}))["secret"]
cls.admin_client = cls.get_client(secret=cls.admin_key)
cls.admin_client = cls.root_client.new_session_client(secret=cls.admin_key)

@classmethod
def tearDownClass(cls):
Expand Down Expand Up @@ -76,14 +76,11 @@ def assertRaisesRegexCompat(self, exception, regexp, callable, *args, **kwds):
self.assertRaisesRegexp(exception, regexp, callable, *args, **kwds)

@classmethod
def get_client(cls, secret=None, observer=None):
if secret is None:
secret = cls.server_key

def _get_client(cls):
args = {"domain": _FAUNA_DOMAIN, "scheme": _FAUNA_SCHEME, "port": _FAUNA_PORT}
# If None, use default instead
non_null_args = {k: v for k, v in args.items() if v is not None}
return FaunaClient(secret=secret, observer=observer, **non_null_args)
return FaunaClient(secret=_FAUNA_ROOT_KEY, **non_null_args)

def assert_raises(self, exception_class, action):
"""Like self.assertRaises and returns the exception too."""
Expand All @@ -93,7 +90,7 @@ def assert_raises(self, exception_class, action):


def mock_client(response_text, status_code=codes.ok):
c = FaunaClient()
c = FaunaClient(secret=None)
c.session = _MockSession(response_text, status_code)
return c

Expand Down
15 changes: 8 additions & 7 deletions tests/test_client.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
from faunadb.objects import Ref
from faunadb.query import create
from faunadb.client import FaunaClient
from faunadb.errors import UnexpectedError
from tests.helpers import FaunaTestCase

class ClientTest(FaunaTestCase):

def test_ping(self):
self.assertEqual(self.client.ping("all"), "Scope all is OK")

def _create_class(self):
return self.client.query(create(Ref("classes"), {"name": "my_class"}))

def _create_instance(self):
return self.client.query(create(Ref("classes/my_class"), {}))
def test_error_on_closed_client(self):
client = FaunaClient(secret="secret")
client.__del__()
self.assertRaisesRegexCompat(UnexpectedError,
"Cannnot create a session client from a closed session",
lambda: client.new_session_client(secret="new_secret"))
3 changes: 2 additions & 1 deletion tests/test_client_logger.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ def test_url_query(self):

def get_logged(self, client_action):
logged_box = []
client = self.get_client(observer=logger(logged_box.append))
client = self.root_client.new_session_client(secret=self.server_key,
observer=logger(logged_box.append))
client_action(client)
return logged_box[0]
4 changes: 2 additions & 2 deletions tests/test_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@ def test_unexpected_error_code(self):

#region FaunaError
def test_unauthorized(self):
client = self.get_client(secret="bad_key")
client = self.root_client.new_session_client(secret="bad_key")
self._assert_http_error(
lambda: client.query(get(self.db_ref)), Unauthorized, "unauthorized")

def test_permission_denied(self):
# Create client with client key
client = self.get_client(
client = self.root_client.new_session_client(
secret=self.root_client.query(
create(Ref("keys"), {"database": self.db_ref, "role": "client"}))["secret"]
)
Expand Down
4 changes: 2 additions & 2 deletions tests/test_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ def test_create_key(self):
"database": Ref("databases/database_for_key_test"),
"role": "server"}))

new_client = self.get_client(secret=resource["secret"])
new_client = self.admin_client.new_session_client(secret=resource["secret"])

new_client.query(query.create_class({"name": "class_for_test"}))

Expand Down Expand Up @@ -345,7 +345,7 @@ def test_login_logout(self):
query.create(self.class_ref, {"credentials": {"password": "sekrit"}}))["ref"]
secret = self.client.query(
query.login(instance_ref, {"password": "sekrit"}))["secret"]
instance_client = self.get_client(secret=secret)
instance_client = self.client.new_session_client(secret=secret)

self.assertEqual(instance_client.query(
query.select("ref", query.get(Ref("classes/widgets/self")))), instance_ref)
Expand Down