Skip to content

Retrieve external API usage snippets using SARIF#2457

Merged
koesie10 merged 8 commits intomainfrom
koesie10/auto-model-usages-sarif
Jun 1, 2023
Merged

Retrieve external API usage snippets using SARIF#2457
koesie10 merged 8 commits intomainfrom
koesie10/auto-model-usages-sarif

Conversation

@koesie10
Copy link
Copy Markdown
Member

This implements the retrieval of external API usage snippets using a SARIF file.

The alternative is retrieving the usage manually given a file and a range within this file. However, this is quite hard to do and would create a separate implementation from the one already present in the CodeQL CLI.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 force-pushed the koesie10/auto-model-usages-sarif branch from 1d36fe3 to ec3fabe Compare May 30, 2023 10:57
@koesie10 koesie10 marked this pull request as ready for review May 30, 2023 11:46
@koesie10 koesie10 force-pushed the koesie10/auto-model-usages-sarif branch from ec3fabe to 5c81671 Compare May 30, 2023 11:46
@koesie10 koesie10 requested review from a team as code owners May 30, 2023 11:46
@koesie10 koesie10 requested review from charisk and starcke May 30, 2023 11:46
Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. But probably also good to get a second review from Charis as it is touching some existing code.

Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model-usages-query.ts Outdated
where
apiName = api.getApiName() and
usage = aUsage(api)
select usage, apiName
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am ok to do it like this for now. But it feels like we should be able to reuse the previous query as it selects all the same data.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I found a way to use a single query for this, so I'll change it to use a single query for both retrieving the external APIs and for retrieving the usages.

Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! The approach looks good to me but I'm worried about the extensive use of Pick/Omit, specially for code outside of the data extensions editor/auto-model. Can we avoid these changes?

Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model-usages-query.ts Outdated
Comment thread extensions/ql-vscode/src/databases/local-databases/database-item-impl.ts Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model-usages-query.ts Outdated
Comment thread extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts Outdated
@koesie10 koesie10 force-pushed the koesie10/auto-model-usages-sarif branch from 8aba87b to c017530 Compare May 31, 2023 13:21
@koesie10 koesie10 requested review from charisk and starcke May 31, 2023 14:32
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

LGTM. I'll defer the CodeQL query changes to @starcke.

Comment thread extensions/ql-vscode/src/data-extensions-editor/auto-model-usages-query.ts Outdated

interface BqrsColumn {
name: string;
name?: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry this is probably a stupid question, but why would a column not have a name?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is dependent on the query. In the query we're now using, the BQRS column definitions look like this:

[
  { name: "usage", kind: "Entity" },
  { name: "apiName", kind: "String" },
  { kind: "String" },
  { kind: "String" },
]

CodeQL is probably not naming the last two columns because it can't automatically determine a name based on the name of a variable:

usage, apiName, supported.toString(), "supported"

Copy link
Copy Markdown
Contributor

@starcke starcke left a comment

Choose a reason for hiding this comment

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

Query looks good to me. It is similar to the approach the CodeML queries have.

Co-authored-by: Charis Kyriakou <charisk@users.noreply.github.com>
@koesie10 koesie10 enabled auto-merge June 1, 2023 08:49
@koesie10 koesie10 merged commit 2f61cfe into main Jun 1, 2023
@koesie10 koesie10 deleted the koesie10/auto-model-usages-sarif branch June 1, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants