Case insensitive fallback check for github repositories by mgsium · Pull Request #961 · github/vscode-codeql

mgsium · 2021-10-04T22:36:47Z

Implements a case-insensitive fallback check for the /g/ provider where a repository is not found on LGTM using a case-sensitive search. Partially fixes #892

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.

aeisenberg

Nice work. Some comments inline.

aeisenberg · 2021-10-04T23:09:03Z

extensions/ql-vscode/src/databaseFetcher.ts


+export async function retrieveCanonicalRepoName(lgtmUrl: string) {
+  const givenRepoName = parseLgtmUrl(lgtmUrl);
+  const repo = await fetch(`https://un5my6tpgjf94hmrq01g.julianrbryant.com/repos/${givenRepoName}`).then(res => res.json());


What happens if there is an error here and something besides 200 is returned? Can you take a look at checkForFailingResponse in this file? It would be best to to generalize this function to handle this case as well. You could add a new parameter that would replace the error message Error downloading database..

aeisenberg · 2021-10-04T23:16:00Z

extensions/ql-vscode/src/databaseFetcher.ts

 }

+function parseLgtmUrl(lgtmUrl: string): string | undefined {
+  const re = new RegExp('https://un5nu71xrxc0.julianrbryant.com/projects/(g|gl|b|git)/(.*)');


Only project slugs that begin with a g are from github (they are the vast majority of projects on lgtm. It may not be safe to match on other slugs. Eg- if two people put up the different projects with the same project name with different capitalization on github and bitbucket, you would get a mismatch. It's a rare case, but it's probably safer to protect against it.

Also, please add a comment that this only supports github slugs.

extensions/ql-vscode/src/databaseFetcher.ts

aeisenberg · 2021-10-04T23:18:06Z

extensions/ql-vscode/src/databaseFetcher.ts

+        throw new Error();
+      }
+      canonicalName = convertRawLgtmSlug(`g/${canonicalName}`);
+      projectJson = await pullLgtmProject(canonicalName);


You should check again if projectJson.code === 404.

extensions/ql-vscode/src/databaseFetcher.ts

aeisenberg

Almost there...

aeisenberg · 2021-10-07T16:27:48Z

extensions/ql-vscode/src/databaseFetcher.ts

  return;
 }
