feat(gate)!: policies should affect visibility on introspection #963
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several changes across workflow configuration, computation logic, type graph runtime, introspection schema, and tests. Updates include minor workflow documentation and caching commands, enhanced null safety in the computation engine, and extensive refactoring of the TypeGraph runtime with new private properties, helper functions, and an introspection type emitter. The changes also update the initialization of runtime references in the Typegate class, revise the visitor pattern naming, and adjust introspection schemas and tests, including new queries and visibility functions. A documentation comment in the utilities file was also refined. Changes
Sequence Diagram(s)sequenceDiagram
participant T as Typegate
participant R as prepareRuntimeReferences
participant TG as TypeGraph
T->>R: Call prepareRuntimeReferences(secretManager)
R-->>T: Return RuntimeRefData
T->>TG: init(tgDS, secretManager, RuntimeRefData, introspectionData)
TG-->>T: Runtime initialized with references
sequenceDiagram
participant C as Client
participant TG as TypeGraph
participant ITE as IntrospectionTypeEmitter
C->>TG: Request introspection schema
TG->>ITE: Invoke emitRoot() for root schema
ITE->>ITE: Process types and apply visibility policies
ITE-->>TG: Return emitted schema
TG-->>C: Deliver introspection result
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
michael-0acf4
changed the title
michael-0acf4
requested review from
Natoandro,
Yohe-Am,
j03-dev and
luckasRanarison
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (24)
tests/introspection/visibility_complex.py (3)
9-14: Consider adding docstrings or inline comments for policy usage.While this section is fairly self-explanatory, adding a brief comment explaining the purpose and effect of each policy (e.g., “passThrough” and “denyAll”) will enhance maintainability and clarity for future contributors.
77-99: Review cross-field and union expansions carefully.Your unions combine multiple structured variants, including a self-referential type (
rec_type_root) and an empty object. As your type graph expands, ensure that recursion is handled gracefully without introducing performance overhead or infinitely expanding payloads in real usage.
101-109: Consider separating read and update logic for clarity.Currently,
identity_readandidentity_updateshare similar code patterns but differ in effect. Splitting them into distinct, well-documented methods (with more explicit naming or references) might improve maintainability and facilitate adding any future logic specific to read vs. update.src/typegate/src/runtimes/typegraph.ts (1)
81-105: Refactor repeated logic in the “default” resolver case.Lines 88–105 repeat the same pattern as 89–103. Factor this out into a helper function or unify the logic for clarity and maintainability.
src/typegate/src/runtimes/typegraph/visibility.ts (3)
45-54: Consider renaming or splitting thereset()behavior.Your constructor invokes
reset(), which reads like it’s re-initializing members plus visiting the type graph. Clarify whether “resetting” also re-scans for new policies each time, or introduce separate methods for scanning vs. clearing. This can make the lifecycle more transparent.
163-215: Avoid direct coupling of policy chain composition with field filtering.
filterAllowedFieldscomposes policy results on the fly, which is fine for small objects. However, more complex or deeply nested objects may benefit from a centralized composition pass, improving performance by avoiding repetitive composition for each field.
218-225: Expand flattenRegardlessOfEffects.If you add new effects beyond create, delete, read, update, or if you want to handle partial definitions, consider expanding logic to handle missing or unknown effect types to avoid future regressions.
src/typegate/src/typegate/mod.ts (3)
25-26: Use consistent import grouping and naming.
The new importsprepareRuntimeReferencesandRuntimeRefDataappear logically consistent, but consider grouping them with related imports likeTypeGraphandSecretManager. This keeps the import sections more organized.
446-446: Abstract runtime reference creation.
This line introducesrtRefResolver, but the namingrtRefResolvermight be improved to convey "runtimeReferencesResolver" or a more meaningful name that clarifies it returns the prepared references.
448-469: Streamline introspection initialization.
This block of code establishes a separate introspection TypeGraph ifenableIntrospectionis true. It’s a good approach for isolating introspection concerns, but watch out for possible duplication in logic. For example, consider extracting some repeated steps (e.g., calls tortRefResolver,initFormatter) into helper functions to keep the method focused and maintainable.src/typegate/src/typegraph/mod.ts (2)
173-173: Document changes to theinitmethod signature.
AddingruntimeRefData: RuntimeRefDataclarifies how runtime references are resolved, but consider adding a docstring explaining what the parameter is used for, particularly for future maintainers.
539-581: Isolate runtime reference logic and potential errors.
The newprepareRuntimeReferencesfunction nicely refactors out the runtime setup. Some suggestions:
- Add error handling to gracefully report or recover if any particular runtime fails to initialize.
- Consider caching or memoizing results if multiple calls to
prepareRuntimeReferencescould happen for the sametypegraph.- Document what the function returns (the shape of
RuntimeRefData) and how it's meant to be used.src/typegate/src/runtimes/typegraph/type_emitter.ts (4)
1-46: Establish explicit separation of concerns.
This newly introduced file provides a class for generating introspection types. The extensive imports for type nodes, visibility, and helpers are appropriate, but ensure that the file remains focused on type emission. If logic for policy-level checks or advanced transformations grows significantly, consider a separate utility module to preserve single responsibility.
49-225: Confirm safe intermixing of checking and generation.
TheIntrospectionTypeEmitterconstructor (and subsequent methods likeemitRootand#filterWithInjectionAnPolicies) handle both filtering logic (policies/injections) and GraphQL schema creation. This is powerful but can become unwieldy if expanded. A recommended approach is to create a dedicated pre-check structure to handle all policy-based filtering before the actual schema emission.🧰 Tools
🪛 Biome (1.9.4)
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
257-416: Handle unexpected type variants gracefully.
In methods like#emitScalar,#emitRawType,#emitWrapperAndReturnSchema, and the union-handling logic, you throw errors for unhandled or unexpected types. This is good for fail-fast, but consider a fallback or user-facing warning if partial introspection is possible. Doing so may provide more resilient schema outputs when the input typegraph is partially invalid or dynamically generated.
418-641: Ensure consistent naming conventions and factor out repeated logic.
The final sections ($requiredSchema, $fieldSchema, etc.) contain repeated patterns for referencing resolvers, building partial schemas, and handling placeholders. Consider factoring out repeated logic into well-named helper functions to keep each method small and easier to maintain. Also, double-check that naming conventions ($prefix vs.#) remain consistent with your style guidelines.tests/introspection/visibility_complex_test.ts (1)
45-45: Fix typo in test description.There's a typo in the test description: "inivible" should be "invisible".
- "have a few fields inivible without proper access", + "have a few fields invisible without proper access",src/typegate/src/runtimes/typegraph/helpers.ts (3)
10-23: Consider adding input validation and error handling.While the function correctly handles scalar types, it could benefit from additional input validation and more descriptive error messages.
export function genOutputScalarVariantWrapper(type: TypeNode, idx: number) { + if (!type) { + throw new Error("Type parameter is required"); + } + if (typeof idx !== "number" || idx < 0) { + throw new Error("Index must be a non-negative number"); + } if (isScalar(type)) { const id = type.type; return { title: `_${id[0].toUpperCase()}${id.slice(1)}`, type: "object", properties: { [id]: idx, }, } as ObjectNode; } - throw `"${type.title}" of type "${type.type}" is not a scalar`; + throw new Error(`Type "${type.title}" of type "${type.type}" is not a scalar`); }
32-45: Add JSDoc comments for better documentation.The function would benefit from JSDoc comments explaining the purpose of each field and their relationship to the GraphQL spec.
+/** + * Returns a common structure for GraphQL fields based on the GraphQL spec. + * @see https://github.com/graphql/graphql-js/blob/main/src/type/introspection.ts + * @returns {Object} Object containing default implementations for field-related properties + */ export function fieldCommon() { return { // https://github.com/graphql/graphql-js/blob/main/src/type/introspection.ts#L207 name: () => null, specifiedByURL: () => null, // logic at https://github.com/graphql/graphql-js/blob/main/src/type/introspection.ts#L453-L490 ofType: () => null, inputFields: () => null, fields: () => null, interfaces: () => [], possibleTypes: () => null, enumValues: () => null, }; }
47-71: Consider using template literals for better readability.The string concatenation in the policy description could be more readable using template literals.
let ret = "\n\nPolicies:\n"; if (policyNames.length > 0) { - ret += policyNames.map((p: string) => `- ${p}`).join("\n"); + ret += policyNames.map((p: string) => `- ${p}`).join('\n'); } else { - ret += "- inherit"; + ret += '- inherit'; } - return ret; + return `${ret}\n`;tests/introspection/visibility_simple.py (2)
15-18: Consider adding type hints for better code clarity.The helper function could benefit from type hints for better code clarity and maintainability.
- def ctxread_or_pass_through(pol_name: str): + def ctxread_or_pass_through(pol_name: str) -> DenoRuntime.Policy:
20-71: Add docstring to explain the test structure.The complex nested structure would benefit from a docstring explaining the purpose and structure of the test data.
g.expose( + # Test structure: + # - a (allow) + # - a_1 + # - a_11_deny_allowed (deny but accessible due to parent allow) + # - a_12 + # - a_2 + # - a_21 + # - b (pass_through) + # - b_1 + # - b_11_deny (deny and not accessible) + # - b_12 + # - b_2 (deny) + # - b_21_allow_denied (allow but not accessible due to parent deny) ctxread_or_pass_through("root_policy"),tests/introspection/common.ts (1)
104-113: Consider adding descriptions to the enumeration queries.The
enumerateTypesandenumerateTypesAndInspectFieldsfunctions could benefit from JSDoc comments explaining their specific use cases and differences.+/** + * Enumerates all types in the schema with their names and kinds. + */ export const enumerateTypes = () => gql` +/** + * Enumerates all types in the schema and inspects their fields. + * More detailed than enumerateTypes but less verbose than fullIntrospection. + */ export const enumerateTypesAndInspectFields = () => gql`Also applies to: 116-128
src/typegate/src/typegraphs/introspection.py (1)
97-97: Key change: Made type kind optional to support visibility policies.Making the
kindfield optional is crucial for implementing type visibility policies in introspection. This allows the system to selectively hide type information based on applied policies.Consider documenting the visibility behavior in a comment to explain when and why a type's kind might be hidden.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
.ghjk/deno.lockis excluded by!**/*.locktests/introspection/__snapshots__/introspection_test.ts.snapis excluded by!**/*.snaptests/introspection/__snapshots__/union_either_test.ts.snapis excluded by!**/*.snaptests/introspection/__snapshots__/visibility_complex_test.ts.snapis excluded by!**/*.snaptests/introspection/__snapshots__/visibility_simple_test.ts.snapis excluded by!**/*.snaptests/simple/__snapshots__/class_syntax_test.ts.snapis excluded by!**/*.snaptests/type_nodes/__snapshots__/either_test.ts.snapis excluded by!**/*.snaptests/type_nodes/__snapshots__/union_test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (18)
.github/workflows/tests.yml(2 hunks)src/typegate/src/engine/computation_engine.ts(1 hunks)src/typegate/src/runtimes/typegraph.ts(2 hunks)src/typegate/src/runtimes/typegraph/helpers.ts(1 hunks)src/typegate/src/runtimes/typegraph/type_emitter.ts(1 hunks)src/typegate/src/runtimes/typegraph/visibility.ts(1 hunks)src/typegate/src/typegate/mod.ts(3 hunks)src/typegate/src/typegraph/mod.ts(4 hunks)src/typegate/src/typegraph/visitor.ts(1 hunks)src/typegate/src/typegraphs/introspection.json(17 hunks)src/typegate/src/typegraphs/introspection.py(1 hunks)tests/introspection/common.ts(1 hunks)tests/introspection/introspection_test.ts(6 hunks)tests/introspection/visibility_complex.py(1 hunks)tests/introspection/visibility_complex_test.ts(1 hunks)tests/introspection/visibility_simple.py(1 hunks)tests/introspection/visibility_simple_test.ts(1 hunks)tools/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tools/utils.ts
- .github/workflows/tests.yml
🧰 Additional context used
🪛 Biome (1.9.4)
src/typegate/src/runtimes/typegraph/type_emitter.ts
[error] 166-166: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 237-237: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-full
- GitHub Check: test-website
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (18)
tests/introspection/visibility_complex.py (1)
20-34: Validate nested fields in test coverage.The input structure contains nested fields, some with explicit default values (e.g., line 23). Ensure these nested and defaulted fields have test scenarios to confirm they behave as intended under all relevant policies.
src/typegate/src/runtimes/typegraph.ts (1)
128-138: Handle concurrency concerns in policy precomputation.
#withPrecomputeVisibilityresets and precomputes all policies before returning. If multiple requests can invoke this method concurrently, ensure that shared state (#visibility and #typeGen) is updated safely, or clarify that concurrency is not applicable.src/typegate/src/runtimes/typegraph/visibility.ts (1)
114-118: Gracefully handle re-computation scenarios.You skip policy re-computation if
#resolvedPolicy.size > 0. Clarify whether partial failures or partial completions can leave this map in a half-populated state on error, potentially causing inconsistent visibility. You might consider a transaction-like approach or clearing the state on error.src/typegate/src/typegate/mod.ts (3)
54-54: Validate runtime modules.
ImportingDenoRuntimeis fine, but ensure it is indeed required in the rest of the code (particularly in the introspection logic). If it is used exclusively within the newly introduced preparatory references block, verify that there are no side effects impacting the broader code.
471-471: Clarify reference usage.
StoringtgRuntimeRefDatain a local constant is helpful. Verify that other call sites or subsequent code blocks use these references consistently.
476-476: Ensure parallel usage.
When passingtgRuntimeRefDatatoTypeGraph.init(), double-check concurrency usage in other typegraph runtimes. If multiple TypeGraphs are initialized concurrently, verify there's no shared mutable state intgRuntimeRefDatathat might inadvertently cause data races.src/typegate/src/typegraph/mod.ts (2)
59-63: Promote clarity of runtime reference structures.
DefiningRuntimeRefDatais a positive step toward type safety and clarity. Ensure that properties likedenoRuntimeIdxanddenoRuntimestay in sync if the code ever reorders or modifies runtimes.
217-217: Leverage destructuring carefully.
Destructuring{ denoRuntimeIdx, runtimeReferences, denoRuntime }is concise, but ensure that these fields are always defined in the returned data. If there's a chance the runtime might be missing or incorrectly configured, add a defensive check or an informative error message.tests/introspection/visibility_simple_test.ts (3)
7-19: LGTM! Well-structured test case for authorized types.The test case effectively validates the enumeration of authorized types with the pass_through policy.
21-31: LGTM! Clear test case for DENY policy.The test case correctly verifies that only a dummy Query type is enumerated when root policy is set to DENY.
33-43: LGTM! Comprehensive test case for ALLOW policy.The test case properly validates that all types are enumerated when root policy is set to ALLOW.
tests/introspection/visibility_complex_test.ts (2)
7-12: LGTM! Well-defined context object.The context object clearly defines the control states for different scenarios.
14-37: LGTM! Comprehensive test case for type enumeration.The test case effectively validates type enumeration both with and without context.
tests/introspection/common.ts (1)
6-101: LGTM! Well-structured introspection query with comprehensive type coverage.The
fullIntrospectionquery is well-organized and covers all essential schema elements including types, fields, arguments, interfaces, and directives.src/typegate/src/typegraph/visitor.ts (1)
7-10: LGTM! Consistent renaming of Path to VisitPath.The renaming is applied consistently across the interface definition and its usage in the visitor pattern implementation.
Also applies to: 14-15, 18-23
tests/introspection/introspection_test.ts (1)
238-242: LGTM! Simplified test using common introspection query.Good refactoring to use the shared
fullIntrospectionquery, improving maintainability.src/typegate/src/engine/computation_engine.ts (1)
188-193: LGTM! Robust null handling improves introspection safety.The addition of nullish coalescing operators (
??) effectively prevents potential runtime errors when fields are not accessible due to policy restrictions. This change aligns well with the PR's objective of ensuring policies influence visibility during introspection.src/typegate/src/typegraphs/introspection.json (1)
8-8: LGTM! Schema updates support policy-based visibility.The increments in property values and the addition of the new optional type
type_kind_type_kind_optionalenhance the schema's ability to handle policy-based visibility during introspection. These changes are consistent and well-structured.Also applies to: 61-68, 87-91
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #963 +/- ##
==========================================
+ Coverage 77.85% 78.20% +0.35%
==========================================
Files 160 164 +4
Lines 19641 20030 +389
Branches 1969 2031 +62
==========================================
+ Hits 15291 15664 +373
- Misses 4329 4347 +18
+ Partials 21 19 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/typegate/src/runtimes/typegraph/type_emitter.ts (2)
171-171: Use optional chaining for clarity.Current code:
const injectionNode = (parentFunction?.injections ?? {})?.[fieldName] ?? null;Suggestion:
- const injectionNode = (parentFunction?.injections ?? {})?.[fieldName] ?? null; + const injectionNode = parentFunction?.injections?.[fieldName] ?? null;This reduces complexity and aligns with the recommendation from the static analysis tool.
🧰 Tools
🪛 Biome (1.9.4)
[error] 171-171: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
244-244: Use optional chaining to simplify the logic.Current code:
const policies = (type.policies ?? {})[fieldName];Suggestion:
- const policies = (type.policies ?? {})[fieldName]; + const policies = type.policies?.[fieldName];This handles a null or undefined
type.policiesmore concisely and improves readability.🧰 Tools
🪛 Biome (1.9.4)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/typegate/src/typegate/mod.ts (1)
457-460: Consider decoupling type generator initialization from Deno runtime.The type generator is tightly coupled with the Deno runtime. Consider introducing an abstraction layer to support different runtime environments.
A more flexible approach would be to:
- Define a runtime interface that different runtimes can implement
- Make the type generator work with this interface instead of a specific runtime
- This would make it easier to support other runtimes in the future
src/typegate/src/runtimes/typegraph.ts (3)
92-106: Remove code duplication in resolver handling.The resolver and default cases contain identical code. Consider extracting the common logic into a separate method.
+ #resolveValue: Resolver = async ({ _: { parent } }) => { + const resolver = parent[stage.props.node]; + return typeof resolver === "function" + ? await resolver() + : resolver; + } #delegate(stage: ComputeStage): Resolver { const name = stage.props.materializer?.name; switch (name) { case "getSchema": return this.#getSchemaResolver; case "getType": return this.#getTypeResolver; case "resolver": - return async ({ _: { parent } }) => { - const resolver = parent[stage.props.node]; - const ret = typeof resolver === "function" - ? await resolver() - : resolver; - return ret; - }; + return this.#resolveValue; default: - return async ({ _: { parent } }) => { - const resolver = parent[stage.props.node]; - const ret = typeof resolver === "function" - ? await resolver() - : resolver; - return ret; - }; + return this.#resolveValue; } }
114-114: Enhance schema description.The current description
${root.type} typegraphis minimal. Consider including more informative details about the schema's purpose and capabilities.
127-129: Add error logging for type resolution failures.When a type is not found, the code silently returns null. Consider logging a warning or debug information to help diagnose schema issues.
#getTypeResolver: Resolver = ({ name }) => { + const type = this.#types.findType(name); + if (!type) { + console.debug(`Type "${name}" not found in schema`); + } - return this.#types.findType(name) ?? null; + return type ?? null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/typegate/src/runtimes/typegraph.ts(2 hunks)src/typegate/src/runtimes/typegraph/type_emitter.ts(1 hunks)src/typegate/src/typegate/mod.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/typegate/src/runtimes/typegraph/type_emitter.ts
[error] 171-171: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 244-244: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-website
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (4)
src/typegate/src/typegate/mod.ts (1)
24-27: LGTM! Import changes align with PR objectives.The new imports are properly organized and support the introspection logic rewrite.
Also applies to: 54-54
src/typegate/src/runtimes/typegraph.ts (3)
22-22: Consider initializing #typeGen in the constructor.Initializing #typeGen as null could lead to runtime errors if methods are called before initTypeGenerator. Consider making initialization mandatory in the constructor or implementing a state machine pattern to prevent invalid states.
46-46: Address the FIXME comment about request handling.The comment indicates a need for proper request state management.
131-138: LGTM! Clean implementation of visibility precomputation.The method properly handles state reset and root emission before resolution.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegate/src/engine/planner/policies.ts (1)
412-454: Consider adding error handling for empty check results.While the refactored stage authorization correctly handles per-field results, it might be good to add explicit handling for the case when
checkResultsis empty.async #checkStageAuthorization( stageId: string, effect: EffectType, getResolverResult: GetResolverResult, ) { const selectedFields = this.#findSelectedFields(stageId); const checkResults = []; + if (selectedFields.length === 0) { + return checkResults; // Early return for no selected fields + } const policiesForStage = this.#stageToPolicies.get(stageId) ?? []; // ... rest of the function
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/typegate/src/engine/planner/policies.ts(4 hunks)tests/policies/policies_composition_test.ts(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: test-website
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (5)
tests/policies/policies_composition_test.ts (2)
34-56: LGTM! Good test coverage for field-level authorization.The new test case verifies that when
control_valueis set to "PASS", only the allowed fieldthreeis accessible whiletwois correctly denied.
20-22: LGTM! Consistent formatting.The addition of trailing commas improves code consistency and makes future diffs cleaner.
Also applies to: 69-71, 87-89, 108-112, 132-136, 150-154, 203-208, 239-246, 273-278
src/typegate/src/engine/planner/policies.ts (3)
206-234: LGTM! Enhanced granularity in policy evaluation.The refactored authorization logic now correctly handles per-field results, allowing for more precise control over field-level access. The switch-case structure properly handles all authorization outcomes.
243-248: LGTM! Simplified path construction.The path construction logic has been simplified while maintaining the same functionality. The removal of the unused
concernedFieldparameter reduces complexity.
461-461: LGTM! Improved field selection logic.The simplified field selection logic is more readable and maintains the same functionality. The use of non-null assertion in the filter is safe here as we're explicitly returning either an object or null.
Also applies to: 464-465
| import { ObjectNode, PolicyIndices } from "../../typegraph/types.ts"; | ||
| import { isScalar, type TypeNode } from "../../typegraph/type_node.ts"; | ||
|
|
||
| // Note: graphql UNION output does not support scalars, only OBJECT |
There was a problem hiding this comment.
Is this wrapper only present on the introspection scheme or is it also used by the graphql API?
Also, make sure to update https://metatype.dev/docs/reference/graphql to explain more of the funky stuff we're doing.
There was a problem hiding this comment.
Is this wrapper only present on the introspection scheme or is it also used by the graphql API?
Just a wrapper.
Actually, this function is unused (I forgot to clean it up), it came from the old implementation but the logic is still applied on the Output UNION type under a specific case:
- If the variants are all structs, we define it according to how UNION works in graphql. But then, when a variant is a struct with empty fields, it is considered as a custom scalar as GraphQL does not allow empty field objects either.. but then again, GraphQL does not allow scalars on the UNION, in that case we are forced to fallback to the same logic as this function (adhoc object with a dummy field).
There was a problem hiding this comment.
Some of these concerns might be better be discussed in MET-883
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/typegate/src/typegate/mod.ts (1)
437-483: Consider adding documentation for the introspection initialization process.The
initQueryEnginemethod has been significantly refactored to handle introspection differently, but lacks documentation explaining the new approach.Add JSDoc comments to explain:
- The purpose of runtime reference preparation
- The introspection initialization process
- The relationship between type graph and introspection
+ /** + * Initializes the query engine with optional introspection support. + * + * @param tgDS - The type graph data structure + * @param secretManager - Manager for handling secrets + * @param customRuntime - Optional custom runtime resolver + * @param enableIntrospection - Whether to enable introspection support + * @returns Initialized query engine + * + * The initialization process involves: + * 1. Preparing runtime references for both introspection and type graph + * 2. Initializing introspection if enabled + * 3. Initializing the main type graph with the prepared references + */ async initQueryEngine( tgDS: TypeGraphDS, secretManager: SecretManager, customRuntime: RuntimeResolver = {}, enableIntrospection: boolean, ): Promise<QueryEngine> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(2 hunks)src/typegate/src/typegate/mod.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: bulid-docker (linux/arm64, custom-arm)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-website
- GitHub Check: pre-commit
🔇 Additional comments (3)
src/typegate/src/typegate/mod.ts (3)
25-27: LGTM! Clean import organization.The new imports are well-organized and properly scoped for the introspection logic rewrite.
Also applies to: 54-54
446-447: Add error handling for runtime reference preparation.The
rtRefResolverinitialization could fail, but there's no error handling.
457-460: Add type safety check for Deno runtime.The code assumes
denoRuntimewill always be present inintrospectionRuntimeRefData.
This ended up becoming a full rewrite of the introspection logic.
For context, the old implementation generated types dynamically but also did a few parts statically (the __schema.types list). This worked as long as we assume all object types fields are not context dependent. Which resulted in a few bugs on injected fields.
In this new implementation, we "define" a type as required (it is added into the __schema.types list), still recursively but this allows emitting types with adhoc suffixes in demand depending on the policies or injections. Then when the type is refered we simply give a reference to it.
Migration notes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Tests