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

Redis库列表显示保留有数据的,并加上key的数量 #2705

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

Conversation

feiazifeiazi
Copy link
Contributor

@feiazifeiazi feiazifeiazi commented Jun 27, 2024

功能需求见:#2608

  1. 变更基类方法 get_all_databases,加上参数。
  2. 当请求来源是 SQL查询时,Redis库列表只返回有数据的。 SQL上线的地方不变。

效果图:
image

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 78.37838% with 8 lines in your changes missing coverage. Please review.

Project coverage is 77.44%. Comparing base (9dbaf0b) to head (987e4e3).

Files Patch % Lines
sql/instance.py 0.00% 4 Missing ⚠️
sql/engines/redis.py 86.36% 3 Missing ⚠️
sql_api/api_instance.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2705   +/-   ##
=======================================
  Coverage   77.43%   77.44%           
=======================================
  Files         119      119           
  Lines       16359    16394   +35     
=======================================
+ Hits        12668    12696   +28     
- Misses       3691     3698    +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@feiazifeiazi feiazifeiazi changed the title Redis库列表显示上加上key的数量 Redis库列表显示保留有数据的DB列表,并加上key的数量 Jun 27, 2024
@feiazifeiazi feiazifeiazi changed the title Redis库列表显示保留有数据的DB列表,并加上key的数量 Redis库列表显示保留有数据的,并加上key的数量 Jun 27, 2024
@@ -313,6 +313,8 @@ def instance_resource(request):
db_name = request.GET.get("db_name", "")
schema_name = request.GET.get("schema_name", "")
tb_name = request.GET.get("tb_name", "")
# request_source: 请求来源。1. SQL查询。 2.SQL提交
request_source = request.GET.get("request_source", "")
Copy link
Collaborator

@LeoQuote LeoQuote Jun 27, 2024

Choose a reason for hiding this comment

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

整体功能确实是一个非常实用的功能, 但这里我不确认这个参数是不是很合理, 有没有可能这样处理:

  1. 返回时全部都返回, 不管是谁请求的, 什么用途
  2. 前端在查询时, 如果是 redis, 遍历其中的内容, 挑选出里面有数据的表, 其他剔除

那这么设计的话, engine 层面就也不需要加参数, 尽力地去获取所有db 的 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.

你说的这种方式应该可以。我要确认一下SQL提交那边的代码 看看要不要改,有没有影响。

Copy link
Contributor Author

@feiazifeiazi feiazifeiazi Jul 1, 2024

Choose a reason for hiding this comment

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

从扩展来说,不同的数据库,可能需要传入不同的参数的。
应该是将post的参数,变成key,value,全部传给方法。这样子方法就自由了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

我没太看懂你的意思,你能展开说一下吗?比如api 设计,后端大致逻辑

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我应该先抛出问题:
get_all_databases() 父类这个方法,从设计上是不是要传参数才好呢?具体怎么传再议。

原因:这个方法现在父类没有参数,
各种engine层在实现这个方法时,因为没有前端的get或post参数,从而限制了子类的这个方法的实现,不够自由。

Copy link
Collaborator

Choose a reason for hiding this comment

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

从名字来看,我个人觉得是不应该加参数的,因为名字就是获取所有的db,没有任何条件。

不过我觉得还是从上层用法来看比较合适,如果说各个engine定制很多,那加参数是必须要的,如果名字不贴切可以再改名字。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

恩。OK
我先试试在前端筛选。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeoQuote 调用/instance/instance_resource/的地方太多,不好改虽然那里不可能选Redis实例,但得要一个个测。 主要是返回结果改了:之前返回一列数据。现在返回了2列(一个key,一个显示值。)
我先不改了。感觉还是支持参数比较好,方便扩展。

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