Skip to content

Python: Model RemoteFlowSources on Django forms/fields #5500

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

Merged
merged 5 commits into from
Mar 25, 2021

Conversation

RasmusWL
Copy link
Member

No description provided.

I'm not feeling 100% confident about `SelfRefMixin`, but since I needed it for
both DjangoViewClass and DjangoFormClass, I wanted to avoid copy-pasting this
code around. However, I'm not so opitimistic about it that I want to add it to a
sharable utility qll file :D
I used some ad-hoc QL queries to help me find all these extra instances, but not
quite ready to share that code yet :P
yoff
yoff previously approved these changes Mar 24, 2021
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM, it seems that the mix-in is not making things worse, are you worried about performance?

exists(DjangoFormFieldClass cls, Function meth |
cls.getAMethod() = meth and
meth.getName() in ["to_python", "validate", "run_validators", "clean"] and
this.getParameter() = meth.getArg(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We have got to fix the naming here, at first reading I thought we needed flow. But this is just stating that the class we define is indeed the 1th parameter of the method.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yoff can you come with a suggestion then? I'm not quite sure I understand the problem 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to change the name of getArg since it gets a parameter. It is out of scope for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

ahh yes, that thing 😄

@RasmusWL
Copy link
Member Author

LGTM, it seems that the mix-in is not making things worse, are you worried about performance?

Not worried about performance. Just that this is probably the first use of Mixin withing CodeQL... and that the fact that we need this helper class multiple times maybe highlights that we need something more structured to make this easy in general.

Co-authored-by: yoff <lerchedahl@gmail.com>
@tausbn
Copy link
Contributor

tausbn commented Mar 24, 2021

I think we should be worried about performance here, as tracking all self references of all classes is absolutely massive. Before we merge this, I would like to see a performance test, ideally including known "problematic" databases like g/zatosource/zato.

@RasmusWL
Copy link
Member Author

RasmusWL commented Mar 24, 2021

I think we should be worried about performance here, as tracking all self references of all classes is absolutely massive. Before we merge this, I would like to see a performance test, ideally including known "problematic" databases like g/zatosource/zato.

Luckily, we will only track self references for a small subset of classes (notice that DjangoViewClass is also abstract) -- but happy to do a performance test for this as well 👍

EDIT: started https://jenkins.internal.semmle.com/job/Changes/job/Python-Differences/479/

@tausbn
Copy link
Contributor

tausbn commented Mar 24, 2021

Ah, that's a good point. I'm less worried now. 😌

@RasmusWL
Copy link
Member Author

Performance test is done, and all is looking good ✔️

@RasmusWL RasmusWL requested a review from yoff March 25, 2021 15:07
Copy link
Contributor

@yoff yoff left a comment

Choose a reason for hiding this comment

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

LGTM

@yoff yoff merged commit 208d515 into github:main Mar 25, 2021
@RasmusWL RasmusWL deleted the django-forms branch March 26, 2021 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants