fix: Улучшена обработка ошибок в RoleConverter и ExtendXStream #568
Conversation
- Добавлен блок catch для неожиданных исключений в ExtendXStream, регистрирующий сообщение об ошибке. - Обновлен RoleConverter для обработки результата ExtendXStream.read, гарантирующий присвоение значения RoleData.EMPTY, если результат чтения не является допустимым экземпляром RoleData.
📝 WalkthroughWalkthroughModify XML deserialization error handling in ExtendXStream and make RoleConverter implementations tolerate non-RoleData results by using runtime type checks, storing fallback RoleData.EMPTY into the reader context, and adding canConvert and dataPath helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant RoleConv as RoleConverter
participant XStream as ExtendXStream
participant Conv as RoleDataConverter
participant Context as ReaderContext
participant Logger as Logger
RoleConv->>XStream: read(Rights.xml)
XStream->>Conv: fromXML(...)
alt successful parse returns RoleData
Conv-->>XStream: RoleData
XStream-->>RoleConv: RoleData
RoleConv->>Context: setValue(DATA_FIELD, RoleData)
else parse throws Conversion/Stream/other Exception
Conv--x XStream: throws Exception
XStream->>Logger: warn(...) / unexpected error warn
XStream-->>RoleConv: null or skipped
RoleConv->>Context: setValue(DATA_FIELD, RoleData.EMPTY)
end
RoleConv-->>RoleConv: return reader context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
…ием уровня WARN - Изменено логирование ошибок с ERROR на WARN для некорректного чтения файлов и непредвиденных ошибок.
…ния понимания кода - Добавлены Javadoc комментарии для методов и класса, описывающие их функциональность и параметры. - Обновлены пути к файлам прав доступа.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/github/_1c_syntax/bsl/reader/common/xstream/ExtendXStream.javasrc/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.javasrc/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/github/_1c_syntax/bsl/reader/common/xstream/ExtendXStream.java
- src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java (3)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/TemplateConverter.java (1)
DesignerConverter(40-98)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/XDTOPackageConverter.java (1)
DesignerConverter(34-63)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/ExchangePlanConverter.java (1)
DesignerConverter(33-54)
⏰ 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). (8)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (25, macOS-latest)
🔇 Additional comments (2)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java (2)
34-48: LGTM! Well-documented converter.The added JavaDoc clearly describes the converter's purpose and the unmarshal method's behavior, including the handling of Rights.xml files. The documentation style is consistent with the codebase.
70-90: LGTM! Clear helper methods with proper documentation.The
canConvertmethod correctly identifies Role types, and thedataPathhelper properly constructs the path to Rights.xml. The JavaDoc clearly documents the behavior and parameters.
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java
Show resolved
Hide resolved
johnnyshut
force-pushed
the
fix/broken-roles
branch
from
b09c113 to
96a415f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java (1)
33-35: Documentation improvements enhance code maintainability.The added Javadoc comments clearly document the purpose and behavior of the converter class and its methods, making the code easier to understand and maintain.
Also applies to: 41-47, 69-74, 80-85
📜 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/reader/common/xstream/ExtendXStream.javasrc/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.javasrc/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java (3)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/TemplateConverter.java (1)
DesignerConverter(40-98)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/XDTOPackageConverter.java (1)
DesignerConverter(34-63)src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/ExchangePlanConverter.java (1)
DesignerConverter(33-54)
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java (1)
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/XDTOPackageConverter.java (1)
EDTConverter(33-61)
⏰ 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). (9)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (17, ubuntu-latest)
🔇 Additional comments (6)
src/main/java/com/github/_1c_syntax/bsl/reader/common/xstream/ExtendXStream.java (2)
125-130: Appropriate log level changes for corrupted file scenarios.Changing from
errortowarnforConversionExceptionandStreamExceptionis correct since corrupted files are an expected scenario that should be logged but not treated as critical errors.However, note that line 130 simplified the message format. Both catch blocks log the exception object (third parameter) but the messages are slightly inconsistent in their phrasing.
131-133: Verify that catch-all exception handling won't mask development errors.The broad
Exceptioncatch provides resilience against unexpected errors during XML deserialization, which aligns with the PR's goal of gracefully handling corrupted files. However, this could potentially mask genuine programming errors (NPE, IllegalArgumentException, etc.) during development.Consider whether this catch-all should:
- Only apply in production mode, or
- Be more selective about which exception types are treated as "corrupted file" vs "programming error"
The full exception is logged, which helps debugging, but developers might not notice warnings as readily as errors during testing.
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java (3)
53-59: Type-safe read with appropriate fallback.The instanceof check with pattern matching prevents ClassCastException and handles cases where the XML file is corrupted or returns an unexpected type. The fallback to
RoleData.EMPTYallows processing to continue gracefully, which directly addresses issue #563.
65-66: Essential fix: Storing data in reader context.The addition of
readerContext.setValue(DATA_FIELD, data)before returning is crucial. This ensures the parsed role data is properly stored in the reader context and accessible to downstream processing. Based on the AI summary indicating this "replaces the previous flow that returned without setting the context value," this appears to fix a bug where role data was being read but not stored.This pattern is consistent with similar converters like
XDTOPackageConverter.
60-63: LGTM: Exception handling for corrupted files.The catch block appropriately handles any exceptions during file reading by falling back to
RoleData.EMPTY, allowing the processing pipeline to continue despite corrupted role data files. This aligns with the PR objective of graceful degradation when encountering invalid XML.src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java (1)
34-36: Consistent implementation mirrors EDT converter.The changes to the designer format
RoleConverterappropriately mirror the fixes applied to the EDT version:
- Added comprehensive Javadoc documentation
- Type-safe instanceof check prevents ClassCastException
- Fallback to
RoleData.EMPTYfor corrupted files- Proper storage of data in reader context via
setValueThe only format-specific difference is the file path (
Rights.xmlfor designer format vsRights.rightsfor EDT format), which is correct.Also applies to: 42-48, 54-60, 66-67, 70-75, 81-87
2 participants
Описание
Связанные задачи
Closes #563
Чеклист
Общие
gradlew precommit)Summary by CodeRabbit
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.