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.

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

H-3755: Improve validation structure for entity properties #5850

Merged

Conversation

TimDiekmann
Copy link
Member

🌟 What is the purpose of this PR?

H-3722 overhauled the validation results but kept the Report of the entity preprocessor. Those results also have to be adjusted to be more structured.
This PR adjusts the validation for properties so they return a structured validation report as well.

🔍 What does this change?

  • Change the pre-processor to return a validation structure instead of plain Reports
  • Redefine the types in TS so they can be properly used.

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

@TimDiekmann TimDiekmann requested a review from CiaranMn December 10, 2024 16:16
@TimDiekmann TimDiekmann self-assigned this Dec 10, 2024
@github-actions github-actions bot added area/deps Relates to third-party dependencies (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team labels Dec 10, 2024
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 22.97%. Comparing base (417362f) to head (59f7210).
Report is 16 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5850      +/-   ##
==========================================
- Coverage   23.00%   22.97%   -0.03%     
==========================================
  Files         568      569       +1     
  Lines       19160    19184      +24     
  Branches     2715     2717       +2     
==========================================
  Hits         4408     4408              
- Misses      14700    14724      +24     
  Partials       52       52              
Flag Coverage Δ
apps.hash-ai-worker-ts 1.32% <ø> (ø)
apps.hash-api 1.16% <ø> (ø)
local.hash-backend-utils 8.80% <ø> (ø)
local.hash-graph-sdk 58.62% <ø> (-41.38%) ⬇️
local.hash-isomorphic-utils 0.99% <ø> (ø)
local.hash-subgraph 24.54% <ø> (ø)

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.

Copy link
Member

@CiaranMn CiaranMn left a comment

Choose a reason for hiding this comment

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

Thanks @TimDiekmann, just a couple of comments. A suggestion on naming we can discuss if needed

error: Report;
}
| {
// The property was found at the entity but the type is not the expected type from Athe schema
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The property was found at the entity but the type is not the expected type from Athe schema
// The property was found at the entity but the property type is not the expected type from the schema

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 80 to 82
* Validation for each constraint in the `oneOf` field. The validation is assumed to pass if exactly one of the
* constraints passes. In this case, this field will be omitted. Whenever this field is present it can be assumed
* that the validation failed.
Copy link
Member

Choose a reason for hiding this comment

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

Exactly one, or at least one? We want the latter (and therefore want to rename oneOf to anyOf at some point)

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently it's exactly one. We can change this as part of this PR if you like.

Copy link
Member

Choose a reason for hiding this comment

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

Let's change it now so it doesn't trip us up later

Copy link
Member Author

Choose a reason for hiding this comment

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

ValueValidationReportGraphApi,
"actual" | "desired" | "abstract" | "incompatible"
> & {
// The actual value has a validation error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The actual value has a validation error
// The value could not be validated against the provided data type

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 140 to 142
actual?: ValueValidationError;
// The value could not be validated against the data type specified in the schema
desired?: ValueValidationError;
Copy link
Member

Choose a reason for hiding this comment

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

I slightly prefer provided over actual because I think it makes it clear that it's what the caller has provided – rather than 'actual' which could mean 'actually provided' or 'actually in the schema'.

Similar for 'desired' – is it desired by the caller or the schema? Although feel less strongly about this one.

Suggested change
actual?: ValueValidationError;
// The value could not be validated against the data type specified in the schema
desired?: ValueValidationError;
provided?: ValueValidationError;
// The value could not be validated against the data type specified in the schema
target?: ValueValidationError;

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 143 to 145
// The actual schema is abstract
abstract?: VersionedUrl;
// The actual schema is incompatible with the desired schema, i.e. the actual schema is not a subtype of the desired schema
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The actual schema is abstract
abstract?: VersionedUrl;
// The actual schema is incompatible with the desired schema, i.e. the actual schema is not a subtype of the desired schema
// The provided DataType is abstract
abstract?: VersionedUrl;
// The provided DataType is incompatible with the desired DataType, i.e. the actual DataType is not a subtype of the target DataType

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimDiekmann TimDiekmann requested a review from CiaranMn December 11, 2024 10:58
CiaranMn
CiaranMn previously approved these changes Dec 11, 2024
@github-actions github-actions bot added the area/tests New or updated tests label Dec 11, 2024
Copy link
Contributor

Benchmark results

@rust/hash-graph-benches – Integrations

representative_read_entity

Function Value Mean Flame graphs
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1 $$15.8 \mathrm{ms} \pm 135 \mathrm{μs}\left({\color{lightgreen}-31.181 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1 $$16.4 \mathrm{ms} \pm 172 \mathrm{μs}\left({\color{gray}1.12 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1 $$16.7 \mathrm{ms} \pm 193 \mathrm{μs}\left({\color{gray}2.10 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1 $$15.9 \mathrm{ms} \pm 180 \mathrm{μs}\left({\color{gray}-1.558 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1 $$17.1 \mathrm{ms} \pm 182 \mathrm{μs}\left({\color{gray}3.32 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2 $$17.1 \mathrm{ms} \pm 194 \mathrm{μs}\left({\color{red}5.28 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1 $$16.4 \mathrm{ms} \pm 189 \mathrm{μs}\left({\color{red}5.32 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1 $$15.9 \mathrm{ms} \pm 176 \mathrm{μs}\left({\color{lightgreen}-31.008 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1 $$16.3 \mathrm{ms} \pm 161 \mathrm{μs}\left({\color{gray}-0.229 \mathrm{\%}}\right) $$ Flame Graph

representative_read_multiple_entities

Function Value Mean Flame graphs
entity_by_property depths: DT=255, PT=255, ET=255, E=255 $$65.4 \mathrm{ms} \pm 231 \mathrm{μs}\left({\color{gray}-0.539 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=0 $$39.2 \mathrm{ms} \pm 281 \mathrm{μs}\left({\color{gray}3.06 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=2, PT=2, ET=2, E=2 $$56.3 \mathrm{ms} \pm 282 \mathrm{μs}\left({\color{gray}0.388 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=2 $$42.5 \mathrm{ms} \pm 296 \mathrm{μs}\left({\color{gray}-1.011 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=2, E=2 $$48.6 \mathrm{ms} \pm 280 \mathrm{μs}\left({\color{gray}1.92 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=2, ET=2, E=2 $$52.5 \mathrm{ms} \pm 263 \mathrm{μs}\left({\color{gray}0.229 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=255, PT=255, ET=255, E=255 $$108 \mathrm{ms} \pm 432 \mathrm{μs}\left({\color{gray}0.112 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=0 $$42.7 \mathrm{ms} \pm 197 \mathrm{μs}\left({\color{gray}-0.216 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=2, PT=2, ET=2, E=2 $$98.1 \mathrm{ms} \pm 625 \mathrm{μs}\left({\color{gray}-1.514 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=2 $$81.1 \mathrm{ms} \pm 345 \mathrm{μs}\left({\color{gray}-0.647 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=2, E=2 $$90.7 \mathrm{ms} \pm 473 \mathrm{μs}\left({\color{gray}-0.130 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=2, ET=2, E=2 $$94.9 \mathrm{ms} \pm 470 \mathrm{μs}\left({\color{gray}-2.274 \mathrm{\%}}\right) $$ Flame Graph

representative_read_entity_type

Function Value Mean Flame graphs
get_entity_type_by_id Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579 $$1.37 \mathrm{ms} \pm 3.73 \mathrm{μs}\left({\color{gray}0.130 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_one_depth

Function Value Mean Flame graphs
entity_by_id 50 entities $$267 \mathrm{ms} \pm 1.32 \mathrm{ms}\left({\color{gray}0.556 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$26.6 \mathrm{ms} \pm 238 \mathrm{μs}\left({\color{gray}1.62 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$20.1 \mathrm{ms} \pm 101 \mathrm{μs}\left({\color{gray}-0.459 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$52.6 \mathrm{ms} \pm 2.53 \mathrm{ms}\left({\color{lightgreen}-7.267 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$175 \mathrm{ms} \pm 937 \mathrm{μs}\left({\color{gray}-0.004 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_linkless

Function Value Mean Flame graphs
entity_by_id 1 entities $$1.92 \mathrm{ms} \pm 5.71 \mathrm{μs}\left({\color{gray}-0.510 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 100 entities $$2.10 \mathrm{ms} \pm 6.38 \mathrm{μs}\left({\color{gray}4.47 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$1.93 \mathrm{ms} \pm 4.54 \mathrm{μs}\left({\color{gray}0.182 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1000 entities $$2.89 \mathrm{ms} \pm 14.3 \mathrm{μs}\left({\color{gray}1.32 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10000 entities $$13.4 \mathrm{ms} \pm 41.6 \mathrm{μs}\left({\color{gray}-1.039 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_zero_depth

Function Value Mean Flame graphs
entity_by_id 50 entities $$3.96 \mathrm{ms} \pm 23.9 \mathrm{μs}\left({\color{gray}-4.968 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$1.94 \mathrm{ms} \pm 5.68 \mathrm{μs}\left({\color{gray}-1.319 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$1.93 \mathrm{ms} \pm 6.59 \mathrm{μs}\left({\color{gray}-0.285 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$2.15 \mathrm{ms} \pm 12.3 \mathrm{μs}\left({\color{gray}-0.059 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$3.32 \mathrm{ms} \pm 11.7 \mathrm{μs}\left({\color{gray}-0.455 \mathrm{\%}}\right) $$ Flame Graph

@TimDiekmann TimDiekmann added this pull request to the merge queue Dec 11, 2024
@TimDiekmann TimDiekmann removed this pull request from the merge queue due to a manual request Dec 11, 2024
@TimDiekmann
Copy link
Member Author

Waiting for #5857 to avoid TF configurations to be overwritten in prod (in touches immutable configs)

@TimDiekmann TimDiekmann added this pull request to the merge queue Dec 11, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 11, 2024
@CiaranMn CiaranMn added this pull request to the merge queue Dec 11, 2024
Merged via the queue into main with commit fed76f3 Dec 11, 2024
164 checks passed
@CiaranMn CiaranMn deleted the t/h-3755-improve-validation-structure-for-entity-properties branch December 11, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deps Relates to third-party dependencies (area) area/libs Relates to first-party libraries/crates/packages (area) area/tests New or updated tests type/eng > backend Owned by the @backend team
Development

Successfully merging this pull request may close these issues.

2 participants