refactor: add entity id #3170

samuelstroschein · 2024-10-15T21:24:05Z

Changes

adds entity_id to a change
removes the & { id: string} requirement from snapshots
cleans tests

changeset-bot · 2024-10-15T21:24:08Z

⚠️ No Changeset found

Latest commit: 754eff8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

samuelstroschein · 2024-10-15T21:25:04Z

lix/packages/sdk/src/change-queue.test.ts

+		.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",
+		])


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

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

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

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") } .... }

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

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

samuelstroschein · 2024-10-15T21:25:41Z

lix/packages/sdk/src/discussion/discussion.test.ts

-	const changes = await lix.db
-		.selectFrom("change")
-		.innerJoin("snapshot", "snapshot.id", "change.snapshot_id")
-		.selectAll()
-		.execute();
-
-	// console.log(await lix.db.selectFrom("queue").selectAll().execute());
-
-	expect(changes).toEqual([
-		{
-			id: changes[0]?.id,
-			author: "Test User",
-			created_at: changes[0]?.created_at,
-			snapshot_id: changes[0]?.snapshot_id,
-			parent_id: null,
-			type: "text",
-			file_id: "test",
-			plugin_key: "mock-plugin",
-			value: {
-				id: "test",
-				text: "inserted text",
-			},
-			meta: null,
-			commit_id: null,
-		},
-	]);
-


The discussion test tested the change queue :D redundant

samuelstroschein · 2024-10-15T21:26:02Z

lix/packages/sdk/src/file-handlers.ts

-					});
-
-					if (previousCommittedDiff?.length === 0) {
+					const previousSnapshot = await trx


Very nice: The snapshot can now be queried on demand.

martin-lysk · 2024-10-16T07:52:58Z

lix/packages/sdk/src/change-queue.test.ts

Shall we remove the lix.commit

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

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

martin-lysk · 2024-10-16T07:59:45Z

lix/packages/sdk/src/file-handlers.ts

 	// console.log({ args });
 	for (const plugin of args.plugins) {
 		// glob expressions are expressed relative without leading / but path has leading /
 		if (!minimatch(normalizePath(args.after.path), "/" + plugin.glob)) {
 			break;
 		}

-		const diffs = await plugin.diff!.file!({
+		const diffs = await plugin.diff?.file?.({


Is the diff function optional for plugins? I would prefer to fail here - while we should check for undefined with a nice error message instead of the ! I would rather fail early instead of returning - no change silently

Will impement that in https://linear.app/opral/issue/LIXDK-107/plugindifffileortype-plugindetectchanges

martin-lysk · 2024-10-16T08:00:31Z

lix/packages/sdk/src/file-handlers.ts


 	for (const plugin of args.plugins) {
 		// glob expressions are expressed relative without leading / but path has leading /
 		if (!minimatch(normalizePath(args.after.path), "/" + plugin.glob)) {
 			break;
 		}

-		const diffs = await plugin.diff!.file!({
+		const diffs = await plugin.diff?.file?.({


see previous comment about failling instead of optional chaining

Will impement this in https://linear.app/opral/issue/LIXDK-107/plugindifffileortype-plugindetectchanges

martin-lysk · 2024-10-16T08:06:02Z

lix/packages/sdk/src/file-handlers.ts

 						author: args.currentAuthor,
 						parent_id: previousChange?.id,
-						snapshot_id: snapshotId,
+						snapshot_id: snapshot.id,


way better/cleaner than the (await ...).id

refactor: add entity id

17d21e8

samuelstroschein commented Oct 15, 2024

View reviewed changes

samuelstroschein requested a review from martin-lysk October 15, 2024 21:26

fixes innerJoin queries on change table

754eff8

martin-lysk reviewed Oct 16, 2024

View reviewed changes

samuelstroschein merged commit e16b755 into lix-integration Oct 16, 2024
2 checks passed

samuelstroschein deleted the lixdk-187-add-changeentity_id branch October 16, 2024 09:04

github-actions bot locked and limited conversation to collaborators Oct 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: add entity id #3170

refactor: add entity id #3170

samuelstroschein commented Oct 15, 2024

changeset-bot bot commented Oct 15, 2024 •

edited

Loading

samuelstroschein Oct 15, 2024

martin-lysk Oct 16, 2024

martin-lysk Oct 16, 2024

martin-lysk Oct 16, 2024 •

edited

Loading

martin-lysk Oct 16, 2024

samuelstroschein Oct 16, 2024

samuelstroschein Oct 15, 2024

samuelstroschein Oct 15, 2024

martin-lysk Oct 16, 2024

samuelstroschein Oct 16, 2024

martin-lysk Oct 16, 2024

martin-lysk Oct 16, 2024

samuelstroschein Oct 16, 2024

martin-lysk Oct 16, 2024

samuelstroschein Oct 16, 2024

martin-lysk Oct 16, 2024

refactor: add entity id #3170

refactor: add entity id #3170

Conversation

samuelstroschein commented Oct 15, 2024

changeset-bot bot commented Oct 15, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-lysk Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Oct 15, 2024 •

edited

Loading

martin-lysk Oct 16, 2024 •

edited

Loading