Introduce support for bi-temporal versioning in the Graph Service #928

Alfred-Mountfield · 2023-02-01T16:54:33Z

🌟 What is the purpose of this PR?

As expressed in previous PRs, we intend to add the foundations of support for bi-temporal datastores within the BP as part of the 0.3 release. This PR modifies the Graph Service to support such use-cases. It is necessarily a large PR as adding generics tends to propagate immensely.

I recommend you review this commit by commit.

🔗 Related links

Asana task (internal)

🚫 Blocked by

N/A

🔍 What does this change?

Please see commits :(

📜 Does this require a change to the docs?

As discussed in previous PRs, this absolutely will need changes to the docs. This is deferred to a further point, but effort has been made to have good in-code documentation.

⚠️ Known issues

We ideally want users to be able to import different views of the package.
- Blocks or EAs which don't care about (or do not support) temporal versioning should ideally be able to import types and functions as if there was no temporal versioning. It would be nice for us to be able to strip the generics, or prefill them in with defaults for these consumers
- Blocks which do care about temporal versioning should ideally be able to import types and functions where it was if there were no generics and instead everything had required temporal versioning. Similar to above, would be nice to be able to prefill or strip them or something.
- Blocks or EAs that need to handle general cases (EAs with support for temporal versioning may need to interface with blocks that do not have it for example) need to be able to import the types which have the generics in place, without default values. An example of such a consumer is MBD which will be updated in the next PR.
- I have been unable to find a way to ergonomically do this in this PR. Because of that, I have consolidated the stdlib and stdlib-temporal endpoints for now. And am exposing the most general implementations of everything on the API
Some other issues have been highlighted by GitHub comments on the diff

🐾 Next steps

Migrate MBD and fix other accompanying packages
Figure out how to expose the various endpoints

🛡 What tests cover this?

I'm unsure, at the very least tsc and eslint.
I wanted to write a proper suite of tests to check this, but made the uncomfortable compromise at the moment to defer that until later on, to unblock some further work.

❓ How to test this?

Confirm that lint:tsc and lint:eslint pass for the @blockprotocol/graph package
See Add support for bi-temporal versioning in MBD #931

📹 Demo

N/A

Alfred-Mountfield · 2023-02-01T16:58:10Z

libs/@blockprotocol/graph/src/stdlib/subgraph/builder.ts

+  // @ts-expect-error -- @todo, how do we convince TS that we're only setting this when the generic is satisfied
+  const subgraph: Subgraph<Temporal, EntityRootType<Temporal>> = {


Didn't want to sink much time into this but completely ignoring tsc here is also not great.. I haven't found a good way to inform TSC that the generic has been narrowed at certain points within function bodies. This is a recurring inconvenience and in the following PR there will be many places where it's something like if (subgraph.temporalAxes !== undefined)... and it would be really great if there was a way to then tell TS "hey Temporal is actually true here"

Alfred-Mountfield · 2023-02-01T17:00:51Z

libs/@blockprotocol/graph/src/stdlib/subgraph/element/map-revisions.ts

+  /*
+   * @todo - we _should_ be able to use `Extract<GraphElementForIdentifier<TemporalSupport, VertexId<any, any>>`
+   *   here to actually get a strong type (like `EntityId` or `BaseUri`). TypeScript seems to break on using it with a
+   *   generic though. So for now we write `string` because all of the baseId's of `VertexId` are string aliases anyway.
+   */


This is actually quite frustrating and if we end up making the BaseIds template strings or branded types this will catch us out.

Alfred-Mountfield · 2023-02-01T17:01:08Z

libs/@blockprotocol/graph/src/stdlib/subgraph/roots.ts

- * - isSubgraph<DataTypeRootType>
- * - isSubgraph<PropertyTypeRootType>
- * - isSubgraph<EntityTypeRootType>
- * - isSubgraph<EntityRootType>


Oops, slipped through on the previous one

Alfred-Mountfield · 2023-02-01T17:02:59Z

libs/@blockprotocol/graph/src/types/subgraph/edges.ts

-export * from "./edges/outward-edge-alias.js";
+export * from "./edges/variants.js";
+
+/** @todo - Re-express these and `Vertices` as `Record`s? */


I'm unsure why I ended up expressing them like this originally, it seems that they'd be better as Records, and perhaps they'd behave slightly nicer sometimes with narrowing.

I don't want to change it here though because I want to make sure it doesn't break any code in usage in MBD, so will probably look into this as part of the follow-up PR.

…temporalAxes

… revisions

CiaranMn

Phew! Thanks @Alfred-Mountfield, fantastic work.

My comments are mostly about naming / explaining stuff.

I started to optimise things but I think you're aware that we can reduce a lot of looping so I stopped.

I haven't looked at the TS errors you flag as I thought I'd get this in and then we can revisit the most egregious ones – let me know which you think might be worth further investigation.

CiaranMn · 2023-02-02T11:51:37Z

libs/@blockprotocol/graph/src/internal/mutate-subgraph.ts

+   * @todo This assumes that the left and right entity ID of a link entity is static for its entire lifetime, that is
+   *   not necessarily going to continue being the case


Presumably the relevant task to link would be whatever change we might introduce which affects this – what is it?

libs/@blockprotocol/graph/src/internal/mutate-subgraph.ts

libs/@blockprotocol/graph/src/types/subgraph/edges/outward-edge.ts

CiaranMn · 2023-02-02T15:19:37Z

libs/@blockprotocol/graph/src/types/subgraph/edges/outward-edge.ts

+export type EntityValidInterval = {
+  entityId: EntityId;
+  validInterval: TimeInterval<LimitedTemporalBound, TemporalBound>;
+};


Can we add a comment to this type explaining this please?

I think it would also help if the name captured that this relates to the period an edge was valid for rather than an entity.

e.g.

export type EdgeApplicableFor = { entityId: EntityId; interval: TimeInterval<LimitedTemporalBound, TemporalBound>; };

I also wondered if introducing the word 'valid' risked confusion with 'valid time'. We don't use it outside this context.

libs/@blockprotocol/graph/src/internal/mutate-subgraph.ts

libs/@blockprotocol/graph/src/stdlib/subgraph/element/map-revisions.ts

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts

CiaranMn · 2023-02-02T19:15:39Z

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts

+          /*
+           these casts are safe as we check for `targetRevisionInformation === undefined` above and that's only ever
+           defined if `Temporal extends true`
+           */
+          (entity as Entity<true>).metadata.temporalVersioning[
+            (subgraph as Subgraph<true>).temporalAxes.resolved.variable.axis
+          ],
+          targetTime,
+        );


Technically there's nothing to stop JS users passing a non-temporal subgraph and targetRevisionInformation but I know that worrying about that is a whole can of worms 😁

I've been slightly concerned about this too.. I hope they'll at least read the documentation? 😅

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts

thehabbos007

Great work, Alfie! I've gotten through most files now and added some comments :)

thehabbos007 · 2023-02-03T09:53:58Z

libs/@blockprotocol/graph/src/stdlib/subgraph/edge/link-entity.ts

+    ) as Entity<Temporal>[];
+  } else {
+    return getEntityRevisionsByEntityId(
+      subgraph as Subgraph<true>,


Wouldn't we have Subgraph<false> in this branch?

You are very correct

Fix temporal generics

thehabbos007 · 2023-02-03T09:58:56Z

libs/@blockprotocol/graph/src/stdlib/subgraph/edge/link-entity.ts

+    ) as Entity<Temporal>[];
+  } else {
+    return getEntityRevisionsByEntityId(
+      subgraph as Subgraph<true>,


Same as above

Same as above (you are very correct), thanks for spotting! Hard to find these things and annoying that TS doesn't care

Fix temporal generics

thehabbos007 · 2023-02-03T10:50:44Z

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts

+        intervalOverlapsInterval(
+          interval,
+          /*
+           these casts are safe as we check for `targetRevisionInformation === undefined` above and that's only ever


This targetRevisionInformation === undefined check doesn't seem t obe present in this function, it happens in getEntityRevision line 70. Is this check meant to happen here as well or is the comment not relevant?

It's a mistake, it should be interval !== undefined

Fix incorrect safety comment

libs/@blockprotocol/graph/src/types/entity.ts

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts

Alfred-Mountfield added 2 commits February 1, 2023 14:48

Make Entity generic over temporal support

f26a973

Make Vertices generic over temporal support

ad224c1

github-actions bot added area: libs Relates to first-party libraries/crates/packages (area) area: libs > @Þ/graph Affects the `@blockprotocol/graph` package (library) labels Feb 1, 2023

vercel bot had a problem deploying to Preview February 1, 2023 16:56 Failure

Alfred-Mountfield commented Feb 1, 2023

View reviewed changes

Alfred-Mountfield added 21 commits February 1, 2023 17:04

Re-express the outward edge unions explicitly

68e3e2a

Introduce EntityValidInterval for outward edges that point to entities

b53092d

Add subgraph TemporalAxes

6de2efd

Extend Subgraph type to optionally support temporal versioning and …

6f8a24d

…temporalAxes

Introduce an identifier to element map type

ad5a705

Fix entity type typedefs

0e15303

Fix subgraph mutation methods

65fa2d1

Add a getLatestInstantIntervalForSubgraph helper function

2358883

Mostly fix buildSubgraph

fccad9a

Fix the stdlib functions for ontology elements

166ce8f

Fix the stdlib functions for getting entities

f9db0d8

Fix getPropertyTypesReferencedByEntityType

1b0c1ec

Fix stdlib functions for link entities

00bc7f8

Add a stdlib function for mapping a collection of elements into their…

6869931

… revisions

Fix roots stdlib methods

f47d43d

Make Entity get* methods take temporal generic information

3cb0e17

Fix block-graph definition

69f68c4

Fix linked-aggregation.ts

87449f6

Fix GraphBlockHandler

a017105

Fix BlockElementBase

1033000

Fix GraphEmbedderHandler

6a8c436

vercel bot deployed to Preview February 2, 2023 18:14 View deployment

Take SubgraphTemporalAxes in buildSubgraph

e00c258

vercel bot deployed to Preview February 2, 2023 18:24 View deployment

CiaranMn reviewed Feb 2, 2023

View reviewed changes

Add doc-comment to EntityIdAndTimestamp

74d386f

thehabbos007 reviewed Feb 3, 2023

View reviewed changes

Alfred-Mountfield added 3 commits February 3, 2023 11:32

Rename TimeInterval types

6dd3528

Rename Axis types

5a6b510

Rename Axes types

3fec1ea

vercel bot had a problem deploying to Preview February 3, 2023 11:45 Failure

CiaranMn reviewed Feb 3, 2023

View reviewed changes

libs/@blockprotocol/graph/src/types/entity.ts Outdated Show resolved Hide resolved

Improve non-existing array initialization

262fdde

vercel bot had a problem deploying to Preview February 3, 2023 11:58 Failure

Alfred-Mountfield added 6 commits February 3, 2023 12:21

Remove deep nesting of rubbish functional iterators

d9e4be5

Replace slice(-1) syntax

538ac50

Replace unnecessary usages of TimeIntervalUnresolved

d4abfd5

Make TimeInterval independent of TimeIntervalUnresolved

619b421

Fix incorrect safety comment

49ee03b

Fix temporal generics

5c873ea

Alfred-Mountfield requested review from CiaranMn and thehabbos007 February 3, 2023 12:44

Alfred-Mountfield added 2 commits February 3, 2023 12:59

Fix incorrect axis

98d2a96

Fix missed Unresolved

df12923

vercel bot deployed to Preview February 3, 2023 14:26 View deployment

CiaranMn approved these changes Feb 3, 2023

View reviewed changes

libs/@blockprotocol/graph/src/stdlib/subgraph/element/entity.ts Outdated Show resolved Hide resolved

Clarity and remove unnecessary call

64d3c6c

Alfred-Mountfield merged commit 5da0787 into 0.3 Feb 3, 2023

Alfred-Mountfield deleted the am/temporal-graph branch February 3, 2023 15:56

vercel bot deployed to Preview February 3, 2023 16:01 View deployment

Alfred-Mountfield mentioned this pull request Feb 20, 2023

Abstract Temporal Versioning away from packages #990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce support for bi-temporal versioning in the Graph Service #928

Introduce support for bi-temporal versioning in the Graph Service #928

Alfred-Mountfield commented Feb 1, 2023 •

edited

Loading

Alfred-Mountfield Feb 1, 2023

Alfred-Mountfield Feb 1, 2023

Alfred-Mountfield Feb 1, 2023

Alfred-Mountfield Feb 1, 2023

CiaranMn left a comment

CiaranMn Feb 2, 2023

CiaranMn Feb 2, 2023

CiaranMn Feb 2, 2023

Alfred-Mountfield Feb 3, 2023

thehabbos007 left a comment

thehabbos007 Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

thehabbos007 Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

thehabbos007 Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

Alfred-Mountfield Feb 3, 2023

		// @ts-expect-error -- @todo, how do we convince TS that we're only setting this when the generic is satisfied
		const subgraph: Subgraph<Temporal, EntityRootType<Temporal>> = {

		* @todo This assumes that the left and right entity ID of a link entity is static for its entire lifetime, that is
		* not necessarily going to continue being the case

Introduce support for bi-temporal versioning in the Graph Service #928

Introduce support for bi-temporal versioning in the Graph Service #928

Conversation

Alfred-Mountfield commented Feb 1, 2023 • edited Loading

🌟 What is the purpose of this PR?

🔗 Related links

🚫 Blocked by

🔍 What does this change?

📜 Does this require a change to the docs?

⚠️ Known issues

🐾 Next steps

🛡 What tests cover this?

❓ How to test this?

📹 Demo

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

CiaranMn left a 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

thehabbos007 left a 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

Alfred-Mountfield commented Feb 1, 2023 •

edited

Loading