-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Python: allow class decorators in .getASubclass()
#9567
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
Conversation
A class decorator could change the class definition in any way. In this specific case, it would be better if we allowed the subclass to be found with API graphs still. inspired by https://github.com/django/django/blob/c2250cfb80e27cdf8d098428824da2800a18cadf/tests/auth_tests/test_views.py#L40-L46
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.
I agree with the heuristic in this PR; I would expect decorators to preserve the sub-class relationship often enough to make this over-approximation useful. Are the evaluation results as you expect?
The suggestion is mostly me being curious, the code might well be better as is.
Co-authored-by: yoff <lerchedahl@gmail.com>
Accepting this change also fixed the tests 👍 😮 The problem was this bit of modeling code (in Flask.qll) class FlaskMethodViewClass extends FlaskViewClass {
FlaskMethodViewClass() {
api_node = Views::MethodView::subclassRef() and
this.getParent() = api_node.getAnImmediateUse().asExpr()
} would no longer recognize the @flask_admin.expose_plugview("/flask-class") # $ routeSetup="/flask-class"
@flask_admin.expose_plugview(url="/flask-class/<arg>") # $ routeSetup="/flask-class/<arg>"
class Nested(MethodView):
def get(self, cls, arg="default"): # $ requestHandler routedParameter=arg
assert isinstance(cls, ExampleClass)
ensure_tainted(arg) # $ tainted
ensure_not_tainted(cls)
return "GET: " + arg # $ HttpResponse |
Results from initial DCA run did not look good at all. I'll do a new one now after having accepted your fix, and hopefully that should do better 🤞 |
I wonder if it would be even more correct to say that (for the purposes of the API graph), the results of applying the decorators are not just subclasses of the same superclass, but in fact the very same class. In some sense, decorating a class should not really change the identity of that class (though it may e.g. wrap it in some other object). This might also be needed in order to get tracking of instances through decorators to work correctly. Consider @foo
class Bar:
...
def baz(self):
...
Bar().baz() we want the call above to resolve |
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.
LGTM
Instead of just adding the test example, I thought I would try to fix it. I'm totally up for discussing whether this is the right approach, but personally I think it aligns with our goals.
We don't currently allow decorators for functions, so there will be a little bit of inconsistency...