Skip to content
This repository has been archived by the owner on Jul 14, 2021. It is now read-only.

First run at everkeys.add_key #5

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
143 changes: 143 additions & 0 deletions modules/evekeys.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
from Queue import queue, Empty
Copy link
Member

Choose a reason for hiding this comment

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

it's Queue.Queue.


import logging
import threading
import time

import evelink
import evelink.cache.sqlite

from kitnirc.modular import Module
Copy link
Member

Choose a reason for hiding this comment

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

Should be grouped with other third party imports


import schema


_log = logging.getLogger(__name__)


class EveKeyError(Exception):
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't appear to be used.

pass


class EveKeysModule(Module):
"""Module to look up details on an EVE API key."""

def __init__(self, *args, **kwargs):
super(EveKeysModule, self).__init__(*args, **kwargs)

self.cache = evelink.cache.sqlite.SqliteCache(".evecache.sqlite3")
self.results = queue()
self.tasks = queue()

for i in range(5):
t = threading.Thread(target=self._worker)
t.daemon = True
t.start()

self._stop = False

def start(self, *args, **kwargs):
super(EveKeysModule, self).start(*args, **kwargs)
self._stop = False

def stop(self, *args, **kwargs):
super(EveKeysModule, self).stop(*args, **kwargs)
self.stop = True
for _ in range(10):
if self.tasks.empty():
break

_log.info("Evekeys still has %d threads outstanding." %
Copy link
Member

Choose a reason for hiding this comment

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

capitalize EveKeys identically to the module name, also this message should mention that it's part of the shutdown process

self.tasks.qsize())
time.sleep(1)

if not self.tasks.empty():
_log.warning("Evekeys giving up with %d threads outstanding." %
Copy link
Member

Choose a reason for hiding this comment

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

EveKeys, shutdown process

self.tasks.qsize())

def _worker():
while not self.stop:
try:
request = self.tasks.get(True, 5)
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason to use 5 seconds here?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we're giving 10 seconds for everything to shut down, checking if we're asked to stop every 5 seconds doesn't seem unreasonable.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but ideally we won't need to use the full 10 seconds. Modules get shut down in serial; we should avoid having to wait longer than we need to.

except Empty:
Copy link
Member

Choose a reason for hiding this comment

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

see note about using fully qualified Queue.Empty above

continue

_add_key(request)
self.tasks.task_done()

def _add_key(request):
keyid = request['keyid']
vcode = request['vcode']
irc_account = request['metadata']['account'].lower()
Copy link
Member

Choose a reason for hiding this comment

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

This lower() is probably redundant, but I guess it's fine to have around.


try:
api = evelink.api.API(api_key(keyid, vcode), cache=self.cache)
account = evelink.account.Account(api=api)
result = account.key_info()
except evelink.APIError as e:
_log.warn("Error loading API key(%s): %s" % (keyid, e))
self.results.put((request, "Failed to load api key."))
Copy link
Member

Choose a reason for hiding this comment

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

Might as well include the key id in the message given to the user.

return

if result:
_log.debug("key: %s, characters: %s" % (keyid,
", ".join(char['name'] for char
in result['characters'].itervalues())))
else:
_log.warn("key: %s, invalid key.")
self.results.put((request, "Invalid key."))
return

try:
summary = _save_key_info(keyid,
vcode,
irc_account,
result['characters'])
self.results.put((request, summary))
Copy link
Member

Choose a reason for hiding this comment

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

Why is this inside the try?

Copy link
Member Author

Choose a reason for hiding this comment

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

If the DB access blew up, we shouldn't put the empty result in, instead entering an error message in the next section.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but you can just put it after the entire try/except block; since you return from the except it won't get run if the except was.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can. Thought this way was more obvious from a logic flow, but will adjust.

return

Copy link
Member

Choose a reason for hiding this comment

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

No blank line here. Keep the except tightly coupled.

except DatabaseError as e:
_log.warn("Database error saving key(%s): %s" % (keyid, e))
self.results.put((request, "Database error, try again later."))
return

def _save_key_info(keyid, vcode, irc_account, characters):
session = schema.Session()

irc_account = metadata['account'].lower()
Copy link
Member

Choose a reason for hiding this comment

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

The account should really get lower()'d before we put it into metadata, rather than sprinkling these everywhere.

account, _ = schema.find_or_create(session,
schema.Account,
account=irc_account)

# add key to the account
Copy link
Member

Choose a reason for hiding this comment

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

Where do the key and account actually get linked? This comment seems misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, that's a bug, since you're right, it doesn't actually associate it with the account, and it should. Same for the characters later.

key, _ = schema.find_or_create(session,
schema.ApiKey,
keyid=keyid,
vcode=vcode)

# add characters
for character in characters.itervalues():
data = { 'name': character['name'],
'corp': character['corp']['name'] }
char, _ = schema.find_or_create(session,
schema.Character,
data=data,
name=character['name'])

session.commit()
return "%d characters added: %s" % (
len(characters),
", ".join([char['name'] for char in characters]))

def add_key(self, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Ew, no. If this has specific args, then define it with specific args. Ideally, have it construct metadata itself.

"""Look up a given API key, associate with account and characters.

Args:
keyid - API key id.
vcode - API key verification.
metadata['account'] - IRC account name.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really clear at all. External functions shouldn't take dicts that have random keys set.

"""
self.tasks.put(kwargs)


# vim: set ts=4 sts=4 sw=4 et:
20 changes: 20 additions & 0 deletions schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,26 @@ def init_db(path):
Base.metadata.create_all(engine)
Session.configure(bind=engine)

def find_or_create(session, model, data=None, **kwargs):
"""Find or create an object.

returns:
(possibly new) objects,
was it created
"""

instance = session.query(model).filter_by(**kwargs).first()

if instance:
return instance, False
else:
if data:
instance = model(**data)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps kwargs.update(data)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that means you can't search using filters you don't want to use when creating the object. I think that should be fine, but it does limit a bit what we can do.

else:
instance = model(**kwargs)
session.add(instance)
return instance, True

account_api_m2m = Table('account_key_m2m', Base.metadata,
Column('account_pk', String, ForeignKey('account.account')),
Column('api_key_pk', Integer, ForeignKey('api_key.keyid'))
Expand Down