Merge pull request #2798 from github/koesie10/group-framework-mode-models

Group framework mode model files by package

extensions/ql-vscode/src/model-editor/extension-pack-name.ts

@@ -37,7 +37,7 @@ export function autoNameExtensionPack(
   };
 }
 
-export function sanitizeExtensionPackName(name: string) {
+function sanitizeExtensionPackName(name: string) {
   // Lowercase everything
   name = name.toLowerCase();
 

extensions/ql-vscode/src/model-editor/model-editor-view.ts

@@ -217,7 +217,6 @@ export class ModelEditorView extends AbstractWebview<
             });
             await saveModeledMethods(
               this.extensionPack,
-              this.databaseItem.name,
               this.databaseItem.language,
               msg.methods,
               msg.modeledMethods,

extensions/ql-vscode/src/model-editor/modeled-method-fs.ts

@@ -13,7 +13,6 @@ import { pathsEqual } from "../common/files";
 
 export async function saveModeledMethods(
   extensionPack: ExtensionPack,
-  databaseName: string,
   language: string,
   methods: Method[],
   modeledMethods: Record<string, ModeledMethod>,
@@ -28,7 +27,6 @@ export async function saveModeledMethods(
   );
 
   const yamls = createDataExtensionYamls(
-    databaseName,
     language,
     methods,
     modeledMethods,

extensions/ql-vscode/src/model-editor/yaml.ts

@@ -8,7 +8,6 @@ import {
 } from "./predicates";
 
 import * as dataSchemaJson from "./data-schema.json";
-import { sanitizeExtensionPackName } from "./extension-pack-name";
 import { Mode } from "./shared/mode";
 import { assertNever } from "../common/helpers-pure";
 
@@ -69,7 +68,6 @@ ${extensions.join("\n")}`;
 }
 
 export function createDataExtensionYamls(
-  databaseName: string,
   language: string,
   methods: Method[],
   newModeledMethods: Record<string, ModeledMethod>,
@@ -86,7 +84,6 @@ export function createDataExtensionYamls(
       );
     case Mode.Framework:
       return createDataExtensionYamlsForFrameworkMode(
-        databaseName,
         language,
         methods,
         newModeledMethods,
@@ -97,31 +94,29 @@ export function createDataExtensionYamls(
   }
 }
 
-export function createDataExtensionYamlsForApplicationMode(
+function createDataExtensionYamlsByGrouping(
   language: string,
   methods: Method[],
   newModeledMethods: Record<string, ModeledMethod>,
   existingModeledMethods: Record<string, Record<string, ModeledMethod>>,
+  createFilename: (method: Method) => string,
 ): Record<string, string> {
-  const methodsByLibraryFilename: Record<
-    string,
-    Record<string, ModeledMethod>
-  > = {};
+  const methodsByFilename: Record<string, Record<string, ModeledMethod>> = {};
 
   // We only want to generate a yaml file when it's a known external API usage
   // and there are new modeled methods for it. This avoids us overwriting other
   // files that may contain data we don't know about.
   for (const method of methods) {
     if (method.signature in newModeledMethods) {
-      methodsByLibraryFilename[createFilenameForLibrary(method.library)] = {};
+      methodsByFilename[createFilename(method)] = {};
     }
   }
 
-  // First populate methodsByLibraryFilename with any existing modeled methods.
+  // First populate methodsByFilename with any existing modeled methods.
   for (const [filename, methods] of Object.entries(existingModeledMethods)) {
-    if (filename in methodsByLibraryFilename) {
+    if (filename in methodsByFilename) {
       for (const [signature, method] of Object.entries(methods)) {
-        methodsByLibraryFilename[filename][signature] = method;
+        methodsByFilename[filename][signature] = method;
       }
     }
   }
@@ -131,14 +126,14 @@ export function createDataExtensionYamlsForApplicationMode(
   for (const method of methods) {
     const newMethod = newModeledMethods[method.signature];
     if (newMethod) {
-      const filename = createFilenameForLibrary(method.library);
-      methodsByLibraryFilename[filename][newMethod.signature] = newMethod;
+      const filename = createFilename(method);
+      methodsByFilename[filename][newMethod.signature] = newMethod;
     }
   }
 
   const result: Record<string, string> = {};
 
-  for (const [filename, methods] of Object.entries(methodsByLibraryFilename)) {
+  for (const [filename, methods] of Object.entries(methodsByFilename)) {
     result[filename] = createDataExtensionYaml(
       language,
       Object.values(methods),
@@ -148,43 +143,34 @@ export function createDataExtensionYamlsForApplicationMode(
   return result;
 }
 
-export function createDataExtensionYamlsForFrameworkMode(
-  databaseName: string,
+export function createDataExtensionYamlsForApplicationMode(
   language: string,
-  unmodeledMethods: Method[],
+  methods: Method[],
   newModeledMethods: Record<string, ModeledMethod>,
   existingModeledMethods: Record<string, Record<string, ModeledMethod>>,
-  prefix = "models/",
-  suffix = ".model",
 ): Record<string, string> {
-  const parts = databaseName.split("/");
-  const libraryName = parts
-    .slice(1)
-    .map((part) => sanitizeExtensionPackName(part))
-    .join("-");
-  const filename = `${prefix}${libraryName}${suffix}.yml`;
+  return createDataExtensionYamlsByGrouping(
+    language,
+    methods,
+    newModeledMethods,
+    existingModeledMethods,
+    (method) => createFilenameForLibrary(method.library),
+  );
+}
 
-  const methods: Record<string, ModeledMethod> = {};
-
-  // First populate methodsByLibraryFilename with any existing modeled methods.
-  for (const [signature, method] of Object.entries(
-    existingModeledMethods[filename] || {},
-  )) {
-    methods[signature] = method;
-  }
-
-  // Add the new modeled methods, potentially overwriting existing modeled methods
-  // but not removing existing modeled methods that are not in the new set.
-  for (const method of unmodeledMethods) {
-    const modeledMethod = newModeledMethods[method.signature];
-    if (modeledMethod) {
-      methods[modeledMethod.signature] = modeledMethod;
-    }
-  }
-
-  return {
-    [filename]: createDataExtensionYaml(language, Object.values(methods)),
-  };
+export function createDataExtensionYamlsForFrameworkMode(
+  language: string,
+  methods: Method[],
+  newModeledMethods: Record<string, ModeledMethod>,
+  existingModeledMethods: Record<string, Record<string, ModeledMethod>>,
+): Record<string, string> {
+  return createDataExtensionYamlsByGrouping(
+    language,
+    methods,
+    newModeledMethods,
+    existingModeledMethods,
+    (method) => createFilenameForPackage(method.packageName),
+  );
 }
 
 export function createFilenameForLibrary(
@@ -214,6 +200,17 @@ export function createFilenameForLibrary(
   return `${prefix}${libraryName}${suffix}.yml`;
 }
 
+export function createFilenameForPackage(
+  packageName: string,
+  prefix = "models/",
+  suffix = ".model",
+) {
+  // A package name is e.g. `com.google.common.io` or `System.Net.Http.Headers`
+  // We want to place these into `models/com.google.common.io.model.yml` and
+  // `models/System.Net.Http.Headers.model.yml` respectively.
+  return `${prefix}${packageName}${suffix}.yml`;
+}
+
 export function loadDataExtensionYaml(
   data: any,
 ): Record<string, ModeledMethod> | undefined {

extensions/ql-vscode/test/unit-tests/model-editor/yaml.test.ts

@@ -3,6 +3,7 @@ import {
   createDataExtensionYamlsForApplicationMode,
   createDataExtensionYamlsForFrameworkMode,
   createFilenameForLibrary,
+  createFilenameForPackage,
   loadDataExtensionYaml,
 } from "../../../src/model-editor/yaml";
 import { CallClassification } from "../../../src/model-editor/method";
@@ -598,7 +599,6 @@ describe("createDataExtensionYamlsForApplicationMode", () => {
 describe("createDataExtensionYamlsForFrameworkMode", () => {
   it("creates the correct YAML files when there are no existing modeled methods", () => {
     const yaml = createDataExtensionYamlsForFrameworkMode(
-      "github/sql2o",
       "java",
       [
         {
@@ -723,7 +723,7 @@ describe("createDataExtensionYamlsForFrameworkMode", () => {
     );
 
     expect(yaml).toEqual({
-      "models/sql2o.model.yml": `extensions:
+      "models/org.sql2o.model.yml": `extensions:
   - addsTo:
       pack: codeql/java-all
       extensible: sourceModel
@@ -751,7 +751,6 @@ describe("createDataExtensionYamlsForFrameworkMode", () => {
 
   it("creates the correct YAML files when there are existing modeled methods", () => {
     const yaml = createDataExtensionYamlsForFrameworkMode(
-      "github/sql2o",
       "java",
       [
         {
@@ -873,7 +872,7 @@ describe("createDataExtensionYamlsForFrameworkMode", () => {
         },
       },
       {
-        "models/sql2o.model.yml": {
+        "models/org.sql2o.model.yml": {
           "org.sql2o.Connection#createQuery(String)": {
             type: "neutral",
             input: "",
@@ -917,7 +916,7 @@ describe("createDataExtensionYamlsForFrameworkMode", () => {
     );
 
     expect(yaml).toEqual({
-      "models/sql2o.model.yml": `extensions:
+      "models/org.sql2o.model.yml": `extensions:
   - addsTo:
       pack: codeql/java-all
       extensible: sourceModel
@@ -1044,3 +1043,40 @@ describe("createFilenameForLibrary", () => {
     },
   );
 });
+
+describe("createFilenameForPackage", () => {
+  const testCases = [
+    {
+      library: "System.Net.Http.Headers",
+      filename: "models/System.Net.Http.Headers.model.yml",
+    },
+    {
+      library: "System.Security.Cryptography.X509Certificates",
+      filename:
+        "models/System.Security.Cryptography.X509Certificates.model.yml",
+    },
+    {
+      library: "com.google.common.io",
+      filename: "models/com.google.common.io.model.yml",
+    },
+    {
+      library: "hudson.cli",
+      filename: "models/hudson.cli.model.yml",
+    },
+    {
+      library: "java.util",
+      filename: "models/java.util.model.yml",
+    },
+    {
+      library: "org.apache.commons.io",
+      filename: "models/org.apache.commons.io.model.yml",
+    },
+  ];
+
+  test.each(testCases)(
+    "returns $filename if package name is $library",
+    ({ library, filename }) => {
+      expect(createFilenameForPackage(library)).toEqual(filename);
+    },
+  );
+});