Merge pull request #2968 from github/koesie10/readonly-modeling-store

Improve immutability of modeling store state
This commit is contained in:
Koen Vlaswinkel
2023-10-13 13:13:27 +02:00
committed by GitHub
17 changed files with 164 additions and 101 deletions

View File

@@ -0,0 +1,14 @@
export type DeepReadonly<T> = T extends Array<infer R>
? DeepReadonlyArray<R>
: // eslint-disable-next-line @typescript-eslint/ban-types
T extends Function
? T
: T extends object
? DeepReadonlyObject<T>
: T;
interface DeepReadonlyArray<T> extends ReadonlyArray<DeepReadonly<T>> {}
type DeepReadonlyObject<T> = {
readonly [P in keyof T]: DeepReadonly<T[P]>;
};

View File

@@ -3,6 +3,7 @@ import { Uri, WebviewViewProvider } from "vscode";
import { WebviewKind, WebviewMessage, getHtmlForWebview } from "./webview-html";
import { Disposable } from "../disposable-object";
import { App } from "../app";
import { DeepReadonly } from "../readonly";
export abstract class AbstractWebviewViewProvider<
ToMessage extends WebviewMessage,
@@ -53,7 +54,7 @@ export abstract class AbstractWebviewViewProvider<
return this.webviewView?.visible ?? false;
}
protected async postMessage(msg: ToMessage): Promise<void> {
protected async postMessage(msg: DeepReadonly<ToMessage>): Promise<void> {
await this.webviewView?.webview.postMessage(msg);
}

View File

@@ -12,6 +12,7 @@ import { App } from "../app";
import { Disposable } from "../disposable-object";
import { tmpDir } from "../../tmp-dir";
import { getHtmlForWebview, WebviewMessage, WebviewKind } from "./webview-html";
import { DeepReadonly } from "../readonly";
export type WebviewPanelConfig = {
viewId: string;
@@ -146,7 +147,7 @@ export abstract class AbstractWebview<
this.panelLoadedCallBacks = [];
}
protected async postMessage(msg: ToMessage): Promise<boolean> {
protected async postMessage(msg: DeepReadonly<ToMessage>): Promise<boolean> {
const panel = await this.getPanel();
return panel.webview.postMessage(msg);
}

View File

@@ -19,8 +19,8 @@ import { groupMethods, sortGroupNames, sortMethods } from "./shared/sorting";
*/
export function getCandidates(
mode: Mode,
methods: Method[],
modeledMethodsBySignature: Record<string, ModeledMethod[]>,
methods: readonly Method[],
modeledMethodsBySignature: Record<string, readonly ModeledMethod[]>,
): MethodSignature[] {
// Sort the same way as the UI so we send the first ones listed in the UI first
const grouped = groupMethods(methods, mode);
@@ -32,8 +32,9 @@ export function getCandidates(
const candidates: MethodSignature[] = [];
for (const method of sortedMethods) {
const modeledMethods: ModeledMethod[] =
modeledMethodsBySignature[method.signature] ?? [];
const modeledMethods: ModeledMethod[] = [
...(modeledMethodsBySignature[method.signature] ?? []),
];
// Anything that is modeled is not a candidate
if (modeledMethods.some((m) => m.type !== "none")) {

View File

@@ -58,8 +58,8 @@ export class AutoModeler {
*/
public async startModeling(
packageName: string,
methods: Method[],
modeledMethods: Record<string, ModeledMethod[]>,
methods: readonly Method[],
modeledMethods: Record<string, readonly ModeledMethod[]>,
mode: Mode,
): Promise<void> {
if (this.jobs.has(packageName)) {
@@ -105,8 +105,8 @@ export class AutoModeler {
private async modelPackage(
packageName: string,
methods: Method[],
modeledMethods: Record<string, ModeledMethod[]>,
methods: readonly Method[],
modeledMethods: Record<string, readonly ModeledMethod[]>,
mode: Mode,
cancellationTokenSource: CancellationTokenSource,
): Promise<void> {

View File

@@ -88,9 +88,16 @@ export function decodeBqrsToMethods(
}
const method = methodsByApiName.get(signature)!;
method.usages.push({
const usages = [
...method.usages,
{
...usage,
classification,
},
];
methodsByApiName.set(signature, {
...method,
usages,
});
});

View File

@@ -2,8 +2,8 @@ import { ResolvableLocationValue } from "../common/bqrs-cli-types";
import { ModeledMethod, ModeledMethodType } from "./modeled-method";
export type Call = {
label: string;
url: ResolvableLocationValue;
readonly label: string;
readonly url: Readonly<ResolvableLocationValue>;
};
export enum CallClassification {
@@ -14,14 +14,14 @@ export enum CallClassification {
}
export type Usage = Call & {
classification: CallClassification;
readonly classification: CallClassification;
};
export interface MethodSignature {
/**
* Contains the version of the library if it can be determined by CodeQL, e.g. `4.2.2.2`
*/
libraryVersion?: string;
readonly libraryVersion?: string;
/**
* A unique signature that can be used to identify this external API usage.
*
@@ -29,33 +29,33 @@ export interface MethodSignature {
* in the form "packageName.typeName#methodName(methodParameters)".
* e.g. `org.sql2o.Connection#createQuery(String)`
*/
signature: string;
readonly signature: string;
/**
* The package name in Java, or the namespace in C#, e.g. `org.sql2o` or `System.Net.Http.Headers`.
*
* If the class is not in a package, the value should be an empty string.
*/
packageName: string;
typeName: string;
methodName: string;
readonly packageName: string;
readonly typeName: string;
readonly methodName: string;
/**
* The method parameters, including enclosing parentheses, e.g. `(String, String)`
*/
methodParameters: string;
readonly methodParameters: string;
}
export interface Method extends MethodSignature {
/**
* Contains the name of the library containing the method declaration, e.g. `sql2o-1.6.0.jar` or `System.Runtime.dll`
*/
library: string;
readonly library: string;
/**
* Is this method already supported by CodeQL standard libraries.
* If so, there is no need for the user to model it themselves.
*/
supported: boolean;
supportedType: ModeledMethodType;
usages: Usage[];
readonly supported: boolean;
readonly supportedType: ModeledMethodType;
readonly usages: readonly Usage[];
}
export function getArgumentsList(methodParameters: string): string[] {
@@ -68,7 +68,7 @@ export function getArgumentsList(methodParameters: string): string[] {
export function canMethodBeModeled(
method: Method,
modeledMethods: ModeledMethod[],
modeledMethods: readonly ModeledMethod[],
methodIsUnsaved: boolean,
): boolean {
return (

View File

@@ -24,16 +24,17 @@ export class MethodsUsageDataProvider
extends DisposableObject
implements TreeDataProvider<MethodsUsageTreeViewItem>
{
private methods: Method[] = [];
private methods: readonly Method[] = [];
// sortedMethods is a separate field so we can check if the methods have changed
// by reference, which is faster than checking if the methods have changed by value.
private sortedMethods: Method[] = [];
private sortedMethods: readonly Method[] = [];
private databaseItem: DatabaseItem | undefined = undefined;
private sourceLocationPrefix: string | undefined = undefined;
private hideModeledMethods: boolean = INITIAL_HIDE_MODELED_METHODS_VALUE;
private mode: Mode = INITIAL_MODE;
private modeledMethods: Record<string, ModeledMethod[]> = {};
private modifiedMethodSignatures: Set<string> = new Set();
private modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>> =
{};
private modifiedMethodSignatures: ReadonlySet<string> = new Set();
private readonly onDidChangeTreeDataEmitter = this.push(
new EventEmitter<void>(),
@@ -55,12 +56,12 @@ export class MethodsUsageDataProvider
* method and instead always pass new objects/arrays.
*/
public async setState(
methods: Method[],
methods: readonly Method[],
databaseItem: DatabaseItem,
hideModeledMethods: boolean,
mode: Mode,
modeledMethods: Record<string, ModeledMethod[]>,
modifiedMethodSignatures: Set<string>,
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
modifiedMethodSignatures: ReadonlySet<string>,
): Promise<void> {
if (
this.methods !== methods ||
@@ -145,10 +146,10 @@ export class MethodsUsageDataProvider
if (this.hideModeledMethods) {
return this.sortedMethods.filter((api) => !api.supported);
} else {
return this.sortedMethods;
return [...this.sortedMethods];
}
} else if (isExternalApiUsage(item)) {
return item.usages;
return [...item.usages];
} else {
return [];
}
@@ -194,7 +195,7 @@ function usagesAreEqual(u1: Usage, u2: Usage): boolean {
);
}
function sortMethodsInGroups(methods: Method[], mode: Mode): Method[] {
function sortMethodsInGroups(methods: readonly Method[], mode: Mode): Method[] {
const grouped = groupMethods(methods, mode);
const sortedGroupNames = sortGroupNames(grouped);

View File

@@ -32,12 +32,12 @@ export class MethodsUsagePanel extends DisposableObject {
}
public async setState(
methods: Method[],
methods: readonly Method[],
databaseItem: DatabaseItem,
hideModeledMethods: boolean,
mode: Mode,
modeledMethods: Record<string, ModeledMethod[]>,
modifiedMethodSignatures: Set<string>,
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
modifiedMethodSignatures: ReadonlySet<string>,
): Promise<void> {
await this.dataProvider.setState(
methods,

View File

@@ -14,8 +14,8 @@ import { pathsEqual } from "../common/files";
export async function saveModeledMethods(
extensionPack: ExtensionPack,
language: string,
methods: Method[],
modeledMethods: Record<string, ModeledMethod[]>,
methods: readonly Method[],
modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
mode: Mode,
cliServer: CodeQLCliServer,
logger: NotificationLogger,

View File

@@ -20,11 +20,11 @@ export type Provenance =
| "manual";
export interface ModeledMethod extends MethodSignature {
type: ModeledMethodType;
input: string;
output: string;
kind: ModeledMethodKind;
provenance: Provenance;
readonly type: ModeledMethodType;
readonly input: string;
readonly output: string;
readonly kind: ModeledMethodKind;
readonly provenance: Provenance;
}
export type ModeledMethodKind = string;

View File

@@ -7,7 +7,7 @@ import { ModeledMethod } from "./modeled-method";
import { INITIAL_HIDE_MODELED_METHODS_VALUE } from "./shared/hide-modeled-methods";
import { INITIAL_MODE, Mode } from "./shared/mode";
interface DbModelingState {
interface InternalDbModelingState {
databaseItem: DatabaseItem;
methods: Method[];
hideModeledMethods: boolean;
@@ -18,40 +18,59 @@ interface DbModelingState {
selectedUsage: Usage | undefined;
}
interface DbModelingState {
readonly databaseItem: DatabaseItem;
readonly methods: readonly Method[];
readonly hideModeledMethods: boolean;
readonly mode: Mode;
readonly modeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>;
readonly modifiedMethodSignatures: ReadonlySet<string>;
readonly selectedMethod: Method | undefined;
readonly selectedUsage: Usage | undefined;
}
interface SelectedMethodDetails {
readonly databaseItem: DatabaseItem;
readonly method: Method;
readonly usage: Usage | undefined;
readonly modeledMethods: readonly ModeledMethod[];
readonly isModified: boolean;
}
interface MethodsChangedEvent {
methods: Method[];
dbUri: string;
isActiveDb: boolean;
readonly methods: readonly Method[];
readonly dbUri: string;
readonly isActiveDb: boolean;
}
interface HideModeledMethodsChangedEvent {
hideModeledMethods: boolean;
isActiveDb: boolean;
readonly hideModeledMethods: boolean;
readonly isActiveDb: boolean;
}
interface ModeChangedEvent {
mode: Mode;
isActiveDb: boolean;
readonly mode: Mode;
readonly isActiveDb: boolean;
}
interface ModeledMethodsChangedEvent {
modeledMethods: Record<string, ModeledMethod[]>;
dbUri: string;
isActiveDb: boolean;
readonly modeledMethods: Readonly<Record<string, ModeledMethod[]>>;
readonly dbUri: string;
readonly isActiveDb: boolean;
}
interface ModifiedMethodsChangedEvent {
modifiedMethods: Set<string>;
dbUri: string;
isActiveDb: boolean;
readonly modifiedMethods: ReadonlySet<string>;
readonly dbUri: string;
readonly isActiveDb: boolean;
}
interface SelectedMethodChangedEvent {
databaseItem: DatabaseItem;
method: Method;
usage: Usage;
modeledMethods: ModeledMethod[];
isModified: boolean;
readonly databaseItem: DatabaseItem;
readonly method: Method;
readonly usage: Usage;
readonly modeledMethods: readonly ModeledMethod[];
readonly isModified: boolean;
}
export class ModelingStore extends DisposableObject {
@@ -65,7 +84,7 @@ export class ModelingStore extends DisposableObject {
public readonly onModifiedMethodsChanged: AppEvent<ModifiedMethodsChangedEvent>;
public readonly onSelectedMethodChanged: AppEvent<SelectedMethodChangedEvent>;
private readonly state: Map<string, DbModelingState>;
private readonly state: Map<string, InternalDbModelingState>;
private activeDb: string | undefined;
private readonly onActiveDbChangedEventEmitter: AppEventEmitter<void>;
@@ -82,7 +101,7 @@ export class ModelingStore extends DisposableObject {
super();
// State initialization
this.state = new Map<string, DbModelingState>();
this.state = new Map<string, InternalDbModelingState>();
// Event initialization
this.onActiveDbChangedEventEmitter = this.push(
@@ -179,6 +198,14 @@ export class ModelingStore extends DisposableObject {
return this.state.get(this.activeDb);
}
private getInternalStateForActiveDb(): InternalDbModelingState | undefined {
if (!this.activeDb) {
return undefined;
}
return this.state.get(this.activeDb);
}
public hasStateForActiveDb(): boolean {
return !!this.getStateForActiveDb();
}
@@ -194,7 +221,7 @@ export class ModelingStore extends DisposableObject {
public getMethods(
dbItem: DatabaseItem,
methodSignatures?: string[],
): Method[] {
): readonly Method[] {
const methods = this.getState(dbItem).methods;
if (!methodSignatures) {
return methods;
@@ -255,7 +282,7 @@ export class ModelingStore extends DisposableObject {
public getModeledMethods(
dbItem: DatabaseItem,
methodSignatures?: string[],
): Record<string, ModeledMethod[]> {
): Readonly<Record<string, readonly ModeledMethod[]>> {
const modeledMethods = this.getState(dbItem).modeledMethods;
if (!methodSignatures) {
return modeledMethods;
@@ -369,8 +396,8 @@ export class ModelingStore extends DisposableObject {
});
}
public getSelectedMethodDetails() {
const dbState = this.getStateForActiveDb();
public getSelectedMethodDetails(): SelectedMethodDetails | undefined {
const dbState = this.getInternalStateForActiveDb();
if (!dbState) {
throw new Error("No active state found in modeling store");
}
@@ -391,7 +418,7 @@ export class ModelingStore extends DisposableObject {
};
}
private getState(databaseItem: DatabaseItem): DbModelingState {
private getState(databaseItem: DatabaseItem): InternalDbModelingState {
if (!this.state.has(databaseItem.databaseUri.toString())) {
throw Error(
"Cannot get state for a database that has not been initialized",
@@ -403,7 +430,7 @@ export class ModelingStore extends DisposableObject {
private changeModifiedMethods(
dbItem: DatabaseItem,
updateState: (state: DbModelingState) => void,
updateState: (state: InternalDbModelingState) => void,
) {
const state = this.getState(dbItem);
@@ -418,7 +445,7 @@ export class ModelingStore extends DisposableObject {
private changeModeledMethods(
dbItem: DatabaseItem,
updateState: (state: DbModelingState) => void,
updateState: (state: InternalDbModelingState) => void,
) {
const state = this.getState(dbItem);

View File

@@ -1,6 +1,6 @@
import { Method } from "../method";
export function calculateModeledPercentage(methods: Method[]): number {
export function calculateModeledPercentage(methods: readonly Method[]): number {
if (methods.length === 0) {
return 0;
}

View File

@@ -3,13 +3,13 @@ import { ModeledMethod } from "../modeled-method";
export type ModelingStatus = "unmodeled" | "unsaved" | "saved";
export function getModelingStatus(
modeledMethods: Array<ModeledMethod | undefined>,
modeledMethods: readonly ModeledMethod[],
methodIsUnsaved: boolean,
): ModelingStatus {
if (modeledMethods.length > 0) {
if (methodIsUnsaved) {
return "unsaved";
} else if (modeledMethods.some((m) => m && m.type !== "none")) {
} else if (modeledMethods.some((m) => m.type !== "none")) {
return "saved";
}
}

View File

@@ -3,7 +3,7 @@ import { Mode } from "./mode";
import { calculateModeledPercentage } from "./modeled-percentage";
export function groupMethods(
methods: Method[],
methods: readonly Method[],
mode: Mode,
): Record<string, Method[]> {
const groupedByLibrary: Record<string, Method[]> = {};
@@ -19,22 +19,24 @@ export function groupMethods(
return groupedByLibrary;
}
export function sortGroupNames(methods: Record<string, Method[]>): string[] {
export function sortGroupNames(
methods: Record<string, readonly Method[]>,
): string[] {
return Object.keys(methods).sort((a, b) =>
compareGroups(methods[a], a, methods[b], b),
);
}
export function sortMethods(methods: Method[]): Method[] {
export function sortMethods(methods: readonly Method[]): Method[] {
const sortedMethods = [...methods];
sortedMethods.sort((a, b) => compareMethod(a, b));
return sortedMethods;
}
function compareGroups(
a: Method[],
a: readonly Method[],
aName: string,
b: Method[],
b: readonly Method[],
bName: string,
): number {
const supportedPercentageA = calculateModeledPercentage(a);

View File

@@ -16,7 +16,7 @@ const ajv = new Ajv({ allErrors: true, allowUnionTypes: true });
const modelExtensionFileSchemaValidate = ajv.compile(modelExtensionFileSchema);
function createDataProperty(
methods: ModeledMethod[],
methods: readonly ModeledMethod[],
definition: ExtensiblePredicateDefinition,
) {
if (methods.length === 0) {
@@ -35,7 +35,7 @@ function createDataProperty(
export function createDataExtensionYaml(
language: string,
modeledMethods: ModeledMethod[],
modeledMethods: readonly ModeledMethod[],
) {
const methodsByType: Record<
Exclude<ModeledMethodType, "none">,
@@ -70,9 +70,11 @@ ${extensions.join("\n")}`;
export function createDataExtensionYamls(
language: string,
methods: Method[],
newModeledMethods: Record<string, ModeledMethod[]>,
existingModeledMethods: Record<string, Record<string, ModeledMethod[]>>,
methods: readonly Method[],
newModeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
existingModeledMethods: Readonly<
Record<string, Record<string, readonly ModeledMethod[]>>
>,
mode: Mode,
) {
switch (mode) {
@@ -97,9 +99,11 @@ export function createDataExtensionYamls(
function createDataExtensionYamlsByGrouping(
language: string,
methods: Method[],
newModeledMethods: Record<string, ModeledMethod[]>,
existingModeledMethods: Record<string, Record<string, ModeledMethod[]>>,
methods: readonly Method[],
newModeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
existingModeledMethods: Readonly<
Record<string, Record<string, readonly ModeledMethod[]>>
>,
createFilename: (method: Method) => string,
): Record<string, string> {
const methodsByFilename: Record<string, Record<string, ModeledMethod[]>> = {};
@@ -119,7 +123,7 @@ function createDataExtensionYamlsByGrouping(
)) {
if (filename in methodsByFilename) {
for (const [signature, methods] of Object.entries(methodsBySignature)) {
methodsByFilename[filename][signature] = methods;
methodsByFilename[filename][signature] = [...methods];
}
}
}
@@ -132,7 +136,7 @@ function createDataExtensionYamlsByGrouping(
const filename = createFilename(method);
// Override any existing modeled methods with the new ones.
methodsByFilename[filename][method.signature] = newMethods;
methodsByFilename[filename][method.signature] = [...newMethods];
}
}
@@ -150,9 +154,11 @@ function createDataExtensionYamlsByGrouping(
export function createDataExtensionYamlsForApplicationMode(
language: string,
methods: Method[],
newModeledMethods: Record<string, ModeledMethod[]>,
existingModeledMethods: Record<string, Record<string, ModeledMethod[]>>,
methods: readonly Method[],
newModeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
existingModeledMethods: Readonly<
Record<string, Record<string, readonly ModeledMethod[]>>
>,
): Record<string, string> {
return createDataExtensionYamlsByGrouping(
language,
@@ -165,9 +171,11 @@ export function createDataExtensionYamlsForApplicationMode(
export function createDataExtensionYamlsForFrameworkMode(
language: string,
methods: Method[],
newModeledMethods: Record<string, ModeledMethod[]>,
existingModeledMethods: Record<string, Record<string, ModeledMethod[]>>,
methods: readonly Method[],
newModeledMethods: Readonly<Record<string, readonly ModeledMethod[]>>,
existingModeledMethods: Readonly<
Record<string, Record<string, readonly ModeledMethod[]>>
>,
): Record<string, string> {
return createDataExtensionYamlsByGrouping(
language,

View File

@@ -16,8 +16,9 @@ describe(MethodName.name, () => {
});
it("renders method name without package name", () => {
const method = createMethod();
method.packageName = "";
const method = createMethod({
packageName: "",
});
render(method);
const name = `${method.typeName}.${method.methodName}${method.methodParameters}`;