database_module #17

Merged
Miroslavsckaya merged 20 commits from database_interaction into master 2022-05-29 20:19:18 +00:00
No description provided.
Miroslavsckaya added 1 commit 2022-05-19 18:17:15 +00:00
init database object
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
f0c4b0425a
skobkin requested changes 2022-05-19 18:37:35 +00:00
.gitignore Outdated
@ -5,1 +5,4 @@
/.venv
# Database
users_subscribes.db
Collaborator

I think we'll have not only subscribers data in this database, so it's pretty safe to just call it something like database.db/bot.db/database.sqlite.

I think we'll have not only subscribers data in this database, so it's pretty safe to just call it something like `database.db`/`bot.db`/`database.sqlite`.
skobkin marked this conversation as resolved
@ -0,0 +1,36 @@
import sqlite3
class Database():
__instance = None
Collaborator

I'm not sure if we need Singleton here at all. We'll better handle database connections using direct injections into class constructors other than just calling them in the middle of the class method.

I'm not sure if we need Singleton here at all. We'll better handle database connections using direct injections into class constructors other than just calling them in the middle of the class method.
skobkin marked this conversation as resolved
@ -0,0 +9,4 @@
return cls.__instance
def __init__(self):
self.conn = sqlite3.connect('users_subscribes.db')
Collaborator

We'd better have database path passed as an argument of constructor than just hardcode it here.

We'd better have database path passed as an argument of constructor than just hardcode it here.
skobkin marked this conversation as resolved
@ -0,0 +11,4 @@
def __init__(self):
self.conn = sqlite3.connect('users_subscribes.db')
self.cur = self.conn.cursor()
self.cur.execute('CREATE TABLE users (id INTEGER, telegram_id NUMERIC, PRIMARY KEY(id))')
Collaborator

It'll most likely cause an error on the second run because the table was already created earlier

So we should do one of:

  • Use CREATE TABLE IF NOT EXISTS (for now)
  • Implement simple migrations as described in #16 right away and make these queries a first migration
It'll most likely cause an error on the second run because the table was already created earlier So we should do one of: * Use `CREATE TABLE IF NOT EXISTS` (for now) * Implement simple migrations as described in #16 right away and make these queries a first migration
skobkin marked this conversation as resolved
skobkin added the
new feature
label 2022-05-19 18:39:40 +00:00
skobkin added this to the MVP 0.1 milestone 2022-05-19 18:39:44 +00:00
Miroslavsckaya was assigned by skobkin 2022-05-19 18:39:47 +00:00
Miroslavsckaya added 3 commits 2022-05-21 10:55:53 +00:00
make unique values in database and add handling exception in add_user method
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
c25e0c78fe
Miroslavsckaya added 1 commit 2022-05-21 10:57:34 +00:00
delete unnecessary line
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
98e6e16ae5
skobkin reviewed 2022-05-21 14:48:42 +00:00
@ -0,0 +9,4 @@
self.cur.execute('CREATE TABLE IF NOT EXISTS feeds_last_items (feed_id INTEGER, link TEXT NOT NULL UNIQUE, title TEXT, description TEXT, date NUMERIC, FOREIGN KEY(feed_id) REFERENCES feeds(id))')
def add_user(self, telegram_id):
try:
Collaborator

Two things concerns me a bit:

  1. Returned value
    • If you want to return a binary result of an operation, you should probably return boolean value. true if INSERT was successful and false if not. 1 is incomprehensible, it's not even a constant and you're also can't take it as a boolean because it's inversed (true in case of an error is not obvious).
    • If you're returning the result of an operation, you should return it in any case, not only when it failed. So you should have at least two return statements in this method.
    • It'd be much better if you used type hints. Unfortunately due to specifics of Python it doesn't work on language level, but it at least provides important data to static analyzers to help you find errors earlier. For example when they're properly used if you try to pass a str to the function which expects int the static analyzer of your IDE will warn you about it even before you finish the code and run it. The same goes for return types.
  2. Transaction usage
    • You're using commit() outside of try block. It can also cause an exception to be risen, so you should move right after the statements inside the try block. This way is you don't forget to fix return values your function will not cause a fail for an entire app.
    • You're doing nothing about transaction if it failed. If the exception is caught, you most likely should do rollback() to roll back everything were made unless you want to mess up somewhere after that when calling another commit().

