Truncate too long delegate aux relation name in runtime #1629
Conversation
WalkthroughWalkthroughThe changes implement a robust identifier length management system across the codebase, ensuring compliance with database constraints. A new constant, Changes
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 as PR comments)
Additionally, you can add 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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- packages/runtime/src/constants.ts (1 hunks)
- packages/runtime/src/cross/utils.ts (2 hunks)
- packages/runtime/src/enhancements/delegate.ts (2 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (8 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/utils.ts (1 hunks)
Additional comments not posted (13)
tests/integration/tests/enhancements/with-delegate/utils.ts (1)
48-50: LGTM! Verify the usage of the new model.The new model
VideoWithVeryLongPropertywith a long property name seems correctly defined. Ensure that it is used appropriately in the tests.packages/runtime/src/constants.ts (1)
71-73: LGTM!The new constant
IDENTIFIER_NAME_MAX_LENGTHis correctly calculated and well-documented.packages/runtime/src/cross/utils.ts (1)
80-101: LGTM! Verify the function usage.The function
truncatecorrectly handles the truncation and indexing logic. Ensure it is used appropriately in the codebase.Verification successful
The function
truncateis used correctly and consistently across the codebase.The
truncatefunction is appropriately utilized in managing identifier lengths and ensuring uniqueness in various parts of the code, particularly in constructing auxiliary relation names and foreign key fields.
packages/schema/src/plugins/prisma/schema-generator.tspackages/runtime/src/enhancements/delegate.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new function `truncate`. # Test: Search for the function usage. Expect: Occurrences of `truncate`. rg --type ts -A 5 $'truncate'Length of output: 5539
Script:
#!/bin/bash # Description: Inspect specific lines where `truncate` is used to ensure correct and consistent usage. # Search for the `truncate` function usage and include more context lines for better understanding. rg --type ts -A 10 -B 5 $'truncate'Length of output: 14520
packages/schema/src/plugins/prisma/schema-generator.ts (5)
37-37: LGTM! Import statement looks correct.The
truncatefunction is correctly imported from@zenstackhq/runtime.
338-338: LGTM!The
truncatefunction is correctly used to ensure the relation field name is truncated.
486-486: LGTM!The
truncatefunction is correctly used to ensure the foreign key field name is truncated.
597-597: LGTM!The
truncatefunction is correctly used to ensure the relation name is truncated.
317-317: LGTM! But verify the function usage in the codebase.The
truncatefunction is correctly used to ensure the auxiliary relation name is truncated.However, ensure that all calls to
truncateare appropriate and consistent across the codebase.Verification successful
Verification required for
truncatefunction usage.The
truncatefunction is used consistently across the codebase to ensure names do not exceed a certain length. This usage appears appropriate, but please verify the definition ofIDENTIFIER_NAME_MAX_LENGTHand ensure that thetruncatefunction is used correctly in all contexts.
schema-generator.ts:
- Lines 317, 322, 327, 332, 337
delegate.ts:
- Line 12
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `truncate` are appropriate and consistent. # Test: Search for the function usage. Expect: Only appropriate and consistent occurrences. rg --type ts -A 5 $'truncate'Length of output: 5539
packages/runtime/src/enhancements/delegate.ts (2)
16-16: LGTM! Import statement looks correct.The
truncatefunction is correctly imported from../cross.
1044-1044: LGTM!The
truncatefunction is correctly used to ensure the auxiliary relation name is truncated.tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (3)
1210-1218: LGTM! Ensure edge cases for truncation are covered.The test case verifies that the very long property name is correctly stored and retrieved. Ensure that edge cases, such as property names exactly at the maximum length, are also tested.
1199-1201: Verify the setup function usage.Ensure that the
setupfunction correctly initializes the database and that it is reused across tests to maintain consistency.Verification successful
Verified: The
setupfunction is consistently used across the test cases.The
setupfunction is correctly initializing the database and is reused across tests to maintain consistency.
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the setup function usage across the test file. # Test: Search for the setup function usage. Expect: Consistent usage across test cases. rg --type ts -A 5 $'setup'Length of output: 26974
1202-1208: Ensure the property name truncation mechanism is tested.The test case creates a record with a very long property name. Verify that the truncation mechanism is applied and tested.
Verification successful
The property name truncation mechanism is indirectly tested.
The test cases involving properties with very long names, such as
averyveryveryveryveryveryveryveryveryveryveryverylongProperty, indicate that the truncation mechanism is indirectly tested.
tests/integration/tests/enhancements/with-delegate/utils.tstests/integration/tests/enhancements/with-delegate/enhanced-client.test.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the property name truncation mechanism. # Test: Search for the truncation function usage. Expect: Consistent application of truncation across the codebase. rg --type ts -A 5 $'truncate'Length of output: 5539
Script:
#!/bin/bash # Description: Verify if the truncation mechanism is tested. # Test: Search for test cases that involve the truncate function or related functionality. rg --type ts -A 5 'truncate' tests/Length of output: 35
Script:
#!/bin/bash # Description: Verify if the truncation mechanism is indirectly tested. # Test: Search for test cases that involve creating or manipulating properties with long names. rg --type ts -A 5 'averyveryveryveryveryveryveryveryveryveryveryverylongProperty' tests/Length of output: 1821
tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/utils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts
- tests/integration/tests/enhancements/with-delegate/utils.ts
|
Hi @irvinzz , I think the current approach is a little brittle because the name shortening is totally based on a sequential suffix, and it can be unreliable at runtime, depending on what model names are processed first. I think a more reliable way is to carry over the short name mapping to runtime (record them inside model metadata) so we can directly look it up instead of recomputing. Do you mind if I make the changes directly on your branch? |
|
Hi @ymc9. Yes. Agree. It's brittle. I've investigated truncate behaviour in all cases and found that it looks useless to truncate field names. Especially in my issue. Fields of concrete models are 'virtual' (not used to communicate with real db) so don't really need to be truncated i guess. Keeped truncate for relation names and uniq constraints. I hope didn't missed something. |
…r to runtime - Changed the direction of name mapping to "full -> short" for easier consumption - Saved the map into model meta - Use the map at runtime to get shortened names
I understand it's a bit confusion. ZenStack generates additional relation fields and constraints to facilitate polymorphic modeling, and these names can exceed the max length limits. The problem is that some of these fields are also used at runtime, so ideally, the mapping should be carried over from generation time to runtime. I've made some changes based on your PR to implement the carry-over. It's a bit unfortunate that this introduced some more coupling among plugins, but I don't see a better way to address this without a bigger refactor. |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (3)
packages/sdk/src/types.ts (2)
33-38: Document theshortNameMapproperty.The
shortNameMapproperty has been added to thePluginOptionstype. Ensure that this property is documented clearly to indicate its purpose and usage./** * An optional map of full names to shortened names * @private */ - shortNameMap?: Map<string, string>; + shortNameMap?: Map<string, string>; // Mapping of full names to shortened names
82-87: Document theshortNameMapproperty.The
shortNameMapproperty has been added to thePluginResulttype. Ensure that this property is documented clearly to indicate its purpose and usage./** * An optional map of full names to shortened names * @private */ - shortNameMap?: Map<string, string>; + shortNameMap?: Map<string, string>; // Mapping of full names to shortened namespackages/runtime/src/cross/model-meta.ts (1)
165-168: Document theshortNameMapproperty.The
shortNameMapproperty has been added to theModelMetatype. Ensure that this property is documented clearly to indicate its purpose and usage./** * Optional map from full names to shortened names, used for extra fields/relations generated by ZenStack */ - shortNameMap?: Record<string, string>; + shortNameMap?: Record<string, string>; // Mapping of full names to shortened names
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- packages/runtime/src/cross/model-meta.ts (1 hunks)
- packages/runtime/src/enhancements/delegate.ts (1 hunks)
- packages/schema/src/cli/plugin-runner.ts (5 hunks)
- packages/schema/src/plugins/enhancer/model-meta/index.ts (1 hunks)
- packages/schema/src/plugins/prisma/index.ts (3 hunks)
- packages/schema/src/plugins/prisma/schema-generator.ts (9 hunks)
- packages/sdk/src/model-meta-generator.ts (4 hunks)
- packages/sdk/src/types.ts (2 hunks)
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts (1 hunks)
- tests/integration/tests/enhancements/with-delegate/utils.ts (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/runtime/src/enhancements/delegate.ts
- packages/schema/src/plugins/prisma/schema-generator.ts
- tests/integration/tests/enhancements/with-delegate/enhanced-client.test.ts
- tests/integration/tests/enhancements/with-delegate/utils.ts
Additional comments not posted (11)
packages/schema/src/plugins/enhancer/model-meta/index.ts (1)
16-16: EnsureshortNameMapis correctly handled ingenerateModelMeta.The new parameter
shortNameMaphas been added to the function call togenerateModelMeta. Ensure thatgenerateModelMetacorrectly handles this new parameter and that it aligns with the intended functionality of truncating long relation names.Verification successful
The
shortNameMapparameter is correctly handled ingenerateModelMeta.The
writeShortNameMapfunction processes theshortNameMapparameter as intended, ensuring that each key-value pair is written correctly.
packages/sdk/src/model-meta-generator.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of the `shortNameMap` parameter in `generateModelMeta`. # Test: Search for the `generateModelMeta` function definition. Expect: Correct handling of `shortNameMap`. rg --type typescript -A 10 $'function generateModelMeta'Length of output: 92
Script:
#!/bin/bash # Description: Verify the handling of the `shortNameMap` parameter in `generateModelMeta`. # Test: Search for the `generateModelMeta` function definition. Expect: Correct handling of `shortNameMap`. rg -t ts -A 10 $'function generateModelMeta'Length of output: 880
Script:
#!/bin/bash # Description: Verify the handling of the `shortNameMap` parameter in `generateModelMeta`. # Test: Search for the `writeShortNameMap` function definition. Expect: Correct handling of `shortNameMap`. rg -t ts -A 10 $'function writeShortNameMap'Length of output: 864
packages/schema/src/plugins/prisma/index.ts (3)
78-78: LGTM!The inclusion of
shortNameMapin the return value is approved.
23-23: EnsureshortNameMapis correctly generated.Verify that
shortNameMapis correctly generated and utilized within the function.Verification successful
shortNameMapis correctly generated and utilized.The
shortNameMapis initialized and populated in thePrismaSchemaGeneratorclass and is correctly returned and used in various parts of the codebase.
packages/schema/src/plugins/prisma/schema-generator.ts: Generation ofshortNameMap.packages/schema/src/plugins/prisma/index.ts: Usage ofshortNameMap.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `shortNameMap`. # Test: Search for the generation and usage of `shortNameMap` in the codebase. Expect: Correct generation and usage. rg --type ts -A 5 $'const { warnings, shortNameMap } = await new PrismaSchemaGenerator(model).generate(mergedOptions)'Length of output: 307
Script:
#!/bin/bash # Description: Verify the generation and usage of `shortNameMap`. # Search for the generation of `shortNameMap` in the codebase. rg --type ts 'shortNameMap' -A 10Length of output: 15563
22-22: Verify the correctness ofmergedOptions.Ensure that
mergedOptionscorrectly consolidates the originaloptionswith the additionaloutputproperty.Verification successful
Verify the correctness of
mergedOptions.The
mergedOptionsobject consolidates the originaloptionswith the additionaloutputproperty correctly and is used appropriately within thegeneratemethod of thePrismaSchemaGeneratorclass.
packages/schema/src/plugins/prisma/index.ts: Line 22packages/schema/src/plugins/prisma/schema-generator.ts: Lines 109-164Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `mergedOptions`. # Test: Search for the usage of `mergedOptions` in the codebase. Expect: Correct consolidation of options. rg --type ts -A 5 $'const mergedOptions = { ...options, output }'Length of output: 217
Script:
#!/bin/bash # Description: Verify the correctness of `mergedOptions`. # Test: Search for the usage of `mergedOptions` in the codebase. Expect: Correct consolidation of options. rg --type ts -A 5 'const mergedOptions = \{ \.\.\.options, output \}'Length of output: 672
Script:
#!/bin/bash # Description: Verify the correctness of `mergedOptions` by checking the implementation of `generate` method. # Test: Search for the implementation of the `generate` method in the `PrismaSchemaGenerator` class. ast-grep --lang typescript --pattern $'class PrismaSchemaGenerator { $$$ async generate($_) { $$$ } $$$ }'Length of output: 82835
packages/schema/src/cli/plugin-runner.ts (4)
148-151: LGTM!The integration of
shortNameMapwithin therunmethod is approved.
140-140: EnsureshortNameMapis passed torunPlugin.Verify that
shortNameMapis correctly passed to therunPluginmethod.Verification successful
Verified: The
shortNameMapis correctly passed to therunPluginmethod inpackages/schema/src/cli/plugin-runner.ts.
packages/schema/src/cli/plugin-runner.ts:140:const r = await this.runPlugin(name, description, run, runnerOptions, options, dmmf, shortNameMap, project);Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the passing of `shortNameMap` to `runPlugin`. # Test: Search for the invocation of `runPlugin` with `shortNameMap` in the codebase. Expect: Correct passing of `shortNameMap`. rg --type ts -A 5 $'await this.runPlugin(name, description, run, runnerOptions, options, dmmf, shortNameMap, project)'Length of output: 119
Script:
#!/bin/bash # Description: Verify the passing of `shortNameMap` to `runPlugin`. # Test: Search for the invocation of `runPlugin` with `shortNameMap` in the codebase. Expect: Correct passing of `shortNameMap`. rg --type ts -A 10 'await this.runPlugin' | grep -B 5 'shortNameMap'Length of output: 1005
135-135: Verify the initialization ofshortNameMap.Ensure that
shortNameMapis correctly initialized and utilized within therunmethod.Verification successful
The initialization and usage of
shortNameMapare correct.
shortNameMapis initialized asundefined.- It is potentially assigned a value from the result of
runPlugin.- This value is then used in subsequent plugin runs.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization of `shortNameMap`. # Test: Search for the initialization and usage of `shortNameMap` in the codebase. Expect: Correct initialization and usage. rg --type ts -A 5 $'let shortNameMap: Map<string, string> | undefined'Length of output: 297904
Script:
#!/bin/bash # Description: Search for all occurrences of `shortNameMap` within `plugin-runner.ts` to verify its initialization and usage. # Test: Search for `shortNameMap` in `plugin-runner.ts`. rg --type ts 'shortNameMap' -A 5 -B 5 packages/schema/src/cli/plugin-runner.tsLength of output: 2748
Line range hint
318-337:
EnsureshortNameMapis correctly included inrunPlugin.Verify that
shortNameMapis correctly included in therunPluginmethod and passed to the plugin's run function.Verification successful
Verification Successful:
shortNameMapis correctly included inrunPlugin.The
shortNameMapparameter is properly defined, passed to therunPluginmethod, and used within the method's body inplugin-runner.ts.
- Defined as
let shortNameMap: Map<string, string> | undefined;- Passed to
runPlugin:const r = await this.runPlugin(name, description, run, runnerOptions, options, dmmf, shortNameMap, project);- Used within the method:
if (r.shortNameMap) { shortNameMap = r.shortNameMap; }Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the inclusion of `shortNameMap` in `runPlugin`. # Test: Search for the usage of `shortNameMap` within the `runPlugin` method. Expect: Correct inclusion and passing of `shortNameMap`. rg --type ts -A 5 $'shortNameMap: Map<string, string> | undefined'Length of output: 297900
Script:
#!/bin/bash # Search for the `runPlugin` method definition and ensure `shortNameMap` is used within its body. ast-grep --lang typescript --pattern $'function runPlugin($_, $_, $_, $_, $_, $_, $_) { $$$ }' -A 20Length of output: 103
Script:
#!/bin/bash # Search for the `runPlugin` method definition and ensure `shortNameMap` is used within its body in the `plugin-runner.ts` file. rg --type ts 'function runPlugin' -A 20 packages/schema/src/cli/plugin-runner.ts # Additionally, search for `shortNameMap` usage within the `plugin-runner.ts` file to ensure it is passed correctly. rg --type ts 'shortNameMap' packages/schema/src/cli/plugin-runner.tsLength of output: 626
packages/sdk/src/model-meta-generator.ts (3)
61-61: LGTM!The addition of the
shortNameMapproperty toModelMetaGeneratorOptionsis approved.
540-550: LGTM!The addition of the
writeShortNameMapfunction is approved.
92-92: EnsurewriteShortNameMapis correctly called.Verify that
writeShortNameMapis correctly called within thegeneratefunction.Verification successful
Ensure
writeShortNameMapis correctly called.The function
writeShortNameMapis correctly implemented to handle theshortNameMapoption. Its invocation within thegeneratefunction appears appropriate, assumingoptionsandwriterare correctly provided.
writeShortNameMapis defined inpackages/sdk/src/model-meta-generator.tsstarting at line 540.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `writeShortNameMap` call. # Test: Search for the invocation of `writeShortNameMap` in the codebase. Expect: Correct invocation within the `generate` function. rg --type ts -A 5 $'writeShortNameMap(options, writer)'Length of output: 56
Script:
#!/bin/bash # Description: Search for any invocation of `writeShortNameMap` in the TypeScript files. rg --type ts 'writeShortNameMap'Length of output: 248
Script:
#!/bin/bash # Description: Extract the definition of `writeShortNameMap` from the TypeScript files. ast-grep --lang ts --pattern $'function writeShortNameMap($_, $_) { $$$ }'Length of output: 940
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (6)
tests/regression/tests/issue-1466.test.ts (6)
11-14: Consider the readability and maintainability of extended model names.The extended model names, such as
UserLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
18-22: Consider the readability and maintainability of extended model names.The extended model names, such as
AssetLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
28-28: Consider the readability and maintainability of extended model names.The extended model names, such as
VideoLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
42-42: Consider the readability and maintainability of extended model names.The extended model names, such as
VideoLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
46-46: Consider the readability and maintainability of extended model names.The extended model names, such as
UserLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
52-57: Consider the readability and maintainability of extended model names.The extended model names, such as
UserLongLongLongLongLongLongLongLongName, might affect readability and maintainability. Consider using shorter, more concise names if possible.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- packages/sdk/src/model-meta-generator.ts (4 hunks)
- tests/regression/tests/issue-1466.test.ts (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- packages/sdk/src/model-meta-generator.ts
Issue
Long relation names cause any db operations failure