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

加入mongodb审核,重写mongodb查询等 #900

Merged
merged 42 commits into from
Oct 13, 2020

Conversation

fancy-lee
Copy link
Contributor

@fancy-lee fancy-lee commented Sep 30, 2020

1.加入mongodb审核,
2.重写mongodb查询支持原生查询语法
3.查询编辑框加入缓存,避免刷新丢失
4.mongo审核时关闭备份下拉框等
5.优化mongodb查询显示效果

Copy link
Collaborator

@LeoQuote LeoQuote left a comment

Choose a reason for hiding this comment

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

感谢提交pr

  1. 单元测试一定要过

其他的事情看评论,另外不要重提pr,请直接在pr分支上提交,如果新开PR我就不再参与了。

sql/engines/mongo.py Outdated Show resolved Hide resolved
sql/engines/mysql.py Outdated Show resolved Hide resolved
sql/templates/sqlsubmit.html Outdated Show resolved Hide resolved
sql/engines/mongo.py Show resolved Hide resolved
sql/engines/mongo.py Show resolved Hide resolved
@LeoQuote
Copy link
Collaborator

说实话这是我们第一个有自己的审核机制的engine,mysql的是调外部审查的,关于这些希望还是保持和之前一样有配套的单元测试,不然后续没人敢改,不好维护的。

@fancy-lee
Copy link
Contributor Author

说实话这是我们第一个有自己的审核机制的engine,mysql的是调外部审查的,关于这些希望还是保持和之前一样有配套的单元测试,不然后续没人敢改,不好维护的。

我把mysql的部分修改回来吧,第一次提pr不太熟悉流程,请多指教

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 1, 2020

不只是mysql的事情,mongo 的engine也是有单元测试的,mongo 的测试也得过

@fancy-lee
Copy link
Contributor Author

不只是mysql的事情,mongo 的engine也是有单元测试的,mongo 的测试也得过
请问Codacy/PR Quality Review 中的提示都得通过吗?Travis CI 测试mongo这块没有通过,但没有看出来问题,请帮忙看一下

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 2, 2020

checks 里travis 里可以点 this build 链接,然后你就可以看到日志

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 2, 2020

我们的engine相关的测试除了mysql外,应该都是patch掉具体的调用,然后看内部逻辑的 #42 参考这个

@codecov
Copy link

codecov bot commented Oct 2, 2020

Codecov Report

Merging #900 into master will decrease coverage by 0.96%.
The diff coverage is 49.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
- Coverage   78.27%   77.31%   -0.97%     
==========================================
  Files          77       76       -1     
  Lines       11030    11503     +473     
==========================================
+ Hits         8634     8893     +259     
- Misses       2396     2610     +214     
Impacted Files Coverage Δ
sql/query.py 70.74% <ø> (ø)
sql/engines/mongo.py 47.70% <46.26%> (+9.32%) ⬆️
sql/engines/tests.py 99.60% <100.00%> (+0.01%) ⬆️
sql/utils/human_time.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20462a1...d98804e. Read the comment docs.

@fancy-lee
Copy link
Contributor Author

我们的engine相关的测试除了mysql外,应该都是patch掉具体的调用,然后看内部逻辑的 #42 参考这个

请问Codacy/PR Quality Review中所有的问题都要修改吗?新加的每个方法都要写单元测试?

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 2, 2020

👏 恭喜测试全过了,你可以看到 codecov 的 覆盖率报告,里面可以看到哪些覆盖到哪些没覆盖,没有覆盖到的方法建议都写一下测试。

quality 的报告可以做参考,主要是风格方面的,也建议遵照改一下。

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 6, 2020

一是上面的提到的事情, 建议都看一下.
另外建议按照engine 设计的来做, 也就是 engine 中的 query_check 方法用来做语句的初检, 如果此时语句为空, 可以返回 bad check , 并且报错, 这部分是有现成的前端返回的, 不需要你在几乎所有的方法里都要校验语句是否为空, 即使校验也是在 query_check 里校验

Archery/sql/query.py

Lines 58 to 63 in 20462a1

query_check_info = query_engine.query_check(db_name=db_name, sql=sql_content)
if query_check_info.get('bad_query'):
# 引擎内部判断为 bad_query
result['status'] = 1
result['msg'] = query_check_info.get('msg')
return HttpResponse(json.dumps(result), content_type='application/json')

然后用 filter_sql 做查询时的语句改写

sql_content = query_engine.filter_sql(sql=sql_content, limit_num=limit_num)

return self.error_info


