Skip to content

Добавлена обработка ошибки чтения содержимого прав роли #564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
theshadowco merged 1 commit into from
Jan 7, 2026
Merged

Добавлена обработка ошибки чтения содержимого прав роли #564

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/main/java/com/github/_1c_syntax/bsl/reader/designer/converter/RoleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package com.github._1c_syntax.bsl.reader.designer.converter;

import com.github._1c_syntax.bsl.mdo.Role;
import com.github._1c_syntax.bsl.mdo.storage.RoleData;
import com.github._1c_syntax.bsl.reader.common.converter.AbstractReadConverter;
import com.github._1c_syntax.bsl.reader.common.xstream.ExtendXStream;
import com.thoughtworks.xstream.converters.UnmarshallingContext;
Expand All @@ -38,8 +39,15 @@ public class RoleConverter extends AbstractReadConverter {
@Override
public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
var readerContext = super.read(reader, context);
readerContext.setValue(DATA_FIELD,
ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath(), readerContext.getName())));
RoleData data;
try {
data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath(), readerContext.getName()));
} catch (Exception e) {
// ничего не делаем, считаем файл битым
data = RoleData.EMPTY;
}
Comment on lines +43 to +48
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add logging and narrow the exception type.

The current implementation catches all exceptions silently, which creates observability and debugging challenges:

  1. Too broad: Catching Exception will swallow unexpected errors beyond the ConversionException mentioned in issue ConversionException:  #563 (e.g., IOException, NullPointerException from other causes).
  2. 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;
   }


readerContext.setValue(DATA_FIELD, data);
Comment on lines +42 to +50
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# First, find the RoleData class definition
find . -type f -name "*.java" | xargs grep -l "class RoleData" | head -5

Repository: 1c-syntax/mdclasses

Length of output: 223

🏁 Script executed:

#!/bin/bash
# Search for RoleData.EMPTY usage
rg "RoleData\.EMPTY" -A 3 -B 3

Repository: 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.java

Repository: 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 -100

Repository: 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.

return readerContext.build();
}

Expand Down
11 changes: 10 additions & 1 deletion src/main/java/com/github/_1c_syntax/bsl/reader/edt/converter/RoleConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
package com.github._1c_syntax.bsl.reader.edt.converter;

import com.github._1c_syntax.bsl.mdo.Role;
import com.github._1c_syntax.bsl.mdo.storage.RoleData;
import com.github._1c_syntax.bsl.reader.common.converter.AbstractReadConverter;
import com.github._1c_syntax.bsl.reader.common.xstream.ExtendXStream;
import com.thoughtworks.xstream.converters.UnmarshallingContext;
Expand All @@ -37,7 +38,15 @@ public class RoleConverter extends AbstractReadConverter {
@Override
public Object unmarshal(HierarchicalStreamReader reader, UnmarshallingContext context) {
var readerContext = super.read(reader, context);
readerContext.setValue(DATA_FIELD, ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath())));
RoleData data;
try {
data = (RoleData) ExtendXStream.read(reader, dataPath(readerContext.getCurrentPath()));
} catch (Exception e) {
// ничего не делаем, считаем файл битым
data = RoleData.EMPTY;
}
Comment on lines +42 to +47
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


readerContext.setValue(DATA_FIELD, data);
return readerContext.build();
}

Expand Down
Loading