fix: bad name on shared types #955
Conversation
📝 WalkthroughWalkthroughThe changes in the Changes
Sequence DiagramsequenceDiagram
participant NP as NamingProcessor
participant TRC as TypeRefCount
participant VC as VisitCollector
NP->>TRC: Initialize reference counters
NP->>TRC: Collect reference info for each property
TRC-->>NP: Populate reference counts
NP->>VC: Create VisitCollector with reference counts
VC->>NP: Track type references
NP->>NP: Generate names based on reference counts
The sequence diagram illustrates the new workflow of reference counting and name generation, showing how the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/typegraph/core/src/utils/postprocess/naming.rs (1)
306-322: Optimize naming logic ingen_nameto prevent excessively long names.The
gen_namefunction builds type names based on the path and type, which might result in long and unwieldy names for deeply nested structures. Consider simplifying the naming convention or limiting the depth of recursion to improve readability and maintainability of the generated names.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegraph/core/src/utils/postprocess/naming.rs(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- 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 (4)
src/typegraph/core/src/utils/postprocess/naming.rs (4)
13-13: Appropriate use ofIndexSetfor tracking referers.The introduction of
IndexSetfrom theindexmapcrate is suitable for maintaining a set of referers with insertion order preserved, which can be beneficial for consistent naming.
37-48: Correct initialization and usage of reference counters.The creation of
ref_countersand the population of reference information usingcollect_ref_infoare properly implemented. Transferring the counts toVisitCollectorensures accurate tracking of type references.
83-91: Efficient implementation ofTypeRefCount::new_hit.The
new_hitmethod effectively updates thecountsHashMap by inserting new referers or modifying existing entries. UsingIndexSetensures that referers are unique and order is maintained.
94-101: Effective check inhas_more_than_one_referer.The method
has_more_than_one_refereraccurately determines if a type has multiple referers by checking the length of the referer set. This is crucial for the naming logic that depends on the number of referers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/typegraph/core/src/utils/postprocess/naming.rs (1)
258-316: Consider simplifying cycle handling based on type system constraints.According to the domain knowledge, cyclic types can only occur with user-renamed types, which are skipped by the naming processor. This means the cycle detection using
visitedset might be unnecessary.You could simplify the function by removing the cycle detection:
-fn collect_ref_info( - cx: &VisitContext, - acc: &mut TypeRefCount, - id: u32, - visited: &mut HashSet<u32>, -) -> anyhow::Result<()> { - if !visited.insert(id) { - return Ok(()); - } +fn collect_ref_info( + cx: &VisitContext, + acc: &mut TypeRefCount, + id: u32, +) -> anyhow::Result<()> { + if cx.user_named.contains(&id) { + return Ok(()); + } let node = &cx.tg.types[id as usize]; match node { // ... existing match arms with updated recursive calls ... }; - visited.remove(&id); Ok(()) }This would make the code simpler while maintaining correctness, as the type system's constraints naturally prevent cycles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/typegraph/core/src/utils/postprocess/naming.rs(5 hunks)
🧰 Additional context used
📓 Learnings (1)
src/typegraph/core/src/utils/postprocess/naming.rs (1)
Learnt from: michael-0acf4
PR: metatypedev/metatype#955
File: src/typegraph/core/src/utils/postprocess/naming.rs:258-305
Timestamp: 2025-01-07T20:11:43.287Z
Learning: In the Metatype codebase's type system (src/typegraph/core/src/utils/postprocess/naming.rs), cyclic types can only occur with user-renamed types, as a reference can only exist after a rename happens. These cycles are naturally handled because the naming post-processor skips processing of user-renamed types.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (5)
src/typegraph/core/src/utils/postprocess/naming.rs (5)
13-13: LGTM! Well-designed data structures for reference tracking.The choice of
IndexSetand the addition of reference counting structures are well-thought-out. TheIndexSetprovides efficient operations while maintaining insertion order, which is beneficial for consistent type naming.Also applies to: 74-74, 78-80
36-48: LGTM! Good separation of concerns in the postprocessing logic.The two-phase approach (collecting references before name generation) is well-structured and maintains clear separation of concerns.
82-91: LGTM! Clean and efficient reference counting implementation.The
new_hitmethod makes good use of the Entry API for atomic operations, handling both new and existing entries efficiently.
93-101: LGTM! Clear and safe implementation of reference checking.The
has_more_than_one_referrermethod is well-named and safely handles the case when a type has no referrers.
325-332: LGTM! Clear strategy for handling shared type names.The implementation elegantly handles shared types by using a different naming scheme when a type has multiple referrers, preventing confusion that could arise from path-based naming.
Also applies to: 343-345
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/metagen/typegraphs/sample/rs/client.rs (1)
236-237: Clarify the naming ofScalarStructShared2PartialThe struct
ScalarStructShared2Partialis used as an argument type in several functions but has a name that may not clearly convey its purpose. Renaming it to something more descriptive, likeIdArgsPartialorCommonArgsPartial, could improve code clarity and maintainability.Also applies to: 430-475
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
src/typegraph/core/src/snapshots/typegraph_core__tests__successful_serialization.snapis excluded by!**/*.snaptests/e2e/typegraph/__snapshots__/typegraph_test.ts.snapis excluded by!**/*.snaptests/metagen/__snapshots__/metagen_test.ts.snapis excluded by!**/*.snaptests/runtimes/graphql/__snapshots__/graphql_test.ts.snapis excluded by!**/*.snaptests/runtimes/grpc/__snapshots__/grpc_test.ts.snapis excluded by!**/*.snaptests/runtimes/kv/__snapshots__/kv_test.ts.snapis excluded by!**/*.snaptests/runtimes/s3/__snapshots__/s3_test.ts.snapis excluded by!**/*.snaptests/runtimes/temporal/__snapshots__/temporal_test.ts.snapis excluded by!**/*.snaptests/runtimes/typegate/__snapshots__/typegate_prisma_test.ts.snapis excluded by!**/*.snaptests/runtimes/typegate/__snapshots__/typegate_runtime_test.ts.snapis excluded by!**/*.snaptests/runtimes/wasm_reflected/rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
examples/typegraphs/metagen/rs/fdk.rs(2 hunks)src/typegate/src/typegraphs/introspection.json(3 hunks)src/typegate/src/typegraphs/typegate.json(13 hunks)src/typegraph/core/src/utils/postprocess/naming.rs(5 hunks)tests/metagen/typegraphs/identities/rs/fdk.rs(4 hunks)tests/metagen/typegraphs/identities/ts/fdk.ts(3 hunks)tests/metagen/typegraphs/sample/py/client.py(16 hunks)tests/metagen/typegraphs/sample/py_upload/client.py(3 hunks)tests/metagen/typegraphs/sample/rs/client.rs(14 hunks)tests/metagen/typegraphs/sample/rs/main.rs(4 hunks)tests/metagen/typegraphs/sample/rs_upload/client.rs(6 hunks)tests/metagen/typegraphs/sample/ts/client.ts(8 hunks)tests/metagen/typegraphs/sample/ts_upload/client.ts(3 hunks)tests/runtimes/wasm_wire/rust/fdk.rs(3 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- tests/metagen/typegraphs/sample/ts_upload/client.ts
- tests/metagen/typegraphs/sample/py_upload/client.py
- tests/metagen/typegraphs/sample/ts/client.ts
- tests/metagen/typegraphs/sample/py/client.py
🧰 Additional context used
📓 Learnings (1)
src/typegraph/core/src/utils/postprocess/naming.rs (1)
Learnt from: michael-0acf4
PR: metatypedev/metatype#955
File: src/typegraph/core/src/utils/postprocess/naming.rs:258-305
Timestamp: 2025-01-07T20:11:43.287Z
Learning: In the Metatype codebase's type system (src/typegraph/core/src/utils/postprocess/naming.rs), cyclic types can only occur with user-renamed types, as a reference can only exist after a rename happens. These cycles are naturally handled because the naming post-processor skips processing of user-renamed types.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (34)
tests/metagen/typegraphs/sample/rs_upload/client.rs (4)
45-48: Updated argument types forRootUploadFnimprove consistencyThe argument types for
RootUploadFnhave been updated to useScalarFileShared1andRootUploadFnInputPathScalarString1Optional, enhancing naming consistency and better reflecting shared scalar types.
65-69: Consistent use of scalar types inRootUploadManyFnThe argument types for
RootUploadManyFnhave been updated toRootUploadManyFnInputPrefixScalarString1OptionalandRootUploadManyFnInputFilesScalarFileShared1List, improving consistency across the codebase by standardizing the naming convention of shared scalar types.
84-99: Addition of scalar type definitions enhances clarityThe introduction of scalar type definitions such as
ScalarFileShared1,ScalarString1, andScalarBoolean1provides clearer and more consistent naming for shared types, improving code readability and maintainability.
130-130: Confirm the change in return type toScalarBoolean1The return types of the
uploadandupload_manyfunctions have been changed toMutationNode<ScalarBoolean1>. Please verify that this change is intentional and ensure that all calling code handles the new return type correctly to prevent potential runtime errors.Also applies to: 148-148
tests/metagen/typegraphs/sample/rs/client.rs (2)
88-90: Updated argument types inRootScalarArgsFnenhance type consistencyThe argument types for
id,slug, andtitlehave been updated to useScalarStringUuid1andScalarString1, aligning with the new scalar type definitions and improving type consistency across the codebase.
227-233: Consistent scalar type definitions intypesmoduleThe
typesmodule now usesScalarStringUuid1andScalarString1for type aliases and struct fields, providing a consistent and clear type system that enhances code readability.tests/metagen/typegraphs/identities/ts/fdk.ts (4)
36-47: Updated scalar type definitions improve code clarityIntroducing scalar types like
ScalarString1,ScalarInteger1, and others standardizes the type definitions, enhancing code clarity and consistency across the codebase.
49-60: RefactoringPrimitivesto use new scalar typesThe
Primitivestype now relies on the updated scalar types, which improves the consistency of type usage and simplifies understanding of the underlying data structures.
65-85: Consistent naming in composite typesComposite types such as
CompositesOptScalarString1OptionalandCompositesListScalarString1Listnow use the new scalar type names, ensuring consistent naming conventions throughout the code.
118-128: Enhanced clarity in cyclic type definitionsThe cyclic types like
SimpleCycles1,SimpleCycles2, andSimpleCycles3have been updated to use the new scalar type definitions, which improves readability and helps prevent potential confusion in the cyclic relationships.examples/typegraphs/metagen/rs/fdk.rs (3)
222-224: LGTM! Type aliases follow the new naming convention.The renaming from specific prefixes to generic
Scalarprefixes aligns with the PR objective to fix naming of shared types.
227-232: LGTM! Struct fields consistently use the new type aliases.The field types are correctly updated to use the new scalar types while preserving the serde attributes.
112-112: Verify version compatibility.The version bump from
rc.8torc.9aligns with the API changes for type naming. Please ensure all dependent packages are updated accordingly.✅ Verification successful
Version update is consistently applied
The version bump to
0.5.0-rc.9has been properly synchronized across all relevant files in the codebase, including package manifests, source files, and tests. The only expected difference is intools/consts.tswherePUBLISHED_VERSIONtracks the previous release.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for version references across the codebase. # Search for version string references rg -g '!target' -g '!*.lock' '"0.5.0-rc.[0-9]"'Length of output: 1565
tests/metagen/typegraphs/sample/rs/main.rs (5)
41-42: LGTM! Type update follows the new naming convention.The type has been correctly updated to use
ScalarStructShared2Partialwhile preserving the closure logic.
101-104: LGTM! Mutation correctly uses the new type.The mutation has been updated to use
ScalarStructShared2Partialwhile maintaining the same structure.
110-116: LGTM! Query correctly uses the new type.The
scalar_unionquery has been updated to useScalarStructShared2Partialconsistently.
123-127: LGTM! Query correctly uses the new type.The
composite_unionquery has been updated to useScalarStructShared2Partialwhile preserving the selection structure.
Line range hint
130-137: LGTM! Query correctly uses the new type.The
mixed_unionquery has been updated to useScalarStructShared2Partialwhile maintaining the selection structure.tests/runtimes/wasm_wire/rust/fdk.rs (3)
112-112: Version update is consistent.The version bump matches other files in the codebase.
222-268: LGTM! Comprehensive update to new type naming convention.The changes consistently apply the new scalar type naming convention across all type aliases and struct fields while preserving serde attributes.
293-293: LGTM! Handler signature updated correctly.The handler's return type has been updated to use the new scalar type.
tests/metagen/typegraphs/identities/rs/fdk.rs (3)
222-233: LGTM! Basic scalar types follow the new convention.The type aliases consistently use the "Scalar" prefix and maintain appropriate primitive type mappings.
236-248: LGTM! Struct fields consistently use new scalar types.The field types have been updated to use the new scalar types while preserving serde attributes and documentation.
Line range hint
254-340: LGTM! Complex types consistently use new naming convention.The changes maintain type relationships and hierarchy while adopting the new scalar type naming convention throughout complex and nested structures.
src/typegraph/core/src/utils/postprocess/naming.rs (8)
13-13: LGTM: Import of IndexSet for efficient reference tracking.The IndexSet provides both set operations and indexing, which is ideal for tracking type references.
36-42: LGTM: Reference counting initialization.The implementation correctly initializes reference counting before processing types, which is essential for tracking shared types.
44-50: LGTM: Enhanced VisitCollector initialization.The collector now includes reference counting data and name occurrence tracking, which supports the new naming strategy.
77-80: LGTM: New fields in VisitCollector.The added fields support reference counting and name tracking:
counts: Maps type IDs to their referrersname_occurences: Tracks name usage frequency
82-95: LGTM: TypeRefCount implementation.Clean implementation of reference counting with:
- Efficient storage using HashMap and IndexSet
- Clear method for updating references
97-117: LGTM: Helper methods for name generation.Well-implemented helper methods:
has_more_than_one_referrer: Clear check for shared typesnext_name: Generates unique names with incrementing counters
275-335: LGTM: Comprehensive reference collection implementation.The
collect_ref_infofunction correctly:
- Handles all type variants
- Skips user-named types
- Uses visited set to prevent redundant processing
Note: Previous concerns about cyclic references are addressed by the codebase's design where cyclic types can only occur with user-renamed types, which are skipped by this processor.
Line range hint
338-374: LGTM: Enhanced name generation logic.The updated
gen_namefunction now:
- Preserves user-named types
- Uses scalar naming for basic types
- Handles shared types with special naming
src/typegate/src/typegraphs/introspection.json (1)
48-48: LGTM: Consistent scalar type naming.The renaming follows the new convention for scalar types:
- Basic string type →
scalar_string_1- Optional string type →
scalar_string_1_optional- Boolean type →
scalar_boolean_1Also applies to: 102-102, 130-130, 136-136
src/typegate/src/typegraphs/typegate.json (1)
86-86: LGTM: Consistent scalar and shared type naming.The renaming follows the established convention:
- Scalar types:
scalar_string_1,scalar_boolean_1, etc.- Shared types:
scalar_struct_shared_1,scalar_struct_shared_2, etc.- Enums:
scalar_string_enum_1,scalar_string_enum_2, etc.Also applies to: 112-112, 116-116, 201-201, 243-243, 299-299, 304-304, 443-443, 480-480, 599-599, 700-700, 706-706, 780-780
|
What was that proposal of yours to avoid stability issues around the numbers? The current names would change when you expose new functions that are not placed at the end. Edit: right, using the reference count? Wouldn't that lead to duplicate names? |
|
How about, alternatively, we use "short hashes of the first reference path" in the name? This way, we get stability based on graph location without having to use the path location in the name. |
No it's not using the ref count, it's just returning the type name and if it's already used it will increase the suffix by 1 (so it's increasing if a dup is encountered). This seemed more natural than keeping the ids. Also, I was curious of the count at which a scalar type was duplicated on our snapshots. |
Like taking the first n chars of each path segment? Or directly hash it...? |
|
Ah, I see what you mean by dup count. It's stability is much better, but all the numbers will change if you add a new func that has uses the same "kind" before the others, the old types of the same kind will have their reference indices incremented.
Directly hashing it. So you'd have |
Since types can be shared, prefixing the name with the first function/path encountered would be confusing for the other types that also refers to it.
This patch disables prefixing for types with more than one referrer.
Migration notes
None
Summary by CodeRabbit
"0.5.0-rc.9"for compatibility checks.