H-345: Fix `hash-graph-client` class names #2872

indietyp · 2023-08-07T17:21:14Z

🌟 What is the purpose of this PR?

This fixes the class names generated by hash-graph-client done through a combination of factors.

use the title to generate the name
supply titles to missing things
factor out common definitions into separate files from models

🔗 Related links

H-346: Add hash-graph-sdk Python functionality #2868 (comment)

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

do not affect the execution graph

📹 Demo

indietyp · 2023-08-07T17:53:57Z

Moved most things into shared (that can be shared). @TimDiekmann please sanity check!

To be able to accommodate these changes I switched from the swagger-cli to redocly/cli. Only problem: Now, propertyNames is no longer resolved.

(turns out that datamodel-code-generator supports them, even tho they are not in the spec, but redocly ignores them).

There are still some left that have numbers in their name, I'll try to remove them in one of the other commits.

indietyp · 2023-08-07T17:56:13Z

The issue in question about patternProperties: Redocly/redocly-cli#792

linear · 2023-08-07T19:32:21Z

H-345

TimDiekmann

The names are much much better!

libs/@local/hash-graph-client/python/graph_client/models.py

TimDiekmann · 2023-08-08T07:24:26Z

libs/@local/hash-graph-client/python/graph_client/models.py



-class VersionedUrl(RootModel):
+class DataType(BaseModel):


I wonder if we can use downstream either the custom models I've written (removing this) or use this as base class for the custom model. I think they match?

That should be possible indeed, only has the side-effect that we do not have a common Schema base (which is helpful in some circumstances), but using --reuse-model we can probably coerce datamodel-code-generator into creating one^^

The types should now have parity with the ones in the hash-graph-types; I'd personally prefer if we keep the "chain," meaning graph-client as the base (as it is the most low level). The only problem I see is that (unlike Rust) Python offers no real way for composing and extending existing classes, except for subclassing.

As this is already a BaseModel this should be fine I guess?

codecov · 2023-08-08T13:26:25Z

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 51.72%. Comparing base (1cede36) to head (34d236f).
Report is 2618 commits behind head on main.

Files with missing lines	Patch %	Lines
apps/hash-graph/lib/graph/src/api/rest.rs	0.00%	13 Missing ⚠️
...hash-graph/lib/graph/src/subgraph/temporal_axes.rs	0.00%	10 Missing ⚠️
apps/hash-graph/lib/graph/src/ontology.rs	0.00%	4 Missing ⚠️
apps/hash-graph/lib/graph/src/api/rest/entity.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2872      +/-   ##
==========================================
- Coverage   51.76%   51.72%   -0.05%     
==========================================
  Files         337      337              
  Lines       28901    28916      +15     
  Branches      431      431              
==========================================
- Hits        14960    14956       -4     
- Misses      13939    13958      +19     
  Partials        2        2

