[PREVIEW] graphQL schema generation for new SQRL language and planner #1066
Conversation
|
|
||
| // process subscriptions and mutations | ||
| if (goal != ExecutionGoal.TEST) { | ||
| if (logManager.hasLogEngine() && System.getenv().get("ENABLE_SUBSCRIPTIONS") != null) { |
There was a problem hiding this comment.
We now longer need these two guarding if conditions. We always generate subscriptions and mutations if those are present
There was a problem hiding this comment.
Yes, this code was just a copy-paste, not yet touched with the new planner, I just added it for general structure (calling createQueriesOrSubscriptionsObjectType on subscriptions as well).
But thanks for pointing out anyway.
|
|
||
| graphQLSchemaBuilder.additionalTypes(new LinkedHashSet<>(objectTypes)); // the cleaned types | ||
|
|
||
| if (queriesObjectType.get().getFields().isEmpty()) { |
There was a problem hiding this comment.
use google guava Preconditions
| // Todo move to dialect? | ||
| public static final AtomicInteger i = new AtomicInteger(0); | ||
|
|
||
| private static String generateUniqueNameForType(NamePath namePath, Set<String> seen, String postfix) { |
There was a problem hiding this comment.
We are guaranteed that root level table names are unique by the compiler. The only time we could get a naming collision for the result type is on relationships (e.g. you can have Orders.rel := and Customer.rel := definitions with the same relationship name but different parents).
My suggestion would be as follows: For relationships that do not have a base table, we generate a result object type where the name is the fullpath of the relationship deliminated by _, i.e. using fullpath.toString("_"). So the type names would be Customer_rel and Orders_rel for the examples above and they would be unique.
This could technically still lead to naming collisions (e.g. if the SQRL script contains a table named Customer_rel) but it is so unlikely that we can ignore it for now and the user can fix it by manually adjusting the GraphQL schema.
There was a problem hiding this comment.
Done for relationship result type.
But generateUniqueNameForType you pointed out is not used for the name of the functions result type, it is used for the inner non-relationship structured fields (ROW etc...) type name. And for both input types and output types.
In that case, when walking nested types, we still need to generate uniq type names (e.g. entriesResult for Orders.entries field), right ?
There was a problem hiding this comment.
Good point, I forgot about nested types. I think we should take the same approach there and derive the name from the path. For example, for Orders.entries we create the type name Orders_entries. And if we have a more deeply nested type, e.g. Customer.location.address (i.e. location is nested underneath customer, and location has the nested address), then its Customer_location_address. I think those names, while longer, are more descriptive than address5. And if the user doesn't like it they can adjust the GraphQL schema.
There was a problem hiding this comment.
Totally agree: the uniquify code keeping track of the seen names is more complex and at the same time less expressive than just naming after the full path. Let's get this simple and expressive !
In addition, the type names right now also concat "Result" for output nested types and "Input" for input nested types. Maybe we should keep similar labels for clarity and expressivity. I'd suggest either:
- "Structured" label for both.
- or "Input" and "Output" labels
=> solution1 does not differenciate input and ouput types, so I'd be in favor of 2.
Before making this change the type mapping code needed a refactoring so I reduced the call stack and improved the naming. Regarding code duplication, I need the new graphQL to be testable end to end ( 😉 ) before deduplicating so that we avoid any regression. So I opened this ticket: #1068 so that I don't forget
echauchot
force-pushed
the
new_graphql
branch
from
e7176ca to
b0e5836
Compare
| @@ -97,7 +97,6 @@ public Optional<GraphQLSchema> generate(ServerPhysicalPlan serverPlan) { | |||
|
|
|||
| // process subscriptions and mutations | |||
| if (goal != ExecutionGoal.TEST) { | |||
There was a problem hiding this comment.
this condition can also go. The planner takes care of filtering out test functions when the goal is not test. The GraphQL schema inference should just translate what it gets.
There was a problem hiding this comment.
Yes I remember that the planner does the hidden and test mode table functions filtering but I thought we were not generating the subscriptions and mutations in test mode.
|
As a general comment: for preview PRs I'll add TODOs to distinguish copy pasted not-yet-touched code 😉 |
…and renamed unused addlTypes to extendedScalarTYpes (DataSQRL#744). (cherry picked from commit 79966e4)
(cherry picked from commit b9d4089)
…ompile configuration) which defaults to false for backward compatibility. So the default package.json does not change (DataSQRL#744). (cherry picked from commit e2f747d)
…iler configuration. [DataSQRL#744]. (cherry picked from commit 001eb1b)
(cherry picked from commit bf468d4)
…on documentation (DataSQRL#744). (cherry picked from commit a559aa1)
…ased on the extendedScalarTypes compiler config parameter(DataSQRL#909).
(cherry picked from commit 193353e)
(cherry picked from commit aab20f5)
). (cherry picked from commit 355731d)
…CI (with UTC-8 timezone) to workaround the local timezone issue. (cherry picked from commit 0d549fe)
…Integer for disambiguation remove unneeded graphql-extended-scalars lib import.
…browsing fields if base table is present and its type already declared, cleaning
…han for result types. Remove all the previous uniquify method (based on names seen)
…ment on why projections don't work with subscriptions
echauchot
force-pushed
the
new_graphql
branch
from
21fc8df to
ab1e2ae
Compare
|
rebased on experiment/simplify and pushed new commits |
…od. More coherent and simple testing of empty queries. Fix empty subscriptions
echauchot
force-pushed
the
new_graphql
branch
from
72e1633 to
3e49640
Compare
NOT READY FOR MERGING:
Also not rebased on master or latest base branch.
big integer mapping feature was cherry picked from master
Generates GraphQL schema for new version of the SQRL language and the related planner:
Supported:
graphQL queries
ralationships
parameters
nested structured types
functions overloading is not yet removed (hence the parameters merging and signatures overlap management not done)
removed the unic names generation. Maybe needed ?
need to re-enable invalid types pruning: fix it, because it removes nestedTypes for now
Next:
Regarding the architecture of the resultType creation, I tried to merge the root table functions generation and relationship generation but I thought that the code path (if/then/else) was less easy to read/maintain so I kept seperate methods and early returns.
wire up the subscriptions (the code should be the same)
I'll do minor simplifications of the function resultType creation.
=> I'll keep updating this PR. I'm sending it for early preview
@mbroecheler PTAL