UPD: By the way you'll probably want to return inserted user ID from this method (instead of int/bool) to be able to create relations with other entities/tables after that.

Two things concerns me a bit: 1. Returned value * If you want to return a binary result of an operation, you should probably return boolean value. `true` if `INSERT` was successful and `false` if not. `1` is incomprehensible, it's not even a constant and you're also can't take it as a boolean because it's inversed (`true` in case of an error is not obvious). * If you're returning the result of an operation, you should return it in any case, not only when it failed. So you should have at least two return statements in this method. * It'd be much better if you used [type hints](https://docs.python.org/3/library/typing.html). Unfortunately due to specifics of Python it doesn't work on language level, but it at least provides important data to static analyzers to help you find errors earlier. For example when they're properly used if you try to pass a `str` to the function which expects `int` the static analyzer of your IDE will warn you about it even before you finish the code and run it. The same goes for return types. 2. Transaction usage * You're using [`commit()`](https://docs.python.org/3/library/sqlite3.html#sqlite3.Connection.commit) outside of `try` block. It can also cause an exception to be risen, so you should move right after the statements inside the `try` block. This way is you don't forget to fix return values your function will not cause a fail for an entire app. * You're doing nothing about transaction if it failed. If the exception is caught, you most likely should do [`rollback()`](https://docs.python.org/3/library/sqlite3.html#sqlite3.Connection.rollback) to roll back everything were made unless you want to mess up somewhere after that when calling another `commit()`. UPD: By the way you'll probably want to return inserted user ID from this method (instead of `int`/`bool`) to be able to create relations with other entities/tables after that.
skobkin marked this conversation as resolved
skobkin reviewed 2022-05-21 15:00:24 +00:00
@ -0,0 +1,34 @@
import sqlite3
Collaborator

By the way just database.py is enough.

By the way just `database.py` is enough.
skobkin marked this conversation as resolved
skobkin added a new dependency 2022-05-22 17:19:55 +00:00
Miroslavsckaya added 4 commits 2022-05-22 19:42:34 +00:00
skobkin requested changes 2022-05-22 20:07:47 +00:00
skobkin left a comment
Collaborator

Looks good, but here's a couple of things to make better.

Looks good, but here's a couple of things to make better.
@ -0,0 +13,4 @@
self.conn.commit()
return self.cur.lastrowid
def get_user(self, telegram_id: str) -> int | None:
Collaborator

There is a semantic convention about database interaction method naming and behavior.

  • get*() should return value or throw an exception if nothing is found
  • find*() should return value or Null (None or empty array/list in case of collections).

So this method should either be named find_user() or throw an exception when user is not found.

There is a [semantic convention](https://tuhrig.de/find-vs-get/#:~:text=The%20difference%20between%20the%20two,JPA%20repository%20throws%20an%20exception.) about database interaction method naming and behavior. * `get*()` should return value or throw an exception if nothing is found * `find*()` should return value or Null (`None` or empty array/list in case of collections). So this method should either be named `find_user()` or throw an exception when user is not found.
skobkin marked this conversation as resolved
@ -0,0 +18,4 @@
user_id = self.cur.fetchone()
if user_id is None:
return None
return user_id[0]
Collaborator

I think it's most likely row (or data) than user_id.

I think it's most likely `row` (or `data`) than `user_id`.
skobkin marked this conversation as resolved
@ -0,0 +20,4 @@
return None
return user_id[0]
def add_feed(self, link: str) -> int:
Collaborator

If this method doesn't handle user's subscription to the feed than we should have somethig like def subscribe_user(user_id: int, feed_id: int) and def unsubscribe_user(user_id: int, feed_id: int).

If this method doesn't handle user's subscription to the feed than we should have somethig like `def subscribe_user(user_id: int, feed_id: int)` and `def unsubscribe_user(user_id: int, feed_id: int)`.
Collaborator

It's better to call this url instead of link.

It's better to call this [`url`](https://en.wikipedia.org/wiki/URL) instead of `link`.
skobkin marked this conversation as resolved
Miroslavsckaya added 6 commits 2022-05-28 19:48:17 +00:00
skobkin requested changes 2022-05-28 20:04:20 +00:00
skobkin left a comment
Collaborator

Things are getting interesting. Looking forward to see this finished 😺

Things are getting interesting. Looking forward to see this finished 😺
database.py Outdated
@ -0,0 +5,4 @@
self.cur = self.conn.cursor()
self.cur.execute('CREATE TABLE IF NOT EXISTS users (id INTEGER, telegram_id NUMERIC NOT NULL UNIQUE, PRIMARY KEY(id))')
self.cur.execute('CREATE TABLE IF NOT EXISTS feeds (id INTEGER, url TEXT NOT NULL UNIQUE, PRIMARY KEY(id))')
self.cur.execute('CREATE TABLE IF NOT EXISTS subscribes (user_id INTEGER, feed_id INTEGER, FOREIGN KEY(user_id) REFERENCES users(id), FOREIGN KEY(feed_id) REFERENCES feeds(id))')
Collaborator

subscribes

subscriptions

You can also add unique index on user_id, feed_id to make it impossible to accidentally subscribe one user to the same feed twice.
This index will also be automatically used when selecting feeds for user in the method returning user's feeds.

> `subscribes` `subscriptions` You can also add unique index on `user_id, feed_id` to make it impossible to accidentally subscribe one user to the same feed twice. This index will also be automatically used when selecting feeds for user in the method returning user's feeds.
skobkin marked this conversation as resolved
database.py Outdated
@ -0,0 +41,4 @@
self.cur.execute('SELECT * FROM feeds')
return self.cur.fetchall()
def get_users_subscribes(self, user_id: int) -> list:
Collaborator

subscribes is a verb. "User subscribes to the feed".

It's better to use "subscriptions" here.

`subscribes` is a verb. "User subscribes to the feed". It's better to use "subscriptions" here.
Collaborator

By the way as you're selecting not subscriptions themselves but the feeds user subscribed to it could be better to call this something like get_user_feeds() or even find_user_feeds().

By the way as you're selecting not subscriptions themselves but the feeds user subscribed to it could be better to call this something like `get_user_feeds()` or even `find_user_feeds()`.
skobkin marked this conversation as resolved
skobkin reviewed 2022-05-28 20:14:15 +00:00
database.py Outdated
@ -0,0 +21,4 @@
return row[0]
def add_feed(self, url: str) -> int:
self.cur.execute('INSERT INTO feeds (url) VALUES (?)', (url,))
Collaborator

It's better to have consistency in the code style.

Here you have trailing comma, but you don't have it in the subscribe_user() in the list of parameters.
So either remove it here, or add it there.

This is not critical in this project in any way. But in real projects such rules are enforced and CI most likely will reject your code if it's not following team guidelines. But you likely will have tools to check if your code is following the rules of the company/team/project.

I just didn't setup anything like that here 🤷 We only have pylama here 😄

It's better to have consistency in the code style. Here you have trailing comma, but you don't have it in the `subscribe_user()` in the list of parameters. So either remove it here, or add it there. This is not critical in this project in any way. But in real projects such rules are enforced and CI most likely will reject your code if it's not following team guidelines. But you likely will have tools to check if your code is following the rules of the company/team/project. I just didn't setup anything like that here 🤷 We only have `pylama` here 😄
skobkin marked this conversation as resolved
Miroslavsckaya added 3 commits 2022-05-29 12:17:10 +00:00
Miroslavsckaya added 1 commit 2022-05-29 14:36:48 +00:00
add docstrings
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
0dc20fc098
Miroslavsckaya added 1 commit 2022-05-29 20:06:11 +00:00
correct docstrings
Some checks failed
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is failing
c9ae6dcc78
Miroslavsckaya changed title from WIP:database_module to database_module 2022-05-29 20:16:42 +00:00
skobkin approved these changes 2022-05-29 20:18:15 +00:00
skobkin left a comment
Collaborator

Good enough.

Good enough.
Miroslavsckaya merged commit 22dcc4ef06 into master 2022-05-29 20:19:18 +00:00
Miroslavsckaya referenced this pull request from a commit 2022-05-29 20:19:18 +00:00
Miroslavsckaya deleted branch database_interaction 2022-05-29 20:19:31 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#18 Refactor/Optimize transactions
Miroslavsckaya/tg_rss_bot
Reference: Miroslavsckaya/tg_rss_bot#17
No description provided.