Реализация Solution v2#639
Merged
Merged
Conversation
…объект - Добавлены классы Solution и ObjectProvenance для слияния конфигураций с отслеживанием происхождения объектов. MDClasses.createSolution() теперь возвращает Solution вместо Configuration, содержа объединённую конфигурацию, базовую конфигурацию, список расширений и карту происхождений. - ConfigurationConverter разделён на реализации для каждого формата - Улучшен MDMerger с полной поддержкой дочерних типов: - копирование реквизитов, табличных частей, предопределённых значений для всех типов-владельцев - поддержка регистров, перечислений, задач, расчётов, внешних источников данных, планов счетов - добавлен mergeAll() с возвратом MergeResult и происхождений
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR adds ChangesSolution and provenance implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MDClasses
participant MDMerger
participant Configuration
participant Solution
Caller->>MDClasses: createSolution(path)
MDClasses->>MDClasses: buildSolution(base, extensions)
alt no extensions
MDClasses->>Configuration: getChildrenByMdoRef()
MDClasses->>Solution: build with base provenance
else has extensions
MDClasses->>MDMerger: mergeAll(base, extensions)
MDMerger->>MDMerger: initProvenance(base), initProvenance(each extension)
MDMerger->>Configuration: merge(accumulated, extension)
MDMerger-->>MDClasses: MergeResult(configuration, provenance)
MDClasses->>Solution: build with merged config and provenance
end
MDClasses-->>Caller: Solution
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Test Results 504 files + 18 504 suites +18 12m 7s ⏱️ - 1m 2s Results for commit f80b023. ± Comparison against base commit cdcc48d. This pull request removes 1 and adds 23 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/test/java/com/github/_1c_syntax/bsl/reader/MDMergerMergeTest.java (1)
121-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen provenance assertions in
testMergeAllReturnsProvenance.The test verifies provenance is non-empty and
objectBelongingis not null, but doesn't assert:
ownerRefis the base configuration'sMdoReferencefor objects originating from basemodifiedByExtensionRefscontains the extension's ref for objects modified by the extension (once that field is populated)- Provenance for objects added exclusively by the extension has the extension as
ownerRef♻️ Suggested additional assertions
var prov = result.provenance().get(catalogRef); assertThat(prov).isNotNull(); assertThat(prov.getObjectBelonging()).isNotNull(); + assertThat(prov.getOwnerRef()).isEqualTo(base.getMdoReference()); + // Once modifiedByExtensionRefs is populated: + // assertThat(prov.getModifiedByExtensionRefs()).contains(ext.getMdoReference());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/github/_1c_syntax/bsl/reader/MDMergerMergeTest.java` around lines 121 - 150, Strengthen the provenance checks in testMergeAllReturnsProvenance by asserting the full provenance state for both base-origin and extension-origin objects. In MDMergerMergeTest, after obtaining the catalog provenance for "Справочник1", verify that ownerRef matches the base configuration’s MdoReference, add an assertion for modifiedByExtensionRefs once MDMerger.mergeAll populates it, and include a case for an object introduced only by the extension to confirm its ownerRef points to the extension’s MdoReference. Keep the existing result and provenance lookups, but expand the assertions around prov to cover these ownership and modification fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/ObjectProvenance.java`:
- Around line 53-54: The existing-entry path in MDMerger.initProvenance is
skipping extension provenance for base objects that already have provenance
entries, so modifiedByExtensionRefs never gets the current extension’s
MdoReference. Update the logic in initProvenance to append the current extension
reference to ObjectProvenance.modifiedByExtensionRefs when the entry already
exists, rather than only creating new entries for missing refs, and keep the
behavior aligned with the ObjectProvenance builder/@Singular list.
In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/Solution.java`:
- Around line 75-86: The `Solution` builder path leaves `provenance` null,
unlike `baseConfiguration` and `extensions`, which causes `getProvenance()`,
`getOwnerRef()`, and `getOwner()` to NPE when they read `provenance`. Add a
default initialization for the `provenance` field in `Solution` (matching the
existing `@Builder.Default` pattern used by the other fields) so the public
builder always creates a safe empty map unless explicitly overridden.
In
`@src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/ConfigurationConverter.java`:
- Around line 64-69: The hasExtensionMarker method in ConfigurationConverter
currently swallows IOException and returns false without any trace, so add error
logging in the catch block before returning false. Use the existing logging
approach in this class or nearby converter code, and include the caught
IOException details so failures in ExtendXStream.getCurrentPath(reader) or
Files.lines(...) are diagnosable while keeping the current fallback behavior.
---
Nitpick comments:
In `@src/test/java/com/github/_1c_syntax/bsl/reader/MDMergerMergeTest.java`:
- Around line 121-150: Strengthen the provenance checks in
testMergeAllReturnsProvenance by asserting the full provenance state for both
base-origin and extension-origin objects. In MDMergerMergeTest, after obtaining
the catalog provenance for "Справочник1", verify that ownerRef matches the base
configuration’s MdoReference, add an assertion for modifiedByExtensionRefs once
MDMerger.mergeAll populates it, and include a case for an object introduced only
by the extension to confirm its ownerRef points to the extension’s MdoReference.
Keep the existing result and provenance lookups, but expand the assertions
around prov to cover these ownership and modification fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ceddfc0-e759-43ef-8a8b-f216452b225c
📒 Files selected for processing (16)
.idea/vcs.xmlsrc/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.javasrc/main/java/com/github/_1c_syntax/bsl/mdclasses/ObjectProvenance.javasrc/main/java/com/github/_1c_syntax/bsl/mdclasses/Solution.javasrc/main/java/com/github/_1c_syntax/bsl/reader/FakeReader.javasrc/main/java/com/github/_1c_syntax/bsl/reader/MDMerger.javasrc/main/java/com/github/_1c_syntax/bsl/reader/MDReader.javasrc/main/java/com/github/_1c_syntax/bsl/reader/designer/DesignerReader.javasrc/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/ConfigurationConverter.javasrc/main/java/com/github/_1c_syntax/bsl/reader/edt/EDTReader.javasrc/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/ConfigurationConverter.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/MDClassesSolutionTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/ObjectProvenanceTest.javasrc/test/java/com/github/_1c_syntax/bsl/mdclasses/SolutionTest.javasrc/test/java/com/github/_1c_syntax/bsl/reader/MDMergerMergeTest.javasrc/test/java/com/github/_1c_syntax/bsl/smoke/RightTest.java
💤 Files with no reviewable changes (5)
- .idea/vcs.xml
- src/main/java/com/github/_1c_syntax/bsl/reader/MDReader.java
- src/main/java/com/github/_1c_syntax/bsl/reader/FakeReader.java
- src/main/java/com/github/_1c_syntax/bsl/reader/designer/DesignerReader.java
- src/main/java/com/github/_1c_syntax/bsl/reader/edt/EDTReader.java
Comment on lines
+64
to
+69
| private static boolean hasExtensionMarker(HierarchicalStreamReader reader) { | ||
| try (var lines = Files.lines(ExtendXStream.getCurrentPath(reader))) { | ||
| return lines.anyMatch(line -> line.contains("ConfigurationExtensionPurpose")); | ||
| } catch (IOException e) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log the IOException before returning false.
Silently swallowing the exception causes the converter to misclassify extensions as configurations without any trace. At minimum, log the error so the issue is diagnosable.
🔧 Proposed fix
private static boolean hasExtensionMarker(HierarchicalStreamReader reader) {
try (var lines = Files.lines(ExtendXStream.getCurrentPath(reader))) {
return lines.anyMatch(line -> line.contains("ConfigurationExtensionPurpose"));
} catch (IOException e) {
+ LOGGER.error("Failed to read configuration file for extension marker detection", e);
return false;
}
}📝 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.
Suggested change
| private static boolean hasExtensionMarker(HierarchicalStreamReader reader) { | |
| try (var lines = Files.lines(ExtendXStream.getCurrentPath(reader))) { | |
| return lines.anyMatch(line -> line.contains("ConfigurationExtensionPurpose")); | |
| } catch (IOException e) { | |
| return false; | |
| } | |
| private static boolean hasExtensionMarker(HierarchicalStreamReader reader) { | |
| try (var lines = Files.lines(ExtendXStream.getCurrentPath(reader))) { | |
| return lines.anyMatch(line -> line.contains("ConfigurationExtensionPurpose")); | |
| } catch (IOException e) { | |
| LOGGER.error("Failed to read configuration file for extension marker detection", e); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/ConfigurationConverter.java`
around lines 64 - 69, The hasExtensionMarker method in ConfigurationConverter
currently swallows IOException and returns false without any trace, so add error
logging in the catch block before returning false. Use the existing logging
approach in this class or nearby converter code, and include the caught
IOException details so failures in ExtendXStream.getCurrentPath(reader) or
Files.lines(...) are diagnosable while keeping the current fallback behavior.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Описание
Реализация Solution - объединения конфигурации и расширений в единый объект
Добавлены классы Solution и ObjectProvenance для слияния конфигураций с отслеживанием происхождения объектов. MDClasses.createSolution() теперь возвращает Solution вместо Configuration, содержа объединённую конфигурацию, базовую конфигурацию, список расширений и карту происхождений.
ConfigurationConverter разделён на реализации для каждого формата
Улучшен MDMerger с полной поддержкой дочерних типов:
Связанные задачи
Closes: #636
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
New Features
Bug Fixes
Tests