Flag	Coverage Δ
backend-integration-tests	`7.34% <ø> (ø)`
hash-graph	`54.40% <0.00%> (-0.08%)`	⬇️
unit-tests	`0.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

indietyp · 2023-08-08T13:40:59Z

Overview of the typescript changes

indietyp · 2023-08-08T13:45:20Z

apps/hash-graph/lib/graph/src/api/rest/json_schemas/status_definitions.json

I needed to move this out, because openapi-generator doesn't understand nested relative paths properly.

If I am pointing at ./shared.json in models/property_type.json it still searches for shared.json in the openapi directory. Like datamodel-code-generator I switched to @redocly/cli to bundle everything beforehand, but that comes with it's own challenges, for status.json it doesn't remove the definitions, and instead of ignoring it, openapi-generator just errors out, moving this to a separate filed solved the problem.

Although i don't know why the problem wasn't there in the first place... 🤔

apps/hash-graph/lib/graph/src/api/rest/json_schemas/status.json

TimDiekmann · 2023-08-08T15:12:21Z

apps/hash-graph/lib/graph/src/shared/identifier/time/axis.rs

+pub trait TemporalAxis {
+    /// The name of the temporal axis.
+    fn noun() -> &'static str;
+}


Honestly: I really don't like this. This feels like adding reflection to a type where it does not belong to. I see why you did that. If we don't find a better solution or want to implement this manually we should move this trait to utoipa_typedefs.rs as this absolutely does not belong here.

I first had this in utoipa_typedefs.rs (under the name TemporalSchema or TemporalAxisInfo), but the schema for the axis isn't located utoipa_typedef, and I didn't want to pub out of api/rest.

(Personally also don't really like this, but it was the only way I found works in a Rust-like way without too much overhead.)

Is it only used in temporal_asxes.rs? Then let's define it there and make it private to that module.

libs/@local/hash-graph-client/typescript/package.json

TimDiekmann · 2023-08-08T15:16:29Z

libs/@local/hash-graph-client/typescript/package.json

@@ -7,7 +7,7 @@
  "main": "index.ts",
  "scripts": {
    "build": "echo 'no build needed, this is here to orchestrate turborepo'",
-    "codegen": "openapi-generator-cli generate"
+    "codegen": "redocly bundle --format=json -o openapi.bundle.json ../../../../apps/hash-graph/openapi/openapi.json && openapi-generator-cli generate && rm openapi.bundle.json"


How does this play together with the caching as this is creating a temporary file?

Nothing changes; it's simply a buffer, as I had some problems directly piping in stdout. The bundle output is the same if the schema is the same, so no worries.

apps/hash-graph/lib/graph/src/api/rest/utoipa_typedef.rs

TimDiekmann · 2023-08-08T15:45:10Z

apps/hash-graph/lib/graph/src/shared/identifier/time/axis.rs

+pub trait TemporalAxis {
+    /// The name of the temporal axis.
+    fn noun() -> &'static str;
+}


Is it only used in temporal_asxes.rs? Then let's define it there and make it private to that module.

Co-authored-by: Tim Diekmann <[email protected]>

feat: use the title for code generation

b74b199

github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team labels Aug 7, 2023

vercel bot deployed to Preview – hashdotdev August 7, 2023 17:26 View deployment

vercel bot deployed to Preview – hashdotdesign August 7, 2023 17:30 View deployment

feat: unify specs

d24b5c6

vercel bot deployed to Preview – hash August 7, 2023 17:35 View deployment

feat: unify even more

fb76e07

github-actions bot added area/deps Relates to third-party dependencies (area) area/apps area/apps > hash-graph labels Aug 7, 2023

vercel bot deployed to Preview – hashdotdev August 7, 2023 17:56 View deployment

vercel bot deployed to Preview – hash August 7, 2023 18:02 View deployment

vilkinsons changed the title ~~Fix hash-graph-client class names~~ H-345: Fix hash-graph-client class names Aug 7, 2023

TimDiekmann previously approved these changes Aug 8, 2023

View reviewed changes

indietyp mentioned this pull request Aug 8, 2023

H-346: Add hash-graph-sdk Python functionality #2868

Merged

3 tasks

indietyp added 9 commits August 8, 2023 09:54

fix: generation

6a099f2

feat: unify PropertyValues

d903513

chore: update models.py

98d133b

feat: include relative pointers to OpenAPI schema

f2b2b3d

chore: update models.py

56d2d05

feat: give CustomEntityType explicit names

0cf53b4

feat: explicitely name axis

3709f06

feat: aliases field_ prefixed items

66f3398

feat: run black on models.py and common base class for types

75ba5c8

indietyp dismissed TimDiekmann’s stale review via 75ba5c8 August 8, 2023 13:07

vercel bot had a problem deploying to Preview – hash August 8, 2023 13:21 Failure

vercel bot deployed to Preview – hashdotdev August 8, 2023 13:26 View deployment

vercel bot deployed to Preview – hashdotdesign August 8, 2023 13:30 View deployment

feat: adjust for openapi-generator

d243e97

github-actions bot added the area/infra Relates to version control, CI, CD or IaC (area) label Aug 8, 2023

chore: lock

ee3aded

indietyp commented Aug 8, 2023

View reviewed changes

vercel bot had a problem deploying to Preview – hash August 8, 2023 13:49 Failure

satisfy lint

6c593d5

vercel bot deployed to Preview – hashdotdesign August 8, 2023 14:07 View deployment

vercel bot deployed to Preview – hashdotdev August 8, 2023 14:12 View deployment

vercel bot deployed to Preview – hash August 8, 2023 14:18 View deployment

TimDiekmann reviewed Aug 8, 2023

View reviewed changes

apps/hash-graph/lib/graph/src/api/rest/json_schemas/status.json Outdated Show resolved Hide resolved

TimDiekmann requested changes Aug 8, 2023

View reviewed changes

TimDiekmann reviewed Aug 8, 2023

View reviewed changes

indietyp and others added 2 commits August 8, 2023 17:50

feat: add comment json schema problems

5e08aea

feat: remove base_url_schema

ed0fd0e

Co-authored-by: Tim Diekmann <[email protected]>

vercel bot deployed to Preview – hash August 8, 2023 16:04 View deployment

chore: move TemporalAxis to TemporalAxisSchema

34d236f

indietyp requested a review from TimDiekmann August 8, 2023 17:04

vercel bot deployed to Preview – hash August 8, 2023 17:07 View deployment

TimDiekmann approved these changes Aug 8, 2023

View reviewed changes

indietyp added this pull request to the merge queue Aug 8, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 8, 2023

TimDiekmann added this pull request to the merge queue Aug 8, 2023

Merged via the queue into main with commit 04eb03a Aug 8, 2023

TimDiekmann deleted the bm/graph/name-fix branch August 8, 2023 20:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

H-345: Fix `hash-graph-client` class names #2872

H-345: Fix `hash-graph-client` class names #2872

indietyp commented Aug 7, 2023

indietyp commented Aug 7, 2023

indietyp commented Aug 7, 2023

linear bot commented Aug 7, 2023

TimDiekmann left a comment

TimDiekmann Aug 8, 2023

indietyp Aug 8, 2023

indietyp Aug 8, 2023

TimDiekmann Aug 8, 2023

codecov bot commented Aug 8, 2023 •

edited

Loading

indietyp commented Aug 8, 2023

indietyp Aug 8, 2023 •

edited

Loading

TimDiekmann Aug 8, 2023

indietyp Aug 8, 2023

TimDiekmann Aug 8, 2023

TimDiekmann Aug 8, 2023

indietyp Aug 8, 2023

TimDiekmann Aug 8, 2023

H-345: Fix hash-graph-client class names #2872

H-345: Fix hash-graph-client class names #2872

Conversation

indietyp commented Aug 7, 2023

🌟 What is the purpose of this PR?

🔗 Related links

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

📜 Does this require a change to the docs?

🕸️ Does this require a change to the Turbo Graph?

📹 Demo

indietyp commented Aug 7, 2023

indietyp commented Aug 7, 2023

linear bot commented Aug 7, 2023

TimDiekmann 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

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

indietyp commented Aug 8, 2023

indietyp Aug 8, 2023 • 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

H-345: Fix `hash-graph-client` class names #2872

H-345: Fix `hash-graph-client` class names #2872

codecov bot commented Aug 8, 2023 •

edited

Loading

indietyp Aug 8, 2023 •

edited

Loading