feat: API записи объектов метаданных (Subsystem, Catalog, Configuration) в форматах EDT и Конфигуратор#595
Open
johnnyshut wants to merge 4 commits into1c-syntax:developfrom
Open
feat: API записи объектов метаданных (Subsystem, Catalog, Configuration) в форматах EDT и Конфигуратор#595johnnyshut wants to merge 4 commits into1c-syntax:developfrom
johnnyshut wants to merge 4 commits into1c-syntax:developfrom
Conversation
- Введены два перегруженных метода writeObject в MDClasses для записи объектов метаданных в файлы, поддерживающие как настройки записи по умолчанию, так и пользовательские настройки записи. - Добавлен новый класс MDCWriteSettings для инкапсуляции конфигураций записи файлов, включая настройки кодировки по умолчанию.
…х в форматах EDT и Designer - Создан класс MDOWriter для записи объектов метаданных в файлы форматов EDT (.mdo) и Designer (.xml). - Добавлены конвертеры для записи объектов Catalog и Configuration в формате Designer. - Реализованы конвертеры для записи объектов Catalog и Configuration в формате EDT. - Добавлен класс ReadWriteDemo для демонстрации чтения и записи метаданных.
…DT и Designer - Созданы тесты MDOWriterDesignerTest и MDOWriterEdtTest для проверки записи объектов метаданных в формате Designer (.xml) и EDT (.mdo). - Тесты включают проверку корректности записанных данных и возможность чтения обратно записанных объектов. - Добавлены проверки на выброс исключений при неверных входных данных.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds write support for metadata objects: new MDCWriteSettings, MDClasses.writeObject overloads, MDOWriter facade, EDT and Designer writers with XStream converters for Subsystem/Catalog/Configuration, a ReadWrite demo, and JUnit tests for both formats and round-trip validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MDClasses as MDClasses
participant MDOWriter as MDOWriter
participant Writer as Writer
participant XStream as XStream
participant FS as FileSystem
Client->>MDClasses: writeObject(path, object[, settings])
MDClasses->>MDOWriter: writeObject(path, object[, settings])
MDOWriter->>MDOWriter: determine extension
alt .mdo
MDOWriter->>Writer: instantiate EDTWriter(settings)
MDOWriter->>Writer: write(path, object)
Writer->>XStream: createXStream() / register EDT converters
Writer->>FS: write temp file (XML header + encoded marshal)
XStream->>FS: marshal(object) to temp
Writer->>FS: atomic move temp -> target
else .xml
MDOWriter->>Writer: instantiate DesignerWriter(settings)
MDOWriter->>Writer: write(path, object)
Writer->>XStream: createXStream() / register Designer converters
Writer->>FS: write XML header and marshal to target
else unsupported
MDOWriter-->>Client: throw UnsupportedOperationException
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (3)
src/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.java (1)
42-46: Consider relocating demo code or adding logging annotation.This demonstration class is placed in production sources. While acceptable for a runnable demo, consider:
- Moving to a separate
examplessource set orsrc/demo/javato clearly separate from library code- Using
@Slf4jfor consistency with other classes in the codebaseIf this is intentionally part of the public API as an example, the current placement is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.java` around lines 42 - 46, ReadWriteDemo is a demo class currently in production sources; either move it into a dedicated demo/examples source set (e.g., src/demo/java or an examples module) to separate sample code from library code, or keep it here but add the standard logging annotation used in the project (e.g., annotate the class with `@Slf4j`) to match other classes; locate the class ReadWriteDemo and apply one of these two fixes (relocate file into the demo/examples source set and update build configuration, or add the `@Slf4j` import and annotation to the ReadWriteDemo class) so the codebase stays consistent.src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java (1)
37-39: @slf4j annotation is unused.The
@Slf4jannotation generates aLOGGERfield, but no logging is performed in this class. Consider removing it unless logging will be added.♻️ Remove unused annotation
`@UtilityClass` -@Slf4j public class MDOWriter {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java` around lines 37 - 39, The `@Slf4j` annotation on class MDOWriter is unused (no logging calls or LOGGER references); remove the `@Slf4j` annotation from the MDOWriter class declaration and any unused generated LOGGER field references so the class only uses `@UtilityClass`, keeping the class tidy and avoiding unused-field warnings.src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java (1)
69-72: ConfigurationExtension implements CF but is not supported by DesignerWriter.The current implementation supports only
Subsystem,Catalog, andConfiguration(as documented in the JavaDoc). IfConfigurationExtensionsupport is planned, consider either extending the validation logic to include it or refactoring the type check to use theCFinterface instead of listing specific classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java` around lines 69 - 72, DesignerWriter currently validates input by checking specific classes (Subsystem, Catalog, Configuration) and rejects ConfigurationExtension even though it implements CF; update the type validation in DesignerWriter to accept any CF implementation (i.e., check "object instanceof CF") or explicitly include ConfigurationExtension in the allowed types if only certain CF subtypes should be supported; modify the condition that currently references Subsystem, Catalog, Configuration to either replace it with a CF interface check or to add ConfigurationExtension to the allowed list and adjust the UnsupportedOperationException message accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.java`:
- Around line 70-79: The Javadoc for MDClasses.writeObject incorrectly states
only EDT (.mdo) is supported; update the comment to reflect both EDT (.mdo) and
Designer (.xml) formats supported by MDOWriter.writeObject, e.g., change the
sentence "формат определяется по расширению пути: .mdo — EDT" to mention ".mdo
(EDT) и .xml (Designer)" and adjust the param/throws text if needed to match
supported object types and formats.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java`:
- Around line 55-74: CatalogDesignerWriteConverter is currently emitting a fixed
template for catalog metadata (hardcoded Owners, SubordinationUse,
Hierarchy/Code fields) which overwrites real catalog state; update the
properties-writing block so writeElement(...) uses the actual Catalog instance
fields (e.g., catalog.getOwners(), catalog.getSubordinationUse(),
catalog.isHierarchical()/catalog.getHierarchyType(),
catalog.getCodeLength()/catalog.getCodeType()/catalog.getCodeSeries(),
catalog.isCheckUnique(), catalog.isAutonumbering(),
catalog.getDefaultPresentation(), etc.) with safe null/default fallbacks (fall
back to existing constants like FALSE or "WholeCatalog" when fields are null)
instead of literal strings; locate the code in CatalogDesignerWriteConverter
where writeElement is called for "Owners", "SubordinationUse", "Hierarchical",
"HierarchyType", "CodeLength", "CodeType", "CodeSeries", "CheckUnique",
"Autonumbering" and replace the hardcoded values with expressions pulling from
the catalog getters.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.java`:
- Around line 149-158: The escapeXml method is redundantly escaping strings
already handled by XStream's setValue, causing double-encoding; update
ConfigurationDesignerWriteConverter by removing uses of escapeXml (replace calls
like setValue(escapeXml(s)) with setValue(s) or setValue(s == null ? "" : s)),
and either delete the escapeXml method or change it to a no-op identity (return
s or ""). Ensure references to escapeXml in the class are removed/updated so
XStream alone performs XML escaping.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java`:
- Around line 60-63: The writer currently hardcodes empty strings for the
"Explanation" and "Content" XML elements, which erases real data; update
SubsystemDesignerWriteConverter to write the subsystem's actual fields (e.g. use
the Subsystem instance's getExplanation()/getContent() or the converter's
value.getExplanation()/value.getContent()) instead of "" when calling
writeElement for "Explanation" and "Content", ensuring nulls are handled (write
"" only when the field is null or empty).
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java`:
- Around line 53-72: The CatalogEdtWriteConverter is writing many hard-coded
defaults via writeElement (e.g.,
"useStandardCommands","fullTextSearchOnInputByString","createOnInput","dataLockControlMode","fullTextSearch","levelCount","foldersOnTop","codeLength","descriptionLength","codeType","codeAllowedLength","autonumbering","defaultPresentation","editType","choiceMode")
instead of serializing values from the catalog object; change the logic in
CatalogEdtWriteConverter to read the corresponding properties from the catalog
model (use getters on catalog and
catalog.getCodeSeries()/catalog.isCheckUnique() already used) and emit those
elements only from the catalog values or only when they differ from documented
defaults, so the writer becomes a faithful serializer rather than a template
generator.
- Around line 88-106: The writeElement method is double-escaping XML by calling
escapeXml(text) before writer.setValue; change writeElement to pass text
directly to PrettyPrintWriter.setValue (i.e., writer.setValue(text)) and remove
or stop using the escapeXml(...) helper (escapeXml method can be deleted or left
unused), ensuring all EDT/Designer write converters use writer.setValue(...)
without manual escaping so PrettyPrintWriter handles encoding.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java`:
- Around line 190-200: writeSynonym currently opens one SYNONYM node and writes
all language entries inside it; change it to mirror writeMultiLang by iterating
synonym.getContent() and for each entry start a separate SYNONYM node, call
writeElement(writer, KEY, entry.getLangKey()) and writeElement(writer, VALUE,
entry.getValue()), then end that SYNONYM node; keep null/empty guard and reuse
the same SYNONYM, KEY, VALUE symbols and writeElement helper.
- Around line 215-224: The code manually escapes XML via escapeXml(String) which
causes double-encoding when passed into XStream's PrettyPrintWriter.setValue();
update ConfigurationEdtWriteConverter to stop calling escapeXml(...) when
supplying values to writer.setValue(...) (pass the original raw string instead)
and remove the escapeXml method if it becomes unused; search for any usages of
escapeXml within this class (and related write methods) and replace them with
the unescaped value so XStream handles escaping.
In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.java`:
- Around line 65-76: EDTWriter currently opens the target Path and writes
directly so a marshal failure can leave a truncated file; change the logic to
marshal to a sibling temporary file and then atomically replace the target:
create the parent dirs as before, create a temp file in the same directory
(using Files.createTempFile with the target parent), open the
OutputStreamWriter/PrettyPrintWriter and call xstream.marshal(object,
prettyWriter) into that temp, close the writer in a try-with-resources, then
move the temp into place with Files.move(temp, path,
StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); ensure the
temp is deleted on failure. Reference symbols: EDTWriter, path, writeSettings,
xstream, PrettyPrintWriter.
- Around line 61-75: The write method currently accepts any Object and lets
xstream.marshal serialize unsupported types; add a fast-fail type check in
EDTWriter.write to ensure the root object is an instance of one of the supported
EDT metadata classes (Subsystem, Configuration, or Catalog) and if not throw an
IllegalArgumentException with a clear message; perform this check before
creating directories or invoking xstream.marshal (reference the write method and
xstream.marshal call) so only valid types are marshalled.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java`:
- Around line 55-65: The SubsystemEdtWriteConverter is currently lossy because
it only writes name, synonym, two flags and children; detect when unsupported
fields (subsystem.getExplanation(), subsystem.getContent(), or
subsystem.getParentSubsystem()) are non-empty and fail-fast instead of emitting
incomplete EDT: in the method that writes a Subsystem (where writeElement,
writeSynonym and the children loop are used) add checks for these fields and
throw an IllegalStateException (or similar runtime exception) with a clear
message referencing SubsystemEdtWriteConverter and the offending field so
callers know the write cannot proceed.
- Around line 80-98: The writeElement method is pre-escaping XML via escapeXml()
before calling writer.setValue(), causing double-escaping because
PrettyPrintWriter (XStream 1.4.21) already escapes text; remove the manual
escaping by changing writeElement to call writer.setValue(text) and delete the
escapeXml method, and apply the identical change in CatalogEdtWriteConverter and
ConfigurationEdtWriteConverter (update uses of escapeXml and remove the helper).
Ensure references to setValue(...) now pass the raw text so PrettyPrintWriter
handles escaping.
In `@src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.java`:
- Around line 45-123: The current unit tests (writeSubsystemDesignerXml,
writeSubsystemWithChildrenHasChildObjects, writeCatalogDesignerXml,
writeConfigurationDesignerXml) only assert substrings and don't verify the
Designer XML is well-formed or consumable; add an integration test (e.g.,
writeAndParseDesignerXmlIntegrationTest) that uses MDClasses.writeObject to
write a real-ish object (or a sample 1C designer XML resource), then parse the
produced file with a standard XML parser
(javax.xml.parsers.DocumentBuilderFactory / DocumentBuilder) to assert the
document is well-formed, the root element and expected uuid/name attributes
exist, required namespaces are present and no double-escaped text occurs; if a
read/round-trip API exists (e.g., MDClasses.readObject or similar) also read the
file back and assert the deserialized object's key properties match the
original.
In `@src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java`:
- Around line 128-136: Replace the fragile try-catch in the test
writeObjectThrowsOnNullPath with an AssertJ exception assertion: call
assertThatThrownBy(() -> MDClasses.writeObject((Path) null,
subsystem)).isInstanceOf(IllegalArgumentException.class); so the test fails if
no exception is thrown; reference the test method writeObjectThrowsOnNullPath
and the target method MDClasses.writeObject to locate the change.
- Around line 138-146: The test writeObjectThrowsOnNullObject currently swallows
any exception via try/catch and only asserts the caught exception's type;
replace this with a direct exception assertion to avoid false positives by using
JUnit's assertThrows to call MDClasses.writeObject(path, null) and expect
IllegalArgumentException (optionally assert the message), referencing the test
method writeObjectThrowsOnNullObject and the target method MDClasses.writeObject
to locate and update the code.
- Around line 148-158: The test writeObjectThrowsOnUnsupportedFormat incorrectly
uses a try/catch instead of asserting the exception; replace the try/catch with
a proper assertion that MDClasses.writeObject(path, subsystem) throws an
UnsupportedOperationException and that its message contains ".mdo" and ".xml"
(e.g. use Assertions.assertThrows(...) or AssertJ's assertThatThrownBy(() ->
MDClasses.writeObject(...)).isInstanceOf(UnsupportedOperationException.class).hasMessageContaining(".mdo").hasMessageContaining(".xml")),
and remove the surrounding try/catch block.
---
Nitpick comments:
In `@src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java`:
- Around line 69-72: DesignerWriter currently validates input by checking
specific classes (Subsystem, Catalog, Configuration) and rejects
ConfigurationExtension even though it implements CF; update the type validation
in DesignerWriter to accept any CF implementation (i.e., check "object
instanceof CF") or explicitly include ConfigurationExtension in the allowed
types if only certain CF subtypes should be supported; modify the condition that
currently references Subsystem, Catalog, Configuration to either replace it with
a CF interface check or to add ConfigurationExtension to the allowed list and
adjust the UnsupportedOperationException message accordingly.
In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java`:
- Around line 37-39: The `@Slf4j` annotation on class MDOWriter is unused (no
logging calls or LOGGER references); remove the `@Slf4j` annotation from the
MDOWriter class declaration and any unused generated LOGGER field references so
the class only uses `@UtilityClass`, keeping the class tidy and avoiding
unused-field warnings.
In `@src/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.java`:
- Around line 42-46: ReadWriteDemo is a demo class currently in production
sources; either move it into a dedicated demo/examples source set (e.g.,
src/demo/java or an examples module) to separate sample code from library code,
or keep it here but add the standard logging annotation used in the project
(e.g., annotate the class with `@Slf4j`) to match other classes; locate the class
ReadWriteDemo and apply one of these two fixes (relocate file into the
demo/examples source set and update build configuration, or add the `@Slf4j`
import and annotation to the ReadWriteDemo class) so the codebase stays
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c622b1c-88eb-4660-bb7a-76f0e8ae7064
📒 Files selected for processing (14)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.javasrc/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.javasrc/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java
Comment on lines
+55
to
+74
| writeElement(writer, NAME, catalog.getName()); | ||
| writeSynonym(writer, catalog.getSynonym()); | ||
| writeElement(writer, COMMENT, catalog.getComment() != null ? catalog.getComment() : ""); | ||
| writeElement(writer, "Hierarchical", "true"); | ||
| writeElement(writer, "HierarchyType", "HierarchyFoldersAndItems"); | ||
| writeElement(writer, "LimitLevelCount", FALSE); | ||
| writeElement(writer, "LevelCount", "2"); | ||
| writeElement(writer, "FoldersOnTop", "true"); | ||
| writeElement(writer, "UseStandardCommands", "true"); | ||
| writeElement(writer, "Owners", ""); | ||
| writeElement(writer, "SubordinationUse", "ToItems"); | ||
| writeElement(writer, "CodeLength", "9"); | ||
| writeElement(writer, "DescriptionLength", "25"); | ||
| writeElement(writer, "CodeType", "String"); | ||
| writeElement(writer, "CodeAllowedLength", "Variable"); | ||
| writeElement(writer, "CodeSeries", catalog.getCodeSeries() != null ? catalog.getCodeSeries().fullName().getEn() : "WholeCatalog"); | ||
| writeElement(writer, "CheckUnique", catalog.isCheckUnique() ? "true" : FALSE); | ||
| writeElement(writer, "Autonumbering", FALSE); | ||
| writeElement(writer, "DefaultPresentation", "AsDescription"); | ||
| writer.endNode(); // Properties |
There was a problem hiding this comment.
Designer export is serializing a fixed catalog template.
Catalog has real ownership/subordination state, but this writer always emits empty <Owners> and a fixed set of hierarchy/code settings. Saving a subordinate or otherwise non-default catalog will silently rewrite its metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java`
around lines 55 - 74, CatalogDesignerWriteConverter is currently emitting a
fixed template for catalog metadata (hardcoded Owners, SubordinationUse,
Hierarchy/Code fields) which overwrites real catalog state; update the
properties-writing block so writeElement(...) uses the actual Catalog instance
fields (e.g., catalog.getOwners(), catalog.getSubordinationUse(),
catalog.isHierarchical()/catalog.getHierarchyType(),
catalog.getCodeLength()/catalog.getCodeType()/catalog.getCodeSeries(),
catalog.isCheckUnique(), catalog.isAutonumbering(),
catalog.getDefaultPresentation(), etc.) with safe null/default fallbacks (fall
back to existing constants like FALSE or "WholeCatalog" when fields are null)
instead of literal strings; locate the code in CatalogDesignerWriteConverter
where writeElement is called for "Owners", "SubordinationUse", "Hierarchical",
"HierarchyType", "CodeLength", "CodeType", "CodeSeries", "CheckUnique",
"Autonumbering" and replace the hardcoded values with expressions pulling from
the catalog getters.
...main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java
Show resolved
Hide resolved
Comment on lines
+53
to
+72
| writeElement(writer, NAME, catalog.getName()); | ||
| writeElement(writer, "useStandardCommands", "true"); | ||
| writeElement(writer, "fullTextSearchOnInputByString", "DontUse"); | ||
| writeElement(writer, "createOnInput", "Use"); | ||
| writeElement(writer, "dataLockControlMode", "Managed"); | ||
| writeElement(writer, "fullTextSearch", "Use"); | ||
| writeElement(writer, "levelCount", "2"); | ||
| writeElement(writer, "foldersOnTop", "true"); | ||
| writeElement(writer, "codeLength", "9"); | ||
| writeElement(writer, "descriptionLength", "25"); | ||
| writeElement(writer, "codeType", "String"); | ||
| writeElement(writer, "codeAllowedLength", "Variable"); | ||
| writeElement(writer, "checkUnique", catalog.isCheckUnique() ? "true" : "false"); | ||
| writeElement(writer, "autonumbering", "false"); | ||
| writeElement(writer, "defaultPresentation", "AsDescription"); | ||
| if (catalog.getCodeSeries() != null) { | ||
| writeElement(writer, "codeSeries", catalog.getCodeSeries().fullName().getEn()); | ||
| } | ||
| writeElement(writer, "editType", "InDialog"); | ||
| writeElement(writer, "choiceMode", "BothWays"); |
There was a problem hiding this comment.
Don't bake catalog defaults into every EDT export.
Most of this block is fixed literals rather than values from catalog. That makes the new write path a template generator, not a faithful serializer, and it will silently drift as soon as callers need non-default hierarchy/code/search settings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java`
around lines 53 - 72, The CatalogEdtWriteConverter is writing many hard-coded
defaults via writeElement (e.g.,
"useStandardCommands","fullTextSearchOnInputByString","createOnInput","dataLockControlMode","fullTextSearch","levelCount","foldersOnTop","codeLength","descriptionLength","codeType","codeAllowedLength","autonumbering","defaultPresentation","editType","choiceMode")
instead of serializing values from the catalog object; change the logic in
CatalogEdtWriteConverter to read the corresponding properties from the catalog
model (use getters on catalog and
catalog.getCodeSeries()/catalog.isCheckUnique() already used) and emit those
elements only from the catalog values or only when they differ from documented
defaults, so the writer becomes a faithful serializer rather than a template
generator.
src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java
Outdated
Show resolved
Hide resolved
src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java (1)
62-77:⚠️ Potential issue | 🟠 MajorHardcoded property values will overwrite actual catalog metadata.
The converter hardcodes
Hierarchical,HierarchyType,LimitLevelCount,LevelCount,FoldersOnTop,UseStandardCommands,SubordinationUse,CodeLength,DescriptionLength,CodeType,CodeAllowedLength,Autonumbering, andDefaultPresentationinstead of reading them from theCatalogmodel. This means serializing and then deserializing a catalog will lose its original property values.While the
Catalogmodel may not expose all these properties currently, hardcoding them silently rewrites metadata. Consider either:
- Reading available properties from the model (e.g., if they're added later)
- Adding a clear comment/TODO indicating this is MVP behavior with known data loss
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java` around lines 62 - 77, CatalogDesignerWriteConverter currently hardcodes many catalog properties via sequential writeElement calls (e.g., "Hierarchical", "HierarchyType", "LimitLevelCount", "LevelCount", "FoldersOnTop", "UseStandardCommands", "SubordinationUse", "CodeLength", "DescriptionLength", "CodeType", "CodeAllowedLength", "Autonumbering", "DefaultPresentation") which will overwrite real metadata from the Catalog model; update the conversion to read each value from the catalog instance (use existing accessors like catalog.getCodeSeries(), catalog.isCheckUnique() and add/consume new getters on Catalog as needed) and only fall back to the previous defaults when the model value is null/absent, or if that's not feasible now, add a clear TODO comment in CatalogDesignerWriteConverter explaining this is MVP behavior that may rewrite metadata so future maintainers add proper getters and replace hardcoded values.src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java (1)
51-61:⚠️ Potential issue | 🟠 Major
Subsystemsupport is still limited to the empty-data case.These guards prevent silent loss, but they also make the EDT writer fail for any subsystem with
explanation,content, orparentSubsystem. Because those are normalSubsystemfields, either serialize them here or narrow the documented support/tests to this subset so callers are not surprised by runtime failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java` around lines 51 - 61, The current SubsystemEdtWriteConverter throws IllegalStateException for non-empty explanation, content, and parentSubsystem, which breaks normal Subsystem serialization; remove those guards and implement serialization for subsystem.getExplanation(), subsystem.getContent(), and subsystem.getParentSubsystem() (convert MdoReference parent via the same reference-writing logic used elsewhere, respecting MdoReference.EMPTY and null checks), following the writer patterns in SubsystemEdtWriteConverter so explanation and content are written out when present and parentSubsystem is emitted as a reference rather than causing a runtime failure.src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java (1)
190-200:⚠️ Potential issue | 🟠 MajorEmit one
<synonym>node per language entry.This helper currently produces one wrapper node containing multiple key/value pairs. EDT synonym entries should follow the same one-entry-per-node shape as
writeMultiLang(), otherwise multilingual synonyms are serialized in a different structure than the rest of this converter.🔧 Suggested shape
private static void writeSynonym(HierarchicalStreamWriter writer, MultiLanguageString synonym) { if (synonym == null || synonym.isEmpty()) { return; } - writer.startNode(SYNONYM); for (var entry : synonym.getContent()) { + writer.startNode(SYNONYM); writeElement(writer, KEY, entry.getLangKey()); writeElement(writer, VALUE, entry.getValue()); + writer.endNode(); } - writer.endNode(); // close SYNONYM }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java` around lines 190 - 200, writeSynonym currently emits a single SYNONYM wrapper containing multiple KEY/VALUE pairs; change it to emit one SYNONYM node per language entry (same shape as writeMultiLang) by iterating over synonym.getContent() and for each entry call writer.startNode(SYNONYM), writeElement(writer, KEY, entry.getLangKey()), writeElement(writer, VALUE, entry.getValue()), then writer.endNode(); keep the null/empty guard and remove the outer single startNode/endNode around the loop.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java (1)
103-110: Consider simplifying the null-handling condition.The condition on line 104 is hard to read and maintain. A clearer approach would be to use a Set of allowed-empty node names:
♻️ Suggested refactor
+ private static final Set<String> ALLOW_EMPTY_NODES = Set.of("Comment", "Explanation", "Picture", "Content"); + private static void writeElement(HierarchicalStreamWriter writer, String nodeName, String text) { - if (text == null && !nodeName.equals("Comment") && !nodeName.equals("Explanation") && !nodeName.equals("Picture") && !nodeName.equals("Content")) { + if (text == null && !ALLOW_EMPTY_NODES.contains(nodeName)) { return; } writer.startNode(nodeName); writer.setValue(text != null ? text : ""); writer.endNode(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java` around lines 103 - 110, The null-handling condition inside writeElement is hard to read; replace the inline chain (!nodeName.equals("Comment") && ...) with a static final Set<String> ALLOWED_EMPTY_NODES (e.g. Collections.unmodifiableSet(Set.of("Comment","Explanation","Picture","Content"))) and change the check to if (text == null && !ALLOWED_EMPTY_NODES.contains(nodeName)) return; Update the writeElement method to use this set so the intent is clearer and easier to maintain.src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.java (1)
77-90: Make the new facade overloads explicit about nullability.These methods are now part of the public write API, but the null contract is only enforced indirectly in
MDOWriter. Adding@NonNulltopath/objectand@NullabletowriteSettingswould make that contract clear to callers and static analysis.As per coding guidelines, "Use `@Nullable` and `@NonNull` annotations from org.jspecify for null safety".♻️ Minimal shape
+import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; ... - public void writeObject(Path path, Object object) throws IOException { + public void writeObject(`@NonNull` Path path, `@NonNull` Object object) throws IOException { MDOWriter.writeObject(path, object); } ... - public void writeObject(Path path, Object object, MDCWriteSettings writeSettings) throws IOException { + public void writeObject(`@NonNull` Path path, `@NonNull` Object object, + `@Nullable` MDCWriteSettings writeSettings) throws IOException { MDOWriter.writeObject(path, object, writeSettings); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.java` around lines 77 - 90, Annotate the public facade overloads in MDClasses (the two writeObject methods) with jspecify nullness: add `@NonNull` to the path and object parameters on both methods, and add `@Nullable` to the writeSettings parameter on the three-arg writeObject; keep the methods delegating to MDOWriter but make the null contracts explicit using org.jspecify.annotations.@NonNull and org.jspecify.annotations.@Nullable so callers and static analysis can rely on them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.java`:
- Around line 65-108: ConfigurationDesignerWriteConverter currently calls
writeChildList for many config getters but omits config.getBots(),
config.getFunctionalOptionsParameters(), config.getWebSocketClients(),
config.getIntegrationServices(), and config.getPaletteColors(), which causes
those child collections to be dropped; update the method to either add
writeChildList(writer, config.getBots()), writeChildList(writer,
config.getFunctionalOptionsParameters()), writeChildList(writer,
config.getWebSocketClients()), writeChildList(writer,
config.getIntegrationServices()), and writeChildList(writer,
config.getPaletteColors()) so they are serialized, or if omission is
intentional, add a clear comment near the MVP note documenting that these five
collections are intentionally skipped to avoid silent loss.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java`:
- Around line 78-87: The helper explanationToString currently drops all but the
first MultiLanguageString entry; update it to handle multiple translations
explicitly: either (A) choose a preferred locale order and return the matching
entry's value (e.g., prefer configuredLocale -> fallbackLocale -> first
available) or (B) preserve all translations by returning a deterministic
representation (e.g., concatenate values with language tags or serialize
getContent() into a single string) and document which approach is used; locate
and modify explanationToString and its use sites so the chosen behavior
(preferred-locale lookup or multi-value serialization) is implemented
consistently and clearly documented.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java`:
- Around line 45-74: The marshal method in CatalogEdtWriteConverter currently
ignores Catalog.explanation and Catalog.owners which causes data loss; update
the marshal(Object source, ...) in class CatalogEdtWriteConverter to detect when
catalog.getExplanation() is non-null/non-empty or catalog.getOwners() is
non-empty and fail fast (throw an IllegalStateException) with a clear message
including the catalog identifier (e.g., catalog.getUuid() or NAME) indicating
these fields are not yet supported, so callers cannot silently lose owner
bindings or explanation text until proper serialization (using
writeElement/writeSynonym or a new writer method) is implemented.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java`:
- Around line 60-80: The Configuration.synonym field is never written; update
ConfigurationEdtWriteConverter to call writeSynonym when marshalling the
configuration (e.g., in the same block where you write NAME and other top-level
elements). Add a call like writeSynonym(writer, config.getSynonym()) at an
appropriate place in the conversion method (the method containing
writeElement(...) calls), so the configuration's synonym is emitted just like
other top-level properties (use the existing writeSynonym(...) helper and the
config.getSynonym() accessor).
In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.java`:
- Around line 62-70: The public method write(Path path, Object object) should
validate the path parameter before calling path.getParent() to avoid an NPE; add
a null-check for path at the top of write(Path path, Object object) and throw an
IllegalArgumentException (consistent with the existing object null check) with a
clear message like "path must not be null" so direct callers of
EDTWriter.receive a stable, consistent error instead of an NPE.
---
Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java`:
- Around line 62-77: CatalogDesignerWriteConverter currently hardcodes many
catalog properties via sequential writeElement calls (e.g., "Hierarchical",
"HierarchyType", "LimitLevelCount", "LevelCount", "FoldersOnTop",
"UseStandardCommands", "SubordinationUse", "CodeLength", "DescriptionLength",
"CodeType", "CodeAllowedLength", "Autonumbering", "DefaultPresentation") which
will overwrite real metadata from the Catalog model; update the conversion to
read each value from the catalog instance (use existing accessors like
catalog.getCodeSeries(), catalog.isCheckUnique() and add/consume new getters on
Catalog as needed) and only fall back to the previous defaults when the model
value is null/absent, or if that's not feasible now, add a clear TODO comment in
CatalogDesignerWriteConverter explaining this is MVP behavior that may rewrite
metadata so future maintainers add proper getters and replace hardcoded values.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java`:
- Around line 190-200: writeSynonym currently emits a single SYNONYM wrapper
containing multiple KEY/VALUE pairs; change it to emit one SYNONYM node per
language entry (same shape as writeMultiLang) by iterating over
synonym.getContent() and for each entry call writer.startNode(SYNONYM),
writeElement(writer, KEY, entry.getLangKey()), writeElement(writer, VALUE,
entry.getValue()), then writer.endNode(); keep the null/empty guard and remove
the outer single startNode/endNode around the loop.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java`:
- Around line 51-61: The current SubsystemEdtWriteConverter throws
IllegalStateException for non-empty explanation, content, and parentSubsystem,
which breaks normal Subsystem serialization; remove those guards and implement
serialization for subsystem.getExplanation(), subsystem.getContent(), and
subsystem.getParentSubsystem() (convert MdoReference parent via the same
reference-writing logic used elsewhere, respecting MdoReference.EMPTY and null
checks), following the writer patterns in SubsystemEdtWriteConverter so
explanation and content are written out when present and parentSubsystem is
emitted as a reference rather than causing a runtime failure.
---
Nitpick comments:
In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.java`:
- Around line 77-90: Annotate the public facade overloads in MDClasses (the two
writeObject methods) with jspecify nullness: add `@NonNull` to the path and object
parameters on both methods, and add `@Nullable` to the writeSettings parameter on
the three-arg writeObject; keep the methods delegating to MDOWriter but make the
null contracts explicit using org.jspecify.annotations.@NonNull and
org.jspecify.annotations.@Nullable so callers and static analysis can rely on
them.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java`:
- Around line 103-110: The null-handling condition inside writeElement is hard
to read; replace the inline chain (!nodeName.equals("Comment") && ...) with a
static final Set<String> ALLOWED_EMPTY_NODES (e.g.
Collections.unmodifiableSet(Set.of("Comment","Explanation","Picture","Content")))
and change the check to if (text == null &&
!ALLOWED_EMPTY_NODES.contains(nodeName)) return; Update the writeElement method
to use this set so the intent is clearer and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2adffb52-2c7f-436f-b04c-6132bf61818f
📒 Files selected for processing (11)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.javasrc/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.java
...main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.java (2)
38-40: Consider usingStandardCharsetsconstant instead of magic string.The
"UTF-8"string is duplicated in two places. UsingStandardCharsets.UTF_8.name()would provide compile-time safety against typos and better express intent.♻️ Proposed fix
package com.github._1c_syntax.bsl.mdclasses; import lombok.Builder; +import org.jspecify.annotations.Nullable; + +import java.nio.charset.StandardCharsets; /** * Настройки записи объектов метаданных в файлы (EDT и Designer). @@ -35,13 +38,13 @@ public record MDCWriteSettings(`@Nullable` String encoding) { /** * Настройки по умолчанию: кодировка UTF-8. */ public static final MDCWriteSettings DEFAULT = MDCWriteSettings.builder() - .encoding("UTF-8") + .encoding(StandardCharsets.UTF_8.name()) .build(); /** * Возвращает кодировку для записи файлов; при null в настройках возвращается "UTF-8". */ public String encoding() { - return encoding != null ? encoding : "UTF-8"; + return encoding != null ? encoding : StandardCharsets.UTF_8.name(); } }Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.java` around lines 38 - 40, Replace the literal "UTF-8" magic string with the StandardCharsets constant to avoid duplication and typos: update both occurrences where MDCWriteSettings.builder().encoding("UTF-8") is used (refer to the MDCWriteSettings.DEFAULT constant and the other builder invocation) to call StandardCharsets.UTF_8.name() instead; import java.nio.charset.StandardCharsets if needed.
33-33: Add@Nullableannotation on theencodingparameter.The
encodingparameter can be null (as handled in the accessor on line 46), but lacks the@Nullableannotation from org.jspecify for null safety documentation.♻️ Proposed fix
package com.github._1c_syntax.bsl.mdclasses; import lombok.Builder; +import org.jspecify.annotations.Nullable; /** * Настройки записи объектов метаданных в файлы (EDT и Designer). * Используется при вызове {`@link` com.github._1c_syntax.bsl.mdclasses.MDClasses#writeObject(java.nio.file.Path, Object, MDCWriteSettings)}. * * `@param` encoding Кодировка записываемых файлов (по умолчанию UTF-8) */ `@Builder` -public record MDCWriteSettings(String encoding) { +public record MDCWriteSettings(`@Nullable` String encoding) {As per coding guidelines: "Use
@Nullableand@NonNullannotations from org.jspecify for null safety".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.java` at line 33, The record MDCWriteSettings currently declares the component String encoding without nullability metadata even though its accessor handles null; add the org.jspecify.annotations.Nullable annotation to the encoding component in the MDCWriteSettings declaration so callers and tools know encoding can be null (update the record header for MDCWriteSettings and import org.jspecify.annotations.Nullable if missing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java`:
- Around line 87-96: The writeSynonym helper in CatalogEdtWriteConverter
currently writes one <synonym> node containing all language entries; change it
to emit one <synonym> node per MultiLanguageString entry by moving
writer.startNode(SYNONYM) and writer.endNode() inside the loop over
synonym.getContent(), and for each entry call writeElement(writer, KEY,
entry.getLangKey()) and writeElement(writer, VALUE, entry.getValue()); keep the
initial null/empty guard and ensure behavior matches
ConfigurationEdtWriteConverter.writeSynonym().
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java`:
- Around line 88-131: marshal() in ConfigurationEdtWriteConverter fails to write
four Configuration collections (bots, webSocketClients, integrationServices,
paletteColors), so data is lost on export; update the marshal method (the block
that calls writeRefList(...)) to call writeRefList(writer, "bots",
config.getBots()), writeRefList(writer, "webSocketClients",
config.getWebSocketClients()), writeRefList(writer, "integrationServices",
config.getIntegrationServices()), and writeRefList(writer, "paletteColors",
config.getPaletteColors()) so those lists from
com.github._1c_syntax.bsl.mdclasses.Configuration are emitted during write.
---
Nitpick comments:
In `@src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.java`:
- Around line 38-40: Replace the literal "UTF-8" magic string with the
StandardCharsets constant to avoid duplication and typos: update both
occurrences where MDCWriteSettings.builder().encoding("UTF-8") is used (refer to
the MDCWriteSettings.DEFAULT constant and the other builder invocation) to call
StandardCharsets.UTF_8.name() instead; import java.nio.charset.StandardCharsets
if needed.
- Line 33: The record MDCWriteSettings currently declares the component String
encoding without nullability metadata even though its accessor handles null; add
the org.jspecify.annotations.Nullable annotation to the encoding component in
the MDCWriteSettings declaration so callers and tools know encoding can be null
(update the record header for MDCWriteSettings and import
org.jspecify.annotations.Nullable if missing).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be77541f-3f22-4c5d-8ffc-3292c5ea1da7
📒 Files selected for processing (10)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.javasrc/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.javasrc/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java
- src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java
- src/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.java
- src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.java
- src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java
src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java
Show resolved
Hide resolved
Contributor
Author
|
Оставшиеся замечания висят, потому что геттеров подходящих нет, mvp пулл реквеста))) |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java (1)
138-217: Add jspecify nullness annotations to the helper contracts.These helpers are deliberately null-tolerant (
compatibilityModeString,writeMdoRef,writeRefList,writeLanguage,writeMultiLang,writeSynonym,writeElement,nullToEmpty), but that contract is implicit today. Annotating nullable parameters/returns will make the omission semantics explicit and let static analysis catch accidental misuse earlier. As per coding guidelines, "Use@Nullableand@NonNullannotations from org.jspecify for null safety".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java` around lines 138 - 217, Add explicit jspecify nullness annotations on the helper contracts: import org.jspecify.annotations.Nullable and org.jspecify.annotations.NonNull, then annotate parameters/returns so the null-tolerant methods are explicit — mark inputs that may be null as `@Nullable` (e.g., parameters of compatibilityModeString, writeMdoRef, writeRefList, writeLanguage, writeMultiLang, writeSynonym, writeElement, nullToEmpty) and mark non-nullable writer and other parameters/returns as `@NonNull` (e.g., HierarchicalStreamWriter writer, NAME constants, and the return of compatibilityModeString and nullToEmpty which return empty string when input is null); apply annotations to the signatures of methods compatibilityModeString, writeMdoRef, writeRefList, writeLanguage, writeMultiLang, writeSynonym, writeElement, and nullToEmpty and add the necessary import lines.src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java (1)
84-96: DuplicatewriteSynonymimplementation across converters.This method is byte-for-byte identical to
SubsystemDesignerWriteConverter.writeSynonym(see context snippet 4). Consider extracting to a shared utility class to reduce duplication.♻️ Suggested refactor: extract to shared utility
Create a shared helper class:
// In new file: DesignerWriterUtils.java public final class DesignerWriterUtils { private static final String V8_ITEM = "v8:item"; private static final String V8_LANG = "v8:lang"; private static final String V8_CONTENT = "v8:content"; public static void writeSynonym(HierarchicalStreamWriter writer, MultiLanguageString synonym, String nodeName) { if (synonym == null || synonym.isEmpty()) { return; } writer.startNode(nodeName); for (var entry : synonym.getContent()) { writer.startNode(V8_ITEM); writeElement(writer, V8_LANG, entry.getLangKey()); writeElement(writer, V8_CONTENT, entry.getValue()); writer.endNode(); } writer.endNode(); } public static void writeElement(HierarchicalStreamWriter writer, String nodeName, String text) { if (text == null) return; writer.startNode(nodeName); writer.setValue(text); writer.endNode(); } private DesignerWriterUtils() {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java` around lines 84 - 96, The writeSynonym method is duplicated (CatalogDesignerWriteConverter.writeSynonym and SubsystemDesignerWriteConverter.writeSynonym); extract it into a shared utility (e.g., DesignerWriterUtils) that exposes writeSynonym(HierarchicalStreamWriter writer, MultiLanguageString synonym, String nodeName) and a helper writeElement(HierarchicalStreamWriter, String, String), move constants V8_ITEM, V8_LANG, V8_CONTENT into that utility, replace both converter implementations to call DesignerWriterUtils.writeSynonym(writer, synonym, SYNONYM) (or appropriate node name), and remove the duplicate private writeSynonym implementations from the converters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java`:
- Around line 84-96: The writeSynonym method is duplicated
(CatalogDesignerWriteConverter.writeSynonym and
SubsystemDesignerWriteConverter.writeSynonym); extract it into a shared utility
(e.g., DesignerWriterUtils) that exposes writeSynonym(HierarchicalStreamWriter
writer, MultiLanguageString synonym, String nodeName) and a helper
writeElement(HierarchicalStreamWriter, String, String), move constants V8_ITEM,
V8_LANG, V8_CONTENT into that utility, replace both converter implementations to
call DesignerWriterUtils.writeSynonym(writer, synonym, SYNONYM) (or appropriate
node name), and remove the duplicate private writeSynonym implementations from
the converters.
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java`:
- Around line 138-217: Add explicit jspecify nullness annotations on the helper
contracts: import org.jspecify.annotations.Nullable and
org.jspecify.annotations.NonNull, then annotate parameters/returns so the
null-tolerant methods are explicit — mark inputs that may be null as `@Nullable`
(e.g., parameters of compatibilityModeString, writeMdoRef, writeRefList,
writeLanguage, writeMultiLang, writeSynonym, writeElement, nullToEmpty) and mark
non-nullable writer and other parameters/returns as `@NonNull` (e.g.,
HierarchicalStreamWriter writer, NAME constants, and the return of
compatibilityModeString and nullToEmpty which return empty string when input is
null); apply annotations to the signatures of methods compatibilityModeString,
writeMdoRef, writeRefList, writeLanguage, writeMultiLang, writeSynonym,
writeElement, and nullToEmpty and add the necessary import lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da5accf1-824e-4d0e-b37f-c8794cf7ac05
📒 Files selected for processing (5)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.java
…х в форматах EDT и Designer - Добавлены аннотации @nonnull и @nullable для параметров методов в классе MDClasses, что улучшает обработку null-значений. - Обновлены комментарии в методах writeObject для более точного описания поддерживаемых форматов и типов объектов. - Введены изменения в класс MDCWriteSettings для поддержки null-значений в параметре кодировки. - Обновлены конвертеры для записи объектов Catalog и Configuration, добавлены новые свойства и улучшена обработка данных. - Реализована атомарная запись в формате EDT с использованием временных файлов для повышения надежности. - Добавлены новые тесты для проверки корректности записи и обработки исключений.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java (1)
65-85:⚠️ Potential issue | 🟠 MajorStill serializing template defaults instead of catalog state.
These literals will rewrite any non-default hierarchy/code/search settings back to the hard-coded values on a read-modify-write flow. Either serialize the actual
Catalogfields or fail fast when the current MVP surface cannot preserve them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java` around lines 65 - 85, CatalogEdtWriteConverter is currently writing hard-coded defaults via repeated writeElement calls (e.g., "useStandardCommands", "fullTextSearchOnInputByString", "createOnInput", etc.), which overwrites any non-default state; update the write logic to read and serialize the actual Catalog fields (use catalog.getX()/catalog.isX() or related getters such as catalog.isCheckUnique() and catalog.getCodeSeries()) instead of literals, and if the Catalog model does not expose a needed property for a field that must be preserved, fail fast by throwing a clear exception from CatalogEdtWriteConverter so callers know the operation is unsupported rather than silently writing defaults.
🧹 Nitpick comments (2)
src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java (1)
48-61: Annotate the public facade with jspecify nullability.
path/objectare rejected at runtime andwriteSettingsis intentionally nullable, but that contract is only visible onMDClasses, not on these new public entry points.As per coding guidelines, `**/*.java`: Use `@Nullable` and `@NonNull` annotations from org.jspecify for null safety.➕ Minimal signature update
+import org.jspecify.annotations.NonNull; +import org.jspecify.annotations.Nullable; @@ - public void writeObject(Path path, Object object) throws IOException { + public void writeObject(`@NonNull` Path path, `@NonNull` Object object) throws IOException { @@ - public void writeObject(Path path, Object object, MDCWriteSettings writeSettings) throws IOException { + public void writeObject(`@NonNull` Path path, `@NonNull` Object object, + `@Nullable` MDCWriteSettings writeSettings) throws IOException {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java` around lines 48 - 61, Annotate the public writeObject overloads in MDOWriter to declare nullability using org.jspecify annotations: mark Path path and Object object as `@NonNull` and mark MDCWriteSettings writeSettings as `@Nullable` (for the two-arg and three-arg writeObject methods), and add the corresponding import(s) for org.jspecify.annotations.Nullable and org.jspecify.annotations.NonNull at the top of the file; ensure the method JavaDoc remains accurate about writeSettings being nullable and that runtime checks (if any) still reject null path/object as before.src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java (1)
47-127: The new round-trip coverage is still too synthetic and shallow.These cases only exercise in-memory builders, and the read-back checks stay light enough that regressions in
Configurationfields or catalog-specific branches likesynonym,checkUnique, and non-defaultcodeSerieswould still pass. Please add at least one fixture-backed EDT round-trip and tighten the assertions on the fields this PR writes.As per coding guidelines,
src/test/java/**/*.java: Use integration tests with real 1C configuration data instead of mock objects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java` around lines 47 - 127, The tests (writeSubsystemThenReadBack, writeConfigurationThenReadBack, writeCatalogThenReadBack) only use synthetic builders and light assertions; update them to use a real EDT/MDO fixture file (place a representative 1C configuration fixture in test resources) and perform a full round-trip via MDClasses.writeObject / EDTReader.read or MDOReader.readConfiguration to validate all relevant fields (for Subsystem: name, uuid, synonym and ordering of includeHelp/includeInCommandInterface; for Catalog: name, uuid, synonym, checkUnique, codeSeries; for Configuration: name, uuid and any non-default fields). Replace or add an integration test that loads the fixture file, writes it out, reads it back using EDTReader or MDOReader.readConfiguration, and assert equality of all non-default and branch-specific properties to catch regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java`:
- Around line 61-73: MDOWriter.writeObject currently dispatches to EDTWriter or
DesignerWriter and can leak an IllegalArgumentException from EDTWriter while
DesignerWriter throws UnsupportedOperationException; update writeObject to
normalize unsupported-type handling by validating the object before dispatch or
by catching IllegalArgumentException from new
EDTWriter(writeSettings).writer.write(...) and rethrowing a consistent
UnsupportedOperationException with the same message format used for the .xml
branch; reference MDOWriter.writeObject, EDTWriter, and DesignerWriter to locate
the code and ensure all unsupported-object failures use
UnsupportedOperationException.
---
Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.java`:
- Around line 65-85: CatalogEdtWriteConverter is currently writing hard-coded
defaults via repeated writeElement calls (e.g., "useStandardCommands",
"fullTextSearchOnInputByString", "createOnInput", etc.), which overwrites any
non-default state; update the write logic to read and serialize the actual
Catalog fields (use catalog.getX()/catalog.isX() or related getters such as
catalog.isCheckUnique() and catalog.getCodeSeries()) instead of literals, and if
the Catalog model does not expose a needed property for a field that must be
preserved, fail fast by throwing a clear exception from CatalogEdtWriteConverter
so callers know the operation is unsupported rather than silently writing
defaults.
---
Nitpick comments:
In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java`:
- Around line 48-61: Annotate the public writeObject overloads in MDOWriter to
declare nullability using org.jspecify annotations: mark Path path and Object
object as `@NonNull` and mark MDCWriteSettings writeSettings as `@Nullable` (for the
two-arg and three-arg writeObject methods), and add the corresponding import(s)
for org.jspecify.annotations.Nullable and org.jspecify.annotations.NonNull at
the top of the file; ensure the method JavaDoc remains accurate about
writeSettings being nullable and that runtime checks (if any) still reject null
path/object as before.
In `@src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java`:
- Around line 47-127: The tests (writeSubsystemThenReadBack,
writeConfigurationThenReadBack, writeCatalogThenReadBack) only use synthetic
builders and light assertions; update them to use a real EDT/MDO fixture file
(place a representative 1C configuration fixture in test resources) and perform
a full round-trip via MDClasses.writeObject / EDTReader.read or
MDOReader.readConfiguration to validate all relevant fields (for Subsystem:
name, uuid, synonym and ordering of includeHelp/includeInCommandInterface; for
Catalog: name, uuid, synonym, checkUnique, codeSeries; for Configuration: name,
uuid and any non-default fields). Replace or add an integration test that loads
the fixture file, writes it out, reads it back using EDTReader or
MDOReader.readConfiguration, and assert equality of all non-default and
branch-specific properties to catch regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3969e2c-3cdb-4e82-94fc-15aa5640d8c3
📒 Files selected for processing (15)
src/main/java/com/github/_1c_syntax/bsl/mdclasses/MDCWriteSettings.javasrc/main/java/com/github/_1c_syntax/bsl/mdclasses/MDClasses.javasrc/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/ConfigurationDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/CatalogEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.javasrc/main/java/com/github/_1c_syntax/bsl/writer/package-info.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.javasrc/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterEdtTest.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/main/java/com/github/_1c_syntax/bsl/writer/designer/CatalogDesignerWriteConverter.java
- src/main/java/com/github/_1c_syntax/bsl/writer/edt/SubsystemEdtWriteConverter.java
- src/main/java/com/github/_1c_syntax/bsl/writer/edt/ConfigurationEdtWriteConverter.java
- src/test/java/com/github/_1c_syntax/bsl/writer/MDOWriterDesignerTest.java
- src/main/java/com/github/_1c_syntax/bsl/writer/designer/SubsystemDesignerWriteConverter.java
- src/main/java/com/github/_1c_syntax/bsl/writer/ReadWriteDemo.java
- src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java
Comment on lines
+61
to
+73
| public void writeObject(Path path, Object object, MDCWriteSettings writeSettings) throws IOException { | ||
| if (path == null || object == null) { | ||
| throw new IllegalArgumentException("path and object must not be null"); | ||
| } | ||
| if (FilenameUtils.isExtension(path.toString(), "mdo")) { | ||
| var writer = new EDTWriter(writeSettings); | ||
| writer.write(path, object); | ||
| } else if (FilenameUtils.isExtension(path.toString(), "xml")) { | ||
| var writer = new DesignerWriter(writeSettings); | ||
| writer.write(path, object); | ||
| } else { | ||
| throw new UnsupportedOperationException("Write is supported only for EDT (.mdo) or Designer (.xml) format, got: " + path); | ||
| } |
There was a problem hiding this comment.
Normalize unsupported-type handling before dispatch.
The facade documents one failure mode, but the .mdo branch can currently leak IllegalArgumentException from src/main/java/com/github/_1c_syntax/bsl/writer/edt/EDTWriter.java:77-80, while the .xml branch uses UnsupportedOperationException via src/main/java/com/github/_1c_syntax/bsl/writer/designer/DesignerWriter.java:67-70. The same unsupported object should not fail differently based only on the extension.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/github/_1c_syntax/bsl/writer/MDOWriter.java` around lines
61 - 73, MDOWriter.writeObject currently dispatches to EDTWriter or
DesignerWriter and can leak an IllegalArgumentException from EDTWriter while
DesignerWriter throws UnsupportedOperationException; update writeObject to
normalize unsupported-type handling by validating the object before dispatch or
by catching IllegalArgumentException from new
EDTWriter(writeSettings).writer.write(...) and rethrowing a consistent
UnsupportedOperationException with the same message format used for the .xml
branch; reference MDOWriter.writeObject, EDTWriter, and DesignerWriter to locate
the code and ensure all unsupported-object failures use
UnsupportedOperationException.
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.
Описание
Реализация API записи объектов метаданных: сериализация объектов MD в файлы форматов EDT (.mdo) и Конфигуратор (Designer .xml).
MDClasses.writeObject(path, object); формат по расширению пути (.mdo→ EDT,.xml→ Designer).src/...(Subsystems/Имя/Имя.mdo, Catalogs/Имя/Имя.mdo, Configuration/Configuration.mdo).Поддерживаемые типы: Subsystem, Catalog, Configuration в обоих форматах.
Связанные задачи
Closes #158
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
New Features
Tests
Chores