-
Notifications
You must be signed in to change notification settings - Fork 1
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 support for LionWeb 2024.1 #98
Conversation
Signed-off-by: Jos Warmer <jos.warmer@openmodeling.nl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm
reports several vulnerabilities that would need looking it, especially since this is a server component. (I know it's not the subject of this PR.)
Still need to validate requests, currently only the admin part is done.
Will take a look at them. Now back to 0 vulnerablities. |
import { getRepositoryData, validateLionWebVersion } from "@lionweb/repository-dbadmin" | ||
import { getLanguageRegistry } from "@lionweb/repository-languages" | ||
import { LionWebJsonChunk, LionWebValidator } from "@lionweb/validation" | ||
// - unpack the request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this at the right place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, moved to line 1 and rephrased.
@@ -2,15 +2,23 @@ import e, { Request, Response } from "express" | |||
import { | |||
getRepositoryParameter, | |||
HttpSuccessCodes, | |||
RepositoryData, | |||
// RepositoryData, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done
@@ -1,4 +1,5 @@ | |||
// functions implementing the LionWeb bulk API | |||
import { getRepositoryData } from "@lionweb/repository-dbadmin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
order of comments / imports correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reordered
let clientId = getStringParam(request, "clientId") | ||
// TODO Change using new repository structure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and fixed
@@ -23,6 +30,7 @@ describe("Repository tests", () => { | |||
"./data/add-new-nodes/Disk-add-new-nodes-single-node.json", | |||
"./data/Disk_A.json", | |||
]) | |||
// await t.dbAdmin.deleteRepository("default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commented code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
@joswarmer My understanding is that with this PR you add a column in the repositories table to record which lionweb version is used for a certain repository, but besides that the db schema does not change. What the bulk import does is to skip the normal path for node insertions (which requires essentially calculating a diff with the current state) as we are dealing with a special situation: we are adding millions of nodes and they are all new, so we skip the diff, and we write directly to the DB, using the special COPY operation. Now, given that the tables we are writing have not changed, my understanding is that these bulk import operations should still work and that we do not need to do anything (which is always ideal :D). Does this make sense? Edit: Maybe we just need to check all nodes are of the lionweb version we want too. We could do that as a preliminary check before starting the insertion, similarly as we do when checking that all sent nodes are new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added minor comments
configuration.md
Outdated
"history": false | ||
"history": false, | ||
// Values can be: "2023.1" | "2024.1" | ||
lionWebVersion: "2023.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think 99% of users will go for the latest version, so I would use 2024.1 in this example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but that makes 2024.1 the default value from now on, as this document describes the default values.
get(key: KEY): VALUE[] { | ||
const keyString = JSON.stringify(key) | ||
let existingValue = this.map.get(keyString) | ||
if (existingValue === undefined) { | ||
existingValue = [] | ||
this.map.set(keyString, existingValue) | ||
existingValue = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a weird indentation (3 spaces)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformatted
@@ -98,7 +98,7 @@ export const nodesForQueryQuery = (nodesQuery: string): string => { | |||
) | |||
containments | |||
FROM node_properties n1 | |||
LEFT JOIN ${CONTAINMENTS_TABLE} con ON con.node_id = n1.id | |||
LEFT JOIN ${CONTAINMENTS_TABLE} con ON con.node_id = n1.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra white space at the end of the line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
configuration.md
Outdated
@@ -36,7 +36,9 @@ Below is the server-config.json with all default values | |||
// Values are "always" | "never" | "if-not-exists" | |||
create: "if-not-exists" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create: "if-not-exists" | |
"create": "if-not-exists", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ""
That is correct.
Indeed, they should still work.
I do not think it is possible to check a node for its LionWeb version, this information is not available. In a chunk of LionWeb nodes the only way to find the LionWeb version is the SerializationFormatVersion. What want to avoid are scenarios like
One way of doing the check is to provide the LionWeb version as e.g. a parameter of the bulk import call, then this can be checked once (so it won't affect the performance). This way in step 1 I can state my expectation that the repo should support 2024.1 |
@@ -245,9 +261,8 @@ export function nodesToChunk(nodes: LionWebJsonNode[]): LionWebJsonChunk { | |||
return { | |||
serializationFormatVersion: "2023.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the serialization format should depend on the lionWebVersion of the repository
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, changes this (at several places)
@@ -245,9 +261,8 @@ export function nodesToChunk(nodes: LionWebJsonNode[]): LionWebJsonChunk { | |||
return { | |||
serializationFormatVersion: "2023.1", | |||
languages: collectUsedLanguages(nodes), | |||
nodes: nodes, | |||
nodes: nodes | |||
} | |||
} | |||
|
|||
export const EMPTY_CHUNK = nodesToChunk([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const EMPTY_CHUNK = nodesToChunk([]) | |
export const EMPTY_CHUNKS: { [K in LionWebVersion]: LionWebJsonChunk } = { | |
"2023.1": nodesToChunk([], "2023.1"), | |
"2024.1": nodesToChunk([], "2024.1"), | |
}; |
I would suggest changing EMPTY_CHUNK into a map and update usages from EMPTY_CHUNK to EMPTY_CHUNKS[repositoryData.repository.lionweb_version]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done as suggested
configuration.md
Outdated
"history": false | ||
"history": false, | ||
// Values can be: "2023.1" | "2024.1" | ||
lionWebVersion: "2024.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lionWebVersion: "2024.1" | |
"lionWebVersion": "2024.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Done, 0 left |
Changes in Version 0.3.0