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

Adds ability to search for templates by title/description messages. #28

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
7 changes: 5 additions & 2 deletions api/src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ import cors from "cors";
import express, { Request, Response } from "express";
import "express-async-errors";
import path from "path";
import templateRouter from "./routes/template";
import templateRouterV1 from "./routes/template-v1";
import templateRouterV2 from "./routes/template-v2";
import auditorsRouter from "./routes/auditors";
import { TemplateService } from "./services/template";

const V1 = "/v1/";
const V2 = "/v2/";

const corsOptions = {
origin: "*",
Expand All @@ -26,7 +28,8 @@ const initApp = (
app.use(cors(corsOptions));
app.use(json());
app.use(urlencoded({ extended: false }));
app.use(V1, templateRouter(templateService, namesJSONFile));
app.use(V1, templateRouterV1(templateService, namesJSONFile));
app.use(V2, templateRouterV2(templateService, namesJSONFile));
app.use(V1, auditorsRouter(auditorsJSONFile));

app.all("*", async (req: Request, res: Response) => {
Expand Down
8 changes: 3 additions & 5 deletions api/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,7 @@ export function getConfig(env) {

const templateManifestFile = env.TEMPLATE_MANIFEST_FILE;

console.log("ENV: ", env);
console.log("accessApi", accessApi);
console.log("databaseMigrationPath: ", databaseMigrationPath);

return {
const _CONFIG = {
port,
accessApi,
dbPath,
Expand All @@ -38,4 +34,6 @@ export function getConfig(env) {
peers,
templateManifestFile,
};

return _CONFIG
}
2 changes: 2 additions & 0 deletions api/src/migrations/20220718235815_create_templates_table.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ export async function up(knex: Knex): Promise<void> {
table.json("json_string");
table.text("mainnet_cadence_ast_sha3_256_hash");
table.text("testnet_cadence_ast_sha3_256_hash");
table.text("messages_title_enUS");
table.text("messages_description_enUS");
table.timestamps(true, true);
});
}
Expand Down
2 changes: 2 additions & 0 deletions api/src/models/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ class Template extends BaseModel {
json_string!: string;
testnet_cadence_ast_sha3_256_hash!: string;
mainnet_cadence_ast_sha3_256_hash!: string;
messages_title_enUS?: string;
messages_description_enUS?: string;

static get tableName() {
return "templates";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,4 +168,4 @@ function templateRouter(
return router;
}

export default templateRouter;
export default templateRouter;
185 changes: 185 additions & 0 deletions api/src/routes/template-v2.ts
Copy link

@bartolomej bartolomej Nov 4, 2023

Choose a reason for hiding this comment

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

Should we update the documentation in README with v2 endpoints?

Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
import express, { Request, Response, Router } from "express";
import { body } from "express-validator";
Copy link
Member

Choose a reason for hiding this comment

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

remove

import { validateRequest } from "../middlewares/validate-request";
import { TemplateService } from "../services/template";
import { genHash } from "../utils/gen-hash";
import { mixpanelTrack } from "../utils/mixpanel";
import { parseCadence } from "../utils/parse-cadence";

function templateRouter(
templateService: TemplateService,
namesJSONFile: JSON
): Router {
const router = express.Router();

router.get("/templates", async (req: Request, res: Response) => {
const name = req.query.name as string;

if (!name) {
mixpanelTrack("get_template_by_name", {
name,
status: 400,
});

res.status(400);
return res.send(
`GET /templates-- Required query parameter "name" not provided.`
);
}

let templateId: string = "";
let _name: string = name;
while (_name !== undefined) {
let foundName = namesJSONFile[_name];
Copy link
Member

Choose a reason for hiding this comment

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

Are names guaranteed to be unique?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the names json file, yes

if (foundName !== undefined) templateId = foundName;
_name = foundName;
}

const template = await templateService.getTemplate(templateId);

if (!template) {
mixpanelTrack("get_template_by_name", {
name,
templateId,
status: 204,
});

res.status(204);
return res.send(
`GET /templates/:template_id -- Did not find template for template_id=${templateId}`
);
}

mixpanelTrack("get_template_by_name", {
name,
templateId,
status: 200,
});

return res.send(template);
});

router.get("/templates/manifest", async (req: Request, res: Response) => {
const templateManifest = await templateService.getTemplateManifest();

if (!templateManifest) {
mixpanelTrack("get_template_manifest", {
status: 204,
});

res.status(204);
return res.send(
`GET /templates/manifest -- Did not find template manifest`
);
}

mixpanelTrack("get_template_manifest", {
status: 200,
});

return res.send(templateManifest);
});

router.get("/templates/:template_id", async (req: Request, res: Response) => {
const templateId = req.params.template_id;

const template = await templateService.getTemplate(templateId);

if (!template) {
mixpanelTrack("get_template", {
templateId,
status: 204,
});

res.status(204);
return res.send(
`GET /templates/:template_id -- Did not find template for template_id=${templateId}`
);
}

mixpanelTrack("get_template", {
templateId,
status: 200,
});

return res.send(template);
});

router.post("/templates/search/messages", async (req: Request, res: Response) => {
Copy link

@bartolomej bartolomej Nov 4, 2023

Choose a reason for hiding this comment

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

Nit: The endpoint path is a bit confusing to me, as it could also be interpreted as searching for a "message" resource. But that's not true, I think it's only searching by "message" fields like title/description.

Could it make more sense for it to be named /templates/search or even just GET /templates?search={query} (where an empty query or no query parameter present could list all templates)? Using the GET verb could also make sense (semantically at least), since this only reads data.

Copy link

@bartolomej bartolomej Nov 4, 2023

Choose a reason for hiding this comment

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

I can see there is also an endpoint for searching by the AST hash. Maybe that could instead just be another query parameter like /templates/search?cadence_ast_hash?

It could lead to simpler API if there was a single endpoint that supported different search options, as opposed to multiple endpoints, each supporting a different search option.

Another search option could be searching by "dependencies", as I mentioned here: #18 (comment)

Thoughts?

const page = req.body.page as number || undefined;
const range = req.body.range as number || undefined;
const searchMessagesTitleENUS = req.body.searchMessagesTitleENUS as string || undefined;
const searchMessagesDescriptionENUS = req.body.searchMessagesDescriptionENUS as string || undefined;

try {
const templates = await templateService.searchTemplates(
page ?? 0,
range ?? 100,
Copy link
Member

Choose a reason for hiding this comment

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

You need a max range or someone could slam it with too large a number

searchMessagesTitleENUS,
searchMessagesDescriptionENUS
)
return res.send(templates)

} catch (e) {
mixpanelTrack("search_template_messages", {
page,
range,
searchMessagesTitleENUS,
searchMessagesDescriptionENUS,
status: 400,
});
res.status(400);
return res.send("POST /templates/search/messages -- Error occurred when getting template");
}
})

router.post("/templates/search/cadence", async (req: Request, res: Response) => {
const cadence_base64 = req.body.cadence_base64 as string;
const network = req.body.network as string

let cadence = Buffer.from(cadence_base64, "base64").toString("utf8");
let cadenceAST = await parseCadence(cadence);

let template;
try {
template = await templateService.getTemplateByCadenceASTHash(
await genHash(cadenceAST),
Copy link
Member

Choose a reason for hiding this comment

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

You're awaiting the genHash function multiple times for the same input. Compute it once.

network
);
} catch (e) {
mixpanelTrack("search_template", {
cadence_ast_hash: await genHash(cadenceAST),
network,
status: 400,
});

res.status(400);
return res.send("POST /templates/search/cadence -- Error occurred when getting template");
}

if (!template) {
mixpanelTrack("search_template", {
cadence_ast_hash: await genHash(cadenceAST),
network,
status: 204,
});
res.status(204);
return res.send(
`POST /templates/search/cadence -- Did not find template for network=${network} cadence=${cadence_base64}`
);
}

mixpanelTrack("search_template", {
cadence_ast_hash: await genHash(cadenceAST),
network,
found_template_id: template.id,
status: 200,
});

return res.send(template);
});

return router;
}

export default templateRouter;
45 changes: 39 additions & 6 deletions api/src/services/template.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as fcl from "@onflow/fcl";
import fetch from "node-fetch";
import fs from "fs";
import { Template } from "../models/template";
import { readFiles } from "../utils/read-files";
import { writeFile } from "../utils/write-file";
Expand Down Expand Up @@ -44,6 +45,41 @@ class TemplateService {
return foundTemplateJson;
}

async searchTemplates(
page: number = 0,
range: number = 100,
searchMessagesTitleENUS?: string,
searchMessagesDescriptionENUS?: string
) {
let queryBuilder = Template.query().page(page, range);

if (searchMessagesTitleENUS) {
queryBuilder = queryBuilder.where("messages_title_enUS", "like", `%${searchMessagesTitleENUS}%`)
}

if (searchMessagesDescriptionENUS) {
queryBuilder = queryBuilder.where("messages_description_enUS", "like", `%${searchMessagesDescriptionENUS}%`)

Choose a reason for hiding this comment

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

Wondering if it would be possible to do full-text, instead of searching for exact substring matches?

I can see that SQLite does support some kind of full-text search capabilities: https://www.sqlite.org/fts5.html. But I'm not sure if this feature is available in better-sqlite3 which is used in this project.

}

const foundTemplatesQueryResult = await queryBuilder

const foundTemplates = foundTemplatesQueryResult.results.map(foundTemplate => {
let foundTemplateJson = foundTemplate?.json_string || null;
if (typeof foundTemplateJson === "string") {
foundTemplateJson = JSON.parse(foundTemplateJson);
return foundTemplateJson;
}
return null
}).filter(foundTemplate => foundTemplate !== null)

return {
page,
range,
count: foundTemplatesQueryResult.total,
results: foundTemplates,
}
}

async getTemplateByCadenceASTHash(cadenceASTHash: string, network: string) {
let foundTemplate: Template | null = null;

Expand Down Expand Up @@ -71,17 +107,12 @@ class TemplateService {
}

async getTemplateManifest() {
let templateManifest;
try {
templateManifest = (
await readFiles(this.config.templateManifestFile)
).map((file: any) => file.content)[0];
templateManifest = JSON.parse(templateManifest);
return JSON.parse(fs.readFileSync(this.config.templateManifestFile, "utf8"))
} catch (e) {
console.error("Error reading manifest file");
return null;
}
return templateManifest;
}

async seed() {
Expand Down Expand Up @@ -186,6 +217,8 @@ class TemplateService {
testnet_cadence_ast_sha3_256_hash: testnet_cadence
? await genHash(await parseCadence(testnet_cadence))
: undefined,
messages_title_enUS: parsedTemplate?.data?.messages?.title?.i18n?.["en-US"],
messages_description_enUS: parsedTemplate?.data?.messages?.description?.i18n?.["en-US"]
});

templateManifest[parsedTemplate.id] = parsedTemplate;
Expand Down