python: Document MaD format #16789

yoff · 2024-06-19T15:01:57Z

Adds documentation for the Python Models as Data format. The structure is heavily based on the JavaScript documentation. Both because that document is quite nice and because it will be easier for users to navigate if the structure is similar.

In addition, this PR

adds a few tests reflecting the documentation
make the mentioned sink-kinds have an effect on relevant queries

- add a few tests reflecting the documentation - make the mentioned sink-kinds have an effect on relevant queries

subatoi

Looks good docs-wise, thank you! Only some really minor comments

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

subatoi

🚀

tausbn

Really good stuff! I have made a bunch of comments/suggestions, but overall I think this is really solid. 👍

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

tausbn · 2024-06-21T11:23:08Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+      data:
+        - ["invoke.Context", "invoke", "Member[context].Member[Context].Instance"]
+
+- The first column, **"invoke.Context"**, is the name of the type to reach.


Suggested change

- The first column, **"invoke.Context"**, is the name of the type to reach.

- The first column, **"invoke.Context"**, is the name of the type that we're defining.

I found "type to reach" to be very vague and confusing. Perhaps this is a better formulation?

I do not feel that we are defining this type, though. In cases where we make up a name, say to model some internal concept, I would say we are defining a type. But here, we are simply describing how one can obtain (an instance of) the type. Given that this can be used in various contexts, I actually like that it is a bit vague.

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

tausbn · 2024-06-21T11:26:46Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+      data:
+        - [
+            "django.db.models.FileField!",
+            "Call.Argument[upload_to:].Parameter[1,filenam:]",


Suggested change

"Call.Argument[upload_to:].Parameter[1,filenam:]",

"Call.Argument[upload_to:].Parameter[1,filename:]",

How do we know that the argument is named filename, though? This depends on how the user defined the helper function, no?

Yes, but that convention is documented (here, link is also included in our docs). Given the way the documentation is written, I would expect many people to call the parameter 'filename' and I think a good model should recognise that parameter.

I was also slightly confused about this part. I think it makes more sense to only have Parameter[1] -- no matter the naming convention, that should still cover everything we care to model.

no matter the naming convention, that should still cover everything we care to model.

That may be (i.e. if people define the call-back with name-only parameters, we do not care about that case). Still, I think the example is more illustrative and usable as a reference the way it is now.

That may be (i.e. if people define the call-back with name-only parameters, we do not care about that case).

if they define it with keyword only parameters, it wouldn't work, since the arguments are passed positionally (src from django)

Still, I think the example is more illustrative and usable as a reference the way it is now.

I agree it's pretty cool if we had an example where we could show off Parameter[<int>,<keyword>:]. However, I don't find this to be a convincing example since the filename: part doesn't add any value. I fear that people reading this will get confused and think they ALWAYS NEED to add the name of the parameter, but maybe I'm reading too much into it.

if they define it with keyword only parameters, it wouldn't work, since the arguments are passed positionally (src from django)

Hm, in that case it should be removed, thanks for digging that up.

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

tausbn · 2024-06-21T11:34:26Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+
+
+- Since we're adding flow through a function call, we add a tuple to the **summaryModel** extensible predicate.
+- The first column, **"builtins"**, begins the search for relevant calls at references to the **builtin** package.


... And also otherwise undefined globals with specific names. Perhaps it would be better to not mention the builtins package explicitly, and just say it finds references to built-in names? (Which includes builtins.whatever.)

Yes, I like that.

Hm, in the end it became somewhat different...see if you like.

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

Co-authored-by: Taus <tausbn@github.com>

- fix wording on `builtins` - add named argument/parameter access path components

RasmusWL

Overall LGTM 👍

Minor detail around code snippets. I've made one suggestion, but you will need to apply it throughout the document

RasmusWL · 2024-06-24T09:45:14Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+In this example, we'll show how to add flow through calls to `re.compile`.
+`re.compile` returns a compiled regular expression for efficient evaluation, but the pattern to be compiled is stored in the `pattern` attribute of the resulting object.


While reading through the rendered version, it became clear to me that reStructured text requires double backticks for code snippets (docs).

Suggested change

In this example, we'll show how to add flow through calls to `re.compile`.

`re.compile` returns a compiled regular expression for efficient evaluation, but the pattern to be compiled is stored in the `pattern` attribute of the resulting object.

In this example, we'll show how to add flow through calls to ``re.compile``.

``re.compile`` returns a compiled regular expression for efficient evaluation, but the pattern to be compiled is stored in the ``pattern`` attribute of the resulting object.

Thanks, it should be replaced everywhere now.

Not quite what I asked for.

If you look at the rendered version (before, after) the change looks like:

Before

After

However, both of them still only use single backticks, meaning the code snippets are not rendered properly.

The previous version looked better to me, so I would recommend reverting cd7031a and 9d62c21 (the double-qoutes in the examples made them easier to read for me)

Not quite what I asked for.

Indeed, I realised once I started looking at the generated artifact 🤦 I should have commented then, so you did not have to pull it out. Thanks for doing that, though. I will get the double back-tics in, and revert to single quotes in headers to align with the other documentation pages (e.g. the one for javascript)). About the double quotes, I removed them on request, but that was mainly a request for consistency, so perhaps they should be in in a consistent way...

