Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add computed fields to SQL #92

Merged
merged 5 commits into from
Dec 15, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/lib/databaseUtils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { Knex } from "knex";
import { MddbFile, MddbTag, MddbLink, MddbFileTag, File } from "./schema.js";

Check warning on line 2 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

'MddbFile' is defined but never used
import path from "path";
import { WikiLink } from "./parseFile.js";

export async function resetDatabaseTables(db: Knex) {
const tableNames = [MddbFile, MddbTag, MddbFileTag, MddbLink];
const tableNames = [MddbTag, MddbFileTag, MddbLink];
// Drop and Create tables
for (const table of tableNames) {
await table.deleteTable(db);
Expand All @@ -12,12 +12,12 @@
}
}

export function mapFileToInsert(file: any) {

Check warning on line 15 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
const { _id, file_path, extension, url_path, filetype, metadata } = file;
return { _id, file_path, extension, url_path, filetype, metadata };
const { tags, links, ...rest } = file;

Check warning on line 16 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

'tags' is assigned a value but never used

Check warning on line 16 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

'links' is assigned a value but never used
return { ...rest };
}

export function mapLinksToInsert(filesToInsert: File[], file: any) {

Check warning on line 20 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
return file.links.map((link: WikiLink) => {
let to: string | undefined;
if (!link.internal) {
Expand Down Expand Up @@ -45,12 +45,12 @@
});
}

export function isLinkToDefined(link: any) {

Check warning on line 48 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
return link.to !== undefined;
}

export function mapFileTagsToInsert(file: any) {

Check warning on line 52 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
return file.tags.map((tag: any) => ({

Check warning on line 53 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
file: file._id,
tag: tag as unknown as string,
}));
Expand All @@ -67,3 +67,17 @@

return uniqueArray;
}

export function getUniqueProperties(objects: any[]): string[] {

Check warning on line 71 in src/lib/databaseUtils.ts

View workflow job for this annotation

GitHub Actions / Lint & format check

Unexpected any. Specify a different type
const uniqueProperties: string[] = [];

for (const object of objects) {
for (const key of Object.keys(object)) {
if (!uniqueProperties.includes(key)) {
uniqueProperties.push(key);
}
}
}

return uniqueProperties;
}
Comment on lines +71 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function getUniqueProperties(objects: any[]): string[] {
const uniqueProperties: string[] = [];
for (const object of objects) {
for (const key of Object.keys(object)) {
if (!uniqueProperties.includes(key)) {
uniqueProperties.push(key);
}
}
}
return uniqueProperties;
}
export function getUniqueProperties(objects: any[]): string[] {
const uniqueProperties = new Set<string>();
for (const object of objects) {
Object.keys(object).forEach(key => uniqueProperties.add(key));
}
return Array.from(uniqueProperties);
}

28 changes: 26 additions & 2 deletions src/lib/markdowndb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ import {
isLinkToDefined,
mapFileTagsToInsert,
getUniqueValues,
getUniqueProperties,
} from "./databaseUtils.js";
import fs from "fs";
import { CustomConfig } from "./CustomConfig.js";
import { FileInfo } from "./process.js";

const defaultFilePathToUrl = (filePath: string) => {
let url = filePath
Expand Down Expand Up @@ -80,14 +82,36 @@ export class MarkdownDB {
pathToUrlResolver?: (filePath: string) => string;
customConfig?: CustomConfig;
}) {
await resetDatabaseTables(this.db);

const fileObjects = indexFolder(
folderPath,
pathToUrlResolver,
customConfig,
ignorePatterns
);
await this.saveDataToDisk(fileObjects);
}

private async saveDataToDisk(fileObjects: FileInfo[]) {
await resetDatabaseTables(this.db);
const properties = getUniqueProperties(fileObjects);
MddbFile.deleteTable(this.db);
const defaultProperties = [
"_id",
"file_path",
"extension",
"url_path",
"filetype",
"metadata",
"tags",
"links",
];
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved
await MddbFile.createTable(
this.db,
properties.filter(
(property) => defaultProperties.indexOf(property) === -1
)
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved
);
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved

const filesToInsert = fileObjects.map(mapFileToInsert);
const uniqueTags = getUniqueValues(
fileObjects.flatMap((file) => file.tags)
Expand Down
118 changes: 86 additions & 32 deletions src/lib/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,50 +25,61 @@ interface File {
url_path: string | null;
filetype: string | null;
metadata: MetaData | null;
[key: string]: string | null | MetaData | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather:

Suggested change
[key: string]: string | null | MetaData | undefined;
[key: string]: unknown;

}

class MddbFile {
static table = Table.Files;
static supportedExtensions = ["md", "mdx"];

_id: string;
file_path: string;
extension: string;
url_path: string | null;
// TODO there should be a separate table for filetypes
// and another one for many-to-many relationship between files and filetypes
filetype: string | null;
metadata: MetaData | null;
file: File = {
_id: "",
file_path: "",
extension: "",
url_path: null,
filetype: null,
metadata: null,
};
Copy link
Member

@olayway olayway Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do this... It's not necessary at all. If you want to be able to assign dynamic properties I think you can just add this to the class:

  [key: string]: any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this way we lose tract of all fields, it's like using any as a type. resulting in a loss of type safety.

Copy link
Member

@olayway olayway Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not what I meant. Use original properties + this one for dynamic ones (aka computed fields)


// TODO type?
constructor(file: any) {
this._id = file._id;
this.file_path = file.file_path;
this.extension = file.extension;
this.url_path = file.url_path;
this.filetype = file.filetype;
this.metadata = file.metadata ? JSON.parse(file.metadata) : null;
this.file._id = file._id;
this.file.file_path = file.file_path;
this.file.extension = file.extension;
this.file.url_path = file.url_path;
this.file.filetype = file.filetype;
this.file.metadata = file.metadata ? JSON.parse(file.metadata) : null;

// Assign dynamic properties using index signature to this.file
for (const key in file) {
if (
key !== "_id" &&
key !== "file_path" &&
key !== "extension" &&
key !== "url_path" &&
key !== "filetype" &&
key !== "metadata"
) {
this.file[key] = file[key];
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be much simpler:

Suggested change
this.file._id = file._id;
this.file.file_path = file.file_path;
this.file.extension = file.extension;
this.file.url_path = file.url_path;
this.file.filetype = file.filetype;
this.file.metadata = file.metadata ? JSON.parse(file.metadata) : null;
// Assign dynamic properties using index signature to this.file
for (const key in file) {
if (
key !== "_id" &&
key !== "file_path" &&
key !== "extension" &&
key !== "url_path" &&
key !== "filetype" &&
key !== "metadata"
) {
this.file[key] = file[key];
}
}
Object.keys(file).forEach(key => {
if (key === "metadata") {
this[key] = file[key] ? JSON.parse(file[key]) : null;
} else {
this[key] = file[key];
}
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree

}

toObject(): File {
return {
_id: this._id,
file_path: this.file_path,
extension: this.extension,
url_path: this.url_path,
filetype: this.filetype,
metadata: this.metadata,
};
return { ...this.file };
}

static async createTable(db: Knex) {
static async createTable(db: Knex, properties: string[]) {
const creator = (table: Knex.TableBuilder) => {
table.string("_id").primary();
table.string("file_path").unique().notNullable(); // Path relative to process.cwd()
table.string("extension").notNullable(); // File extension
table.string("url_path"); // Sluggfied path relative to content folder
table.string("filetype"); // Type field in frontmatter if it exists
table.string("metadata"); // All frontmatter data
table.string("file_path").unique().notNullable();
table.string("extension").notNullable();
table.string("url_path");
table.string("filetype");
table.string("metadata");
for (let index = 0; index < properties.length; index++) {
table.string(properties[index]);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if a computed property returns an array or a boolean? Shouldn't it be table.<some type>? Also, were the comments incorrect?

Suggested change
for (let index = 0; index < properties.length; index++) {
table.string(properties[index]);
}
properties.forEach((property) => {
table.string(property);
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you are saying, I thought about a couple of scenarios:

  • a field is array => there's no way to store arrays in SQL
  • a field is a number => what type of number? float, double, ...
  • what if a field is an object => there's no way to store objects in SQL
  • what if a field has a different type ( number/string ) across two files, should it be defaulted to string?

Copy link
Member

@olayway olayway Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, use any SQL types that are most suited for each JS/TS type. But it's not like everything needs to be stringified 😅 If it's an object/array, stringify to JSON.

what if a field has a different type ( number/string ) across two files, should it be defaulted to string?

You could type the computed field function signature in a way that it always should return a single type defined by the user. But for now, ok, just stringify everything. We'll properly implement it when we move computed fields definitions to their final destination = document types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the issue is currently computedFields don't return any thing.

the current implementation of ComputedField is unpredictable as the user changes the fileObject directly...

};
const tableExists = await db.schema.hasTable(this.table);

Expand All @@ -88,15 +99,58 @@ class MddbFile {
if (!areUniqueObjectsByKey(files, "file_path")) {
throw new Error("Files must have unique file_path");
}

const serializedFiles = files.map((file) => {
return {
...file,
metadata: JSON.stringify(file.metadata),
};
const serializedFile: any = {};
const defaultProperties = [
"_id",
"file_path",
"extension",
"url_path",
"filetype",
];

for (const key in file) {
if (Object.prototype.hasOwnProperty.call(file, key)) {
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved
const value = file[key];
// If the value is undefined, default it to null
serializedFile[key] = defaultProperties.includes(key)
? value
: value !== undefined
? JSON.stringify(value)
: null;
}
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved
}

return serializedFile;
});

return db.batchInsert(Table.Files, serializedFiles);
}

get _id(): string {
return this.file._id;
}

get file_path(): string {
return this.file.file_path;
}

get extension(): string {
return this.file.extension;
}

get url_path(): string | null {
return this.file.url_path;
}

get filetype(): string | null {
return this.file.filetype;
}

get metadata(): MetaData | null {
return this.file.metadata;
}
mohamedsalem401 marked this conversation as resolved.
Show resolved Hide resolved
}

interface Link {
Expand Down
Loading