v6 tracing (new native Dart ORM) #484
Conversation
The `Map` class name conflicts with the built-in `dart:core` Map type. Renaming to `MapTo` avoids the conflict while preserving the annotation's purpose of mapping to a database name.
This comment was marked as outdated.
This comment was marked as outdated.
📝 WalkthroughWalkthroughAdds an analyzer plugin (rule + fix + utils + tests) that enforces/inserts a required ORM config, renames annotation Changes
Sequence Diagram(s)sequenceDiagram
participant Analyzer as Dart Analyzer
participant Plugin as AnalysisPlugin
participant Rule as ConfigRequiredRule
participant Visitor as _Visitor
participant Fix as ConfigRequiredFix
participant Builder as ChangeBuilder
Note over Analyzer,Plugin: plugin registration
Analyzer->>Plugin: register(PluginRegistry)
Plugin->>Rule: registerWarningRule(ConfigRequiredRule)
Plugin->>Fix: registerFixForRule(ConfigRequiredRule.code, ConfigRequiredFix)
Note over Analyzer,Rule: analysis pass
Analyzer->>Rule: analyze file (orm.config.dart)
Rule->>Visitor: visitCompilationUnit(node)
Visitor->>Visitor: check for required const Config
alt missing config
Visitor->>Analyzer: report diagnostic (Config required)
else config present
Visitor->>Analyzer: no diagnostic
end
Note over Analyzer,Fix: fix application flow
Analyzer->>Fix: request fix for diagnostic
Fix->>Fix: findConfigImport(), compute offsets & texts
Fix->>Builder: insert import text (if needed)
Fix->>Builder: insert config declaration text
Builder->>Analyzer: apply edits
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Comment |
- Add playground directory with pubspec and analysis options - Configure ORM plugin and SQLite provider - Remove unused SQLite annotations file - Update config to only support SQLite provider
![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
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
playground/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
playground/analysis_options.yamlplayground/orm.config.dartplayground/pubspec.yamlpub/orm/lib/config.dartpub/orm/lib/main.dartpub/orm/lib/src/sqlite/annotations.dart
💤 Files with no reviewable changes (1)
- pub/orm/lib/src/sqlite/annotations.dart
🔇 Additional comments (8)
playground/analysis_options.yaml (1)
1-4: LGTM! Standard analysis configuration.The analysis options file correctly includes recommended lints and configures the ORM plugin with the appropriate relative path.
playground/pubspec.yaml (3)
8-9: LGTM! Path dependency configured correctly.The local path dependency on the ORM package is properly configured for the playground.
12-12: The lints package version 6.0.0 is available on pub.dev as of January 2026. The version constraint^6.0.0is valid and no changes are needed.
5-5: No action needed. The Dart SDK constraint^3.10.1is valid; Dart 3.10.1 exists, and the constraint will resolve to compatible versions (3.10.1 through 3.10.3 are all available as of January 2026).playground/orm.config.dart (1)
1-3: LGTM! ORM configuration is correct.The configuration correctly uses the SQLite provider with enhanced enum syntax and specifies an appropriate output directory for generated code.
pub/orm/lib/main.dart (2)
6-8: LGTM! Plugin class structure is correct.The plugin class properly extends
Pluginand declares its name as 'orm', which matches the configuration inplayground/analysis_options.yaml.
1-4: analysis_server_plugin v0.3.4 is compatible with Dart SDK ^3.10.1.Version 0.3.4 requires Dart SDK 3.9 (minimum), which is satisfied by the ^3.10.1 constraint in the playground package.
pub/orm/lib/config.dart (1)
3-3: The breaking API change is safe for this codebase.While removing
mysql,postgresql, andsqlserverfrom theDatabaseProviderenum is a breaking API change, no code in the repository references these removed providers. The change does not impact existing code.
The plugin now provides a lint rule that checks for the required `const config = Config(...);` in `orm.config.dart` files and offers a quick fix to add it when missing.
- Rename AddConfigConstantFix to ConfigRequiredFix - Add check to skip fix if config variable already exists - Improve rule detection to handle imported Config types - Set rule severity to ERROR
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pub/orm/lib/src/analyzer/fixes/config_required_fix.dart (1)
64-89: Consider extracting shared helpers to reduce duplication.Both
_findConfigImportand_hasConfigVariableduplicate logic fromConfigRequiredRule._Visitor. Consider extracting these to a shared utility to maintain consistency and reduce code duplication.pub/orm/lib/src/analyzer/rules/config_required_rule.dart (1)
55-57: Consider narrowing the error span for better UX.Reporting on the entire
CompilationUnitwill highlight the whole file. For better IDE integration, consider reporting at a more specific location (e.g., the first line or a synthetic node offset) so users can quickly identify where to add the missing config.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
playground/orm.config.dartpub/orm/lib/main.dartpub/orm/lib/src/analyzer/assists/.gitkeeppub/orm/lib/src/analyzer/fixes/config_required_fix.dartpub/orm/lib/src/analyzer/rules/config_required_rule.dartpub/orm/pubspec.yaml
✅ Files skipped from review due to trivial changes (1)
- playground/orm.config.dart
🔇 Additional comments (3)
pub/orm/lib/main.dart (1)
7-18: LGTM - plugin implementation is now complete.The
registermethod now properly registers theConfigRequiredRuleas a warning and associatesConfigRequiredFixfor corrections. The top-levelplugininstance follows the expected pattern for Dart analyzer plugins.pub/orm/lib/src/analyzer/rules/config_required_rule.dart (1)
8-33: LGTM - rule implementation is well-structured.The rule correctly identifies
orm.config.dartfiles and validates the presence of a properly typedconst config = Config(...)declaration. The element-based resolution with prefix fallback handles edge cases appropriately.pub/orm/pubspec.yaml (1)
14-14: LGTM - dependency versions are compatible.The
analyzer_plugin: ^0.13.11addition is compatible withanalyzer: ^9.0.0. The analyzer_plugin version 0.13.11 specifies analyzer 9.0.0 as its dependency, which falls within the ^9.0.0 constraint.
- Add test_reflective_loader and analyzer_testing dependencies - Create README for analyzer plugin layout - Extract common config fix logic into base class - Add config replacement fix for existing config variables - Add unit tests for config required rule and fix kinds
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
playground/orm.config.dartpub/orm/lib/main.dartpub/orm/lib/src/analyzer/README.mdpub/orm/lib/src/analyzer/fixes/_config_fix.dartpub/orm/lib/src/analyzer/fixes/config_required_fix.dartpub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dartpub/orm/pubspec.yamlpub/orm/test/analyzer/config_required_fix_test.dartpub/orm/test/analyzer/config_required_rule_test.dart
✅ Files skipped from review due to trivial changes (1)
- pub/orm/lib/src/analyzer/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pub/orm/pubspec.yaml
🔇 Additional comments (16)
playground/orm.config.dart (1)
5-5: TODO comment is appropriate for playground code.The empty output path with a TODO comment is reasonable for a playground/example file. Just ensure this configuration is updated before any production or real usage.
pub/orm/test/analyzer/config_required_fix_test.dart (1)
6-15: LGTM!The test correctly verifies that both fix classes expose the expected fix kind identifiers, ensuring consistency with the plugin registration.
pub/orm/lib/main.dart (2)
8-21: Plugin registration implemented correctly.The register method now properly registers the warning rule and both associated fixes. This addresses the previous review comment about the TODO.
23-23: LGTM!The plugin instance follows the standard pattern for analyzer plugin initialization.
pub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dart (2)
8-15: LGTM!The fix kind definition and constructor are properly structured. The fix ID matches the test expectations and the description clearly indicates the fix purpose.
24-36: LGTM!The compute method correctly validates that the config variable is suitable for replacement (single declaration without metadata) before proceeding with the fix.
pub/orm/lib/src/analyzer/fixes/config_required_fix.dart (2)
8-15: LGTM!The fix kind definition and constructor are correctly implemented, with clear identification and description.
24-31: LGTM!The compute method correctly checks for an existing config variable and only proceeds with insertion when the config is missing.
pub/orm/test/analyzer/config_required_rule_test.dart (2)
13-28: Good test setup with mocked ORM package structure.The setUp properly registers the rule, creates a pubspec, and simulates the
ormpackage'sconfig.dartwith the expectedConfigclass andDatabaseProviderenum. This provides realistic test isolation.
30-40: Helper methods are well-structured.
_assertMissingConfigand_assertValidConfigprovide clean abstractions for common assertion patterns, improving test readability.pub/orm/lib/src/analyzer/fixes/_config_fix.dart (6)
9-20: LGTM: Clean import directive discovery.The method correctly iterates directives, filters for
ImportDirective, and matches the expected package URI.
22-34: LGTM: Config variable discovery is straightforward.Properly searches top-level declarations for a variable named
config.
36-59: LGTM: Offset calculations handle edge cases.Both methods correctly handle empty directives by returning 0, and properly calculate the next line offset for insertions.
76-109: LGTM: Insert logic handles import/config placement well.The method correctly handles the case where import and config offsets coincide (single insertion) vs. separate insertions.
111-147: LGTM: Replace logic mirrors insert logic correctly.Consistently handles the combined vs. separate edit scenarios.
150-157: TheisSinglegetter is already in use.It's accessed in
config_required_replace_fix.dartat line 31 as part of a conditional check. No removal is needed.Likely an incorrect or invalid review comment.
Extract common config helper methods from ConfigFix base class into ConfigUtils mixin Update ConfigRequiredFix and ConfigRequiredReplaceFix to use the mixin instead of inheritance Remove ConfigFix base class as it's no longer needed
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pub/orm/lib/src/analyzer/fixes/_config_utils.dart (1)
18-30: Consider explicit constructor syntax for clarity.Line 25 uses the constructor shorthand
.new(declaration, variable). While valid in Dart 2.15+, the explicit formConfigVariableInfo(declaration, variable)may be clearer for readers.🔎 Optional refactor for improved readability
for (final variable in declaration.variables.variables) { if (variable.name.lexeme == 'config') { - return .new(declaration, variable); + return ConfigVariableInfo(declaration, variable); } }pub/orm/lib/src/analyzer/rules/config_required_rule.dart (1)
92-103: Consider extracting shared logic to avoid duplication.The
_findConfigImportmethod duplicatesfindConfigImportfromConfigUtilsin_config_utils.dart. While keeping rules independent from fix utilities has merit, consider:
- Having the rule mix in
ConfigUtilsfor DRY benefits- Extracting the common logic to a shared non-fix-specific utility module
🔎 Option: Reuse ConfigUtils
-class _Visitor extends SimpleAstVisitor<void> { +import '../fixes/_config_utils.dart'; + +class _Visitor extends SimpleAstVisitor<void> with ConfigUtils { final AnalysisRule rule; final RuleContext context; _Visitor(this.rule, this.context); @override void visitCompilationUnit(CompilationUnit node) { // ... existing code ... } bool _hasRequiredConfig(CompilationUnit unit) { - final configImport = _findConfigImport(unit); + final configImport = findConfigImport(unit); final hasLocalConfig = _hasLocalConfigDeclaration(unit); // ... rest of method ... } - ImportDirective? _findConfigImport(CompilationUnit unit) { - for (final directive in unit.directives) { - if (directive is! ImportDirective) { - continue; - } - final uri = directive.uri.stringValue; - if (uri == 'package:orm/config.dart') { - return directive; - } - } - return null; - }Note: This requires
_Visitorto have access toResolvedCorrectionProducerproperties likeutils, which may not be available in the rule context. Verify compatibility before applying.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
playground/orm.config.dartpub/orm/lib/src/analyzer/fixes/_config_utils.dartpub/orm/lib/src/analyzer/fixes/config_required_fix.dartpub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dartpub/orm/lib/src/analyzer/rules/config_required_rule.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- playground/orm.config.dart
- pub/orm/lib/src/analyzer/fixes/config_required_fix.dart
🔇 Additional comments (14)
pub/orm/lib/src/analyzer/fixes/_config_utils.dart (6)
5-16: LGTM: Clear logic for finding config import.The method correctly iterates through directives, filters for imports, and checks for the specific ORM config package URI.
32-37: LGTM: Correct insertion offset calculation.The method properly handles both empty files and files with existing directives.
39-55: LGTM: Proper import insertion logic.The method correctly prioritizes the last import directive, falls back to library directive, and handles files without either.
57-57: LGTM.
59-71: LGTM: Well-structured config text generation.The method correctly handles import prefixes and generates appropriate config code with a TODO reminder for the output path, which appropriately requires user attention.
74-81: LGTM: Clean data class.The
ConfigVariableInfoclass is well-designed as a simple data holder with a usefulisSinglehelper.pub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dart (3)
9-23: LGTM: Standard fix configuration.The fix is properly configured with appropriate kind, priority, and applicability settings.
39-75: LGTM: Robust replacement logic with edge case handling.The method correctly handles the edge case where
importOffset == replaceRange.offset(lines 56-63), preventing duplicate insertions. The conditional import insertion (lines 65-69) followed by the config replacement (lines 71-73) is well-structured.
25-37: Metadata check prevents silent data loss.The restriction on line 32 that skips the fix when
info.declaration.metadata.isNotEmptyis intentional and necessary. Since the fix replaces the entireTopLevelVariableDeclaration(lines 52–53), applying it to an annotated declaration like@deprecated const config = ...;would silently remove the annotation. The check prevents this data loss by declining to apply the fix in such cases.This is defensive programming, not an overly restrictive limitation.
pub/orm/lib/src/analyzer/rules/config_required_rule.dart (5)
8-34: LGTM: Well-defined analysis rule.The rule is properly configured with clear messaging and appropriate severity. The registration pattern is standard for analyzer rules.
42-58: LGTM: Correct file targeting logic.The visitor properly restricts the rule to
orm.config.dartat the package root and reports diagnostics appropriately.
60-90: LGTM: Comprehensive config validation.The method correctly validates that the config is a const top-level variable with a proper
Configinitializer.
105-116: LGTM: Correct local declaration detection.The method properly identifies local
Configdeclarations while appropriately skipping function declarations.
118-153: LGTM: Robust multi-path validation.The method uses element resolution as the primary validation mechanism (lines 127-134), with well-designed fallback logic for prefix matching. The final check (line 152) correctly handles unprefixed usage by ensuring no local
Configdeclaration exists, avoiding ambiguity.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pub/orm/lib/src/analyzer/utils/analyzer_constants.dart (1)
1-15: LGTM! Clear and consistent constant definitions.All constants are well-named and serve clear purposes for the analyzer plugin configuration.
Consider adding documentation comments to explain the purpose and usage of these constants, especially for future maintainers. For example:
/// The filename for the ORM configuration file that must exist at the package root. const String ormConfigFileName = 'orm.config.dart'; /// The import URI for the ORM config package. const String ormConfigImportUri = 'package:orm/config.dart';pub/orm/lib/src/analyzer/utils/config_utils.dart (2)
69-140: Well-structured change application logic.Both
insertConfigandreplaceConfigcorrectly handle the edge case where import and config operations occur at the same offset (lines 83-90, 121-128), preventing duplicate insertions and ensuring atomic changes.Optional: Extract duplicate special-case logic
The special-case handling (when
importOffset == targetOffset) is duplicated between the two methods. You could extract this into a helper method to reduce duplication:Future<void> _applyConfigChange( ChangeBuilder builder, { required int importOffset, required int targetOffset, required String importText, required String configText, required String eol, required bool needsImport, required bool isReplacement, SourceRange? replaceRange, }) async { await builder.addDartFileEdit(file, (builder) { if (needsImport && importOffset == targetOffset) { final operation = isReplacement && replaceRange != null ? (builder.addReplacement(replaceRange, (b) { ... })) : (builder.addInsertion(targetOffset, (b) { ... })); // ... combined insertion/replacement logic return; } if (needsImport) { builder.addInsertion(importOffset, (builder) { builder.write(importText); }); } // ... separate insertion/replacement logic }); }However, the current implementation is clear and maintainable as-is.
9-141: Consider adding documentation for the public API.The
ConfigUtilsmixin provides a useful abstraction for managing ORM configuration, but lacks documentation comments. Adding doc comments would help maintainers understand:
- The purpose and responsibilities of each method
- Expected parameters and return values
- Edge cases handled (e.g., empty directives, missing imports)
- The relationship between
insertConfigandreplaceConfigExample:
/// Utilities for locating and modifying ORM configuration in Dart source files. /// /// This mixin provides methods to find, insert, and replace ORM configuration /// declarations along with their required imports. mixin ConfigUtils on ResolvedCorrectionProducer { /// Finds the ORM config import directive in the compilation unit. /// /// Returns `null` if the import is not present. ImportDirective? findConfigImport(CompilationUnit unit) { // ... } // ... additional doc comments }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pub/orm/lib/src/analyzer/README.mdpub/orm/lib/src/analyzer/fixes/config_required_fix.dartpub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dartpub/orm/lib/src/analyzer/rules/config_required_rule.dartpub/orm/lib/src/analyzer/utils/analyzer_constants.dartpub/orm/lib/src/analyzer/utils/config_ast_utils.dartpub/orm/lib/src/analyzer/utils/config_utils.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- pub/orm/lib/src/analyzer/README.md
🔇 Additional comments (11)
pub/orm/lib/src/analyzer/utils/config_ast_utils.dart (1)
5-33: LGTM! Well-structured AST utility helpers.The three helper functions are correctly implemented:
findOrmConfigImportproperly iterates and matches import URIshasLocalConfigDeclarationappropriately skips function declarations (which can't shadow type names) while checking for any named declaration matching the config class nameisConfigVariableprovides a clean predicate for variable identificationpub/orm/lib/src/analyzer/fixes/config_required_fix.dart (1)
25-32: LGTM! Clean fix implementation.The compute logic is correct:
- Properly checks if config already exists before attempting insertion
- Delegates to
insertConfigwith the appropriate offset- The past syntax generation bug was addressed in commit d231b02
pub/orm/lib/src/analyzer/rules/config_required_rule.dart (3)
11-37: LGTM! Well-structured lint rule.The rule definition is clean and follows analyzer plugin conventions:
- Appropriate ERROR severity for a required configuration
- Clear diagnostic messages with correction suggestions
- Proper registration of the visitor
45-61: LGTM! Correct file targeting logic.The visitor properly:
- Guards against incomplete context
- Restricts analysis to only
orm.config.dartfiles- Reports at the compilation unit level (appropriate for missing config)
63-129: LGTM! Thorough config validation logic.The detection logic correctly handles all scenarios:
- Identifies const config variables with the expected name
- Validates the initializer is an
InstanceCreationExpressionforConfig- Uses library URI as the definitive check when available
- Properly handles import prefix matching (lines 116-128):
- Import with prefix requires usage to match that prefix
- Import without prefix rejects prefixed usage
- Unprefixed usage without import is valid only when no local
Configdeclaration existspub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dart (1)
33-35: Add explanatory comment for metadata exclusion logic.The metadata check is intentional—
replaceConfigreplaces the entire declaration (including any annotations), so skipping when metadata is present prevents loss of annotations like@deprecated. However, this behavior should be documented with an inline comment explaining the design decision and its limitation.pub/orm/lib/src/analyzer/utils/config_utils.dart (5)
1-7: LGTM!Imports are clean and appropriate for the analyzer plugin utilities being implemented.
10-51: LGTM!The helper methods correctly handle edge cases (empty directives, missing anchors) and use appropriate AST traversal patterns for locating import and config insertion points.
53-67: LGTM!The text builders correctly handle qualified vs. unqualified imports and generate properly formatted config code. The empty output path with TODO (line 63) is intentional, prompting users to specify their output location. The hardcoded SQLite provider aligns with the PR's database restrictions.
143-150: LGTM!The
ConfigVariableInfoclass appropriately encapsulates variable declaration context. TheisSinglegetter provides a useful check for determining whether the variable can be safely replaced without affecting other variables in the same declaration.
1-4: Package versions are confirmed current and APIs are correctly used.The specified versions (analysis_server_plugin 0.3.4 and analyzer_plugin 0.13.11) are the latest available, and the code uses the exposed APIs (
ResolvedCorrectionProducer,ChangeBuilder, and AST node types) in standard ways consistent with analysis plugin patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pub/orm/test/analyzer/config_required_rule_test.dart (5)
42-47: Test method should returnFuture<void>, notvoid.The method uses
asyncbut declares return typevoid. This can cause the test runner to complete before the async body finishes.🔎 Proposed fix
- void test_missingConfig() async { + Future<void> test_missingConfig() async { await _assertMissingConfig(r''' // ignore_for_file: unused_import import 'package:orm/config.dart'; '''); }
49-58: Same async/void mismatch intest_validConfig.Change the return type to
Future<void>.🔎 Proposed fix
- void test_validConfig() async { + Future<void> test_validConfig() async {
60-69: Same async/void mismatch intest_prefixedImport.Change the return type to
Future<void>.🔎 Proposed fix
- void test_prefixedImport() async { + Future<void> test_prefixedImport() async {
71-79: Same async/void mismatch intest_localConfigClass.Change the return type to
Future<void>.🔎 Proposed fix
- void test_localConfigClass() async { + Future<void> test_localConfigClass() async {
81-90: Same async/void mismatch intest_notConst.Change the return type to
Future<void>.🔎 Proposed fix
- void test_notConst() async { + Future<void> test_notConst() async {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pub/orm/lib/src/analyzer/README.mdpub/orm/lib/src/analyzer/fixes/config_required_fix.dartpub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dartpub/orm/lib/src/analyzer/rules/config_required_rule.dartpub/orm/lib/src/analyzer/utils/config_utils.dartpub/orm/test/analyzer/config_required_fix_test.dartpub/orm/test/analyzer/config_required_rule_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- pub/orm/lib/src/analyzer/README.md
- pub/orm/lib/src/analyzer/rules/config_required_rule.dart
🔇 Additional comments (19)
pub/orm/lib/src/analyzer/utils/config_utils.dart (9)
3-14: LGTM!The function correctly searches for the ORM config import directive.
16-27: LGTM!The function correctly identifies local
Configtype declarations (class, enum, typedef) that would shadow the imported Config.
29-31: LGTM!Simple and correct variable name check.
33-45: LGTM!The function correctly locates the top-level
configvariable and returns its declaration info.
47-52: LGTM!Correctly computes the insertion offset for config declaration after directives.
54-70: LGTM!The function correctly determines the insertion point for new imports based on existing directives.
72-72: LGTM!Simple and correct import text generation.
91-98: LGTM!The helper correctly computes the offset of the next line.
100-107: LGTM!Clean data class with a useful
isSinglegetter.pub/orm/test/analyzer/config_required_fix_test.dart (1)
1-15: LGTM!Clean test verifying the fix identifiers.
pub/orm/lib/src/analyzer/fixes/config_required_replace_fix.dart (3)
9-23: LGTM!Standard fix producer setup with appropriate applicability and priority.
25-43: LGTM!Proper validation and preparation logic. The checks for single variable and absence of metadata are prudent safeguards.
45-68: LGTM!The edit logic correctly handles offset conflicts by merging import+config when their insertion points coincide. The ChangeBuilder API will handle offset adjustments for separate insertions.
Note: This depends on
buildConfigTextfrom config_utils.dart, which has a critical bug that needs to be fixed.pub/orm/test/analyzer/config_required_rule_test.dart (3)
7-28: LGTM!Proper test setup with necessary package configuration and mock Config class.
30-40: LGTM!Helper methods are correctly typed as
Future<void>and provide clear assertion logic.
93-97: LGTM!Proper reflective test suite registration.
pub/orm/lib/src/analyzer/fixes/config_required_fix.dart (3)
8-22: LGTM!Standard fix producer setup.
24-38: LGTM!Proper validation and setup. Early return when config already exists is correct.
40-60: LGTM!The edit logic correctly handles offset conflicts by combining import and config insertion when offsets coincide.
Note: This depends on
buildConfigTextfrom config_utils.dart, which has a critical bug that needs to be fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pub/orm/lib/main.dart (1)
1-18: Consider adding library-level documentation.Adding a library doc comment would help developers understand the plugin's purpose and usage pattern.
📝 Suggested documentation
+/// Analyzer plugin for the ORM package. +/// +/// This plugin provides static analysis rules and automated fixes for ORM +/// configuration, including detection of missing required configuration +/// declarations. +library; + import 'package:analysis_server_plugin/plugin.dart'; import 'package:analysis_server_plugin/registry.dart';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pub/orm/lib/main.dartpub/orm/lib/src/analyzer/rules/config_required_rule.dartpub/orm/test/analyzer/config_required_fix_test.dartpub/orm/test/analyzer/config_required_rule_test.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- pub/orm/test/analyzer/config_required_fix_test.dart
- pub/orm/test/analyzer/config_required_rule_test.dart
- pub/orm/lib/src/analyzer/rules/config_required_rule.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T16:59:51.067Z
Learnt from: medz
Repo: medz/dart-orm PR: 484
File: pub/orm/lib/src/analyzer/utils/config_utils.dart:74-89
Timestamp: 2026-01-04T16:59:51.067Z
Learning: In Dart 3.10+, use dot shorthand (e.g., .sqlite) when the type can be inferred from context, such as named parameters with declared types. This shortens code but only where the receiver type is unambiguous. Ensure the project uses a Dart language version that supports this syntax and prefer clarity when the shorthand could be ambiguous.
Applied to files:
pub/orm/lib/main.dart
🔇 Additional comments (2)
pub/orm/lib/main.dart (2)
7-16: Well-implemented plugin registration!The
AnalysisPluginimplementation properly registers the warning rule and its associated fix. The registration logic is clean and follows the standard pattern for analyzer plugins.
1-2: The imports are using the latest stable version ofanalysis_server_plugin(0.3.4).No security vulnerabilities have been reported for this version. The code is current and properly maintained.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playground/orm.schema.dartpub/orm/lib/src/schema/relation.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T16:59:51.067Z
Learnt from: medz
Repo: medz/dart-orm PR: 484
File: pub/orm/lib/src/analyzer/utils/config_utils.dart:74-89
Timestamp: 2026-01-04T16:59:51.067Z
Learning: In Dart 3.10+, use dot shorthand (e.g., .sqlite) when the type can be inferred from context, such as named parameters with declared types. This shortens code but only where the receiver type is unambiguous. Ensure the project uses a Dart language version that supports this syntax and prefer clarity when the shorthand could be ambiguous.
Applied to files:
pub/orm/lib/src/schema/relation.dartplayground/orm.schema.dart
🔇 Additional comments (1)
pub/orm/lib/src/schema/relation.dart (1)
32-35: Type change toSet<String>is appropriately applied and all usages are compliant.The type strengthening from
Iterable<String>toSet<String>correctly enforces field name uniqueness. Verification confirms the single usage in the codebase (playground/orm.schema.dart) already uses the correct Set literal syntax (references: {'authorId'}), so all call sites are compatible with the breaking change.
playground/orm.schema.dart
Outdated
| String content, | ||
| String authorId, | ||
|
|
||
| @Relation(references: {'authorId'}) User author, |
There was a problem hiding this comment.
Critical: Incorrect relation field mapping.
The references parameter should specify fields in the referenced model (User), not the current model (Post). According to the Relation documentation:
references: "fields of the model on the other side of the relation" (User model)fields: "fields of the current model" (Post model)
Since authorId is a field in Post and should reference id in User, the correct annotation should specify both parameters.
🔎 Proposed fix
- @Relation(references: {'authorId'}) User author,
+ @Relation(fields: {'authorId'}, references: {'id'}) User author,This establishes that Post.authorId (current model field) references User.id (other model field).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Relation(references: {'authorId'}) User author, | |
| @Relation(fields: {'authorId'}, references: {'id'}) User author, |
🤖 Prompt for AI Agents
In playground/orm.schema.dart around line 13, the Relation annotation
incorrectly lists the current model's field in `references`; update the
decorator to declare the current model field via `fields: ['authorId']` and the
referenced model field via `references: ['id']` so that Post.authorId references
User.id (i.e., add the missing `fields` parameter and change `references` to
point to the User primary key).
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playground/orm.schema.dartpub/orm/lib/src/schema/relation.dart
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T16:59:51.067Z
Learnt from: medz
Repo: medz/dart-orm PR: 484
File: pub/orm/lib/src/analyzer/utils/config_utils.dart:74-89
Timestamp: 2026-01-04T16:59:51.067Z
Learning: In Dart 3.10+, use dot shorthand (e.g., .sqlite) when the type can be inferred from context, such as named parameters with declared types. This shortens code but only where the receiver type is unambiguous. Ensure the project uses a Dart language version that supports this syntax and prefer clarity when the shorthand could be ambiguous.
Applied to files:
pub/orm/lib/src/schema/relation.dartplayground/orm.schema.dart
🔇 Additional comments (2)
pub/orm/lib/src/schema/relation.dart (2)
32-35: LGTM! Type change toSet<String>enforces uniqueness appropriately.Using
Set<String>instead ofIterable<String>is a good design choice—it semantically enforces that field names in a relation must be unique, preventing accidental duplicates. This is a breaking change but appropriate for a v6 major release.
47-54: Constructor signature change is correctly implemented.Making
fieldsrequired while keepingreferencesoptional is a reasonable API design—relations must specify their local fields, while the referenced fields can potentially be inferred or defaulted by the ORM.All existing usages of
Relationhave been properly updated to passfieldsas a required named parameter. The single call site found (playground/orm.schema.dart:13) correctly includesfields: {'authorId'}.
| String content, | ||
| String authorId, | ||
|
|
||
| @Relation(fields: {'authorId'}) User author, |
There was a problem hiding this comment.
Still missing the references parameter for complete relation mapping.
The fields parameter is now correctly specified, but the relation annotation is incomplete. According to ORM relation patterns, you need both:
fields: The foreign key field in the current model (Post) →{'authorId'}✓references: The primary key field in the referenced model (User) →{'id'}✗
Without the references parameter, the ORM cannot definitively establish which field in User the authorId foreign key points to.
🔎 Complete the relation mapping
- @Relation(fields: {'authorId'}) User author,
+ @Relation(fields: {'authorId'}, references: {'id'}) User author,This explicitly maps Post.authorId → User.id.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Relation(fields: {'authorId'}) User author, | |
| import 'package:orm/schema.dart'; | |
| typedef User = ( | |
| @id String id, | |
| String email, | |
| ); | |
| typedef Post = ( | |
| @id String id, | |
| String title, | |
| String content, | |
| String authorId, | |
| @Relation(fields: {'authorId'}, references: {'id'}) User author, | |
| ); |
🤖 Prompt for AI Agents
In playground/orm.schema.dart around line 13, the @Relation annotation declares
fields: {'authorId'} but omits the references parameter; add references: {'id'}
to explicitly map Post.authorId to User.id so the ORM knows which User primary
key the foreign key points to, ensuring the relation is fully specified.
1 participant
Summary by CodeRabbit
New Features
Refactor
Breaking Changes
Tests
Documentation / Chores
✏️ Tip: You can customize this high-level summary in your review settings.