Case insensitive fallback check for github repositories #961

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

@@ -107,6 +107,15 @@ export async function promptImportLgtmDatabase(
  return;
 }

+export async function retrieveCanonicalRepoName(lgtmUrl: string) {
+  const givenRepoName = parseLgtmUrl(lgtmUrl);
+  const repo = await fetch(`https://api.github.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

@@ -409,23 +418,31 @@ function convertRawLgtmSlug(maybeSlug: string): string | undefined {
  return;
 }

+function parseLgtmUrl(lgtmUrl: string): string | undefined {
+  const re = new RegExp('https://lgtm.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

@@ -408,24 +418,36 @@ function convertRawLgtmSlug(maybeSlug: string): string | undefined {
  }
  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://lgtm.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

        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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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

4d7d8b5

Co-authored-by: Andrew Eisenberg <[email protected]>

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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Case insensitive fallback check for github repositories #961

Case insensitive fallback check for github repositories #961

mgsium commented Oct 4, 2021 •

edited

Loading

aeisenberg left a comment

aeisenberg Oct 4, 2021

aeisenberg Oct 4, 2021

aeisenberg Oct 4, 2021

aeisenberg Oct 4, 2021

aeisenberg left a comment

aeisenberg Oct 7, 2021

aeisenberg Oct 7, 2021

aeisenberg Oct 7, 2021

aeisenberg left a comment

aeisenberg Oct 7, 2021

aeisenberg left a comment

aeisenberg Oct 12, 2021

mgsium Oct 14, 2021

aeisenberg Oct 13, 2021

aeisenberg Oct 13, 2021

aeisenberg Oct 13, 2021

aeisenberg Oct 13, 2021

aeisenberg Oct 13, 2021

mgsium commented Oct 19, 2021

		const uri = Uri.file(resolved.resolvedPath);
		// Something wrong with the uri

Case insensitive fallback check for github repositories #961

Case insensitive fallback check for github repositories #961

Conversation

mgsium commented Oct 4, 2021 • edited Loading

Checklist

aeisenberg 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

aeisenberg 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

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg 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

mgsium commented Oct 19, 2021

mgsium commented Oct 4, 2021 •

edited

Loading