I will get the double back-tics in, and revert to single quotes in headers

👍

About the double quotes, I removed them on request, but that was mainly a request for consistency, so perhaps they should be in in a consistent way...

Ah, I see they are present in the JS docs as well, so 🤷

RasmusWL · 2024-06-25T08:28:03Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+In this example, we'll show how to add flow through calls to `re.compile`.
+`re.compile` returns a compiled regular expression for efficient evaluation, but the pattern to be compiled is stored in the `pattern` attribute of the resulting object.


Not quite what I asked for.

If you look at the rendered version (before, after) the change looks like:

Before

After

However, both of them still only use single backticks, meaning the code snippets are not rendered properly.

The previous version looked better to me, so I would recommend reverting cd7031a and 9d62c21 (the double-qoutes in the examples made them easier to read for me)

RasmusWL · 2024-06-25T08:37:39Z

docs/codeql/codeql-language-guides/customizing-library-models-for-python.rst

+      data:
+        - [
+            "django.db.models.FileField!",
+            "Call.Argument[upload_to:].Parameter[1,filenam:]",


That may be (i.e. if people define the call-back with name-only parameters, we do not care about that case).

if they define it with keyword only parameters, it wouldn't work, since the arguments are passed positionally (src from django)

Still, I think the example is more illustrative and usable as a reference the way it is now.

I agree it's pretty cool if we had an example where we could show off Parameter[<int>,<keyword>:]. However, I don't find this to be a convincing example since the filename: part doesn't add any value. I fear that people reading this will get confused and think they ALWAYS NEED to add the name of the parameter, but maybe I'm reading too much into it.

yoff · 2024-06-25T10:14:07Z

Alright, my quote edits had made a mess. I did a small rebase, things should look reasonable now. I am looking forward to seeing the generated artifacts... :-)

RasmusWL

👍 LGTM

tausbn

One small typo fix in the change note, otherwise this looks good to me! 👍

tausbn · 2024-06-25T12:31:52Z

python/ql/lib/change-notes/2024-06-24-Data-extensions-supported-by-queries-and-documented.md

+---
+category: minorAnalysis
+---
+* A number of Python queries now support sinks defined vi data extensions. The format of data extensions for Python has been documented.


Suggested change

* A number of Python queries now support sinks defined vi data extensions. The format of data extensions for Python has been documented.

* A number of Python queries now support sinks defined using data extensions. The format of data extensions for Python has been documented.

Typo, but also maybe a more plain word here will suffice.

Oops, I just saw the approval and not this comment..

python: Document MaD format

5cb37f5

- add a few tests reflecting the documentation - make the mentioned sink-kinds have an effect on relevant queries

yoff requested a review from a team as a code owner June 19, 2024 15:01

github-actions bot added documentation Python labels Jun 19, 2024

sidshank added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jun 19, 2024

subatoi previously approved these changes Jun 20, 2024

View reviewed changes

yoff dismissed subatoi’s stale review via b4fdf3c June 20, 2024 08:57

Apply suggestions from code review

b4fdf3c

Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com>

yoff requested a review from subatoi June 20, 2024 08:58

Python: autoformat

f0e6888

subatoi previously approved these changes Jun 20, 2024

View reviewed changes

Python: include new documentation

a7386b6

yoff dismissed subatoi’s stale review via a7386b6 June 20, 2024 09:25

tausbn requested changes Jun 21, 2024

View reviewed changes

yoff and others added 4 commits June 24, 2024 10:29

Apply suggestions from code review

d603b48

Co-authored-by: Taus <tausbn@github.com>

Python: address reviewer comments

21a0f8a

- fix wording on `builtins` - add named argument/parameter access path components

Python: recognize fabric.operations

00fbada

Python: update doc to use operations module

4626e13

yoff requested a review from tausbn June 24, 2024 08:59

RasmusWL reviewed Jun 24, 2024

View reviewed changes

yoff requested a review from RasmusWL June 24, 2024 10:01

RasmusWL requested changes Jun 25, 2024

View reviewed changes

yoff added 5 commits June 25, 2024 11:54

Python: add change note

b902dd5

Python: compact types in type models

aa4fd19

Python: remove named parameter filename

6d4e993

Python: double back-tics

09905ee

Python: consistent double quotes in examples

6524b8e

yoff force-pushed the python/document-models-as-data branch from 259dbf4 to 6524b8e Compare June 25, 2024 10:12

yoff requested a review from RasmusWL June 25, 2024 10:14

RasmusWL approved these changes Jun 25, 2024

View reviewed changes

tausbn approved these changes Jun 25, 2024

View reviewed changes

yoff merged commit 58b6b3f into github:main Jun 25, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python: Document MaD format #16789

python: Document MaD format #16789

yoff commented Jun 19, 2024 •

edited by sidshank

Loading

subatoi left a comment

subatoi left a comment

tausbn left a comment

tausbn Jun 21, 2024

yoff Jun 24, 2024

tausbn Jun 21, 2024

yoff Jun 24, 2024

RasmusWL Jun 24, 2024

yoff Jun 24, 2024

RasmusWL Jun 25, 2024

yoff Jun 25, 2024

tausbn Jun 21, 2024 •

edited

Loading

yoff Jun 24, 2024

yoff Jun 24, 2024

RasmusWL left a comment

RasmusWL Jun 24, 2024

yoff Jun 24, 2024

RasmusWL Jun 25, 2024

yoff Jun 25, 2024

RasmusWL Jun 25, 2024

RasmusWL Jun 25, 2024

RasmusWL Jun 25, 2024

yoff commented Jun 25, 2024

RasmusWL left a comment

tausbn left a comment

tausbn Jun 25, 2024

yoff Jun 25, 2024

	- The first column, "invoke.Context", is the name of the type to reach.
	- The first column, "invoke.Context", is the name of the type that we're defining.

	"Call.Argument[upload_to:].Parameter[1,filenam:]",
	"Call.Argument[upload_to:].Parameter[1,filename:]",



		- Since we're adding flow through a function call, we add a tuple to the summaryModel extensible predicate.
		- The first column, "builtins", begins the search for relevant calls at references to the builtin package.

		In this example, we'll show how to add flow through calls to `re.compile`.
		`re.compile` returns a compiled regular expression for efficient evaluation, but the pattern to be compiled is stored in the `pattern` attribute of the resulting object.

	* A number of Python queries now support sinks defined vi data extensions. The format of data extensions for Python has been documented.
	* A number of Python queries now support sinks defined using data extensions. The format of data extensions for Python has been documented.

python: Document MaD format #16789

python: Document MaD format #16789

Conversation

yoff commented Jun 19, 2024 • edited by sidshank Loading

subatoi left a comment

Choose a reason for hiding this comment

subatoi left a comment

Choose a reason for hiding this comment

tausbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tausbn Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Before

After

Choose a reason for hiding this comment

yoff commented Jun 25, 2024

RasmusWL left a comment

Choose a reason for hiding this comment

tausbn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoff commented Jun 19, 2024 •

edited by sidshank

Loading

tausbn Jun 21, 2024 •

edited

Loading