class JsonDecoder:
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个 decoder , 如果有现成的用现成的, 没有现成的所有方法尽量都加上测试. 不然除了作者基本不敢动

Copy link
Contributor Author

Choose a reason for hiding this comment

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

目前还没有找到现成好用的decoder,这个类可以单独拿出来做一下测试的,我这边本地的测试环境配置有点问题,辛苦你加一下?

Copy link
Collaborator

Choose a reason for hiding this comment

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

我没有办法帮你加测试环境呀,你得自己看是什么问题然后解决

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我没有办法帮你加测试环境呀,你得自己看是什么问题然后解决

它只是一个独立的工具类,它的主要工功能是对语句中db.col.find({"id":123})的{}部分解析成dict,它可以单独拿出来做测试和修改,应该不会像你说的"除了作者不敢动”这么严重,因为我本地测试环境有问题,只能push上来做测试,所以希望你帮忙加一下测试

sql/query.py Show resolved Hide resolved
fancy-lee and others added 2 commits October 6, 2020 21:36
Co-authored-by: Leo Q <LeoQuote@users.noreply.github.com>
@fancy-lee
Copy link
Contributor Author

一是上面的提到的事情, 建议都看一下.
另外建议按照engine 设计的来做, 也就是 engine 中的 query_check 方法用来做语句的初检, 如果此时语句为空, 可以返回 bad check , 并且报错, 这部分是有现成的前端返回的, 不需要你在几乎所有的方法里都要校验语句是否为空, 即使校验也是在 query_check 里校验

Archery/sql/query.py

Lines 58 to 63 in 20462a1

query_check_info = query_engine.query_check(db_name=db_name, sql=sql_content)
if query_check_info.get('bad_query'):
# 引擎内部判断为 bad_query
result['status'] = 1
result['msg'] = query_check_info.get('msg')
return HttpResponse(json.dumps(result), content_type='application/json')

然后用 filter_sql 做查询时的语句改写

sql_content = query_engine.filter_sql(sql=sql_content, limit_num=limit_num)

我把语句为空的这个校验去了

Copy link
Collaborator

@LeoQuote LeoQuote left a comment

Choose a reason for hiding this comment

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

就这一个地方改了就可以m了,单元测试什么的没条件后续再补吧

如果新的审核需要安装mongo client ,也需要改一下 dockerfile

self.db_name = self.db_name or 'admin'
conn = pymongo.MongoClient(self.host, self.port, authSource=self.db_name, connect=True, connectTimeoutMS=10000)
self.db_name = db_name or 'admin' #=======这里要注意一下
self.db_name = 'admin'
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个按照讨论改一下吧

@fancy-lee
Copy link
Contributor Author

@LeoQuote
Copy link
Collaborator

LeoQuote commented Oct 9, 2020

https://github.com/fancy-lee/Archery/blob/archery-fancy/src/docker/Dockerfile-base 帮忙改一下 dockerfile 吧, 在最后面加上你说的那些步骤就可以了.

@fancy-lee
Copy link
Contributor Author

https://github.com/fancy-lee/Archery/blob/archery-fancy/src/docker/Dockerfile-base 帮忙改一下 dockerfile 吧, 在最后面加上你说的那些步骤就可以了.

已经加完了

Copy link
Collaborator

@LeoQuote LeoQuote left a comment

Choose a reason for hiding this comment

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

👏 LGTM

@LeoQuote LeoQuote requested a review from hhyo October 12, 2020 06:26
@hhyo
Copy link
Owner

hhyo commented Oct 12, 2020

感谢两位

@LeoQuote LeoQuote merged commit 3343a70 into hhyo:master Oct 13, 2020
@@ -125,6 +127,10 @@
<input id="btn-submitsql" type="button" class="btn btn-success" value="SQL提交"/>
</div>
</div>
<div class="text-info">
<li>提交的语句请以分号结束</li>
<li>[指定执行时间]用于定时任务,不是必选项</li>
Copy link
Owner

Choose a reason for hiding this comment

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

执行时间可能理解有误,这个并不是用于定时,是用于错峰执行

@@ -266,6 +272,7 @@ <h4 class="modal-title text-danger">提交信息确认</h4>
$("#instance_name").change(function () {
var optgroup = $('#instance_name :selected').parent().attr('label');
$("#div-backup").show();
if (optgroup != "MySQL") {$("#div-backup").hide(); $("#is_backup").val("False");}
Copy link
Owner

Choose a reason for hiding this comment

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

目前Oracle也支持备份,这块后面我处理一下

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants