Update Entity Type endpoints to return Subgraph #1220

Alfred-Mountfield · 2022-10-19T16:32:24Z

🌟 What is the purpose of this PR?

This follows on from #1198. It modifies property type methods front to back to use the Subgraph type.

It fixes a bug with the GraphQL definitions to correctly allow the depths to be configured, and updates the hooks to use the new definitions.

It introduces a suite of helper methods for the Subgraph object to be used in the frontend (for now, these should probably be moved elsewhere when they are more mature)

It also updates frontend code to use the new helper methods and deal with the new return types. Relevant people have been tagged

🔗 Related links

Asana task (internal)

🔍 What does this change?

Commits should be very nice and clean, please consult them.

📜 Does this require a change to the docs?

It probably will, this will happen as a follow-up (tracked in this asana task (internal))

⚠️ Known issues

The GraphQL types are quite strange and unergonomic while we're in the interim state right now (e.g. Entity having EntityTypeRootedSubgraph on it which is actually a Subgraph object)
Knowing where to import types from is neither intuitive, nor consistent. The main culprits are the Persisted* types which can be imported from various GraphQL definitions, the hash-graph-client package, or from the actual original source of the scalars that go into GraphQL. I'm not entirely sure how to solve this right now. I think the proper solution is establishing a clear boundary of what does and doesn't reach the frontend through the BP, and then defining those types universally through the type system package.
The strategy to pick depths to query is also unclear, at the moment some guesses have been made within the hook implementations. We probably want to expose them optionally to the end user.

🐾 Next steps

Migrate LinkType endpoints to use Subgraph
Migrate Link endpoints to use Subgraph
Migrate Entity endpoints to use Subgraph
Remove any code that has been made redundant as a result of this work
Update documentation

🛡 What tests cover this?

This was a full stack change, so all the tests of HASH should be checked. These include the ones outlined in the Graph README, as well as workspace integration tests, and manual testing of the application.

❓ How to test this?

Follow the READMEs for the graph and for HASH to run tests
Look at CI
Run yarn external-services up --build and yarn dev
- Try the normal pages
- Try the type-editor route
- Try following testing instructions from entity type editor PRs such as Entity type editor – Show correct properties in property selector, add edit bar, and minor changes #1184
- Try following testing instructions from entity editor PRs such as Improve entity editor - 1 #1207

Screen.Recording.2022-10-19.at.17.26.18.mov

Alfred-Mountfield · 2022-10-19T16:33:25Z

packages/hash/frontend/src/lib/subgraph.ts

+
+// ------------------- Type Guards to use inside .filters -------------------
+
+export const isDataTypeVertex = (vertex: Vertex): vertex is DataTypeVertex => {


These are necessary because Typescript doesn't seem to understand that vertex.kind === "dataType" constrains the type if you continue past a filter (for example with a map)

Alfred-Mountfield · 2022-10-19T16:35:50Z

packages/hash/frontend/src/lib/subgraph.ts

@@ -0,0 +1,231 @@
+import {


cc: @yusufkinatas @nathggns @luisbettencourt @CiaranMn

As the Subgraph becomes more prevalent in the frontend code, I suspect we'll want more helper methods to interact with it. I have created this file to hold them while we flesh such things out. Tagging you so you know they're here, hopefully it'll save you frustration

Alfred-Mountfield · 2022-10-19T16:38:11Z

packages/hash/frontend/src/lib/entities.ts

-        .find((item) => item.propertyTypeId.startsWith(propertyTypeId))
-        ?.propertyType.title.toLowerCase(),
-    }));
+  const propertyTypes: { title?: string; propertyTypeBaseUri: string }[] =


@yusufkinatas just wanted to flag that this has been changed.

Alfred-Mountfield · 2022-10-19T16:39:11Z

packages/hash/frontend/src/lib/entities.ts

+        propertyTypeBaseUri,
+      );
+
+      /** @todo - pick the latest version? */


We're going through the properties of an entity here, so the keys are base URIs. That means that we could potentially have multiple versions of the type that matches.

I suspect the best thing to do would be to pick the latest, or to look at the entity's entity type and find the version of the property type used inside that.

Alfred-Mountfield · 2022-10-19T16:39:46Z

packages/hash/frontend/src/pages/[account-slug]/entities/[entity-id].page/property-table.tsx

