Оптимизация хранения данных #537
Conversation
WalkthroughSystematic refactor: custom Lazy wrappers replaced by Lombok @Getter(lazy = true) fields initialized via LazyLoader; several Lombok value classes converted to Java records (with accessor/signature changes); read-settings accessors renamed from isSkip*() to skip*(); some small API/type removals and helper improvements (LazyLoader.isUnmodifiableList). Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client code
participant Getter as Lombok-generated getXxx()
participant LazyLoader as LazyLoader.computeXxx()
note right of LazyLoader #f0f4ff: centralized computation
Caller->>Getter: call getChildren()
Getter->>LazyLoader: call computeChildren(this) on first access
LazyLoader->>LazyLoader: compute and return List<MD>
LazyLoader-->>Getter: return computed list
Getter-->>Caller: return cached list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Reason: wide, heterogeneous changes across many packages — many repetitive Lazy→Lombok refactors (lower per-file effort) plus several signature-breaking record conversions, removed types, and cross-cutting read-settings renames that require careful API/consumer review and test validation. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/mdo/ExternalDataSource.java (1)
44-47: Same builder/lazy interaction risk as above — exclude lazy fields from the class-level builderchildren/plainChildren are lazy-initialized and reference
this, which cannot be expressed via @Builder.Default. Use constructor-level @builder (builderMethodName="builder") that does not include these fields, or another exclusion strategy; otherwise, risk NPEs in generated getters. (projectlombok.org)Also applies to: 70-74
src/main/java/com/github/_1c_syntax/bsl/mdo/AccountingRegister.java (1)
47-50: Builder must not own lazy-computed fields (allModules/allAttributes/children)All three are @Getter(lazy = true) and rely on initializer logic; with class-level @builder they risk being constructed with null internal refs. Move @builder to a ctor excluding these fields (keep builder() name), or equivalent. (projectlombok.org)
Also applies to: 73-75, 85-87, 93-95
🧹 Nitpick comments (15)
src/main/java/com/github/_1c_syntax/bsl/reader/edt/EDTReader.java (1)
125-130: API alignment to MDCReadSettings.record looks goodRenamed check to readSettings.skipSupport() is consistent with the record accessor. No behavior change.
Consider a small guard (Objects.requireNonNull(readSettings)) in the ctor and extracting the support-data load into a shared helper used by both readers to reduce duplication.
src/main/java/com/github/_1c_syntax/bsl/reader/designer/DesignerReader.java (1)
121-126: API alignment OKSwitched to readSettings.skipSupport(); behavior unchanged.
Same as EDTReader: consider a shared helper for ParentConfigurations.bin handling and a null-guard for readSettings.
src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/AllStringConverter.java (1)
39-48: Replace recursion with an iterative descent; also return String from the helper.Avoids stack growth on deeply nested inputs and makes state restoration explicit. Behavior unchanged for a single-child chain.
Please also confirm the XML shape is always a single-child chain; if siblings can occur, we may need to scan descendants instead of choosing the first child.
- private static Object read(HierarchicalStreamReader reader) { - if (!reader.hasMoreChildren()) { - return reader.getValue(); - } else { - reader.moveDown(); - var result = read(reader); - reader.moveUp(); - return result; - } - } + private static String read(HierarchicalStreamReader reader) { + int depth = 0; + // descend to the left‑most leaf + while (reader.hasMoreChildren()) { + reader.moveDown(); + depth++; + } + var value = reader.getValue(); + // restore the cursor to the original node + for (int i = 0; i < depth; i++) { + reader.moveUp(); + } + return value; + }src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormHandler.java (1)
31-31: Enforce non-null for record components to match compareTo assumptions.compareTo uses String.compareTo; nulls would NPE. Make non-null explicit:
+import lombok.NonNull; @@ -public record FormHandler(String event, String name) implements Comparable<FormHandler> { +public record FormHandler(@NonNull String event, @NonNull String name) implements Comparable<FormHandler> { @@ - public FormHandler(String event, String name) { + public FormHandler(String event, String name) { this.event = stringInterner.intern(event); this.name = stringInterner.intern(name); }Also applies to: 36-39
src/main/java/com/github/_1c_syntax/bsl/mdo/HTTPService.java (1)
66-68: Ensure plainChildren is immutable.If LazyLoader.computePlainChildren(this) returns a modifiable list, @value’s immutability is compromised. Wrap at source.
Apply:
- List<MD> plainChildren = LazyLoader.computePlainChildren(this); + List<MD> plainChildren = java.util.List.copyOf(LazyLoader.computePlainChildren(this));src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/SimpleFormItem.java (1)
53-55: Make plainItems unmodifiable.Mirror @value semantics by copying to an unmodifiable list unless LazyLoader already does it.
- List<FormItem> plainItems = LazyLoader.computePlainFormItems(this); + List<FormItem> plainItems = java.util.List.copyOf(LazyLoader.computePlainFormItems(this));src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceTable.java (1)
82-84: Children/allModules should be immutable.Guard against accidental mutation if LazyLoader returns mutable lists.
- List<MD> children = LazyLoader.computeChildren(this); + List<MD> children = java.util.List.copyOf(LazyLoader.computeChildren(this)); - List<Module> allModules = LazyLoader.computeAllModules(this); + List<Module> allModules = java.util.List.copyOf(LazyLoader.computeAllModules(this));Also applies to: 91-93
src/main/java/com/github/_1c_syntax/bsl/mdo/WebService.java (1)
67-69: Immutable plainChildren.Same rationale as other classes: copy to unmodifiable unless LazyLoader guarantees it.
- List<MD> plainChildren = LazyLoader.computePlainChildren(this); + List<MD> plainChildren = java.util.List.copyOf(LazyLoader.computePlainChildren(this));src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceCubeDimensionTable.java (1)
82-84: Return unmodifiable lists for lazy getters.Align with @value expectations and avoid external mutation.
- List<MD> children = LazyLoader.computeChildren(this); + List<MD> children = java.util.List.copyOf(LazyLoader.computeChildren(this)); - List<Module> allModules = LazyLoader.computeAllModules(this); + List<Module> allModules = java.util.List.copyOf(LazyLoader.computeAllModules(this));Also applies to: 91-93
src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceCube.java (1)
82-86: Harden lazy lists against mutation.Copy results from LazyLoader to unmodifiable lists to preserve immutability.
- List<MD> children = LazyLoader.computeChildren(this); + List<MD> children = java.util.List.copyOf(LazyLoader.computeChildren(this)); - List<MD> plainChildren = LazyLoader.computePlainChildren(this); + List<MD> plainChildren = java.util.List.copyOf(LazyLoader.computePlainChildren(this)); - List<Module> allModules = LazyLoader.computeAllModules(this); + List<Module> allModules = java.util.List.copyOf(LazyLoader.computeAllModules(this)); - List<Attribute> allAttributes = LazyLoader.computeAllAttributes(this); + List<Attribute> allAttributes = java.util.List.copyOf(LazyLoader.computeAllAttributes(this));Also confirm LazyLoader.computeAllAttributes(this) aggregates dimensions, resources, and dimensionTables as intended.
Also applies to: 93-95, 124-126
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/DataCompositionSchema.java (3)
50-52: Return an unmodifiable plainDataSets list.Prevent external mutation of the flattened view.
@Getter(lazy = true) - List<DataSet> plainDataSets = computePlainDataSets(); + List<DataSet> plainDataSets = java.util.List.copyOf(computePlainDataSets()); @@ - return result; + return java.util.List.copyOf(result);Also applies to: 69-74
59-62: Defensive copy of constructor input.dataSets should be copied to avoid external mutation after construction.
- dataSets = dataSetsTree; + dataSets = java.util.List.copyOf(dataSetsTree);
104-105: dataPath now String: validate and document allowed format.Switching to String removes type safety. Add constraints (e.g., regex) or validation in the record constructor, and test edge cases.
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/XdtoPackageData.java (1)
41-54: Nice record conversion; consider List.of() for EMPTY for symmetryRecord + @Builder/@Singular looks good; nested records read cleanly. For EMPTY, List.of() is equivalent and concise, but optional. Approving as-is.
src/main/java/com/github/_1c_syntax/bsl/mdo/Document.java (1)
73-75: Computed lazy fields exposed via builder require design fix: move @builder to custom constructor excluding them.Document class uses
@Builderat class level, which generates setter methods for all fields, including the computed lazy fields (allModules,storageFields,plainStorageFields,children,plainChildren). This allows callers to bypass the lazy computation contract:Document.builder().allModules(customList).build() // circumvents LazyLoader.computeAllModulesWhile all LazyLoader methods return
Collections.unmodifiableList()orCollections.unmodifiableMap(), ensuring the returned collections are immutable, the invariant that computed fields should only be set through lazy computation is violated at the builder level.Refactor by moving
@Builderto a custom constructor that excludes these computed fields, or document that builder input for these fields is unsupported. (Also applies to 85-89, 96-99 and similar lazy field definitions in other classes likeCalculationRegister,Task, etc.)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (42)
src/test/resources/fixtures/mdclasses/Catalogs.Справочник1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Catalogs.Справочник1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/DataProcessors.Обработка1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/DataProcessors.Обработка1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Documents.Документ1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Documents.Документ1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExchangePlans.ПланОбмена1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExchangePlans.ПланОбмена1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExternalDataSources.ТекущаяСУБД.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/ExternalDataSources.ТекущаяСУБД_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Languages.Албанский.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Languages.Японский.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Reports.Отчет1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses/Reports.Отчет1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_27/ExternalDataSources.ВнешнийИсточникДанных1.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/mdclasses_3_27/ExternalDataSources.ВнешнийИсточникДанных1_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/BusinessProcesses.Задание.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/BusinessProcesses.Задание_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.ВерсииФайлов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.Заметки.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Catalogs.Заметки_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ChartsOfCharacteristicTypes.ДополнительныеРеквизитыИСведения_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/CommonForms.Вопрос.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/CommonForms.Вопрос_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DataProcessors.ЗагрузкаКурсовВалют.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DataProcessors.ЗагрузкаКурсовВалют_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DocumentJournals.Взаимодействия.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/DocumentJournals.Взаимодействия_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Documents.Анкета.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Documents.Анкета_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/ExchangePlans.ОбновлениеИнформационнойБазы_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/FilterCriteria.ФайлыВТоме.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/FilterCriteria.ФайлыВТоме_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Reports.АнализВерсийОбъектов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Reports.АнализВерсийОбъектов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/SettingsStorages.ХранилищеВариантовОтчетов.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/SettingsStorages.ХранилищеВариантовОтчетов_edt.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Tasks.ЗадачаИсполнителя.jsonis excluded by!src/test/resources/**src/test/resources/fixtures/ssl_3_1/Tasks.ЗадачаИсполнителя_edt.jsonis excluded by!src/test/resources/**
📒 Files selected for processing (66)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/Configuration.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/ConfigurationExtension.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalDataProcessor.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalReport.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalSource.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCReadSettings.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdclasses/helpers/Rights.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/AccountingRegister.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/AccumulationRegister.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/BusinessProcess.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/CalculationRegister.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Catalog.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfAccounts.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfCalculationTypes.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfCharacteristicTypes.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/CommonModule.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/DataProcessor.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Document.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/DocumentJournal.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Enum.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/ExchangePlan.java(6 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/ExternalDataSource.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/FilterCriterion.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/HTTPService.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/InformationRegister.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Report.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/SettingsStorage.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Subsystem.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/Task.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/WebService.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceCube.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceCubeDimensionTable.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/children/ExternalDataSourceTable.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/DataCompositionSchema.java(3 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/ManagedFormData.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/QuerySource.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/RoleData.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/XdtoPackageData.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormAttribute.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormDataPath.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormHandler.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormItem.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/SimpleFormItem.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/support/SourcePosition.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/utils/LazyLoader.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/TransformationUtils.java(4 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDCReaderContext.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/context/std_attributes/StdAttributeFiller.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/AllStringConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/DataCompositionSchemaConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/DataSetConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/ExchangePlanAutoRecordConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/QuerySourceConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/RoleDataConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/XdtoPackageDataConverter.java(5 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/designer/DesignerReader.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/FormDataPathConverter.java(0 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/FormElementConverter.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/edt/EDTReader.java(1 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/FormItemConverter.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/mdo/DocumentTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/mdo/ExchangePlanTest.java(2 hunks)src/test/java/com/github/_1c_syntax/bsl/mdo/ReportTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/mdo/RoleTest.java(1 hunks)src/test/java/com/github/_1c_syntax/bsl/mdo/XDTOPackageTest.java(1 hunks)
💤 Files with no reviewable changes (5)
- src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormDataPath.java
- src/main/java/com/github/_1c_syntax/bsl/mdo/support/SourcePosition.java
- src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormAttribute.java
- src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/FormDataPathConverter.java
- src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalSource.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/java/com/github/_1c_syntax/bsl/mdo/ExchangePlanTest.java (1)
src/test/java/com/github/_1c_syntax/bsl/test_utils/assertions/Assertions.java (1)
Assertions(24-29)
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/ManagedFormData.java (1)
src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/ProtectedModuleInfo.java (1)
Getter(36-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: QA
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, ubuntu-latest)
🔇 Additional comments (55)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/FormElementConverter.java (1)
41-41: LGTM! Method rename aligns with the broader refactoring.The change from
isSkipFormElementItems()toskipFormElementItems()is consistent with the repository-wide standardization of read-settings accessor methods mentioned in the PR objectives. The logic remains unchanged, and since the tests have passed (as indicated in the checklist), this confirms that all call sites have been properly updated.src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/DataSetConverter.java (1)
49-53: LGTM! Defensive initialization ensures safe defaults.The explicit default initialization for optional fields (
name,dataSource,querySource) is good defensive programming that prevents null values when these XML nodes are absent. The pattern is consistent with the existing empty-string initialization inreadField(lines 74-75), and the builder pattern correctly allows these defaults to be overwritten by actual XML values during parsing.src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/RoleDataConverter.java (1)
42-42: Method rename is correct.The change from
isSkipRoleData()toskipRoleData()is verified as correct. MDCReadSettings is a Java record with askipRoleDataboolean field, and Java records automatically generate accessor methods that match the field name exactly. This is the standard pattern for record accessors (without the "is" prefix).src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDCReaderContext.java (1)
79-79: Method rename successfully applied across the codebase.Verification confirms the refactoring from
isSkipSupport()toskipSupport()is complete with no stale references. The method is consistently used across 11 files (readers, settings builders, tests, benchmarks), including the change at line 79 in MDCReaderContext.java. All callers have been updated.src/main/java/com/github/_1c_syntax/bsl/mdclasses/helpers/Rights.java (1)
138-151: LGTM! Record accessor migration applied correctly.The accessor updates from getter-style methods to record component accessors are consistent and maintain the original logic correctly.
src/test/java/com/github/_1c_syntax/bsl/mdo/RoleTest.java (1)
50-70: LGTM! Test assertions updated correctly.The test assertions have been properly updated to use the new record-style accessors while maintaining equivalent test coverage and logic.
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/RoleData.java (3)
27-44: LGTM! Clean migration to Java record.The RoleData migration to a record with Lombok
@Builderis well-executed. The javadoc parameters align with the record components, and the use of@NonNulland@Singularensures proper builder functionality and null-safety.
47-53: LGTM! Nested record properly structured.The ObjectRight record follows the same clean pattern with appropriate annotations and documentation.
55-61: LGTM! Final nested record completes the migration.The Right record is properly defined and completes the consistent migration of all nested structures to records.
src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java (1)
96-101: Good: updated to skipSupport()Accessor rename is correct; logic unchanged.
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/FormItemConverter.java (1)
40-42: Accessor rename correctUses skipFormElementItems() from the record; logic intact.
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCReadSettings.java (1)
35-47: Record + builder migration verified and approvedThe record-based MDCReadSettings with @builder and presets (DEFAULT/SKIP_SUPPORT) is clean and immutable. Toolchain is properly configured: Java 17 (build.gradle.kts:89-90) supports records, Lombok 9.0.0 supports @builder on records, and no stale
isSkip*accessors remain in the codebase. Accessors (skipXxx) correctly match updated call sites.src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/XdtoPackageDataConverter.java (3)
59-60: Good addition of @nonnull contract.The
@NonNullannotation appropriately strengthens the null-safety contract for this public method.
87-87: LGTM! Consistent @nonnull annotations on helper methods.The
@NonNullannotations onreadObjectType,readValueType,readProperty, andgetAttributeappropriately document and enforce non-null contracts. The implementations guarantee non-null returns through builder patterns and explicit null-checks.Also applies to: 102-102, 120-120, 172-173
62-62: ****The method rename is correct and intentional.
MDCReadSettingsis a Java record with all boolean fields following a consistentskip*naming pattern—not theis*prefix. Java records automatically generate accessor methods matching field names, soskipXdtoPackage()is the proper accessor for theskipXdtoPackagefield. This naming is semantically valid (a boolean query: "should we skip XdtoPackage?") and consistent with sibling fields likeskipSupport,skipRoleData,skipFormElementItems, andskipDataCompositionSchema. No orphaned references to the old name exist. This is not a problem.Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/AllStringConverter.java (3)
31-33: Verify converter discovery.Confirm that @CommonConverter has RUNTIME retention and is picked up by your XStream registration path in common.converter. If it’s in another package, add an explicit import.
34-37: LGTM on delegation.unmarshal cleanly delegates and preserves reader position via read().
50-53: canConvert scope is correct.Restricting to String.class is appropriate for this converter.
src/main/java/com/github/_1c_syntax/bsl/mdo/Subsystem.java (1)
69-71: Lombok lazy getter alignment looks good. Please verify builder/toBuilder paths.Using @Getter(lazy = true) with initializer is consistent. Double-check that no builder/toBuilder code sets plainChildren to a non-null placeholder (which would skip lazy compute).
src/main/java/com/github/_1c_syntax/bsl/reader/common/TransformationUtils.java (1)
171-187: Non-static computeFieldType under @UtilityClass is fine.@UtilityClass makes members static; no action needed.
src/main/java/com/github/_1c_syntax/bsl/mdo/FilterCriterion.java (1)
70-72: Lazy getters migration looks correct.Consistent with LazyLoader pattern; no issues spotted.
Please confirm toBuilder() usage continues to rely on lazy compute (i.e., not setting these fields explicitly).
Also applies to: 79-81
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/FormItem.java (1)
55-55: Breaking change correctly implemented—all implementors and callers updated.The interface change to
String getDataPath()is properly satisfied: SimpleFormItem's Lombok@Valueannotation auto-generatesString getDataPath()matching the field type. DocumentTest already uses the String API correctly. Only one in-tree implementor (SimpleFormItem) exists and is compatible.Consider documenting this breaking change in CHANGELOG with the return type and path format details to aid downstream consumers.
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/form/SimpleFormItem.java (1)
49-49: dataPath type change: confirm contract and empty-string semantics.FormDataPath → String is a breaking API change. Document/validate allowable formats and whether "" means “no path” to avoid ambiguous states. Add tests around parsing/validation.
src/test/java/com/github/_1c_syntax/bsl/mdo/DocumentTest.java (1)
78-78: LGTM! Simplified path checking.The assertion now checks the data path prefix directly, eliminating intermediate accessor layers. This aligns with the broader refactor toward flatter, more direct accessors.
src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/ExchangePlanAutoRecordConverter.java (1)
82-82: LGTM! Simplified to boolean flag.The conversion from
AutoRecordTypeenum to a boolean flag simplifies the API while preserving the semantics (ALLOW maps to true, all others to false).src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/DataCompositionSchemaConverter.java (1)
49-49: LGTM! Updated to record-style accessor.The method call now uses the record-style accessor
skipDataCompositionSchema()instead of the bean-styleisSkipDataCompositionSchema(), consistent with the broader refactor to Lombok-generated accessors.src/test/java/com/github/_1c_syntax/bsl/mdo/ExchangePlanTest.java (2)
31-31: LGTM! Updated import to use custom Assertions class.The import now uses the project's custom
Assertionsclass which extends AssertJ and provides additional JSON assertion capabilities.
80-82: LGTM! Enhanced test coverage.The new assertions validate additional ExchangePlan properties (
isDistributedInfoBase,isIncludeConfigurationExtensions, and content size), improving test coverage for the SSL 3.1 exchange plan.src/main/java/com/github/_1c_syntax/bsl/mdo/storage/QuerySource.java (1)
29-38: LGTM! Simplified record structure.The record now directly exposes
lineandcolumnfields instead of wrapping them in aSourcePositionobject. This flattens the data structure and makes it more straightforward to use. The@NonNullannotation ontextQueryensures null safety, and the staticEMPTYconstant provides a cleaner initialization pattern.src/main/java/com/github/_1c_syntax/bsl/mdo/DocumentJournal.java (2)
37-37: LGTM! Added Lombok Getter import.Added to support the new
@Getter(lazy = true)annotations on lazy-initialized fields.
73-83: LGTM! Modernized lazy initialization with Lombok.The refactor replaces explicit getter methods and private compute helpers with Lombok's
@Getter(lazy = true)backed byLazyLoader. This reduces boilerplate while maintaining thread-safe lazy initialization semantics. The public API remains unchanged—getChildren()andgetAllModules()are now Lombok-generated.src/main/java/com/github/_1c_syntax/bsl/reader/common/converter/QuerySourceConverter.java (1)
39-39: LGTM! Updated to match new QuerySource constructor.The converter now constructs
QuerySourcedirectly with line and column values instead of creating an intermediateSourcePositionobject. This aligns with the simplifiedQuerySourcerecord structure and makes the code more concise.src/test/java/com/github/_1c_syntax/bsl/mdo/XDTOPackageTest.java (1)
56-92: LGTM! Updated to record-style accessors.The test assertions now use record-style accessor methods (e.g.,
name(),type(),properties()) instead of bean-style getters (e.g.,getName(),getType(),getProperties()). This aligns with the conversion ofXdtoPackageDataand its nested types to Java records, providing a more modern and concise API.src/main/java/com/github/_1c_syntax/bsl/mdo/Enum.java (1)
36-36: LGTM! Clean refactoring to Lombok-based lazy initialization.The migration from explicit
Lazy<T>wrappers to Lombok's@Getter(lazy = true)with centralizedLazyLoaderinitialization reduces boilerplate and standardizes lazy loading across the codebase. Lombok's lazy getter implementation uses double-checked locking, ensuring thread safety.Also applies to: 69-70, 78-79
src/main/java/com/github/_1c_syntax/bsl/mdo/CommonModule.java (1)
33-33: LGTM! Elegant self-referencing module list.The lazy initialization using
List.of(this)is appropriate sinceCommonModuleimplementsModule. This provides an immutable, lazily-computed singleton list containing the module itself, which is cleaner than the previousLazywrapper approach.Also applies to: 76-77
src/main/java/com/github/_1c_syntax/bsl/mdclasses/Configuration.java (1)
93-93: LGTM! Comprehensive lazy initialization refactoring.The systematic conversion of seven lazy-loaded collections (
allModules,plainChildren,modulesByType,modulesByURI,modulesByObject,commonModulesByName,childrenByMdoRef) to Lombok-based lazy getters significantly reduces boilerplate in this root configuration class while centralizing initialization logic inLazyLoader.Also applies to: 153-154, 263-275
src/main/java/com/github/_1c_syntax/bsl/mdo/SettingsStorage.java (1)
34-34: LGTM! Consistent lazy initialization pattern.The refactoring of
childrenandallModulesfields follows the same clean pattern applied across the codebase, removing boilerplate while maintaining functionality.Also applies to: 67-68, 76-77
src/test/java/com/github/_1c_syntax/bsl/mdo/ReportTest.java (1)
78-82: LGTM! Tests updated for fluent accessor API.The test assertions now use fluent accessors (
name(),type(),items(),querySource()) instead of bean-style getters. This indicates thatDataSetandQuerySourcehave been refactored (likely to records), aligning with the PR's broader API modernization.Also applies to: 87-90
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/ManagedFormData.java (1)
31-31: LGTM! Clean lazy initialization refactoring.The
plainItemsfield conversion to Lombok's lazy getter withLazyLoader.computePlainFormItems(this)follows the established pattern and reduces boilerplate.Also applies to: 51-52
src/main/java/com/github/_1c_syntax/bsl/mdo/Report.java (1)
36-36: LGTM! Systematic refactoring of five lazy collections.The conversion of
children,plainChildren,allModules,storageFields, andplainStorageFieldsto Lombok lazy getters withLazyLoaderinitialization is clean and consistent. The explicitgetAllAttributes()override appropriately remains unchanged since it's a simple delegation without lazy computation.Also applies to: 72-75, 83-84, 107-110
src/main/java/com/github/_1c_syntax/bsl/mdo/Task.java (1)
37-37: LGTM! Consistent lazy initialization across five collections.The refactoring of
allModules,storageFields,plainStorageFields,children, andplainChildrenfollows the same clean pattern applied throughout the PR, successfully centralizing lazy initialization logic.Also applies to: 74-75, 86-89, 97-100
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/XdtoPackageData.java (1)
61-66: Builder on nested records — LGTM@nonnull and @Singular on components generate the expected null checks and unmodifiable lists. No further action.
Also applies to: 74-75, 87-94
src/main/java/com/github/_1c_syntax/bsl/mdo/DataProcessor.java (1)
45-46: Looking at the verification results, I need to examine whether there's actual evidence of the claimed issue in the codebase or if the review comment is based on a misunderstanding of how Lombok's@Getter(lazy=true)interacts with@Builder.Based on my comprehensive verification of the code and Lombok's documented behavior, the original review comment contains a fundamental misunderstanding of how
@Getter(lazy=true)works with@Builder.When @value is used, a constructor is generated that covers every argument except final fields that are initialized in the field declaration. Critically, @Getter(lazy=true) generates a private final AtomicReference backing field with initialization:
private final AtomicReference<Object> students = new AtomicReference<Object>();.This AtomicReference initialization is not a field initializer that
@Builderwould skip—it's a direct field declaration with its own initializer. The initializer expression (likeLazyLoader.computeChildren(this)) is captured and becomes part of the generated lazy getter logic, not a traditional field initializer. Since the backing AtomicReference is always initialized independently at field declaration time,@Builderdoes not cause it to be null.After verifying 29+ classes across the codebase, I found:
- No evidence of NPEs related to this pattern
- All classes consistently use the same pattern with no workarounds
- No GitHub issues documenting
@Builder+@Getter(lazy=true)conflicts- All web search results show this pattern working correctly
Likely an incorrect or invalid review comment.
src/main/java/com/github/_1c_syntax/bsl/mdo/CalculationRegister.java (1)
76-78: Guard lazy-computed fields against builder overrides; verify aggregation semantics are preserved.
- The builder exposes setters for
allModules,allAttributes, andchildren—these are lazy-computed fields meant to be derived, but builder calls like.clearChildren().children(...)(seen in MDMerger.java:131) can override them. This circumvents the lazy computation. Consider either excluding these fields from@Buildervia@Builder.Defaultannotation or documenting that they should not be set manually.- Confirmed:
LazyLoader.computeAllAttributes()aggregates attributes, resources, and dimensions for Register types (including CalculationRegister), which appears semantically correct.Also applies to: 88-90, 96-97
src/main/java/com/github/_1c_syntax/bsl/mdo/Catalog.java (1)
73-75: Confirm lazy fields are excluded from builder or document clear constraints.LazyLoader.computeAllModules() and other compute* methods return
Collections.unmodifiableList(), which correctly enforces immutability. However, the@Builderannotation on Catalog does not exclude lazy-computed fields (allModules,storageFields,plainStorageFields,children,plainChildren), allowing them to be set via builder methods and potentially bypassing lazy initialization logic. Either exclude these fields from the builder (using exclusion annotations or refactoring), or add clear documentation that they must not be manually set.This applies to all classes using the same pattern: Document, Task, ChartOfCalculationTypes, Report, and others.
src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfCharacteristicTypes.java (1)
73-75: Now I need to directly inspect the ChartOfCharacteristicTypes.java file to verify the actual implementation:Based on the verification results and web research, I can now provide the rewritten review comment. The script output confirms that:
LazyLoader methods correctly return
Collections.unmodifiableList()- all compute* methods (lines 116, 132, 151, 168, 216) properly wrap returns withCollections.unmodifiableList().Builder exposure remains a valid concern - Lombok's @builder annotation does not have an exclude attribute by default and exposes all fields unless explicitly excluded using mechanisms like
@Builder.Defaultor custom constructors.Lazy fields are present - The script confirmed lazy fields exist in ChartOfCharacteristicTypes.java (lines 73-75, 85-89, 96-99).
From the Lombok documentation research, I confirmed that:
- Since Lombok v1.16.16, the @Builder.Default annotation can set a default value for fields to exclude from the builder
- @builder can be put on constructors as well as classes, and only offers setting those attributes that come up in the signature
- Although the @builder annotation in Lombok does not have an 'exclude' attribute for its Builder pattern, there are effective workarounds to control field inclusion
Verify builder field exposure and document immutability guarantees for lazy-initialized collections.
The lazy fields (
allModules,storageFields,plainStorageFields,children,plainChildren) are confirmed to returnCollections.unmodifiableList()via LazyLoader, satisfying immutability on retrieval. However, ensure these fields are explicitly excluded from@Builder(via@Builder.Default, custom constructor with@Builder, or@Getter(AccessLevel.NONE)) to prevent circumventing immutability during object construction.Also applies to: 85-89, 96-99
src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalReport.java (1)
38-38: Consistent lazy migration; validate LazyLoader behavior and immutability.
- Change is coherent with the refactor; fine to merge.
- Please confirm computeAllModules/compute(Storage|PlainStorage)Fields/compute(Plain)?Children don’t call back into getters and never return null; prefer empty, unmodifiable collections.
Use the same script shared on BusinessProcess to validate LazyLoader.
Also applies to: 73-75, 88-92, 99-103
src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfAccounts.java (1)
38-38: Lazy getters OK; confirm allAttributes semantics and collection guarantees.
- Looks correct; approve.
- Please verify LazyLoader.computeAllAttributes(this):
- aggregates base + derived attributes as before,
- does not call getAllAttributes() internally,
- never returns null and is unmodifiable.
Reuse the earlier LazyLoader verification script to check recursion and immutability.
Also applies to: 73-75, 81-83, 87-91, 98-101
src/main/java/com/github/_1c_syntax/bsl/mdclasses/ExternalDataProcessor.java (1)
38-38: LGTM; align LazyLoader guarantees with other classes.
- Change matches the project-wide pattern; approve.
- As with others, confirm no recursion, no nulls, and unmodifiable results from LazyLoader.
Also applies to: 68-70, 83-87, 94-97
src/main/java/com/github/_1c_syntax/bsl/mdclasses/ConfigurationExtension.java (1)
92-92: Code implementation is correct; all review concerns are not present.Verification confirms:
- All five compute methods properly return unmodifiable maps using
Collectors.toUnmodifiableMap()orCollections.unmodifiableMap().- No recursion risk: methods receive parameters (mdo, childrenOwner, cf) and call getters on those parameters, not on ConfigurationExtension lazy getters.
- No null returns: stream collectors handle empty collections gracefully.
- No changes needed.
src/main/java/com/github/_1c_syntax/bsl/mdo/BusinessProcess.java (1)
36-36: No recursion or immutability issues detected; code changes are safe to approve.Verification confirms:
- No recursion risk: The lazy initialization chain terminates safely at regular (non-lazy) field access.
computeChildren()calls only@Singularbuilder fields;computePlainChildren()delegates to child objects;computeAllModules()and downstream methods do not cycle back to themselves.- Immutability enforced: All
LazyLoadercompute methods return results wrapped inCollections.unmodifiableList()orCollectors.toUnmodifiableMap().- Null-safety guaranteed: No
nullreturns detected; all methods consistently return unmodifiable empty collections or populated results.- API parity maintained: Lombok's
@Getter(lazy = true)provides thread-safe lazy initialization with identical public method signatures.The optional refactor suggestion (explicit
Collections.unmodifiableList()wrapping at call site) is redundant;LazyLoaderalready ensures immutability at the source.src/main/java/com/github/_1c_syntax/bsl/mdo/ExchangePlan.java (4)
37-37: LGTM!The
lombok.Getterimport is correctly added to support the@Getter(lazy = true)annotations used throughout this refactoring.
149-150: Logic correctly adapted for the newRecordContentstructure.The
autoRecordmethod now computes theAutoRecordTypefrom theboolean allowfield instead of reading it directly. The mapping is correct:
allow = true→AutoRecordType.ALLOWallow = falseor not found →AutoRecordType.DENYThis maintains backward compatibility at call sites while adapting to the refactored data structure.
187-199: Original review comment is inaccurate; no breaking changes exist.The changes to
RecordContentare an internal refactoring that properly maintains backward compatibility:
toBuilder = trueremoval: No usage ofRecordContent.toBuilder()exists in the codebase. The onlytoBuilder()call found is onCommonConfiguration(unrelated).Field change (
autoRecord→allow): Already properly updated throughout:
ExchangePlanAutoRecordConverter.java:82correctly callsbuilder.allow(...)with the boolean conversion- All construction sites use the new
.allow()methodAPI-level compatibility maintained:
ExchangePlan.autoRecord(MdoReference)method (lines 145–151) properly adapts the internal boolean back toAutoRecordTypeenum, preserving the public API contract.Tests pass without changes and no compilation errors would occur.
Likely an incorrect or invalid review comment.
74-75: Thread-safety guarantees are properly maintained in the migration.The migration from custom
Lazy<T>to@Getter(lazy=true)is correct. Lombok's lazy getter implementation uses synchronized double-checked locking, and LazyLoader methods are pure, stateless utility functions that compute and return unmodifiable collections. Thethisreference in field initializers is valid (evaluated after construction), and no code depends on the oldLazy<T>wrapper API. The pattern is consistently applied across 30+ files in the codebase.
src/main/java/com/github/_1c_syntax/bsl/mdclasses/helpers/Rights.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/mdo/ChartOfCalculationTypes.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/mdo/storage/DataCompositionSchema.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java
Outdated
Show resolved
Hide resolved
.../java/com/github/_1c_syntax/bsl/reader/common/context/std_attributes/StdAttributeFiller.java
Outdated
Show resolved
Hide resolved
| @Nullable | ||
| public Object builder(@NonNull Class<?> clazz) { | ||
| var method = getMethod(clazz, BUILDER_METHOD_NAME); | ||
| if (method != null) { | ||
| try { | ||
| return method.invoke(clazz); | ||
| } catch (IllegalAccessException | InvocationTargetException e) { | ||
| LOGGER.error(LOGGER_MESSAGE_PREF, clazz, BUILDER_METHOD_NAME, e); | ||
| } | ||
| } | ||
| return null; | ||
| return methodInvoke(clazz, BUILDER_METHOD_NAME); | ||
| } |
There was a problem hiding this comment.
Reflection bug: wrong receiver passed to reflective calls (breaks toBuilder, brittle for builder).
- toBuilder currently calls methodInvoke(object.getClass(), ...), invoking on Class instead of the instance.
- builder should invoke static method with null receiver, not the Class object.
Apply this fix:
@@
- public Object builder(@NonNull Class<?> clazz) {
- return methodInvoke(clazz, BUILDER_METHOD_NAME);
- }
+ public Object builder(@NonNull Class<?> clazz) {
+ return methodInvoke(clazz, BUILDER_METHOD_NAME, null);
+ }
@@
- public Object toBuilder(@NonNull Object object) {
- return methodInvoke(object.getClass(), TO_BUILDER_METHOD_NAME);
- }
+ public Object toBuilder(@NonNull Object object) {
+ return methodInvoke(object.getClass(), TO_BUILDER_METHOD_NAME, object);
+ }
@@
- private Object methodInvoke(@NonNull Class<?> clazz, String methodName) {
- var method = getMethod(clazz, methodName);
+ private Object methodInvoke(@NonNull Class<?> clazz, String methodName, @Nullable Object receiver) {
+ var method = getMethod(clazz, methodName);
if (method != null) {
try {
- return method.invoke(clazz);
+ return method.invoke(receiver);
} catch (IllegalAccessException | InvocationTargetException e) {
LOGGER.error(LOGGER_MESSAGE_PREF, clazz, methodName, e);
}
}
return null;
}Also applies to: 125-127, 188-199
🤖 Prompt for AI Agents
In
src/main/java/com/github/_1c_syntax/bsl/reader/common/TransformationUtils.java
around lines 113-116 (and similarly at 125-127 and 188-199), reflective calls
are using the wrong receiver: instance methods are being invoked on Class
objects and static methods are being invoked on Class instances. Fix by passing
the correct receiver to methodInvoke/field/method reflective helpers: for
instance method calls use the actual object instance (not object.getClass()),
and for static calls pass null as the receiver (not the Class object); update
the three affected call sites accordingly so toBuilder uses the instance as
receiver and builder uses null for static invocation.
This reverts commit 14c4d15.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java (2)
171-183: Parallel read over synchronizedList can race; snapshot before streaming.Concurrent writes to value during parallelStream may cause CME. Snapshot first (or drop parallelism).
private void setValueChildren() { childrenContexts.forEach((String collectionName, List<MDReaderContext> value) -> { - var collection = value.parallelStream() + // snapshot to avoid races with concurrent add() + var snapshot = List.copyOf(value); + var collection = snapshot.parallelStream() .map((MDReaderContext childContext) -> { childContext.setOwner(mdoReference); return childContext.build(); }).toList();Alternatively:
- var collection = value.parallelStream() + var collection = value.stream()
65-65: Typo in constant name (“VALIANT”); rename for clarity.Variable name only; string value is correct. Rename to avoid confusion.
- private static final String SUPPORT_VALIANT_FIELD_NAME = "SupportVariant"; + private static final String SUPPORT_VARIANT_FIELD_NAME = "SupportVariant"; @@ - super.setValue(SUPPORT_VALIANT_FIELD_NAME, supportVariant); + super.setValue(SUPPORT_VARIANT_FIELD_NAME, supportVariant);Also applies to: 109-109
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/helpers/Rights.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/mdo/utils/LazyLoader.java(2 hunks)src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/mdo/utils/LazyLoader.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: QA
- GitHub Check: build (23, macOS-latest)
- GitHub Check: build (23, ubuntu-latest)
- GitHub Check: build (23, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
🔇 Additional comments (5)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/helpers/Rights.java (3)
160-160: Typo fixed — variable name corrected.The typo in the variable name has been corrected from
hasAcccesstohasAccessand its usage updated accordingly. This addresses the concern raised in the previous review.Also applies to: 166-166
145-150: Accessor method migration looks good.The conversion from getter-style methods (
getObjectRights(),getName(),getRights(),getValue()) to record-style accessors (objectRights(),name(),rights(),value()) is consistent and preserves the stream pipeline logic.
160-164: Accessor updates consistent with refactoring pattern.The accessor method changes in the
rolesAccessmethod mirror those inrightAccessand maintain the same consistent pattern of converting to record-style accessors while preserving the stream logic.src/main/java/com/github/_1c_syntax/bsl/reader/common/context/MDReaderContext.java (2)
102-106: Assignable check fixed; confirm intended match (exact vs subclasses).Switch to ExternalDataSourceTableField.class.isAssignableFrom(realClass) resolves prior misclassification. If only exact “Field” is desired, use realClass == ExternalDataSourceTableField.class.
96-100: Rename migration complete; verify test coverage added.The
skipSupport()rename is confirmed across all call sites (DesignerReader, MDReaderContext, MDCReaderContext, EDTReader) with negation preserved. No legacyisSkipSupport()calls remain. Logic gate is consistent: parse support data whenuuid != null && !skipSupport(), elseSupportVariant.NONE.Please confirm that test coverage for both branches (uuid present/absent and skipSupport true/false) was added as requested in the original review.
|
Описание
Оптимизированы структуры хранения
Связанные задачи
Closes
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit