Skip to content

Conversation

RasmusWL
Copy link
Member

@RasmusWL RasmusWL commented Apr 7, 2022

Besides just adding new tests, I also fixed up the modeling, and did a small bit of refactoring. We want to promote this query at some point anyway, so could just as well do it now 🤷

@RasmusWL RasmusWL requested a review from a team as a code owner April 7, 2022 14:34
@RasmusWL RasmusWL added the no-change-note-required This PR does not need a change note label Apr 7, 2022
@github-actions github-actions bot added the Python label Apr 7, 2022
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

This looks good to me! 👍

Only one minor comment (that does not need to be addressed before merging).

Comment on lines 52 to 55
exists(SubscriptNode subscript |
subscript.getObject() = mongoInstance().getAUse().asCfgNode() and
subscript.getObject() = mongoClientInstance().getAUse().asCfgNode() and
result.asCfgNode() = subscript
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be written as

result.asCfgNode() = any(SubscriptNode subscript | subscript.getObject() = mongoClientInstance().getAUse().asCfgNode())

though it's debatable whether this is an improvement.

Also, we really ought to add support for subscripting in the API graph (as a call to __getitem__). It really bothers me that we have to resort to type tracking here. 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, we really ought to add support for subscripting in the API graph (as a call to getitem). It really bothers me that we have to resort to type tracking here. cry

Yes, that would be good 👍

@RasmusWL RasmusWL merged commit 2421076 into github:main May 10, 2022
@RasmusWL RasmusWL deleted the new-nosql-examples branch May 10, 2022 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants