Minor bug fixes and enhancements - #1
Merged
Merged
Conversation
Declare the entities, links, and namespaces tables on the catalog Base.metadata so fresh databases create them via initialize_database, matching the migration used for existing databases. The graph store no longer creates its own schema at runtime and reuses the catalog nodes table for node id resolution.
| Use the interactive <em>GraphQL</em> playground to query the links graph. | ||
| </h2> | ||
| <a href="{{ root_url }}/graphql" target="_blank" rel="noreferrer"> | ||
| <a href="{{ root_url }}/api/graphql" target="_blank" rel="noreferrer"> |
There was a problem hiding this comment.
On second thought...since we're marking this new feature as experimental, do we even want to link to it from the tiled landing page?
Author
There was a problem hiding this comment.
We can leave it out for now -- easy to add later. @danielballan, what do you think?
dylanmcreynolds
left a comment
dylanmcreynolds
left a comment
There was a problem hiding this comment.
Great, thank you for this PR!
These are great changes. I have added a couple of questions before merging.
Match the catalog's naming convention (tiled.catalog.orm), where SQLAlchemy table definitions on Base.metadata live in an orm module. No behavior change.
The entity/link graph is recursively traversable (Entity.outgoingLinks -> Link.object -> Entity.outgoingLinks -> ...), so an unbounded nested query could force arbitrarily deep and expensive resolution. Add a QueryDepthLimiter (max depth 10) to the schema. Introspection queries are exempt, so the GraphiQL Docs panel is unaffected.
…ecks create_link previously issued two SELECTs to confirm the subject and object entities exist before inserting, duplicating work the schema layer already does and leaving a window between the checks and the insert. Insert directly and rely on the subject_id/object_id foreign keys (SQLite enforces these too, via the shared pool's PRAGMA foreign_keys=ON). Only on a constraint violation does it look up which endpoint is missing, so the success path is a single atomic INSERT while error messages are unchanged.
Author
|
I've also added a limit on recursive queries and made inserting a link reliant on DB-level integrity constraints. Overall, I think it's ready; we can add anything else in future PRs. Thank you, Dylan! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Bug fixes
Schema provisioning refactor
GraphiQL UX