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.

Sign up for GitHub

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

Jump to bottom

refactor: add entity id #3170

Merged
merged 2 commits into from
Oct 16, 2024
Merged

refactor: add entity id #3170

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17d21e8
refactor: add entity id
samuelstroschein Oct 15, 2024
754eff8
fixes innerJoin queries on change table
martin-lysk Oct 16, 2024
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
115 changes: 59 additions & 56 deletions lix/packages/sdk/src/change-queue.test.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we remove the lix.commit

Copy link
Member Author

Choose a reason for hiding this comment

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

I propose to remove it in another PR. One step at a time

Copy link
Contributor

Choose a reason for hiding this comment

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

was just about that one line in the test (as you where cleaning up the discussion tests as well - but not important)

Original file line number Diff line number Diff line change
Expand Up @@ -4,64 +4,28 @@ import { newLixFile } from "./newLix.js";
import type { DiffReport, LixPlugin } from "./plugin.js";

test("should use queue and settled correctly", async () => {
const mockPlugin: LixPlugin<{
text: { id: string; text: string };
}> = {
const mockPlugin: LixPlugin = {
key: "mock-plugin",
glob: "*",
diff: {
file: async ({ before, after }) => {
const dec = new TextDecoder();
// console.log("diff", after, before?.data, after?.data);
const textBefore = dec.decode(after?.data);
const textAfter = dec.decode(before?.data);
const textBefore = before
? new TextDecoder().decode(before?.data)
: undefined;
const textAfter = after
? new TextDecoder().decode(after.data)
: undefined;

if (textBefore === textAfter) {
return [];
}

return await mockPlugin.diff.text({
before: before
? {
id: "test",
text: textAfter,
}
: undefined,
after: after
? {
id: "test",
text: textBefore,
}
: undefined,
});
},
text: async ({ before, after }) => {
// console.log("text", before, after);
if (before?.text === after?.text) {
return [];
}

return [
!before
? {
type: "text",
before: undefined,
after: {
id: "test",
text: after?.text,
},
}
: {
type: "text",
before: {
id: "test",
text: before?.text,
},
after: {
id: "test",
text: after?.text,
},
},
{
type: "text",
entity_id: "test",
before: textBefore ? { text: textBefore } : undefined,
after: textAfter ? { text: textAfter } : undefined,
},
];
},
},
Expand Down Expand Up @@ -118,7 +82,20 @@ test("should use queue and settled correctly", async () => {
const changes = await lix.db
.selectFrom("change")
.innerJoin("snapshot", "snapshot.id", "change.snapshot_id")
.selectAll()
.select([
"change.id",
"change.author",
"change.created_at",
"change.snapshot_id",
"change.parent_id",
"change.entity_id",
"change.type",
"change.file_id",
"change.plugin_key",
"snapshot.value as value",
"change.meta",
"change.commit_id",
])
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required because an .innerJoin("snapshot", "snapshot.id", "change.snapshot_id") select all overshadows the change.id with the snapshot.id

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh god what a beast - this combined with flaky run envs in test (importing from dist...) Sounds like fun.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into the code - seems there are a couple more places where this is still a problem

Copy link
Contributor

@martin-lysk martin-lysk Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we can use some bits in the uuid v7 to encode the entity type (like id of a change vs id of a snapshot) - In a former project we used some of the machine id bits in snowflake to signal the type of the id. This allowed us to runtime assert correct entities early based on passed id's - saved us hours, if not days of debugging and post mortems.

Not the solution - and I need into uuid v7 but to outline what I have in mind:

If every id of a change must end with a C
40e97286-9b78-4a35-8fb1-0036bdfbbf7C

Every id of a snapshot ends with an S
40e97286-9b78-4a35-8fb1-0036bdfbbf7S

We could assert in a function like getLeafChange that the id is an expected one:

getLeafChange(change) {
  if (!change.id.endsWith('C')) {
    throw new WrongObjectTypePassedError("The Id passed is not the id of a change")
  } 
 ....
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I double checked kysely we can make use selectAll with 'change' as parameter and and an explicit select

Suggested change
.select([
"change.id",
"change.author",
"change.created_at",
"change.snapshot_id",
"change.parent_id",
"change.entity_id",
"change.type",
"change.file_id",
"change.plugin_key",
"snapshot.value as value",
"change.meta",
"change.commit_id",
])
+ .selectAll('change')
+ .select('snapshot.value')

which is translated to:

SELECT
  "change".*,
  "snapshot"."value"

I will commit on this pr and fix the other places where we make use of innerjoin as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow I was looking for select('change.*'). I didn't think about using selectAll('change'). Nice.

.execute();

expect(changes).toEqual([
Expand All @@ -128,11 +105,11 @@ test("should use queue and settled correctly", async () => {
created_at: changes[0]?.created_at,
snapshot_id: changes[0]?.snapshot_id,
parent_id: null,
entity_id: "test",
type: "text",
file_id: "test",
plugin_key: "mock-plugin",
value: {
id: "test",
text: "test",
},
meta: null,
Expand Down Expand Up @@ -187,7 +164,20 @@ test("should use queue and settled correctly", async () => {
const updatedChanges = await lix.db
.selectFrom("change")
.innerJoin("snapshot", "snapshot.id", "change.snapshot_id")
.selectAll()
.select([
"change.id",
"change.author",
"change.created_at",
"change.snapshot_id",
"change.parent_id",
"change.entity_id",
"change.type",
"change.file_id",
"change.plugin_key",
"snapshot.value as value",
"change.meta",
"change.commit_id",
])
.execute();

expect(updatedChanges).toEqual([
Expand All @@ -197,11 +187,11 @@ test("should use queue and settled correctly", async () => {
created_at: updatedChanges[0]?.created_at,
snapshot_id: updatedChanges[0]?.snapshot_id,
parent_id: null,
entity_id: "test",
type: "text",
file_id: "test",
plugin_key: "mock-plugin",
value: {
id: "test",
text: "test",
},
meta: null,
Expand All @@ -210,6 +200,7 @@ test("should use queue and settled correctly", async () => {
{
author: null,
commit_id: null,
entity_id: "test",
created_at: updatedChanges[1]?.created_at,
snapshot_id: updatedChanges[1]?.snapshot_id,
file_id: "test",
Expand All @@ -219,7 +210,6 @@ test("should use queue and settled correctly", async () => {
plugin_key: "mock-plugin",
type: "text",
value: {
id: "test",
text: "test updated text",
},
},
Expand All @@ -232,10 +222,10 @@ test("should use queue and settled correctly", async () => {
id: updatedChanges[2]?.id,
meta: null,
parent_id: updatedChanges[1]?.id,
entity_id: "test",
plugin_key: "mock-plugin",
type: "text",
value: {
id: "test",
text: "test updated text second update",
},
},
Expand All @@ -252,8 +242,21 @@ test("changes should contain the author", async () => {
file: vi.fn().mockResolvedValue([
{
type: "mock",
entity_id: "mock",
before: undefined,
after: {} as any,
after: {
text: "value1",
},
},
{
type: "mock",
entity_id: "mock",
before: {
text: "value1",
},
after: {
text: "value2",
},
},
] satisfies DiffReport[]),
},
Expand Down
Loading
Loading