+
+function parseLgtmUrl(lgtmUrl: string): string | undefined {


This name is misleading. This function does not parse a url. It really just extracts the project slug. Could you rename it to something like: extractProjectSlug

aeisenberg · 2021-10-07T16:29:08Z

extensions/ql-vscode/src/databaseFetcher.ts

+      // fallback check for github repos with same name but different case
+      let canonicalName = await retrieveCanonicalRepoName(lgtmUrl);
+      if (!canonicalName) {
+        throw new Error();


Include an error message here.

aeisenberg · 2021-10-07T16:29:12Z

extensions/ql-vscode/src/databaseFetcher.ts

+      canonicalName = convertRawLgtmSlug(`g/${canonicalName}`);
+      projectJson = await downloadLgtmProjectMetadata(canonicalName);
+      if (projectJson.code === 404) {
+        throw new Error();


aeisenberg

Tried this out locally and found a handful of more issues.

extensions/ql-vscode/src/databaseFetcher.ts

aeisenberg · 2021-10-07T17:15:47Z

extensions/ql-vscode/src/databaseFetcher.ts

+
+function parseLgtmUrl(lgtmUrl: string): string | undefined {
+  // Only matches the '/g/' provider (github)
+  const re = new RegExp('https://un5nu71xrxc0.julianrbryant.com/projects/g/(.*)');


This also needs to handle the case when there is a trailing /. You can either change the regex so that it will ignore capturing the / or add a check after and remove it.

aeisenberg

Looks good. Just some smaller suggestions.

aeisenberg · 2021-10-12T16:31:32Z

extensions/ql-vscode/src/extension.ts

+        let resolved;
+        try {
+          resolved = await cliServer.resolveQlref(selectedQuery.path.replace(/^\/([a-zA-Z]:)/, '$1'));
+        } catch {
+          throw new Error('Referenced .ql file could not be found.');
+        }


Is this part of the same PR?

No, this is related to #970, I must have mixed up the commits for these.

aeisenberg · 2021-10-13T01:11:27Z

extensions/ql-vscode/CHANGELOG.md

 ## 1.5.6 - 07 October 2021

 - Add progress messages to LGTM download option. This makes the two-step process (selecting a project, then selecting a language) more clear. [#960](https://github.com/github/vscode-codeql/pull/960)
- Remove line about selecting a language from the dropdown when downloading database from LGTM. This makes the download progress visible when the popup is not expanded. [#957](https://github.com/github/vscode-codeql/pull/957)


I don't think this should have been deleted.

aeisenberg · 2021-10-13T01:12:38Z

extensions/ql-vscode/src/extension.ts

+          throw new Error('Referenced .ql file could not be found.');
+        }
        const uri = Uri.file(resolved.resolvedPath);
+        // Something wrong with the uri


I think this comment can be removed.

aeisenberg · 2021-10-13T01:16:22Z

extensions/ql-vscode/src/databaseFetcher.ts

+  const uri = Uri.parse(lgtmUrl, true);
+  const paths = ['api', 'v1.0'].concat(
+    uri.path.split('/').filter((segment) => segment)
+  ).slice(0, 6);


Why 0, 6? Could you add a comment to explain?

aeisenberg · 2021-10-13T01:17:57Z

extensions/ql-vscode/src/databaseFetcher.ts

+      // fallback check for github repos with same name but different case
+      let canonicalName = await retrieveCanonicalRepoName(lgtmUrl);
+      if (!canonicalName) {
+        throw new Error('Repository not found.');


Include the lgtmUrl in the error message.

aeisenberg · 2021-10-13T01:19:56Z

extensions/ql-vscode/src/databaseFetcher.ts


    if (projectJson.code === 404) {
-      throw new Error();
+      // fallback check for github repos with same name but different case


Also mention that this will fail for all non-github providers.

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

mgsium · 2021-10-19T18:14:41Z

Unrelated changes in the PR; reopening to clean up.

Made suggested changes; comments can be found at github#961

mgsium requested a review from a team as a code owner October 4, 2021 22:36

aeisenberg reviewed Oct 4, 2021

View reviewed changes

aeisenberg reviewed Oct 7, 2021

View reviewed changes

mgsium requested a review from aeisenberg October 12, 2021 16:03

aeisenberg reviewed Oct 13, 2021

View reviewed changes

marcnjaramillo and others added 19 commits October 19, 2021 18:33

Create rough solution for handling non-printing characters in results

f00c7c7

Make edits per feedback

d3f53f9

Add a check for strings with only new line chars

c9bebe8

Finish creating check for non-printing characters

3210039

Add 1.5.6 header

f5c0f3f

Bump version to v1.5.7

81cb789

Add changes to changelog

9f84867

Simplify changelog entry

8bdf06b

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Add fallback search for /g/ provider

dc8373e

Replaced regex literal with RegExp object for clarity

e855bdd

Refactored LGTM request to a separate function

cfff4ad

Refactor pullLgtmProject to return a Promise

dc06601

Update CHANGELOG.md

054eb11

Update extensions/ql-vscode/src/databaseFetcher.ts

54756d8

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Update extensions/ql-vscode/src/databaseFetcher.ts

4d7d8b5

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Make suggested changes

539a2cd

Replaced double quotes with single quotes

8112b55

Make suggested changes

a75278e

Update extensions/ql-vscode/src/databaseFetcher.ts

feec0e8

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

mgsium force-pushed the main branch from 394f518 to feec0e8 Compare October 19, 2021 17:35

mgsium closed this Oct 19, 2021

mgsium added a commit to mgsium/vscode-codeql that referenced this pull request Oct 20, 2021

Update databaseFetcher.ts

99215f4

Made suggested changes; comments can be found at github#961

Conversation

mgsium commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgsium commented Oct 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mgsium commented Oct 4, 2021 •

edited

Loading