@@ -61,6 +62,7 @@ interface PropertyTableProps {

 type EnrichedPropertyType = PropertyType & {
  value: any;
+  /** @todo - Correct this, it is a property type BaseUri not an ID (it's unversioned) */


@yusufkinatas I didn't want to change too much in the PR but we should try and distinguish between baseUris and the IDs of types (VersionedUris)

And to flag that the most recent commit moves these changes and TODO 9ffa4cd

Alfred-Mountfield · 2022-10-19T16:40:12Z

packages/hash/frontend/src/pages/[account-slug]/entities/[entity-id].page/property-table.tsx

@@ -69,15 +71,22 @@ const getDataTypesOfPropertyType = (
  propertyType: PropertyType,
  entity: EntityResponse,
 ) => {
-  /** @todo check why propertyValue does not have with a proper type  */


It did have the proper type, I'm not sure what this was about 😁

Alfred-Mountfield · 2022-10-19T16:41:40Z

packages/hash/frontend/src/pages/[account-slug]/entities/[entity-id].page/property-table.tsx

+      if (dataTypeVertex?.kind !== "dataType") {
+        throw new Error(
+          `Expected "dataType" vertex but got ${dataTypeVertex.kind}`,
+        );
+      }


Not sure how we want to handle these sorts of things. I wrote this before I wrote the helper methods so we could use isDataType here, but generally speaking I think we should trust that a datatype ID will go to a data type? And just use type assertions?

Not sure, maybe we'd prefer more safety, or maybe we'd prefer it to be a bit faster and avoid the checks.

cc: @CiaranMn if you have any thoughts

I expect we would need to have millions of these checks before it made any discernible difference to performance.

We can remove them if something appears slow and it would make any difference.

Valid, but also a question of code ergonomics. Do we want to be writing the checks everywhere, or do we just want to write as DataTypeVertex

I'm not against asserting the type if we're 100% sure it logically must be what we are asserting it is, but I don't think we should do so because of performance worries in this instance.

Do we want to be writing the checks everywhere, or do we just want to write as DataTypeVertex

Presumably there's only a limited number of ways in which we want to retrieve a thing from the subgraph and check it is what it's supposed to be, so we should have a limited number of functions that do that, and keep the checks inside them.

I don't feel strongly about this, if I saw this and it had an assertion with a comment explaining that it was definitely going to be the thing I wouldn't care. But we should comment any assertion.

Do we want to be adding a comment everywhere we do this:

hash/packages/hash/frontend/src/pages/[account-slug]/entities/[entity-id].page/types-section.tsx

Lines 43 to 47 in fb8ad44

const entityTypeTitle = (

entity.entityTypeRootedSubgraph.vertices[

entity.entityTypeId

]! as EntityTypeVertex

).inner.inner.title;

Or do we want to just create methods for all of them:
getDataType(dataTypeId: VersionedUri, subgraph: Subgraph): PersistedDataType
getPropertyType(propertyTypeId: VersionedUri, subgraph: Subgraph): PersistedPropertyType

and so on

I would create methods for them. Again, I don't feel strongly about this, I'm offering an opinion because you've asked, feel free to think it's too cumbersome

I'll add the methods :)

I've added them: 6dc0054
And used them: 871caba

Seem to improve ergonomics quite a bit. I'm wondering about naming, they'll clash a bit with BP hooks if we remove the Persisted prefix, (e.g. getDataType) so I'm wondering about maybe extractDataType?

Alfred-Mountfield · 2022-10-19T16:42:28Z

packages/hash/frontend/src/pages/[account-slug]/entities/[entity-id].page/property-table.tsx

-        val.propertyTypeId.startsWith(propertyTypeId),
-      ) ?? {};
+  return Object.keys(entity.properties).map((propertyTypeBaseUri) => {
+    const propertyTypeVersions = getPropertyTypesByBaseUri(


I've made a little helper method to encapsulate finding types by their Base URI. Importantly this returns a list now, not just using find, because there could be multiple.

Alfred-Mountfield · 2022-10-19T16:43:26Z

packages/hash/frontend/src/pages/[account-slug]/types/entity-type/use-entity-type.tsx

@@ -35,19 +35,23 @@ export const useEntityType = (
    entityTypeRef.current = null;

    void aggregateEntityTypes({ data: {} }).then(async (res) => {


cc: @nathggns, just want to check you're happy with these specific changes

Yeah I'm happy with this, thanks @Alfred-Mountfield

Alfred-Mountfield · 2022-10-19T16:44:13Z

packages/hash/frontend/src/pages/entity-editor.page.tsx

+        ? propertyTypeIds.map((propertyTypeId) => {
+            const vertex = entityTypeRootedSubgraph.vertices[propertyTypeId];
+
+            /** @todo - what to do here if the property type isn't in the subgraph, make another call? */


This is a wider question I have and am unsure of. It maybe can be solved when we also allow the depths to be configured through the BP functions?

cc: @CiaranMn for thoughts on BP ergonomics

I expect in practice that most people are going to interact with types via queries that resolve all property, data, and link types referred to by a type (including within property types etc). We should make that easy.

If they choose not to they can make another call to get missing things.

I think for the entity type -> property type -> data type tree, having max depth parameters (255) automatically populated is probably reasonable for most queries. I'm not sure about entity type -> entity type though. (Constraints on links for example), that could blow up the query very fast.

And it gets a lot less clear to me when we're dealing with knowledge graph stuff, not just ontology. For queries like getEntity or aggregateEntities do we really want to return the full entity type -> property type -> data type tree?

It maybe can be solved when we also allow the depths to be configured through the BP functions?

Hence me thinking that. But it's probably good thinking about the patterns whereby we're going to need to make additional lookups (or account for it). Do we want to just assume that the full thing will fit in 255 (pretty safe bet for types in my eyes) and fatally error if it's not there? Or do we want to have fallback logic for follow-up queries on every single element kind, any time we look for something in the subgraph.

Do we want to just assume that the full thing will fit in 255 (pretty safe bet for types in my eyes) and fatally error if it's not there?

I think that's fine for types.

Or do we want to have fallback logic for follow-up queries on every single element kind, any time we look for something in the subgraph.

For some uses we will want that. I don't think it will come up in this particular one. We should not pollute the 'get thing from subgraph' function with follow-up query logic though. There could be some other function that does that and calls the 'get thing from subgraph'to check first, and then gets it from the API as needed, but I wouldn't worry about that until we need it.

We should not pollute the 'get thing from subgraph' function with follow-up query logic though

100% on the same page there, otherwise you'd have to pass in all the hooks, it'd be a massive pain.

We can leave it as is then, I just have a mild concern we won't think about the edge-cases where we will want that until we discover a bug in prod

...lug]/entities/[entity-id].page/property-table/extract-enriched-property-types-from-entity.ts

thehabbos007

Thanks Alfie! It is looking good so far. Have some things we should look at, though
(thought this was submitted as a review, but I had added single comments. My bad.)

lgtm-com · 2022-10-20T11:48:43Z

This pull request introduces 3 alerts and fixes 2 when merging 20e9512 into 244d2b6 - view on LGTM.com

new alerts:

3 for Unused variable, import, function or class

fixed alerts:

2 for Useless conditional

Alfred-Mountfield · 2022-10-20T11:53:13Z

packages/hash/frontend/src/pages/[account-slug]/types/entity-type/use-property-types.tsx

  const [propertyTypes, setPropertyTypes] =
    useState<PropertyTypesContextValues | null>(null);
  const { aggregatePropertyTypes } = useBlockProtocolAggregatePropertyTypes();

  useEffect(() => {
-    void aggregatePropertyTypes({ data: {} }).then(({ data }) => {
+    if (typeSystemLoading) {


@nathggns I've had to add this check, is that alright?

I thought we were going to wrap all of the pages in these but it seems we're calling them in individual places?

I think we're doing this on an individual page level still – but i've changed this in a follow up PR to share the status by context. This seems a sensible change

thehabbos007

Thanks! We got the last issues ironed out and it looks good to me. Tested various pages locally

lgtm-com · 2022-10-20T12:03:39Z

This pull request fixes 2 alerts when merging d52565a into 244d2b6 - view on LGTM.com

fixed alerts:

2 for Useless conditional

Alfred-Mountfield added 13 commits October 19, 2022 15:00

Return Subgraph for get_entity_type

b77b05f

Regenerate api.ts

1c5a4be

Update GraphQL typedefs and resolvers

6f1095c

Update all Subgraph related GraphQL endpoints to take depth params

6955416

Fix resolvers

3990a47

Fix BP hooks

abdef74

Fix entity-type.model.ts

c5225ed

Add subgraph helper functions

13dba4c

Fix generateEntityLabel

4eb4107

Fix property-table.tsx

490dab1

Fix types-section.tsx

1529415

Fix use-entity-type.tsx

351520b

Fix entity-editor-page.tsx

ddd6c77

Alfred-Mountfield requested a review from a team October 19, 2022 16:32

github-actions bot added A-backend area/apps > hash* Affects HASH (a `hash-*` app) area/apps > hash-api Affects the HASH API (app) labels Oct 19, 2022

Alfred-Mountfield commented Oct 19, 2022

View reviewed changes

Alfred-Mountfield force-pushed the am/subgraph-entity-type branch from 9b9bff2 to 66cc9af Compare October 19, 2022 16:46

Driveby: fix dumb mistake

6c91714

Improve docs

5dd3e18

thehabbos007 reviewed Oct 20, 2022

View reviewed changes

...lug]/entities/[entity-id].page/property-table/extract-enriched-property-types-from-entity.ts Show resolved Hide resolved

thehabbos007 suggested changes Oct 20, 2022

View reviewed changes

vercel bot deployed to Preview – hash October 20, 2022 11:07 View deployment

Use destructuring

a7b47cc

Alfred-Mountfield requested a review from thehabbos007 October 20, 2022 11:10

vercel bot deployed to Preview – hash October 20, 2022 11:14 View deployment

Alfred-Mountfield mentioned this pull request Oct 20, 2022

Update Link Type endpoints to return Subgraph #1227

Merged

Alfred-Mountfield and others added 3 commits October 20, 2022 12:23

Temporarily remove usages of mustBeVersionedUri

d80fc1e

Merge branch 'main' into am/subgraph-entity-type

b27688d

Temporarily remove one more usage of mustBeVersionedUri

4020340

vercel bot deployed to Preview – hash October 20, 2022 11:37 View deployment

Cleanup

20e9512

vercel bot deployed to Preview – hash October 20, 2022 11:45 View deployment

Add type system loading check

ae95899

Alfred-Mountfield commented Oct 20, 2022

View reviewed changes

Cleanup

d52565a

thehabbos007 approved these changes Oct 20, 2022

View reviewed changes

vercel bot deployed to Preview – hash October 20, 2022 12:02 View deployment

hashintel deleted a comment from lgtm-com bot Oct 20, 2022

Alfred-Mountfield merged commit 99e5b7d into main Oct 20, 2022

Alfred-Mountfield deleted the am/subgraph-entity-type branch October 20, 2022 12:45

Alfred-Mountfield mentioned this pull request Oct 21, 2022

Return Subgraph from Entity endpoints in the Graph API #1236

Merged

vilkinsons added type/eng > frontend Owned by the @frontend team type/eng > backend Owned by the @backend team and removed area: frontend labels Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Entity Type endpoints to return Subgraph #1220

Update Entity Type endpoints to return Subgraph #1220

Alfred-Mountfield commented Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 20, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

CiaranMn Oct 19, 2022

Alfred-Mountfield Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 19, 2022

CiaranMn Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 20, 2022

Alfred-Mountfield Oct 19, 2022

Alfred-Mountfield Oct 19, 2022

nathggns Oct 20, 2022

Alfred-Mountfield Oct 19, 2022

CiaranMn Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 20, 2022

thehabbos007 left a comment •

edited

Loading

lgtm-com bot commented Oct 20, 2022

Alfred-Mountfield Oct 20, 2022

nathggns Oct 20, 2022

thehabbos007 left a comment

lgtm-com bot commented Oct 20, 2022


		// ------------------- Type Guards to use inside .filters -------------------

		export const isDataTypeVertex = (vertex: Vertex): vertex is DataTypeVertex => {

	const entityTypeTitle = (
	entity.entityTypeRootedSubgraph.vertices[
	entity.entityTypeId
	]! as EntityTypeVertex
	).inner.inner.title;

		@@ -35,19 +35,23 @@ export const useEntityType = (
		entityTypeRef.current = null;

		void aggregateEntityTypes({ data: {} }).then(async (res) => {

Update Entity Type endpoints to return Subgraph #1220

Update Entity Type endpoints to return Subgraph #1220

Conversation

Alfred-Mountfield commented Oct 19, 2022 • edited Loading

🌟 What is the purpose of this PR?

🔗 Related links

🔍 What does this change?

📜 Does this require a change to the docs?

⚠️ Known issues

🐾 Next steps

🛡 What tests cover this?

❓ How to test this?

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 Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

CiaranMn Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

CiaranMn Oct 19, 2022 • 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

CiaranMn Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Alfred-Mountfield Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

CiaranMn Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thehabbos007 left a comment • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Oct 20, 2022

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

lgtm-com bot commented Oct 20, 2022

Alfred-Mountfield commented Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

Alfred-Mountfield Oct 19, 2022 •

edited

Loading

CiaranMn Oct 19, 2022 •

edited

Loading

thehabbos007 left a comment •

edited

Loading