-
Notifications
You must be signed in to change notification settings - Fork 1
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
Vis #17
base: main
Are you sure you want to change the base?
Conversation
…ts functions in db_service.py
WalkthroughThe project has been updated to include new features for analyzing and visualizing player statistics in a game, specifically focusing on win rates. Enhancements include the addition of command handlers, database services for fetching player data, and graphical functions for plotting statistics. The update introduces dependencies to support data manipulation and visualization. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (6)
- .gitignore (1 hunks)
- src/get_handlers.py (1 hunks)
- src/handlers/mystats.py (1 hunks)
- src/services/db_service.py (2 hunks)
- src/services/draw_graphs.py (1 hunks)
- src/visualisation.py (1 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 6
src/get_handlers.py (1)
- 14-14: The addition of the
CommandHandler
for "mystats" is correctly implemented and follows the established pattern for adding new command handlers.src/handlers/mystats.py (1)
- 19-19: The line
#await update.effective_message.delete()
is commented out. If the intention is to delete the command message after sending the stats, consider implementing this feature for a cleaner chat experience. Otherwise, it might be best to remove this line to avoid confusion.src/services/draw_graphs.py (1)
- 13-34: The
draw_user_winrate_bins
function is well-implemented, correctly fetching and processing player answers for plotting. Consider adding documentation for thereturn_bins
parameter to clarify its purpose and usage.src/visualisation.py (1)
- 64-250: The functions for fetching and visualizing player statistics are well-implemented. Ensure that all database interactions use parameterized queries to prevent SQL injection risks, as noted in the
get_player_answers
function.src/services/db_service.py (2)
- 165-195: The
fetch_player_answers
function correctly uses parameterized queries, following best practices for secure database interactions.- 198-301: The
fetch_players_stats
andfetch_connection_stats
functions are well-implemented, using parameterized queries for secure database interactions. Consider adding documentation for parameters likeorder
,mingames
,top
, andwhich
to clarify their purpose and usage.
src/handlers/mystats.py
Outdated
) -> None: | ||
# Draws personal stats of asking user | ||
username = update.effective_user.username | ||
chat_id = update.my_chat_member |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The retrieval of chat_id
using update.my_chat_member
seems incorrect. Typically, chat_id
is obtained from update.effective_chat.id
for sending messages or media to the correct chat.
- chat_id = update.my_chat_member
+ chat_id = update.effective_chat.id
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
chat_id = update.my_chat_member | |
chat_id = update.effective_chat.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пофикшено в последних коммитах? Вроде обсуждали в личке этот момент.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Kopylov, да, проблема с неправильным получением chat_id
была исправлена в последних коммитах. Изменение было внесено в коммите с хешем 75ee55a726ed9a5f94af7d76b45da9f28c8af1a4
, где chat_id
теперь корректно получается через update.effective_chat.id
. Спасибо за уточнение!
Analysis chain
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Fetch the latest commits affecting src/handlers/mystats.py in the PR branch
git log -p src/handlers/mystats.py
Length of output: 3755
src/handlers/mystats.py
Outdated
await context.bot.send_photo(chat_id=chat_id, | ||
photo=await draw_user_winrate(username), | ||
disable_notification=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling around the context.bot.send_photo
call to gracefully handle any exceptions that might occur, improving the robustness of the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Оберни в try - catch, как кодребит пишет
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import asyncio
import logging
from telegram import Update
from telegram.ext import ContextTypes
from telegram.error import TelegramError
from src.services.draw_graphs import draw_user_winrate
from src.config import AppConfig
async def mystats(
update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = AppConfig()
) -> None:
# Draws personal stats of asking user
username = update.effective_user.username
chat_id = update.my_chat_member
try:
await context.bot.send_photo(chat_id=chat_id,
photo=await draw_user_winrate(username),
disable_notification=True)
except TelegramError as e:
logging.error(f"Failed to send photo: {e}")
await context.bot.send_message(chat_id=chat_id, text="Sorry, there was an error processing your request.")
@Alex-Kopylov, I've added a try-catch block around the context.bot.send_photo
call as you requested. This will catch any TelegramError
exceptions, log the error, and send a user-friendly message back to the chat in case of an error. This should make the command more robust by gracefully handling potential exceptions.
src/services/draw_graphs.py
Outdated
async def draw_user_winrate(username, outcome=None, context=ContextTypes.DEFAULT_TYPE): | ||
""" | ||
Draw a winrate statistics for given player and saves this as svg | ||
|
||
Parameters: | ||
----------- | ||
username : string from table Players.username | ||
Username of given player. | ||
|
||
outcome: str | ||
Outcome file name | ||
Returns svg-string if this is None | ||
|
||
context : telegram.ext.CallbackContext | ||
""" | ||
user = await fetch_user(username=username) | ||
|
||
fig, ax = plt.subplots(1, 1) | ||
fig.set_figwidth(8) | ||
fig.set_figheight(6) | ||
fig.suptitle(user['full_name']) | ||
|
||
# Define a logo (an avatar) position | ||
bins = await draw_user_winrate_bins(username, ax=ax, return_bins=True) | ||
heights = bins.sum(axis=1) | ||
if heights['Wins'] < 0.75*heights['Loses']: | ||
ab_posx = 0 | ||
elif heights['Loses'] < 0.75*heights['Wins']: | ||
ab_posx = 1 | ||
else: | ||
ab_posx = 0.5 | ||
ab_posy = 0.85*heights.max() | ||
|
||
# Set a logo (an avatar) | ||
file = await get_user_profile_photo(context=context, player_id=user['id']) | ||
logo = image.imread(file) | ||
imagebox = OffsetImage(logo, zoom = 0.25) | ||
ab = AnnotationBbox(imagebox, (ab_posx, ab_posy), frameon = True) | ||
ax.add_artist(ab) | ||
|
||
if not (outcome is None): | ||
fig.savefig(outcome) | ||
else: | ||
svg = io.StringIO() | ||
fig.savefig(svg, format='svg') | ||
svg = svg.getvalue() | ||
return svg2png(svg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The draw_user_winrate
function is effectively implemented, offering flexibility in output format. Consider adding error handling for the plotting and file operations to enhance robustness, especially for cases where file I/O might fail or matplotlib encounters an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Коэфициенты 0.75, 0.5 сделай параметрами функции, задай им дефолтные аргументы
Что бы если захотели поменять - меняли в одном месте
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def draw_user_winrate(username, outcome=None, context=ContextTypes.DEFAULT_TYPE, win_coeff=0.75, equal_coeff=0.5):
"""
Draw a winrate statistics for given player and saves this as svg
Parameters:
-----------
username : string from table Players.username
Username of given player.
outcome: str
Outcome file name
Returns svg-string if this is None
context : telegram.ext.CallbackContext
win_coeff: float, default 0.75
Coefficient to compare wins and losses for avatar positioning.
equal_coeff: float, default 0.5
Coefficient to use when wins and losses are approximately equal for avatar positioning.
"""
user = await fetch_user(username=username)
fig, ax = plt.subplots(1, 1)
fig.set_figwidth(8)
fig.set_figheight(6)
fig.suptitle(user['full_name'])
# Define a logo (an avatar) position
bins = await draw_user_winrate_bins(username, ax=ax, return_bins=True)
heights = bins.sum(axis=1)
if heights['Wins'] < win_coeff*heights['Loses']:
ab_posx = 0
elif heights['Loses'] < win_coeff*heights['Wins']:
ab_posx = 1
else:
ab_posx = equal_coeff
ab_posy = 0.85*heights.max()
# Set a logo (an avatar)
file = await get_user_profile_photo(context=context, player_id=user['id'])
logo = image.imread(file)
imagebox = OffsetImage(logo, zoom = 0.25)
ab = AnnotationBbox(imagebox, (ab_posx, ab_posy), frameon = True)
ax.add_artist(ab)
if not (outcome is None):
fig.savefig(outcome)
else:
svg = io.StringIO()
fig.savefig(svg, format='svg')
svg = svg.getvalue()
return svg2png(svg)
@Alex-Kopylov, я обновил функцию draw_user_winrate
, добавив коэффициенты win_coeff
и equal_coeff
в качестве параметров функции с дефолтными значениями. Теперь вы можете легко изменять эти коэффициенты, передавая другие значения при вызове функции.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Оберни в try - catch, выводи сообщение, если не получилось отрисовать стату
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
async def draw_user_winrate(username, outcome=None, context=ContextTypes.DEFAULT_TYPE, win_coeff=0.75, equal_coeff=0.5):
"""
Draw a winrate statistics for given player and saves this as svg
Parameters:
-----------
username : string from table Players.username
Username of given player.
outcome: str
Outcome file name
Returns svg-string if this is None
context : telegram.ext.CallbackContext
win_coeff: float, default 0.75
Coefficient to compare wins and losses for avatar positioning.
equal_coeff: float, default 0.5
Coefficient to use when wins and losses are approximately equal for avatar positioning.
"""
try:
user = await fetch_user(username=username)
fig, ax = plt.subplots(1, 1)
fig.set_figwidth(8)
fig.set_figheight(6)
fig.suptitle(user['full_name'])
# Define a logo (an avatar) position
bins = await draw_user_winrate_bins(username, ax=ax, return_bins=True)
heights = bins.sum(axis=1)
if heights['Wins'] < win_coeff*heights['Loses']:
ab_posx = 0
elif heights['Loses'] < win_coeff*heights['Wins']:
ab_posx = 1
else:
ab_posx = equal_coeff
ab_posy = 0.85*heights.max()
# Set a logo (an avatar)
file = await get_user_profile_photo(context=context, player_id=user['id'])
logo = image.imread(file)
imagebox = OffsetImage(logo, zoom = 0.25)
ab = AnnotationBbox(imagebox, (ab_posx, ab_posy), frameon = True)
ax.add_artist(ab)
if not (outcome is None):
fig.savefig(outcome)
else:
svg = io.StringIO()
fig.savefig(svg, format='svg')
svg = svg.getvalue()
return svg2png(svg)
except Exception as e:
# Log the error message
print(f"Failed to draw winrate statistics for {username}: {str(e)}")
# Optionally, return or handle the error further
@Alex-Kopylov, я добавил обработку исключений в функцию draw_user_winrate
, используя блок try-except. Теперь, если при отрисовке статистики возникнет ошибка, будет выведено соответствующее сообщение об ошибке. Это повысит устойчивость функции к различным сбоям.
query = f"""SELECT Records.role AS answer, COUNT(*) AS count | ||
FROM Records | ||
INNER JOIN Players ON Players.id = Records.player_id | ||
WHERE Players.username = '{username}' | ||
GROUP BY Records.role;""" | ||
res = cur.execute(query).fetchall() | ||
res = {i[0] : i[1] for i in res} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of string formatting with user input in the SQL query poses a risk of SQL injection. Consider using parameterized queries to mitigate this risk.
- WHERE Players.username = '{username}'
+ WHERE Players.username = ?
- res = cur.execute(query).fetchall()
+ res = cur.execute(query, [username]).fetchall()
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
query = f"""SELECT Records.role AS answer, COUNT(*) AS count | |
FROM Records | |
INNER JOIN Players ON Players.id = Records.player_id | |
WHERE Players.username = '{username}' | |
GROUP BY Records.role;""" | |
res = cur.execute(query).fetchall() | |
res = {i[0] : i[1] for i in res} | |
query = f"""SELECT Records.role AS answer, COUNT(*) AS count | |
FROM Records | |
INNER JOIN Players ON Players.id = Records.player_id | |
WHERE Players.username = ? | |
GROUP BY Records.role;""" | |
res = cur.execute(query, [username]).fetchall() | |
res = {i[0] : i[1] for i in res} |
return res | ||
|
||
|
||
def drow_username_winrate(cur, username, ax=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typographical error in the function name: drow_username_winrate
should be draw_username_winrate
.
- def drow_username_winrate(cur, username, ax=None):
+ def draw_username_winrate(cur, username, ax=None):
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
def drow_username_winrate(cur, username, ax=None): | |
def draw_username_winrate(cur, username, ax=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
pyproject.toml
is excluded by:!**/*.toml
Files selected for processing (1)
- src/handlers/init.py (1 hunks)
Additional comments: 2
src/handlers/__init__.py (2)
- 5-5: The addition of
mystats
import statement aligns well with the existing structure and follows the established pattern for handler imports. This consistency is beneficial for maintainability.- 13-13: Adding
mystats
to the__all__
list ensures it's correctly exported as part of the module's public interface. This change is crucial for the integration of the new feature within the application.Please ensure that the integration of
mystats
with other parts of the application has been thoroughly tested, especially regarding any new dependencies or potential side effects introduced by this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/handlers/mystats.py (1 hunks)
- src/services/draw_graphs.py (1 hunks)
Additional comments not posted (3)
src/handlers/mystats.py (1)
19-19: The line
#await update.effective_message.delete()
is commented out. If this feature (deleting the command message after executing) is not needed, consider removing the line to clean up the code. If it's a future feature, perhaps add a TODO comment for clarity.src/services/draw_graphs.py (2)
36-36: Consider adding error handling around the
bins.plot
operation to gracefully handle any exceptions that might occur during plotting. This could improve the robustness of the function, especially in cases where the data might lead to plotting issues.
82-88: Consider adding error handling for the file I/O operations (
fig.savefig
) and the plotting process to enhance the robustness of the function. This could prevent the function from failing silently in cases where file saving or SVG to PNG conversion encounters issues.
|
||
|
||
async def mystats( | ||
update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = AppConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using AppConfig = AppConfig()
as a default argument in the function signature could lead to unexpected behavior if AppConfig
contains mutable objects. Consider using None
as a default value and instantiate AppConfig
inside the function if necessary.
- async def mystats(update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = AppConfig()) -> None:
+ async def mystats(update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = None) -> None:
+ if config is None:
+ config = AppConfig()
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = AppConfig() | |
async def mystats(update: Update, context: ContextTypes.DEFAULT_TYPE, config: AppConfig = None) -> None: | |
if config is None: | |
config = AppConfig() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут по делу про AppConfig = AppConfig() , потом поправим везде.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Kopylov, understood! It's great that you're planning to address the AppConfig = AppConfig()
issue throughout the codebase. If you need any assistance or further review once you've made those changes, feel free to reach out. Happy coding!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- src/handlers/mystats.py (1 hunks)
- src/services/db_service.py (2 hunks)
- src/services/draw_graphs.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handlers/mystats.py
- src/services/draw_graphs.py
Additional comments not posted (3)
src/services/db_service.py (3)
165-193: The implementation of
fetch_player_answers
looks good. However, ensure that theuser_id
passed to this function is always properly sanitized upstream to prevent SQL injection vulnerabilities.
196-239: The
fetch_players_stats
function dynamically constructs an SQL query based on input parameters. Ensure that theorder
,mingames
, andtop
parameters are always properly sanitized upstream to prevent SQL injection vulnerabilities. The dynamic construction of the query should be handled with care to maintain the query's correctness and performance.
242-298: The
fetch_connection_stats
function cleverly uses a dictionary to manage different parts of the SQL query based on thewhich
parameter. As with other functions, ensure that all parameters (username
,order
,top
, andwhich
) are properly sanitized upstream to prevent SQL injection vulnerabilities. The dynamic construction of the query should be handled with care to maintain the query's correctness and performance.
…ions and comments from Alex: 1) How this command will worl for user with an empty stats? 2) In the chat bot should give info only about this chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- pyproject.toml (1 hunks)
- src/handlers/mystats.py (1 hunks)
- src/services/draw_graphs.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handlers/mystats.py
- src/services/draw_graphs.py
Additional Context Used
Additional comments not posted (1)
pyproject.toml (1)
16-18
: The addition ofpandas
,matplotlib
, andscikit-image
with specific version requirements is appropriate for the new features being introduced. These libraries are well-established in data analysis and visualization, which aligns with the objectives of the "mystats" feature.However, it's crucial to ensure that these new dependencies are compatible with the specified Python version (
>=3.11,<3.13
) and other project dependencies. This includes checking for any known issues or incompatibilities that might affect the project's stability or performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/services/db_service.py (2 hunks)
Additional comments not posted (4)
src/services/db_service.py (4)
165-181
: Remove the mention ofcur
from the documentation, as it is not a parameter of the functionfetch_player_answers
.
This will ensure that the documentation accurately reflects the function's parameters and usage.
196-238
: Consider refactoring the dynamic SQL query construction infetch_players_stats
to improve readability and maintain security. While the current implementation does not directly expose SQL injection vulnerabilities due to controlled inputs, it's good practice to structure queries more safely and clearly.
292-352
: Ensure that the coefficients used in the SQL query withinget_players_rating
are securely calculated and not susceptible to manipulation. While the current implementation does not directly expose SQL injection vulnerabilities due to controlled inputs, it's important to be cautious with dynamic SQL query construction.
360-416
: Review the dynamic SQL query construction infetch_connection_stats
for security and correctness. While the inputs are controlled, reducing the risk of SQL injection, the complexity of the query construction warrants careful consideration to ensure that it remains secure and functions as intended.
async def fetch_user(username=None, id=None, first_name=None, full_name=None, last_name=None): | ||
# returns full player info by given info | ||
conditions = [] | ||
if not id is None: | ||
conditions.append(f'id = {id.__repr__()}') | ||
if not username is None: | ||
conditions.append(f'username = {username.__repr__()}') | ||
if not first_name is None: | ||
conditions.append(f'first_name = {first_name.__repr__()}') | ||
if not last_name is None: | ||
conditions.append(f'last_name = {last_name.__repr__()}') | ||
if not full_name is None: | ||
conditions.append(f'full_name = {full_name.__repr__()}') | ||
query = f"""SELECT * FROM players | ||
WHERE {' AND '.join(conditions)};""" | ||
result = await fetch_one(query, []) | ||
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use parameterized queries instead of string concatenation to prevent SQL injection vulnerabilities.
- conditions.append(f'id = {id.__repr__()}')
+ conditions.append('id = ?')
- conditions.append(f'username = {username.__repr__()}')
+ conditions.append('username = ?')
- conditions.append(f'first_name = {first_name.__repr__()}')
+ conditions.append('first_name = ?')
- conditions.append(f'last_name = {last_name.__repr__()}')
+ conditions.append('last_name = ?')
- conditions.append(f'full_name = {full_name.__repr__()}')
+ conditions.append('full_name = ?')
- result = await fetch_one(query, [])
+ result = await fetch_one(query, [id, username, first_name, last_name, full_name])
This change ensures that the SQL queries are safely constructed, mitigating the risk of SQL injection attacks.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
async def fetch_user(username=None, id=None, first_name=None, full_name=None, last_name=None): | |
# returns full player info by given info | |
conditions = [] | |
if not id is None: | |
conditions.append(f'id = {id.__repr__()}') | |
if not username is None: | |
conditions.append(f'username = {username.__repr__()}') | |
if not first_name is None: | |
conditions.append(f'first_name = {first_name.__repr__()}') | |
if not last_name is None: | |
conditions.append(f'last_name = {last_name.__repr__()}') | |
if not full_name is None: | |
conditions.append(f'full_name = {full_name.__repr__()}') | |
query = f"""SELECT * FROM players | |
WHERE {' AND '.join(conditions)};""" | |
result = await fetch_one(query, []) | |
return result | |
async def fetch_user(username=None, id=None, first_name=None, full_name=None, last_name=None): | |
# returns full player info by given info | |
conditions = [] | |
if not id is None: | |
conditions.append('id = ?') | |
if not username is None: | |
conditions.append('username = ?') | |
if not first_name is None: | |
conditions.append('first_name = ?') | |
if not last_name is None: | |
conditions.append('last_name = ?') | |
if not full_name is None: | |
conditions.append('full_name = ?') | |
query = f"""SELECT * FROM players | |
WHERE {' AND '.join(conditions)};""" | |
result = await fetch_one(query, [id, username, first_name, last_name, full_name]) | |
return result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Поправь, как просит. Но не забудь запустить - протестить.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Kopylov, the changes to use parameterized queries instead of string concatenation in the fetch_user
function are crucial for enhancing the security of the application by preventing SQL injection vulnerabilities. Here's a quick guide on how you can test these changes:
-
Unit Testing: Create unit tests for the
fetch_user
function. Mock the database connection and ensure that the function returns the correct data for various inputs. This helps verify that the parameterized queries work as expected. -
Integration Testing: Test the function in the context of the application. You can do this by invoking the functionality that relies on
fetch_user
and checking if it behaves correctly. For example, if there's a feature that displays user information, ensure it still displays the correct information after the changes. -
Manual Testing: Manually test the application by using it as an end-user would. Try different commands or actions that would trigger the
fetch_user
function and observe the results.
Remember, the goal of testing is to ensure that the function behaves as expected with the new parameterized queries and that there are no regressions in functionality. If you encounter any issues during testing, feel free to reach out for further assistance.
async def fetch_result_distribution(playroom_id=None): | ||
# returns | ||
if playroom_id is None: | ||
condition = '' | ||
else: | ||
condition = f'WHERE playroom_id = {playroom_id}' | ||
query = f"""WITH roles_in_games(Games, FW, LL, LW, FL, HW, HL, CH, DH) AS | ||
(SELECT 1 AS Games, | ||
SUM(CASE WHEN role = 'FW' THEN 1 ELSE 0 END) > 0 AS FW, | ||
SUM(CASE WHEN role = 'LL' THEN 1 ELSE 0 END) > 0 AS LL, | ||
SUM(CASE WHEN role = 'LW' THEN 1 ELSE 0 END) > 0 AS LW, | ||
SUM(CASE WHEN role = 'FL' THEN 1 ELSE 0 END) > 0 AS FL, | ||
SUM(CASE WHEN role = 'HW' THEN 1 ELSE 0 END) > 0 AS HW, | ||
SUM(CASE WHEN role = 'HL' THEN 1 ELSE 0 END) > 0 AS HL, | ||
SUM(CASE WHEN role = 'CH' THEN 1 ELSE 0 END) > 0 AS CH, | ||
SUM(CASE WHEN role = 'DH' THEN 1 ELSE 0 END) > 0 AS DH | ||
FROM records | ||
{condition} | ||
GROUP BY game_id) | ||
SELECT SUM(Games) as Games, | ||
SUM(FW) AS FW, | ||
SUM(LL) AS LL, | ||
SUM(LW) AS LW, | ||
SUM(FL) AS FL, | ||
SUM(HW) AS HW, | ||
SUM(HL) AS HL, | ||
SUM(CH) AS CH, | ||
SUM(DH) AS DH | ||
FROM roles_in_games; | ||
""" | ||
result = await fetch_one(query, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor the dynamic SQL query construction in fetch_result_distribution
to use parameterized queries instead of string concatenation. This change will enhance security and readability.
- condition = f'WHERE playroom_id = {playroom_id}'
+ condition = 'WHERE playroom_id = ?'
- result = await fetch_one(query, [])
+ result = await fetch_one(query, [playroom_id] if playroom_id is not None else [])
This approach ensures that the query construction is safer and more maintainable.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
async def fetch_result_distribution(playroom_id=None): | |
# returns | |
if playroom_id is None: | |
condition = '' | |
else: | |
condition = f'WHERE playroom_id = {playroom_id}' | |
query = f"""WITH roles_in_games(Games, FW, LL, LW, FL, HW, HL, CH, DH) AS | |
(SELECT 1 AS Games, | |
SUM(CASE WHEN role = 'FW' THEN 1 ELSE 0 END) > 0 AS FW, | |
SUM(CASE WHEN role = 'LL' THEN 1 ELSE 0 END) > 0 AS LL, | |
SUM(CASE WHEN role = 'LW' THEN 1 ELSE 0 END) > 0 AS LW, | |
SUM(CASE WHEN role = 'FL' THEN 1 ELSE 0 END) > 0 AS FL, | |
SUM(CASE WHEN role = 'HW' THEN 1 ELSE 0 END) > 0 AS HW, | |
SUM(CASE WHEN role = 'HL' THEN 1 ELSE 0 END) > 0 AS HL, | |
SUM(CASE WHEN role = 'CH' THEN 1 ELSE 0 END) > 0 AS CH, | |
SUM(CASE WHEN role = 'DH' THEN 1 ELSE 0 END) > 0 AS DH | |
FROM records | |
{condition} | |
GROUP BY game_id) | |
SELECT SUM(Games) as Games, | |
SUM(FW) AS FW, | |
SUM(LL) AS LL, | |
SUM(LW) AS LW, | |
SUM(FL) AS FL, | |
SUM(HW) AS HW, | |
SUM(HL) AS HL, | |
SUM(CH) AS CH, | |
SUM(DH) AS DH | |
FROM roles_in_games; | |
""" | |
result = await fetch_one(query, []) | |
async def fetch_result_distribution(playroom_id=None): | |
# returns | |
if playroom_id is None: | |
condition = '' | |
else: | |
condition = 'WHERE playroom_id = ?' | |
query = f"""WITH roles_in_games(Games, FW, LL, LW, FL, HW, HL, CH, DH) AS | |
(SELECT 1 AS Games, | |
SUM(CASE WHEN role = 'FW' THEN 1 ELSE 0 END) > 0 AS FW, | |
SUM(CASE WHEN role = 'LL' THEN 1 ELSE 0 END) > 0 AS LL, | |
SUM(CASE WHEN role = 'LW' THEN 1 ELSE 0 END) > 0 AS LW, | |
SUM(CASE WHEN role = 'FL' THEN 1 ELSE 0 END) > 0 AS FL, | |
SUM(CASE WHEN role = 'HW' THEN 1 ELSE 0 END) > 0 AS HW, | |
SUM(CASE WHEN role = 'HL' THEN 1 ELSE 0 END) > 0 AS HL, | |
SUM(CASE WHEN role = 'CH' THEN 1 ELSE 0 END) > 0 AS CH, | |
SUM(CASE WHEN role = 'DH' THEN 1 ELSE 0 END) > 0 AS DH | |
FROM records | |
{condition} | |
GROUP BY game_id) | |
SELECT SUM(Games) as Games, | |
SUM(FW) AS FW, | |
SUM(LL) AS LL, | |
SUM(LW) AS LW, | |
SUM(FL) AS FL, | |
SUM(HW) AS HW, | |
SUM(HL) AS HL, | |
SUM(CH) AS CH, | |
SUM(DH) AS DH | |
FROM roles_in_games; | |
""" | |
result = await fetch_one(query, [playroom_id] if playroom_id is not None else []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Монструозно выглядит запрос. Много кода переиспользуется. Может его можно как-то отрефакторить?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Попробуй у чатгпт спросить, как можно улучшить запрос
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Kopylov, рассмотрев запрос, я вижу, что основная проблема заключается в повторяющемся шаблоне для каждой роли. Один из способов упростить запрос - использовать динамический SQL для генерации частей запроса, связанных с ролями, автоматически. Однако, это может увеличить сложность кода и потребовать дополнительной обработки на стороне Python для безопасного создания таких запросов.
В качестве альтернативы, можно рассмотреть возможность нормализации структуры базы данных, чтобы уменьшить необходимость в таких сложных запросах. Например, создание отдельной таблицы для ролей с ссылками на игры может упростить агрегацию данных и сделать запросы более читаемыми.
Если структура базы данных не может быть изменена, другой подход может заключаться в создании вспомогательной функции на стороне Python, которая будет генерировать части SQL-запроса для каждой роли, чтобы избежать дублирования кода в самом запросе.
Важно отметить, что любые изменения в структуре запросов или базы данных должны быть тщательно протестированы, чтобы убедиться, что они не влияют на производительность или точность результатов.
К сожалению, без доступа к схеме базы данных и полному контексту использования этого запроса, мои предложения остаются общими. Если у вас есть возможность предоставить больше информации о структуре базы данных и о том, как используются данные, я смогу предложить более конкретные рекомендации.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alex-Kopylov, для улучшения запроса в функции fetch_result_distribution
, можно рассмотреть несколько подходов:
-
Использование динамического SQL: Это позволит автоматически генерировать части запроса для каждой роли, уменьшая дублирование кода. Однако, это может увеличить сложность и требует тщательного управления параметрами для предотвращения SQL-инъекций.
-
Нормализация базы данных: Пересмотр структуры базы данных может помочь упростить запросы. Например, создание отдельной таблицы для ролей с ссылками на игры упростит агрегацию данных.
-
Вспомогательные функции на стороне приложения: Можно создать функции на Python, которые генерируют SQL-запросы или их части на основе параметров. Это поможет избежать дублирования кода в самом запросе и улучшить его читаемость.
-
Использование представлений (Views): Если определенные агрегации данных используются регулярно, можно создать представление в базе данных, которое инкапсулирует сложную логику запроса. Это позволит упростить основные запросы, ссылаясь на представление.
-
Индексация: Убедитесь, что поля, используемые в условиях
WHERE
,JOIN
и агрегациях, индексированы. Это может значительно улучшить производительность запроса.
Важно отметить, что выбор подхода зависит от конкретной структуры базы данных и требований приложения. Рекомендуется тщательно тестировать любые изменения на предмет производительности и точности результатов. Если возможно, предоставьте больше контекста о структуре базы данных и требованиях к данным, чтобы получить более конкретные рекомендации.
…omments: add arguments to draw_graphs.py draw_user_winrate; Add try-catch construction to draw_graphs.py draw_user_winrate and handlers/mystats.py mystats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- src/handlers/mystats.py (1 hunks)
- src/services/draw_graphs.py (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/handlers/mystats.py
- src/services/draw_graphs.py
Add mystats handler: drawing personal statistics
Summary by CodeRabbit
.gitignore
to exclude specific file patterns.pandas
,matplotlib
,scikit-image
).