-
Notifications
You must be signed in to change notification settings - Fork 84
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-3371, H-3722: Overhaul return behavior for entity validation #5820
Merged
TimDiekmann
merged 11 commits into
main
from
t/h-3722-return-validation-error-more-structured
Dec 7, 2024
Merged
H-3371, H-3722: Overhaul return behavior for entity validation #5820
TimDiekmann
merged 11 commits into
main
from
t/h-3722-return-validation-error-more-structured
Dec 7, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
area/deps
Relates to third-party dependencies (area)
area/infra
Relates to version control, CI, CD or IaC (area)
area/libs
Relates to first-party libraries/crates/packages (area)
type/eng > backend
Owned by the @backend team
area/tests
New or updated tests
area/libs > chonky
Affects the `chonky` crate (library)
labels
Dec 6, 2024
TimDiekmann
changed the title
H-3722: Overhaul return behavior for entity validation
H-3371, H-3722: Overhaul return behavior for entity validation
Dec 6, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5820 +/- ##
=======================================
Coverage 23.00% 23.00%
=======================================
Files 568 568
Lines 19165 19160 -5
Branches 2715 2716 +1
=======================================
Hits 4408 4408
+ Misses 14705 14700 -5
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
area/apps > hash*
Affects HASH (a `hash-*` app)
area/apps > hash-api
Affects the HASH API (app)
area/apps
labels
Dec 7, 2024
TimDiekmann
commented
Dec 7, 2024
Benchmark results
|
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph |
representative_read_multiple_entities
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
entity_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
entity_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=255, PT=255, ET=255, E=255 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=0 | Flame Graph | |
link_by_source_by_property | depths: DT=2, PT=2, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=0, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=0, ET=2, E=2 | Flame Graph | |
link_by_source_by_property | depths: DT=0, PT=2, ET=2, E=2 | Flame Graph |
representative_read_entity_type
Function | Value | Mean | Flame graphs |
---|---|---|---|
get_entity_type_by_id | Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579
|
Flame Graph |
scaling_read_entity_complete_one_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph |
scaling_read_entity_linkless
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 100 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 1000 entities | Flame Graph | |
entity_by_id | 10000 entities | Flame Graph |
scaling_read_entity_complete_zero_depth
Function | Value | Mean | Flame graphs |
---|---|---|---|
entity_by_id | 50 entities | Flame Graph | |
entity_by_id | 5 entities | Flame Graph | |
entity_by_id | 1 entities | Flame Graph | |
entity_by_id | 10 entities | Flame Graph | |
entity_by_id | 25 entities | Flame Graph |
CiaranMn
approved these changes
Dec 7, 2024
TimDiekmann
deleted the
t/h-3722-return-validation-error-more-structured
branch
December 7, 2024 14:59
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
area/apps > hash*
Affects HASH (a `hash-*` app)
area/apps > hash-api
Affects the HASH API (app)
area/apps
area/deps
Relates to third-party dependencies (area)
area/infra
Relates to version control, CI, CD or IaC (area)
area/libs > chonky
Affects the `chonky` crate (library)
area/libs
Relates to first-party libraries/crates/packages (area)
area/tests
New or updated tests
type/eng > backend
Owned by the @backend team
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🌟 What is the purpose of this PR?
To allow proper handling of validation errors, this PR completely overhauls how validation errors are returned from the graph. Most notably is that the validation function itself won't error anymore but returned a well-defined structure.
🔍 What does this change?
Add a validation structure which looks similar to this:
The
properties
field in the top level is not adjusted yet. For now it just contains aReport
(basically all the errors which could happen in property validation). This will be adjusted in a follow-up PR as the structure of the preprocessor needs to change quite a bit for that.metadata.properties
should include metadata validation for properties which currently does nothing. The OpenAPI generator uses any for this (please don’t ask me why), it should always be empty but was kept to follow the code paths in Rust.The Graph API endpoint itself are changed so they return
{ [key: string]: EntityValidationReport }
whilekey
is the position of the entity in the parameter (0-based index, but JSON does not support numbers as keys). Any parameter with a failed validation will be reported.This means:
createEntities
andupdateEntity
will return the result as previously, but if a validation error happens, the above structure is passed alongside the reports in the responsevalidateEntities
will always return the map. An empty map means no validation errors.Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🐾 Next steps
Overhaul the property validation reporting as well