Добавлена обработка ошибки чтения содержимого прав роли #564
Conversation
📝 WalkthroughWalkthroughTwo RoleConverter classes add exception handling during XML deserialization. When ExtendXStream.read fails, both converters now catch the exception and default to RoleData.EMPTY rather than propagating the error, allowing processing to continue gracefully. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
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
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: 3
🤖 Fix all issues with AI agents
In
@src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java:
- Around line 42-50: The catch block in RoleConverter (where RoleData is loaded
via ExtendXStream.read) currently swallows all exceptions and replaces them with
RoleData.EMPTY; change this to log the failure before falling back: catch the
Exception e, call the class logger (e.g., RoleConverter's logger) to emit an
error or warn message that includes the exception e and contextual info such as
readerContext.getCurrentPath(), readerContext.getName() or the dataPath(...)
used for reading, then assign RoleData.EMPTY and continue; ensure the log
message clearly states that role data failed to load and includes the exception
stack/message and the path/name so operators can investigate.
In
@src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java:
- Around line 42-47: Replace the broad catch in RoleConverter around
ExtendXStream.read(...) with a narrower catch for ConversionException and log
the failure before falling back to RoleData.EMPTY; add or reuse a logger (e.g.,
a private static final Logger) in RoleConverter, call logger.warn(...) or
logger.error(...) including a clear message with readerContext.getCurrentPath()
and the exception stack/message, and only swallow the ConversionException while
allowing other unexpected exceptions to propagate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.javasrc/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, ubuntu-latest)
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: QA
- GitHub Check: build (25, macOS-latest)
- GitHub Check: build (25, windows-latest)
- GitHub Check: build (17, macOS-latest)
- GitHub Check: build (17, windows-latest)
- GitHub Check: build (21, windows-latest)
- GitHub Check: build (21, macOS-latest)
- GitHub Check: build (17, ubuntu-latest)
- GitHub Check: build (25, ubuntu-latest)
- GitHub Check: build (21, ubuntu-latest)
🔇 Additional comments (1)
src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java (1)
41-49: Verify security implications of silently skipping role data.Identical concern to the designer converter: confirm that
RoleData.EMPTYis a safe authorization default and that administrators can detect failed role configurations.The verification script from the designer converter review will check both implementations.
| RoleData data; | ||
| try { | ||
| data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath(), readerContext.getName())); | ||
| } catch (Exception e) { | ||
| // ничего не делаем, считаем файл битым | ||
| data = RoleData.EMPTY; | ||
| } | ||
|
|
||
| readerContext.setValue(DATA_FIELD, data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the RoleData class definition
find . -type f -name "*.java" | xargs grep -l "class RoleData" | head -5Repository: 1c-syntax/mdclasses
Length of output: 223
🏁 Script executed:
#!/bin/bash
# Search for RoleData.EMPTY usage
rg "RoleData\.EMPTY" -A 3 -B 3Repository: 1c-syntax/mdclasses
Length of output: 5143
🏁 Script executed:
#!/bin/bash
# Find the RoleData class file
find . -type f -name "RoleData.java"Repository: 1c-syntax/mdclasses
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Examine the RoleData class definition
cat -n src/main/java/com/github/_1c_syntax/bsl/mdo/storage/RoleData.javaRepository: 1c-syntax/mdclasses
Length of output: 2638
🏁 Script executed:
#!/bin/bash
# Check the RoleConverter file to see if there's any logging
cat -n src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java | head -100Repository: 1c-syntax/mdclasses
Length of output: 2702
Add logging for silently caught role data loading failures.
RoleData.EMPTY is a safe default—it provides no permissions (all flags false, empty rights list), and downstream code in Rights.java explicitly filters out empty role data to prevent unintended permission grants.
However, the silent exception handling lacks any logging or alerting mechanism. When role configuration fails to load, administrators have no visibility into the failure, only a comment stating the file is "considered corrupted." For security-sensitive code, silent failures should be logged to inform operators that role configurations could not be loaded.
🤖 Prompt for AI Agents
In
@src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java
around lines 42 - 50, The catch block in RoleConverter (where RoleData is loaded
via ExtendXStream.read) currently swallows all exceptions and replaces them with
RoleData.EMPTY; change this to log the failure before falling back: catch the
Exception e, call the class logger (e.g., RoleConverter's logger) to emit an
error or warn message that includes the exception e and contextual info such as
readerContext.getCurrentPath(), readerContext.getName() or the dataPath(...)
used for reading, then assign RoleData.EMPTY and continue; ensure the log
message clearly states that role data failed to load and includes the exception
stack/message and the path/name so operators can investigate.
| try { | ||
| data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath(), readerContext.getName())); | ||
| } catch (Exception e) { | ||
| // ничего не делаем, считаем файл битым | ||
| data = RoleData.EMPTY; | ||
| } |
There was a problem hiding this comment.
Add logging and narrow the exception type.
The current implementation catches all exceptions silently, which creates observability and debugging challenges:
- Too broad: Catching
Exceptionwill swallow unexpected errors beyond theConversionExceptionmentioned in issue ConversionException: #563 (e.g.,IOException,NullPointerExceptionfrom other causes). - No observability: Silent failures make it difficult to detect broken role configurations in production. Operators won't know that role permissions failed to load, which could have security implications.
Consider logging the error at WARN or ERROR level and narrowing the catch to expected exception types.
📋 Suggested improvement
+import com.thoughtworks.xstream.converters.ConversionException;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
@DesignerConverter
public class RoleConverter extends AbstractReadConverter {
// ...
RoleData data;
try {
data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath(), readerContext.getName()));
- } catch (Exception e) {
- // ничего не делаем, считаем файл битым
+ } catch (ConversionException e) {
+ var rightsPath = dataPath(readerContext.getCurrentPath(), readerContext.getName());
+ log.warn("Can't read role rights file '{}' - it's broken (skipped)", rightsPath, e);
data = RoleData.EMPTY;
}| try { | ||
| data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath())); | ||
| } catch (Exception e) { | ||
| // ничего не делаем, считаем файл битым | ||
| data = RoleData.EMPTY; | ||
| } |
There was a problem hiding this comment.
Add logging and narrow the exception type.
Same concern as in the designer converter: catching all exceptions silently creates observability gaps and could hide unexpected errors beyond the ConversionException from issue #563.
Consider logging at WARN/ERROR level and narrowing to ConversionException.
📋 Suggested improvement
+import com.thoughtworks.xstream.converters.ConversionException;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
@EDTConverter
public class RoleConverter extends AbstractReadConverter {
// ...
RoleData data;
try {
data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath()));
- } catch (Exception e) {
- // ничего не делаем, считаем файл битым
+ } catch (ConversionException e) {
+ var rightsPath = dataPath(readerContext.getCurrentPath());
+ log.warn("Can't read role rights file '{}' - it's broken (skipped)", rightsPath, e);
data = RoleData.EMPTY;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
around lines 42 - 47, Replace the broad catch in RoleConverter around
ExtendXStream.read(...) with a narrower catch for ConversionException and log
the failure before falling back to RoleData.EMPTY; add or reuse a logger (e.g.,
a private static final Logger) in RoleConverter, call logger.warn(...) or
logger.error(...) including a clear message with readerContext.getCurrentPath()
and the exception stack/message, and only swallow the ConversionException while
allowing other unexpected exceptions to propagate.
|
Описание
Связанные задачи
Closes #563
Чеклист
Общие
gradlew precommit)Дополнительно
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.