Skip to content

Resolve model editor queries from CodeQL packs if present#2872

Merged
koesie10 merged 2 commits intomainfrom
koesie10/resolve-queries-from-ql
Sep 28, 2023
Merged

Resolve model editor queries from CodeQL packs if present#2872
koesie10 merged 2 commits intomainfrom
koesie10/resolve-queries-from-ql

Conversation

@koesie10
Copy link
Copy Markdown
Member

This will start using the model editor endpoints queries from the CodeQL packs (codeql/java-queries and codeql/csharp-queries) if we can find the queries in those packs.

There are two cases (example language is Java):

  • In case the queries are present in the codeql/java-queries, we don't need to write our own queries to disk. We still need to create a synthetic query pack so we can pass the queryDir to the query resolver without caring about whether the queries are present in the pack or not.
  • In case the queries are not present in the codeql/java-queries, we need to write our own queries to disk. We will create a synthetic query pack and install its dependencies so it is fully independent and we can simply pass it through when resolving the queries.

These steps together ensure that later steps of the process don't need to keep track of whether the queries
are present in codeql/java-queries or in our own query pack. They just need to resolve the query.

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/resolve-queries-from-ql branch from c33b4d0 to de2b08c Compare September 27, 2023 13:27
@koesie10 koesie10 force-pushed the koesie10/resolve-queries-from-ql branch from de2b08c to e9b67dd Compare September 27, 2023 13:32
@koesie10 koesie10 marked this pull request as ready for review September 27, 2023 13:54
@koesie10 koesie10 requested review from a team as code owners September 27, 2023 13:54
@koesie10 koesie10 requested a review from starcke September 27, 2023 13:54
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.

I think it looks ok, but a bit fiddy to have both paths active at the same time. Hopefully we can get rid of the writing after codeql have upgraded to the new packs.

packsToSearch: string[],
name: string,
constraints: QueryConstraints,
allowNoQueriesFound = false,
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 wonder if it would be neater to have two functions:

function tryResolveQueries(...) {
  const queries = await resolveQueriesFromPacks(...);
  return queries;
}

function resolveQueries(...)
  queries = tryResolveQueries(...)
  if (queries.length > 0) {
    return queries;
  }
   // No queries found. Determine the correct error message for the various scenarios.
  const humanConstraints = [];
...
}

and the caller could then call tryResolveQueries if the error handling is not needed.

Alternatively one could also call resolveQueriesFromPacks directly?

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 think we can call resolveQueriesFromPacks directly, we don't need any of the additional functionality offered by resolveQueries.

Comment thread extensions/ql-vscode/src/local-queries/query-resolver.ts Outdated
Comment thread extensions/ql-vscode/src/model-editor/model-editor-queries.ts
await cliServer.packInstall(queryDir);
// Set up a synthetic pack so CodeQL doesn't crash later when we try
// to resolve a query within this directory
const syntheticQueryPack = {
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.

This is basically unused right? it just needs to exist to satisfy the resolveEndpointsQuery inside runExternalApiQueries?

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.

Yes, exactly. This is just to reduce the amount of state that would need to be passed around between setUpPack and runExternalApiQueries.

@koesie10 koesie10 merged commit 93652fc into main Sep 28, 2023
@koesie10 koesie10 deleted the koesie10/resolve-queries-from-ql branch September 28, 2023 10:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants