Add test-generator script + add generated models for Spring summary steps#6030
Add test-generator script + add generated models for Spring summary steps#6030aschackmull merged 32 commits intogithub:mainfrom
Conversation
| } | ||
|
|
||
| private SummaryComponent interpretComponent(string c) { | ||
| SummaryComponent interpretComponent(string c) { |
There was a problem hiding this comment.
I don't think the minor use of this predicate warrants making it public. The only added reference seems like something that could be written without it.
|
|
||
| private class SummarizedCallableExternal extends SummarizedCallable { | ||
| SummarizedCallableExternal() { summaryElement(this, _, _, _) } | ||
| class SummarizedCallableExternal extends SummarizedCallable { |
There was a problem hiding this comment.
Making use of SummarizedCallableExternal is potentially problematic as that means we only generate tests for those rows that translate without problems. It there's a typo in a signature, or method name, or anything like that then we don't generate any test case, so we fail to catch the error.
There was a problem hiding this comment.
I think, for a csv row with a package name matching something we specify when running the generator, we'll want to be able to generate test cases for csv rows that are wrong in ways that cause the test case to not compile.
There was a problem hiding this comment.
The invalid-row problem is addressed by the Python component, which introduces
query string getAFailedRow() {
result = any(GenRow row | not exists(RowTestSnippet r | exists(r.getATestSnippetForRow(row))) | row)
}
This spots rows that didn't lead to any test generation, e.g. because a SummarizedCallableExternal wasn't matched, or a SummaryComponentStack parser didn't match.
There was a problem hiding this comment.
Gotcha. This code snippet could move into the qll file.
|
To rephrase that a bit, I want to be able to:
How would you be happy to expose those? |
|
Here's what I was thinking:
I think this keeps the API surface to a minimum and should cover:
For the input to the test generation, this can be any sort of filter on the existing rows (i.e. the 9-tuples). Using the complete row as a single string is fine and can be matched by string concatenation.
Not sure what you need this for. |
Creating the annotations that go in the test |
Right, but using |
72ba5b9 to
134f564
Compare
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1622,181,13,6,6,,33,1,58
+ Totals,,84,1665,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1622,181,13,6,6,,33,1,58
+ Totals,,84,1665,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
|
@aschackmull I've reduced the churn in ExternalFlow.qll etc by implementing the mapping back from Content to an appropriate model string myself, and by exposing On exposing the
You mentioned above there was a problem relating to future use of an external predicate to supply source/sink/summary models, but surely even in that world we're likely to extract and parse rows from that external source, and so continue to have some appropriate |
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1622,181,13,6,6,,33,1,58
+ Totals,,84,1665,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
0b25f2c to
1c47076
Compare
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1637,181,13,6,6,,33,1,58
+ Totals,,84,1680,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1637,181,13,6,6,,33,1,58
+ Totals,,84,1680,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1637,181,13,6,6,,33,1,58
+ Totals,,84,1680,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
csharp/ql/src/semmle/code/csharp/dataflow/internal/FlowSummaryImpl.qll
Outdated
Show resolved
Hide resolved
javaGenerated file changes for java
- `Spring <https://spring.io/>`_,``org.springframework.*``,29,,41,,,,,14,,27
+ `Spring <https://spring.io/>`_,``org.springframework.*``,29,43,41,,,,,14,,27
- Totals,,84,1835,181,13,6,6,,33,1,58
+ Totals,,84,1878,181,13,6,6,,33,1,58
+ org.springframework.cache,,,12,,,,,,,,,,,,,,,12
+ org.springframework.data.repository,,,6,,,,,,,,,,,,,,,6
+ org.springframework.ui,,,25,,,,,,,,,,,,,,,25 |
java/ql/src/semmle/code/java/frameworks/spring/SpringDataRepository.qll
Outdated
Show resolved
Hide resolved
| [ | ||
| "org.springframework.data.repository;CrudRepository;true;findAll;;;MapValue of Argument[-1];Element of ReturnValue;value", | ||
| "org.springframework.data.repository;CrudRepository;true;findAllById;;;MapValue of Argument[-1];Element of ReturnValue;value", | ||
| // note for review; return value is an Option<T>; assuming Element is used. |
There was a problem hiding this comment.
Do we have any models for that? Otherwise this will be a dead end.
Document some, make some private, and delete the needless modules surrounding the spring models.
Type Content has moved into DataFlowUtil
While these are semicolon-delimited, we use CSV as a generic term for delimited values
550548e to
bb5fefa
Compare
|
@aschackmull all comments applied, and demonstration tests dropped from this PR |
| (i = -1 or exists(callable.getParameter(i))) and | ||
| if baseInput = SummaryComponentStack::argument(i) | ||
| then result = "in" | ||
| else ( |
There was a problem hiding this comment.
Superfluous parenthesis (parsing is unambiguous).
| else ( | ||
| if baseOutput = SummaryComponentStack::argument(i) | ||
| then result = "out" | ||
| else ( |
| kind = "value" and result = "// $hasValueFlow" | ||
| or | ||
| kind = "taint" and result = "// $hasTaintFlow" |
There was a problem hiding this comment.
Minor preference:
| kind = "value" and result = "// $hasValueFlow" | |
| or | |
| kind = "taint" and result = "// $hasTaintFlow" | |
| kind = "value" and result = "// $ hasValueFlow" | |
| or | |
| kind = "taint" and result = "// $ hasTaintFlow" |
| not exists(SummaryComponentStack next | next.tail() = prev and next = input.drop(_)) and | ||
| result = baseInput |
There was a problem hiding this comment.
This disjunct does not bind prev.
| /** | ||
| * Returns a call to `source()` wrapped in `newWith` methods as needed according to `input`. | ||
| * For example, if the input specification is `ArrayElement of MapValue of Argument[0]`, this | ||
| * will return `newWithMapValue(newWithArrayElement(source()))`. | ||
| * | ||
| * This requires a slightly awkward walk, out from the element above the root (`Argument[0]` above) | ||
| * climbing up towards the outer `ArrayElement of...`. This is implemented by treating the stack as | ||
| * a circular list, walking it backwards and treating the root as a sentinel indicating we should | ||
| * emit `source()`. | ||
| */ | ||
| string getInput(SummaryComponentStack componentStack) { | ||
| componentStack = input.drop(_) and | ||
| ( | ||
| if componentStack = baseInput | ||
| then result = "source()" | ||
| else | ||
| result = | ||
| "newWith" + contentToken(getContent(componentStack.head())) + "(" + | ||
| this.getInput(nextInput(componentStack)) + ")" | ||
| ) | ||
| } |
There was a problem hiding this comment.
How about simplifying this as follows:
string getInput(SummaryComponentStack stack) {
stack = input and result = "source()"
or
exists(SummaryComponentStack s |
result = "newWith" + contentToken(getContent(s.head())) + "(" + this.getInput(s) + ")" and
stack = s.tail()
)
}
Then the desired result will be this.getInput(baseInput). Then you can also delete the confusing nextInput above.
…ement Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
|
@aschackmull all comments applied |
| * will return `newWithMapValue(newWithArrayElement(source()))`. | ||
| */ | ||
| string getInput(SummaryComponentStack stack) { | ||
| stack = input.drop(_) and |
There was a problem hiding this comment.
This conjunct is superfluous.
This Python + QL script generates tests for flow summary specifications. It is invoked by providing a semicolon-separated value file giving the specifications to generate tests for + a Maven POM file that pulls sufficient dependencies to resolve the relevant methods, and emits a test CodeQL file, java file and (empty) expected file.
This PR also tests the generator by using it to add tests to @sauyon's draft PR, #5895
I imagine this will probably want to be split up. Subcomponents of the PR:
Exposing elements of the CSV parser so I can relate SSV rows to the functions modelled.
The test-generator script itself
The tests generated for #5895
Sauyon's models, altered as proved necessary to generate tests for them
Stubs introduced to support the generated tests (these used
java-autostub, not @joefarebrother's new stub generator)