Обновление зависимостей и собранные изменения с разных репозиториев #6
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the documentation generation system from a Velocity-based templating approach to Handlebars, upgrades the Gradle build system to Java 17 with git-versioning support, introduces a new component-based rendering pipeline (AutodocManager, StructureBuilder, StructureRender), adds comprehensive filtering and content management, and includes test configuration resources for the 1C:Enterprise platform. Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
theshadowco
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In `@build.gradle.kts`:
- Line 32: Update the JACKSON_VERSION constant from "2.15.2" to "2.19.0" and
update all dependency references that use JACKSON_VERSION (the dependency
declarations that currently pull jackson-databind / jackson-module-kotlin /
jackson-annotations or similar usages) so they resolve to 2.19.0; leave Commons
IO (commons-io) unchanged. Ensure the JACKSON_VERSION symbol is the single
source-of-truth so all jackson artifacts pick up 2.19.0.
In `@src/main/java/ru/alkoleft/bsl/doc/AutodocManager.java`:
- Around line 72-79: The clearOutput method uses reference comparison (it !=
destination) when filtering Paths, which can incorrectly include the destination
path; change the filter to use value equality (it.equals(destination) or
!it.equals(destination)) so the destination Path is reliably excluded, keeping
the rest of the logic
(Files.walk(destination).filter(...).forEach(this::deletePath)) and leaving
deletePath and destination unchanged.
- Around line 72-79: The clearOutput method can attempt to delete parent
directories before children because Files.walk yields depth-first but the stream
may process in forward order; change the deletion stream in clearOutput to
traverse paths sorted in reverse order (largest/deepest paths first) before
calling deletePath (or replace the walk+delete loop with a recursive utility
such as PathUtils.deleteDirectory); specifically update the
Files.walk(destination).filter(...).forEach(this::deletePath) usage in
AutodocManager.clearOutput to collect/sort paths in reverse
(Comparator.reverseOrder()) or call an existing recursive delete helper so
children are removed before their parents.
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbol.java`:
- Around line 42-75: The createTypeDescription path can loop if hyperlinks form
cycles; modify MethodSymbol.getReturnedValue to call an overloaded
createTypeDescription that accepts a Set<String> (or Set<URI>) of visited links
initialized empty, and change/rescue createTypeDescription to take this visited
set, record hyperlinkTypeDescription.hyperlink().link() before resolving, and if
the link is already in the visited set skip resolution (warn and return
baseDescription) to break cycles; ensure any recursive call to
methodInfo.getMethod().getReturnedValue() uses the same visited set (or
propagates via the new overload) so cycles are detected across resolutions.
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbolComputer.java`:
- Around line 111-125: In compute(BSLTokenizer tokenizer) clear the per-instance
region state before use by resetting regionData (and regions if present) and
methods to empty collections so repeated calls don't leak prior regions; then
keep the post-binding loop but skip any region with a null region.range (guard
with an if (region.range == null) continue) before calling
SimpleRange.containsRange and method.setRegion to avoid NPE on unterminated
regions; references: MethodSymbolComputer.compute, regionData, regions,
visitFile, region.range, SimpleRange.containsRange, method.setRegion.
In `@src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommand.java`:
- Around line 101-107: RenderCommand builds a Filter from commandOptions but
calls commandOptions.getSubsystems().forEach(...) which can NPE because
subsystems may be null; update the code to guard against null by either
initializing subsystems to an empty list in options() or by checking
commandOptions.getSubsystems() != null before iterating (e.g., only call forEach
when non-null), ensuring you reference the same methods: options(),
commandOptions.getSubsystems(), and Filter.builder().rootSubsystem(...) so
subsystems are handled as an empty collection or skipped when null.
In `@src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommandOptions.java`:
- Around line 29-33: RenderCommandOptions.load currently uses a plain
ObjectMapper so Jackson will fail deserializing java.nio.file.Path fields (e.g.,
PageBlock.path and the manualDocs field) at runtime; fix by either changing
those fields to String and converting to Path later, or register a Path
deserializer on the ObjectMapper before calling readValue: create a SimpleModule
that adds a StdDeserializer<Path> (or reuse an existing Path deserializer) and
call mapper.registerModule(...) in RenderCommandOptions.load so PageBlock.path
and manualDocs are deserialized correctly.
In `@src/main/java/ru/alkoleft/bsl/doc/content/processor/TitleProcessor.java`:
- Around line 8-11: TitleProcessor.getInstance() can return null if
Factory.instance wasn't set by Factory.create(); update getInstance() to guard
against that by checking Factory.instance and throwing an IllegalStateException
with a clear message (e.g. "TitleProcessor not initialized; call
Factory.create() before getInstance()") or alternatively perform lazy
initialization by calling Factory.create() when instance is null; reference
TitleProcessor.getInstance(), Factory.instance and Factory.create() when making
the change.
In `@src/main/java/ru/alkoleft/bsl/doc/indexer/FileIndexer.java`:
- Around line 15-21: The pagePaths method currently returns an unclosed Stream
from Files.walk which leaks directory handles; change pagePaths to return a
List<Path> instead and use a try-with-resources to close the Files.walk stream:
inside FileIndexer.pagePaths(Path rootPath) open Stream<Path> walk =
Files.walk(rootPath) in a try(...) block, filter with Files::isRegularFile and
this::matchFileExtension, then collect to a List (Collectors.toList()) and
return that list; update the method signature accordingly and remove reliance on
path.toFile().isFile().
In `@src/main/java/ru/alkoleft/bsl/doc/model/Links.java`:
- Around line 10-18: The getPageLink(Page from, Page to) and getPageLink(Page
to) methods currently call Page.getPath() and Path.relativize() without guards;
add null checks and cross-root handling: validate that from, to and their
Page.getPath() results are non-null (and currentPath is non-null for the
single-arg overload) and return a safe fallback (e.g., empty string or
to.getPath().toString()) when nulls are encountered; before calling
Path.relativize(), detect differing roots (compare Path.getRoot() or use
Path.isAbsolute() logic) and if roots differ return an absolute or canonical
string of the target instead of relativizing to avoid IllegalArgumentException;
update getPageLink(Page from, Page to), getPageLink(Page to), currentPath, and
usages of Page.getPath() accordingly.
In `@src/main/java/ru/alkoleft/bsl/doc/render/BaseRender.java`:
- Around line 10-25: The static mutable renderContext in BaseRender creates
thread-safety and NPE risks; update BaseRender so renderToFile and render
defensively validate the context (e.g., use
Objects.requireNonNull(BaseRender.renderContext, "renderContext not
initialized") at start of renderToFile and render) and either guard
setContext/reads with a thread-safe holder (AtomicReference<RenderContext>
renderContext) or synchronize access in setContext/get paths (or prefer removing
static state and accept a RenderContext parameter on renderToFile/render);
change references to the field and the setter (renderContext, setContext,
renderToFile, render) accordingly to ensure no unsynchronized null reads occur.
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/Links.java`:
- Around line 21-24: The apply method in Links currently returns the input
unchanged, so implement link processing using the class's LINK_PATTERN and
WARNING_PATTERN: in Links.apply(String context, Options options) parse the
context string, find matches of LINK_PATTERN and replace them with the desired
HTML/handlebars output (and handle WARNING_PATTERN similarly), preserving
original text when no match is found; use the existing LINK_PATTERN and
WARNING_PATTERN fields and the Options parameter for escaping/triple-stash logic
as needed, and ensure the method returns the transformed string instead of the
raw context.
- Around line 26-45: The two private methods replaceTo(...) and
handleWarning(...) and the unused context field are dead code; either remove
them (and the `@SneakyThrows` on replaceTo) or wire them into apply() so they are
used: replace the inline link transformation logic in apply() to call
replaceTo(baseValue, link, options) (so methodInfo lookup and context usage
comes from BslContext.getCurrent() inside replaceTo) and run the final rendered
string through handleWarning(result) before returning; if you choose deletion,
also remove the unused context field and any imports related to these helpers.
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.java`:
- Around line 84-89: The renderToFile method currently opens the output with
FileWriter (in RenderContext.renderToFile), which uses the platform default
encoding; change it to create the writer with an explicit charset from the
Handlebars configuration (e.g., use the Handlebars charset or a Charset field on
RenderContext) and pass that writer to template.apply(context, writer) instead
of FileWriter so output encoding is consistent for non-ASCII documents.
- Around line 40-48: The constructor RenderContext(TemplatesDefinition
templatesDefinition) can NPE when baseURL is missing and builds URLs incorrectly
by string concatenation; update the constructor to requireNonNull on the result
of getClass().getClassLoader().getResource(templatesDefinition.path())
(assigning to baseURL) to fail fast, and change the URL creation inside the
anonymous URLTemplateLoader.getResource to use the URL(URL base, String spec)
constructor (new URL(baseURL, location)) instead of concatenating strings so
relative template paths resolve correctly.
In `@src/main/java/ru/alkoleft/bsl/doc/render/output/MergeStrategy.java`:
- Around line 25-27: The merge currently discards the merge marker captured as
parts.group(2) in MergeStrategy, causing the marker to be lost after the first
save; update the merge branch where parts.find() is true to include the marker
when composing the result (use parts.group(1), parts.group(2), content,
parts.group(3)) and then call super.save(itemPath, result) so future merges
still find the marker; locate this logic around the parts matcher usage in the
save method of MergeStrategy.
In `@src/main/java/ru/alkoleft/bsl/doc/render/output/Writer.java`:
- Around line 7-13: The Writer class currently has an empty save(Path path,
String content) and an unused OutputFormat format field; implement save to
actually persist content (e.g., write String bytes using Files.write or
BufferedWriter with StandardCharsets.UTF_8 and create parent directories as
needed) and either initialize/provide access to format (add a constructor
Writer(OutputFormat format) and/or a setter getFormat()/setFormat()) or remove
the field if not used; alternatively, if this class is intentionally
unsupported, make save throw new UnsupportedOperationException with a clear
message or add a TODO comment—target the Writer class, its save(Path,String)
method, and the OutputFormat format field when making the changes.
In `@src/main/java/ru/alkoleft/bsl/doc/render/PathResolver.java`:
- Around line 10-16: Replace the incorrect `@AllArgsConstructor` on class
PathResolver with a custom constructor that accepts only the externally-provided
fields destination and format; initialize the internal mutable fields paths (new
Stack<>()) and currentPath (new Lazy<>(this::computeCurrentPath)) inside that
constructor so callers cannot supply or override them. Update any usages that
relied on the generated four-arg constructor to use the two-arg constructor and
ensure computeCurrentPath remains referenced for the Lazy initialization. Ensure
imports/annotations are adjusted (remove `@AllArgsConstructor`) and visibility of
fields remains the same.
🟡 Minor comments (18)
build.gradle.kts-56-56 (1)
56-56: Use SLF4J 2.0.17 (stable) instead of 2.1.0-alpha1.The 2.1 series is experimental and not recommended for production use. SLF4J 2.0.17 (latest stable, released Feb 25, 2025) provides Java 17 compatibility and avoids the risks of alpha releases.
src/test/resources/configuration/src/Catalogs/Справочник2/ObjectModule.bsl-6-8 (1)
6-8: Parameter documentation format appears malformed.The parameter documentation on lines 7-8 is missing the standard format with type and description separators. Standard 1C documentation format is:
// П1 - <Тип> - <Описание> // П2 - <Тип> - <Описание>If this is intentional for testing edge cases in the documentation parser, consider adding a comment to clarify.
📝 Suggested fix (if standard format is intended)
// Параметры: -// П1 П1 -// П2 П2 +// П1 - Произвольный - Описание параметра П1 +// П2 - Произвольный - Описание параметра П2src/main/java/ru/alkoleft/bsl/doc/render/PathResolver.java-23-26 (1)
23-26:exit()can throwEmptyStackExceptionif called without matchingentrance().Consider adding a guard or documenting the precondition.
Proposed defensive fix
public void exit() { + if (paths.isEmpty()) { + throw new IllegalStateException("exit() called without matching entrance()"); + } paths.pop(); currentPath.clear(); }src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/SingleLine.java-14-19 (1)
14-19: Potential NullPointerException when context is null.If
contextis null andtagTypeis notSECTION, callingcontext.toString()on line 18 will throw NPE.Proposed fix
String content; if (options.tagType == TagType.SECTION) { content = options.fn().toString(); } else { - content = context.toString(); + content = context != null ? context.toString() : ""; }src/main/resources/md/subsystem.hbs-9-10 (1)
9-10: Fix whitespace in block parameter syntax.The block parameter syntax
as | item|has unexpected whitespace. Standard Handlebars syntax isas |item|without extra spaces inside the pipes.Proposed fix
-{{`#each` childrenPages as | item|}} +{{`#each` childrenPages as |item|}}src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.java-18-30 (1)
18-30: Hardcoded/tmppath is not portable and lacks cleanup.The path
/tmp/bsl-doc-fixtureis Unix-specific and will fail on Windows. Consider using@TempDiras inrunFromFile()for platform independence and automatic cleanup.Suggested fix
`@Test` - void run() { + void run(`@TempDir` Path tempDir) { RenderCommand.builder() .sources(getResource("configuration")) - .destination(Path.of("/tmp/bsl-doc-fixture")) + .destination(tempDir.resolve("bsl-doc-fixture")) .format(OutputFormat.ConfluenceMarkdown)src/main/java/ru/alkoleft/bsl/doc/bsl/Filter.java-13-13 (1)
13-13: Add@Builder.Defaultannotation to explicitly preserve the field initializer.Lombok's
@Builderby default ignores field initializers and uses Java defaults instead. Whileboolean onlyCommonAndManagerModules = falseaccidentally works (since the Java default forbooleanisfalse), this violates Lombok best practices and is fragile. Without@Builder.Default, the intent is implicit and brittle—if the default changes totruelater, the builder won't honor it. Add@Builder.Defaulton the field to make the default explicit:`@Builder.Default` boolean onlyCommonAndManagerModules = false;This ensures
Filter.builder().build()respects the initializer across all primitive defaults and future changes.src/main/java/ru/alkoleft/bsl/doc/content/processor/MarkdownTitleProcessor.java-22-32 (1)
22-32: Inconsistent return values and swallowed exception.The method returns
nullwhen no title line is found (line 27) but returns""onIOException(line 31). This inconsistency makes it difficult for callers to distinguish between "no title found" and "read error". Additionally, the exception is silently swallowed.Suggested fix for consistency
`@Override` public String getTitle(Path path) { try (var lines = Files.lines(path)) { return lines .filter(it -> it.startsWith("# ")) .findFirst() .map(it -> it.substring(2).trim()).orElse(null); } catch (IOException e) { - // nothing + // Return null for consistency with "no title found" case } - return ""; + return null; }src/main/java/ru/alkoleft/bsl/doc/content/processor/MarkdownTitleProcessor.java-16-19 (1)
16-19: Stub method returnsnull- potential NPE risk for callers.
getTitle(String content)always returnsnull. If callers don't handle this, it may causeNullPointerException. Consider implementing the method or throwingUnsupportedOperationExceptionif not intended to be used.Option 1: Implement the method
`@Override` public String getTitle(String content) { - return null; + return content.lines() + .filter(it -> it.startsWith("# ")) + .findFirst() + .map(it -> it.substring(2).trim()) + .orElse(null); }src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/MdoPresent.java-11-18 (1)
11-18: Add null check for context parameter.If the template passes a null value,
context.toString()on line 17 will throw NPE.Proposed fix
`@Override` public Object apply(Object context, Options options) { + if (context == null) { + return ""; + } if (context instanceof MD) { return MDOHelper.getPresent((MD) context); } else if (context instanceof SubsystemContext) { return MDOHelper.getPresent(((SubsystemContext) context).getSubsystem()); } else { return context.toString(); } }src/main/java/ru/alkoleft/bsl/doc/model/ContentModel.java-34-41 (1)
34-41: Potential NPE when pagePath is at root level.If
pagePath.getParent()returns null (file at root), the call togetParent().getParent()on line 37 will throw NPE.Proposed defensive fix
boolean isChild(Page page, Path path) { var pagePath = page.getPath(); + var parent = pagePath.getParent(); + if (parent == null) { + return false; + } if (pagePath.getFileName().toString().startsWith("index.")) { - return pagePath.getParent().getParent().equals(path); + var grandParent = parent.getParent(); + return grandParent != null && grandParent.equals(path); } else { - return pagePath.getParent().equals(path); + return parent.equals(path); } }src/main/java/ru/alkoleft/bsl/doc/bsl/ModuleInfo.java-32-34 (1)
32-34: Potential NPE in getMethod when methods is null.
isNotEmpty()andisEmpty()handlemethods == null, butgetMethod()callsgetMethods().stream()directly, which will throw NPE ifmethodsis null.Suggested fix
public Optional<MethodSymbol> getMethod(String name) { + if (methods == null) { + return Optional.empty(); + } return getMethods().stream().filter(it -> name.equalsIgnoreCase(it.getName())).findAny(); }src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/ModuleComputer.java-30-40 (1)
30-40: Potential IndexOutOfBoundsException if filtered lines list is empty.After filtering on line 34, if all lines match the
/+regex pattern,linescould become empty. Line 37'slines.get(0)would then throwIndexOutOfBoundsException.Suggested fix
if (isDescription) { var lines = comments.stream() .map(Token::getText) .map(it -> it.substring(2)) .filter(it -> it.isEmpty() || !it.matches("/+")) .collect(Collectors.toList()); + if (lines.isEmpty()) { + return ""; + } + var chars = lines.get(0).toCharArray();src/main/java/ru/alkoleft/bsl/doc/bsl/ModuleInfo.java-20-22 (1)
20-22: Add@NonNullannotation to theownerfield to document and enforce null-safety.The
getName()method callsowner.getName()without a null check. While current usages always setownervia the builder, the field lacks a null-safety annotation, which could allow null values if the builder is misused or the class is modified later. Adding@NonNullannotation to theownerfield will document the requirement and enable compile-time or runtime validation through Lombok.src/main/resources/confluence-md/module.hbs-17-17 (1)
17-17: Remove spacing in Handlebars block parameter syntax.The block parameter syntax
as | parameter|uses non-standard spacing. Handlebars canonical syntax requires pipes immediately surrounding the identifier with no extra spaces:as |parameter|.Suggested fix
-{{`#each` parameters as | parameter|}} +{{`#each` parameters as |parameter|}}src/main/java/ru/alkoleft/bsl/doc/structure/StructureBuilder.java-12-17 (1)
12-17: Guard against nulloptionsin the factory.This avoids an unclear NPE when the factory is called with a null options reference.
🛠️ Proposed fix
static StructureBuilder builder(OutputOptions options) { - if (Objects.requireNonNull(options.getHierarchy()) == OutputHierarchy.SUBSYSTEM) { + Objects.requireNonNull(options, "options"); + var hierarchy = Objects.requireNonNull(options.getHierarchy(), "options.hierarchy"); + if (hierarchy == OutputHierarchy.SUBSYSTEM) { return new SubsystemsStructureBuilder(); } return new FlatStructureBuilder(); }src/main/java/ru/alkoleft/bsl/doc/model/ContentModelBuilder.java-15-18 (1)
15-18: Preserve filename fallback when no title is found.Overwriting with a null/blank title can leave pages untitled.
🛠️ Proposed fix
- files.map(it -> new Page(it, it.getFileName().getName(0).toString(), PageType.MANUAL)) - .peek(it -> it.setTitle(TitleProcessor.getInstance().getTitle(it.getPath()))) + files.map(it -> new Page(it, it.getFileName().getName(0).toString(), PageType.MANUAL)) + .peek(it -> { + var resolved = TitleProcessor.getInstance().getTitle(it.getPath()); + if (resolved != null && !resolved.isBlank()) { + it.setTitle(resolved); + } + }) .forEach(model.getPages()::add);src/main/java/ru/alkoleft/bsl/doc/bsl/Links.java-22-32 (1)
22-32: Potential NPE and confusing naming in link resolution logic.
Potential NPE at line 25: If
methodisnull, callingBslContext.getCurrent().contains(null)may throw or behave unexpectedly.Confusing naming: When
owneris null butmethodis found in BslContext (line 25-26), the code createsLink(method, null)— placing the value inownerName. This is semantically confusing since the variable is calledmethod.Consider adding a null guard and clarifying the naming:
public Link createLink(String owner, String method) { if (owner != null && method != null) { return new Link(owner, method); - } else if (BslContext.getCurrent().contains(method)) { + } else if (method != null && BslContext.getCurrent().contains(method)) { return new Link(method, null); } else if (method != null) { return new Link(null, method); } else { return null; } }
🧹 Nitpick comments (41)
.gitignore (1)
29-31: Consider using a more comprehensive pattern for backup files.The pattern
*.*~will only match backup files with an extension (e.g.,file.txt~) but will miss files without extensions (e.g.,README~,Makefile~). Consider using*~instead to catch all backup files ending with tilde.📝 Suggested improvement
*.orig -*.*~ +*~src/test/resources/configuration/src/CommonModules/СтроковыеФункции/Module.bsl (1)
19-21: Exported functions are empty — confirm fixtures or fail fast.All four
Экспортfunctions have empty bodies, so they returnНеопределено. If these are test fixtures, please confirm and consider a fail‑fast stub to prevent accidental runtime use. If they are meant to be functional, they need implementations.💡 Optional fail‑fast stub (apply to each function)
Функция ПолучитьСтрокуИзКоллекцииПодстрок(Коллекция, Разделитель = ",", Колонки = Неопределено, ИгнорироватьОшибки = Ложь) Экспорт - + ВызватьИсключение "Нереализовано"; КонецФункцииAlso applies to: 38-40, 54-56, 69-70
src/main/resources/logback.xml (1)
1-10: Consider making DEBUG configurable by env/system property.
Keeps production logs quieter while preserving easy opt-in for troubleshooting.♻️ Suggested tweak
<configuration> + <property name="BSLDOC_LOG_LEVEL" value="${BSLDOC_LOG_LEVEL:-INFO}"/> <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender"> <encoder> <pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %msg%n</pattern> </encoder> </appender> - <logger name="ru.alkoleft.bsl.doc" level="DEBUG" /> + <logger name="ru.alkoleft.bsl.doc" level="${BSLDOC_LOG_LEVEL}" /> <root level="INFO"> <appender-ref ref="STDOUT" /> </root> </configuration>src/main/java/ru/alkoleft/bsl/doc/render/TemplatesDefinition.java (1)
1-4: LGTM!Good use of Java record for an immutable configuration data carrier. The design is clean and integrates well with
RenderContext.Factory.Consider adding
@Nullableannotations or compact constructor validation if null values forpath,headerTemplate, orfooterTemplateshould be explicitly handled or rejected.src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/Strings.java (1)
1-10: LGTM!Clean utility class with correct null-safe implementation. The
@UtilityClassannotation properly handles the boilerplate.If whitespace-only strings should also be considered "empty" in the consuming code, consider adding an
isNullOrBlankvariant usingString.isBlank()(Java 11+). The current implementation is correct for strict empty-string semantics.src/main/java/ru/alkoleft/bsl/doc/options/OutputOptions.java (1)
8-15: LGTM!Clean immutable value class with appropriate Lombok annotations. The
@Valueannotation correctly generates the necessary boilerplate for an immutable data object.Consider adding
@Builder.Defaultfor any fields that should have sensible defaults, or@NonNullannotations if certain fields are mandatory:💡 Optional: Add null safety annotations
+import lombok.NonNull; + `@Builder` `@Value` public class OutputOptions { + `@NonNull` OutputFormat outputFormat; OutputHierarchy hierarchy; + `@NonNull` Path destination; ChildLayout childLayout; }src/main/java/ru/alkoleft/bsl/doc/options/OutputFormat.java (1)
8-15: LGTM!Clean enum definition with Lombok annotations for accessors and constructor. The
pathfield appears to map to template directories andextensioncorrectly specifies the output file extension.Java convention typically uses
UPPER_SNAKE_CASEfor enum constants (e.g.,DOCUSAURUS,MARKDOWN,CONFLUENCE_MARKDOWN). The current PascalCase naming may be intentional for serialization/display purposes, but consider aligning with standard Java conventions if not required otherwise.src/main/resources/docusaurus/value-definition.hbs (1)
1-26: LGTM!Well-structured recursive Handlebars template with inline partials for nested field/type rendering. The use of
details/summaryfor collapsible sections and theindent/addhelpers for level management is appropriate.Minor whitespace consideration: Lines 5 and 13 contain uncontrolled whitespace that may introduce blank lines in the output. If strict output formatting is needed, consider adding whitespace control (
~):💡 Optional: Tighten whitespace control
{{~#if this.fields}} <details><summary> - +{{~}} {{/if~}}{{~/each}} - +{{~}} </details>src/main/java/ru/alkoleft/bsl/doc/model/Links.java (1)
10-11: Avoid mutable global state for currentPath if rendering can be concurrent.A static mutable
currentPathcan race across threads or suggests hidden state. Consider passingPath baseintogetPageLink(...)or using a scoped context object instead.src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/SingleLine.java (1)
21-21: Consider output format compatibility.Hardcoding
<br/>works for HTML/Confluence but may not render correctly in pure Markdown output. If this helper is used across multiple output formats, consider parameterizing the line break replacement or restricting usage to HTML-based formats.src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/Trees.java (1)
69-88: Recursive traversal could overflow stack on extremely long comment blocks.The recursive
fillCommentsCollectionmethod could cause aStackOverflowErrorfor modules with an unusually high number of consecutive comment lines. While unlikely in practice, consider iterative traversal for robustness.Iterative alternative
private static void fillCommentsCollection(List<Token> tokens, Token currentToken, List<Token> lines) { - - int index = currentToken.getTokenIndex(); - - if (index == 0) { - return; - } - - var previousToken = tokens.get(index - 1); - - if (abortSearchComments(previousToken, currentToken)) { - return; - } - - fillCommentsCollection(tokens, previousToken, lines); - int type = previousToken.getType(); - if (type == BSLParser.LINE_COMMENT) { - lines.add(previousToken); + int index = currentToken.getTokenIndex(); + List<Token> tempComments = new ArrayList<>(); + + while (index > 0) { + var previousToken = tokens.get(index - 1); + if (abortSearchComments(previousToken, currentToken)) { + break; + } + if (previousToken.getType() == BSLParser.LINE_COMMENT) { + tempComments.add(previousToken); + } + currentToken = previousToken; + index = currentToken.getTokenIndex(); } + // Reverse to maintain original order + Collections.reverse(tempComments); + lines.addAll(tempComments); }src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbolComputer.java (1)
156-189: Consider capturing the full method range, not just the keyword token.Line 188 currently sets the range from the start keyword to itself, which makes the method range zero-length. If downstream logic expects the full method span (or if region markers can appear inside a method), consider passing the end token (e.g.,
ctx.getStop()) intohandleMethod.♻️ Suggested adjustment
- private void handleMethod(TerminalNode startNode, Token subName, BSLParser.ParamListContext paramList, boolean function, boolean export) { + private void handleMethod(TerminalNode startNode, Token subName, Token endToken, + BSLParser.ParamListContext paramList, boolean function, boolean export) { @@ - .range(SimpleRange.create(startNode.getSymbol(), startNode.getSymbol())) + .range(SimpleRange.create(startNode.getSymbol(), endToken)) .build(); methods.add(method); }- handleMethod(startNode, declaration.subName().getStart(), declaration.paramList(), true, declaration.EXPORT_KEYWORD() != null); + handleMethod(startNode, declaration.subName().getStart(), ctx.getStop(), declaration.paramList(), true, declaration.EXPORT_KEYWORD() != null); @@ - handleMethod(startNode, declaration.subName().getStart(), declaration.paramList(), false, declaration.EXPORT_KEYWORD() != null); + handleMethod(startNode, declaration.subName().getStart(), ctx.getStop(), declaration.paramList(), false, declaration.EXPORT_KEYWORD() != null);src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/ParameterDefinition.java (1)
36-38: Add a null guard fordefaultValueinisRequired()(defensive).If any builder call omits
defaultValue, Line 37 will throw. A small guard keeps this safe.🛠️ Suggested defensive tweak
public boolean isRequired() { - return defaultValue.type == ParameterType.EMPTY; + return defaultValue == null || defaultValue.type == ParameterType.EMPTY; }src/main/java/ru/alkoleft/bsl/doc/render/output/AppendStrategy.java (1)
7-9: Consider renamingisNotContainsto improve readability.The double-negative naming
isNotContainsis awkward. A clearer alternative would beisAbsent(location)or inverting to!manualContent.contains(location).src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.java (1)
53-56: PotentialNullPointerExceptionif resource is not found.
getClass().getClassLoader().getResource(name)returnsnullwhen the resource doesn't exist, causing an NPE on.toURI(). Consider adding a null check with a descriptive error message.Suggested fix
`@SneakyThrows` private Path getResource(String name) { - return Path.of(getClass().getClassLoader().getResource(name).toURI()); + var resource = getClass().getClassLoader().getResource(name); + if (resource == null) { + throw new IllegalArgumentException("Resource not found: " + name); + } + return Path.of(resource.toURI()); }src/main/resources/confluence-md/subsystem.hbs (2)
13-14: Unusual spacing in Handlebars block parameter syntax.The syntax
{{#eachconstants as | item|}}has extra spaces around the pipe character. Standard Handlebars syntax is{{#eachconstants as |item|}}. While this may work depending on the Handlebars version, it's inconsistent with conventions.Suggested fix
-{{`#each` constants as | item|}} +{{`#each` constants as |item|}} * {{mdo-present item}} ({{item.name}})
18-21: Consider conditional rendering for "Содержимое" section.Unlike the "Константы" section (which is wrapped in
{{#ifconstants}}), the "Содержимое" section always renders even whenchildrenPagesis empty. This will display an empty section with just the header.Also, same spacing issue in the each block syntax.
Suggested fix
+{{~#if childrenPages}} + ## Содержимое -{{`#each` childrenPages as | item|}} +{{`#each` childrenPages as |item|}} * [{{item.title}}]({{page-link item}}) {{~/each}} +{{~/if}}src/main/java/ru/alkoleft/bsl/doc/render/contexts/BaseContext.java (1)
10-20: Potential issue: mutable field included inequals/hashCode.The
outputPathfield is mutable (via@Setter), but@EqualsAndHashCodeincludes all fields by default. If aBaseContextinstance is stored in a hash-based collection andoutputPathis later modified, the instance may become unretrievable.Consider excluding the mutable field from equality checks if hash-based collection storage is expected.
Suggested fix
`@Getter` `@AllArgsConstructor` -@EqualsAndHashCode +@EqualsAndHashCode(exclude = "outputPath") public class BaseContext {src/main/java/ru/alkoleft/bsl/doc/render/output/MergeStrategy.java (1)
3-3: Avoid using internal API from Handlebars library.
com.github.jknack.handlebars.internal.Filesis an internal class that may change or be removed without notice in future versions. Consider usingjava.nio.file.Filesinstead.Suggested fix
-import com.github.jknack.handlebars.internal.Files; +import java.nio.file.Files;And update the usage at line 22:
- var fileContent = Files.read(itemPath.toFile(), StandardCharsets.UTF_8); + var fileContent = Files.readString(itemPath, StandardCharsets.UTF_8);src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/ModuleContextBuilder.java (2)
19-19: Use parameterized logging instead of string concatenation.String concatenation in log statements is evaluated even when the log level is disabled, causing unnecessary overhead.
Proposed fix
- log.debug("Parse module: " + owner.getName() + "." + bslModule.getModuleType()); + log.debug("Parse module: {}.{}", owner.getName(), bslModule.getModuleType());
41-43: Clarify the buildFilteredModuleContext implementation.The method calls
buildModuleContexttwice in a nested manner, which may be intentional but is not immediately clear. Consider adding a brief comment or renaming for clarity.public ModuleInfo buildFilteredModuleContext(Module bslModule) { - return buildModuleContext(buildModuleContext(bslModule)); + // First build full context, then apply filtering + ModuleInfo fullContext = buildModuleContext(bslModule); + return buildModuleContext(fullContext); }src/main/java/ru/alkoleft/bsl/doc/model/ContentModel.java (1)
11-13: @value with mutable ArrayList contradicts immutability intent.Lombok's
@Valueimplies immutability, but thepagesfield is a mutableArrayListthat gets modified byappend(). This can lead to unexpected behavior.Consider using
@Datainstead, or document that this class is intentionally mutable despite the annotation.src/test/resources/configuration/src/Subsystems/ППИ/Subsystems/Прикладные/Прикладные.mdo (1)
4-11: Minor XML indentation inconsistency.The
<explanation>element (lines 8-11) uses 4-space indentation for its children, while<synonym>(lines 4-7) uses 2-space indentation. Consider aligning for consistency:Suggested fix
<synonym> <key>ru</key> <value>Прикладные объекты</value> </synonym> - <explanation> - <key>ru</key> - <value>Подробное описание подсистемы</value> - </explanation> + <explanation> + <key>ru</key> + <value>Подробное описание подсистемы</value> + </explanation>src/main/resources/confluence-md/module.hbs (1)
20-23: Clarify the else branch placement in nested iteration.The
{{~else}}on line 22 appears to be the fallback for whenparameter.typesis empty, rendering the description with links. However, the indentation and placement make this hard to follow. Consider adding a comment or restructuring for clarity.src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/ModuleComputer.java (1)
39-51: Common prefix algorithm is correct but could use a brief comment.The nested loop correctly computes the longest common whitespace prefix across all lines. Consider adding a brief comment explaining the purpose for future maintainers.
src/main/java/ru/alkoleft/bsl/doc/render/contexts/ModuleContext.java (1)
20-33: Defensively copymethodsto preserve immutability.Without a copy, callers can mutate the list after construction.
♻️ Proposed fix
- this.methods = methods; + this.methods = methods == null ? null : List.copyOf(methods);src/main/java/ru/alkoleft/bsl/doc/render/contexts/ContextFactory.java (1)
54-75: Remove unusedgetPresent(ModuleType)or wire it in.Keeping dead code increases maintenance cost and confuses intent.
🧹 Proposed cleanup
- private String getPresent(ModuleType moduleType) { - switch (moduleType) { - case ManagerModule: - return "Модуль менеджера"; - case BotModule: - case ObjectModule: - return "Модуль объекта"; - case HTTPServiceModule: - return "Модуль http-сервиса"; - case WEBServiceModule: - return "Модуль web-сервиса"; - case CommonModule: - return "Модуль"; - case CommandModule: - return "Модуль команды"; - case RecordSetModule: - return "Модуль набора записей"; - case ValueManagerModule: - return "Модуль менеджера значений"; - default: - return moduleType.name(); - } - }src/main/java/ru/alkoleft/bsl/doc/render/contexts/SubsystemContext.java (1)
45-50: Consider using.toList()instead ofCollectors.toList().Since the project targets Java 17 (per AI summary), you can simplify the stream termination:
public List<Constant> getConstants() { return BslContext.getCurrent().getSubsystemObjects(subsystem) .filter(Constant.class::isInstance) .map(Constant.class::cast) - .collect(Collectors.toList()); + .toList(); }This also returns an unmodifiable list, which is safer if the caller shouldn't modify it.
src/main/java/ru/alkoleft/bsl/doc/bsl/Links.java (1)
14-20: Consider usingmatches()instead offind()for anchored patterns.Since both patterns are anchored with
^and$, usingmatches()is more semantically appropriate thanfind():public Link parseLink(String link, boolean withSee) { - var matcher = (withSee ? PATTERN_WITH_SEE : PATTERN).matcher(link); - if (matcher.find()) { + var matcher = (withSee ? PATTERN_WITH_SEE : PATTERN).matcher(link); + if (matcher.matches()) { return createLink(matcher.group(2), matcher.group(3)); } return null; }src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommandOptions.java (1)
56-65: File is re-read on every call togetBlockContent().When
contentisnullandpathis set,Files.readString(path)is called on every invocation. If these methods are called multiple times, consider caching the result:♻️ Optional: Cache the file content
`@SneakyThrows` public String getBlockContent() { if (content != null) { return content; } else if (path != null) { - return Files.readString(path); + content = Files.readString(path); + return content; } else { return null; } }src/main/java/ru/alkoleft/bsl/doc/manual/ManualContent.java (2)
42-50: Consider renamingisNotContainsfor clarity.The method name is grammatically awkward. Consider
doesNotContain(Path)or simply inverting the logic tocontains(Path)and letting callers negate it:- public boolean isNotContains(Path path) { + public boolean doesNotContain(Path path) {
56-59: Redundant parameters increateDestinationPage.The
localanddestparameters are always passed aslocationanddestination(the class fields). Consider using the fields directly:- private Page createDestinationPage(Page localPage, Path local, Path dest) { - var destPath = dest.resolve(local.relativize(localPage.getPath())); + private Page createDestinationPage(Page localPage) { + var destPath = destination.resolve(location.relativize(localPage.getPath())); return new Page(destPath, localPage.getTitle(), localPage.getType()); }And update the call site on line 38:
- .map(it -> createDestinationPage(it, location, destination)) + .map(this::createDestinationPage)src/main/java/ru/alkoleft/bsl/doc/AutodocManager.java (2)
34-51: Side-effect initialization of TitleProcessor singleton.Line 50 calls
TitleProcessor.Factory.create()solely for its side effect of setting the singleton instance. Consider assigning the result to a field or documenting this intent more clearly to avoid confusion.
81-87: ReplaceprintStackTrace()with proper logging.Using
e.printStackTrace()is not suitable for production code. Consider using a logging framework (e.g., SLF4J) for consistent error reporting.♻️ Suggested improvement
+import lombok.extern.slf4j.Slf4j; ... +@Slf4j public class AutodocManager { ... private void deletePath(Path path) { try { PathUtils.delete(path); } catch (IOException e) { - e.printStackTrace(); + log.error("Failed to delete path: {}", path, e); } }src/main/java/ru/alkoleft/bsl/doc/render/output/OutputStrategy.java (1)
24-30: Consider exhaustive switch without default.Using
defaultin an enum switch may hide future enum values added toManualMergeStrategy. If the enum has onlyAPPENDandMERGE, consider throwing an exception for unexpected values instead.♻️ Suggested approach
public static OutputStrategy create(ManualMergeStrategy strategy) { return switch (strategy) { case APPEND -> new AppendStrategy(); case MERGE -> new MergeStrategy(); - default -> new OutputStrategy(); + case NONE -> new OutputStrategy(); }; }If a
NONEor similar value doesn't exist, consider:default -> throw new IllegalArgumentException("Unsupported strategy: " + strategy);src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/BslFilter.java (1)
36-46: Confusing method name:setFilterdoesn't set anything.The method
setFilter(Stream<MethodSymbol>)actually filters the stream rather than setting a filter. Consider renaming toapplyFilterorfilterMethodsfor clarity.♻️ Suggested rename
- public Stream<MethodSymbol> setFilter(Stream<MethodSymbol> stream) { + public Stream<MethodSymbol> applyFilter(Stream<MethodSymbol> stream) {src/main/java/ru/alkoleft/bsl/doc/content/processor/TitleProcessor.java (1)
23-25: Unused parameter:formatis ignored.The
create()method accepts anOutputFormatparameter but always returnsMarkdownTitleProcessor. Either use the parameter to select an appropriate implementation or remove it.♻️ Suggested implementation
public TitleProcessor create(OutputFormat format) { - return instance = new MarkdownTitleProcessor(); + return instance = switch (format) { + case MARKDOWN, DOCUSAURUS -> new MarkdownTitleProcessor(); + case CONFLUENCE -> new ConfluenceTitleProcessor(); // if exists + default -> new MarkdownTitleProcessor(); + }; }src/main/java/ru/alkoleft/bsl/doc/render/StructureRender.java (1)
68-85: Duplicate directory creation logic.Lines 76-78 create directories before saving, but
OutputStrategy.save()(line 55 in OutputStrategy.java) also callsFiles.createDirectories(). Consider removing the duplicate here or consolidating directory creation in one place.♻️ Suggested simplification
public void visit(ModuleItem item, int index) { var moduleContext = BslContext.getCurrent().getModuleContext(item.getModule()); if (moduleContext.isEmpty()) { return; } var path = pathResolver.getFilePath(MDOHelper.getOwner(item.getModule()).getMdoReference().getMdoRefRu()); - if (!Files.exists(path.getParent())) { - Files.createDirectories(path.getParent()); - } if (outputStrategy.needRender(path)) { Links.setCurrentPath(path); var content = BslRender.renderModule(moduleContext, index); outputStrategy.save(path, content) .setType(PageType.MODULE); } }src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.java (1)
62-69: Make theaddhelper null-safe.If a template passes null or a non-number,
context.intValue()will throw. A defensive cast avoids hard failures in template rendering.♻️ Suggested update
- handlebars.registerHelper("add", new Helper<Number>() { - `@Override` - public Object apply(Number context, Options options) { - // context — это первое число, options.param(0) — второе - int addition = options.param(0, 1); // 1 — значение по умолчанию - return context.intValue() + addition; - } - }); + handlebars.registerHelper("add", (Object context, Options options) -> { + int base = (context instanceof Number) ? ((Number) context).intValue() : 0; + int addition = options.param(0, 1); + return base + addition; + });src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommand.java (1)
116-122: Prefer parameterized logging.This avoids eager
toString()work when debug is off.♻️ Suggested update
- log.debug("Filter: " + filter.toString()); - log.debug("Options: " + options.toString()); - log.debug("Sources: " + sources); - log.debug("Manual: " + commandOptions.getManualDocs()); - log.debug("Output: " + destination); - log.debug("Merge manual: " + commandOptions.getMergeStrategy()); + log.debug("Filter: {}", filter); + log.debug("Options: {}", options); + log.debug("Sources: {}", sources); + log.debug("Manual: {}", commandOptions.getManualDocs()); + log.debug("Output: {}", destination); + log.debug("Merge manual: {}", commandOptions.getMergeStrategy());src/main/java/ru/alkoleft/bsl/doc/bsl/BslContext.java (1)
83-107: Avoid returning null fromgetMethodInfo.Both overloads can return null while adjacent APIs favor
Optional, increasing NPE risk. Consider returningOptional<MethodInfo>(or annotate/document nullability) and ensure callers handle absence explicitly.
| public TypeDescription getReturnedValue() { | ||
| var result = fullDescription.map(MethodDescription::getReturnedValue).filter(it -> !it.isEmpty()).map(it -> it.get(0)).orElse(null); | ||
| var result = fullDescription | ||
| .map(MethodDescription::getReturnedValue) | ||
| .filter(it -> !it.isEmpty()) | ||
| .map(it -> it.get(0)) | ||
| .orElse(null); | ||
|
|
||
| if (result != null) { | ||
| return new TypeDescription(result.getName(), result.getDescription(), result.getParameters(), Strings.isNullOrEmpty(result.getLink()) ? null : "см. " + result.getLink(), result.isHyperlink()); | ||
| return createTypeDescription(result); | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| private TypeDescription createTypeDescription(TypeDescription baseDescription) { | ||
| TypeDescription result = null; | ||
| HyperlinkTypeDescription hyperlinkTypeDescription = null; | ||
| if (baseDescription instanceof HyperlinkTypeDescription) { | ||
| hyperlinkTypeDescription = (HyperlinkTypeDescription) baseDescription; | ||
| } | ||
| if (hyperlinkTypeDescription != null && !Strings.isNullOrEmpty(hyperlinkTypeDescription.hyperlink().link())) { | ||
| var methodInfo = BslContext.getCurrent().getMethodInfo(hyperlinkTypeDescription.hyperlink().link()); | ||
|
|
||
| if (methodInfo != null && methodInfo.getMethod() != null) { | ||
| result = methodInfo.getMethod().getReturnedValue(); | ||
| } else { | ||
| log.warn("Bad link: " + hyperlinkTypeDescription.hyperlink().link()); | ||
| } | ||
| } | ||
| if (result == null) { | ||
| result = baseDescription; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Guard against cyclic hyperlink resolution to avoid stack overflows.
createTypeDescription can recurse through getReturnedValue() indefinitely if hyperlinks form a cycle (including self-references). Consider tracking visited links to break cycles.
🛠️ Proposed fix with cycle detection
-import java.util.List;
-import java.util.Optional;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
@@
- public TypeDescription getReturnedValue() {
- var result = fullDescription
- .map(MethodDescription::getReturnedValue)
- .filter(it -> !it.isEmpty())
- .map(it -> it.get(0))
- .orElse(null);
-
- if (result != null) {
- return createTypeDescription(result);
- } else {
- return null;
- }
- }
+ public TypeDescription getReturnedValue() {
+ return getReturnedValue(new HashSet<>());
+ }
+
+ private TypeDescription getReturnedValue(Set<String> visited) {
+ var result = fullDescription
+ .map(MethodDescription::getReturnedValue)
+ .filter(it -> !it.isEmpty())
+ .map(it -> it.get(0))
+ .orElse(null);
+
+ return result != null ? createTypeDescription(result, visited) : null;
+ }
@@
- private TypeDescription createTypeDescription(TypeDescription baseDescription) {
+ private TypeDescription createTypeDescription(TypeDescription baseDescription, Set<String> visited) {
TypeDescription result = null;
HyperlinkTypeDescription hyperlinkTypeDescription = null;
if (baseDescription instanceof HyperlinkTypeDescription) {
hyperlinkTypeDescription = (HyperlinkTypeDescription) baseDescription;
}
- if (hyperlinkTypeDescription != null && !Strings.isNullOrEmpty(hyperlinkTypeDescription.hyperlink().link())) {
- var methodInfo = BslContext.getCurrent().getMethodInfo(hyperlinkTypeDescription.hyperlink().link());
+ if (hyperlinkTypeDescription != null && !Strings.isNullOrEmpty(hyperlinkTypeDescription.hyperlink().link())) {
+ var link = hyperlinkTypeDescription.hyperlink().link();
+ if (!visited.add(link)) {
+ log.warn("Cyclic link: " + link);
+ return baseDescription;
+ }
+ var methodInfo = BslContext.getCurrent().getMethodInfo(link);
if (methodInfo != null && methodInfo.getMethod() != null) {
- result = methodInfo.getMethod().getReturnedValue();
+ result = methodInfo.getMethod().getReturnedValue(visited);
} else {
- log.warn("Bad link: " + hyperlinkTypeDescription.hyperlink().link());
+ log.warn("Bad link: " + link);
}
}
if (result == null) {
result = baseDescription;
}
return result;
}🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbol.java` around lines
42 - 75, The createTypeDescription path can loop if hyperlinks form cycles;
modify MethodSymbol.getReturnedValue to call an overloaded createTypeDescription
that accepts a Set<String> (or Set<URI>) of visited links initialized empty, and
change/rescue createTypeDescription to take this visited set, record
hyperlinkTypeDescription.hyperlink().link() before resolving, and if the link is
already in the visited set skip resolution (warn and return baseDescription) to
break cycles; ensure any recursive call to
methodInfo.getMethod().getReturnedValue() uses the same visited set (or
propagates via the new overload) so cycles are detected across resolutions.
| public List<MethodSymbol> compute(BSLTokenizer tokenizer) { | ||
| this.tokenizer = tokenizer; | ||
| methods.clear(); | ||
| visitFile(tokenizer.getAst()); | ||
|
|
||
| // Используется постпривязка регионов, тк порядок обхода не соответствует порядку следования объект (в метод заходит раньше региона) | ||
| for (var region : regionData) { | ||
| for (var method : methods) { | ||
| if (SimpleRange.containsRange(region.range, method.getRange())) { | ||
| method.setRegion(region.region); | ||
| } | ||
| } | ||
| } | ||
| return methods; | ||
| } |
There was a problem hiding this comment.
Clear region state per compute to avoid stale bindings.
regionData and regions are never reset, so reusing the same MethodSymbolComputer instance can leak regions and mis-assign methods. Also, guard against null region ranges to avoid a potential NPE on unterminated regions.
🛠️ Proposed fix
public List<MethodSymbol> compute(BSLTokenizer tokenizer) {
this.tokenizer = tokenizer;
methods.clear();
+ regionData.clear();
+ regions.clear();
visitFile(tokenizer.getAst());
// Используется постпривязка регионов, тк порядок обхода не соответствует порядку следования объект (в метод заходит раньше региона)
for (var region : regionData) {
+ if (region.range == null) {
+ continue;
+ }
for (var method : methods) {
- if (SimpleRange.containsRange(region.range, method.getRange())) {
+ if (method.getRange() != null && SimpleRange.containsRange(region.range, method.getRange())) {
method.setRegion(region.region);
}
}
}
return methods;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<MethodSymbol> compute(BSLTokenizer tokenizer) { | |
| this.tokenizer = tokenizer; | |
| methods.clear(); | |
| visitFile(tokenizer.getAst()); | |
| // Используется постпривязка регионов, тк порядок обхода не соответствует порядку следования объект (в метод заходит раньше региона) | |
| for (var region : regionData) { | |
| for (var method : methods) { | |
| if (SimpleRange.containsRange(region.range, method.getRange())) { | |
| method.setRegion(region.region); | |
| } | |
| } | |
| } | |
| return methods; | |
| } | |
| public List<MethodSymbol> compute(BSLTokenizer tokenizer) { | |
| this.tokenizer = tokenizer; | |
| methods.clear(); | |
| regionData.clear(); | |
| regions.clear(); | |
| visitFile(tokenizer.getAst()); | |
| // Используется постпривязка регионов, тк порядок обхода не соответствует порядку следования объект (в метод заходит раньше региона) | |
| for (var region : regionData) { | |
| if (region.range == null) { | |
| continue; | |
| } | |
| for (var method : methods) { | |
| if (method.getRange() != null && SimpleRange.containsRange(region.range, method.getRange())) { | |
| method.setRegion(region.region); | |
| } | |
| } | |
| } | |
| return methods; | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbolComputer.java`
around lines 111 - 125, In compute(BSLTokenizer tokenizer) clear the
per-instance region state before use by resetting regionData (and regions if
present) and methods to empty collections so repeated calls don't leak prior
regions; then keep the post-binding loop but skip any region with a null
region.range (guard with an if (region.range == null) continue) before calling
SimpleRange.containsRange and method.setRegion to avoid NPE on unterminated
regions; references: MethodSymbolComputer.compute, regionData, regions,
visitFile, region.range, SimpleRange.containsRange, method.setRegion.
| var commandOptions = options(); | ||
|
|
||
| var filterBuilder = Filter.builder() | ||
| .isExport(true); | ||
| regions.forEach(filter::region); | ||
| commandOptions.getRegions().forEach(filterBuilder::region); | ||
| commandOptions.getSubsystems().forEach(filterBuilder::rootSubsystem); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and read the specific lines
cat -n src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommand.java | sed -n '95,115p'Repository: 1CDevFlow/BSL-Doc
Length of output: 793
🏁 Script executed:
# Find RenderCommandOptions class
fd -t f "RenderCommandOptions.java" src/Repository: 1CDevFlow/BSL-Doc
Length of output: 129
🏁 Script executed:
# Check how getRegions() and getSubsystems() are used elsewhere
rg "getRegions|getSubsystems" -t java -B 2 -A 2 src/Repository: 1CDevFlow/BSL-Doc
Length of output: 2824
🏁 Script executed:
# Read the RenderCommandOptions class to check defaults
cat -n src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommandOptions.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 2361
Guard against null subsystems in filter.
The regions field has a default value and will never be null, but subsystems is uninitialized and can be null, causing getSubsystems().forEach() on line 106 to throw NPE. Add a null check or ensure subsystems defaults to an empty list.
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommand.java` around lines
101 - 107, RenderCommand builds a Filter from commandOptions but calls
commandOptions.getSubsystems().forEach(...) which can NPE because subsystems may
be null; update the code to guard against null by either initializing subsystems
to an empty list in options() or by checking commandOptions.getSubsystems() !=
null before iterating (e.g., only call forEach when non-null), ensuring you
reference the same methods: options(), commandOptions.getSubsystems(), and
Filter.builder().rootSubsystem(...) so subsystems are handled as an empty
collection or skipped when null.
| private RenderContext(TemplatesDefinition templatesDefinition) { | ||
| this.templatesDefinition = templatesDefinition; | ||
| this.baseURL = getClass().getClassLoader().getResource(templatesDefinition.path()); | ||
| handlebars = new Handlebars() | ||
| .with(new URLTemplateLoader() { | ||
| @Override | ||
| protected URL getResource(String location) throws IOException { | ||
| return new URL(baseURL + location); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the RenderContext.java file
find . -type f -name "RenderContext.java" | head -20Repository: 1CDevFlow/BSL-Doc
Length of output: 195
🏁 Script executed:
# Read the RenderContext.java file in the handlebars directory
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 5881
🏁 Script executed:
# Check if there's any validation of templatesDefinition.path() in Factory or elsewhere
rg -A5 -B5 "TemplatesDefinition" src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 1402
🏁 Script executed:
# Check if TemplatesDefinition has any validation
find . -type f -name "TemplatesDefinition.java"Repository: 1CDevFlow/BSL-Doc
Length of output: 128
🏁 Script executed:
# Read the TemplatesDefinition file
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/TemplatesDefinition.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 222
🏁 Script executed:
# Verify URL constructor behavior - check Java documentation and test the difference
python3 << 'EOF'
# Demonstrating URL constructor behavior
# 1. String concatenation approach can create malformed URLs
base_without_slash = "file:///templates"
location_with_slash = "/main.hbs"
location_without_slash = "main.hbs"
# String concatenation result
concat_result1 = base_without_slash + location_with_slash # OK: file:///templates/main.hbs
concat_result2 = base_without_slash + location_without_slash # WRONG: file:///templatesmain.hbs
print("String concatenation approach:")
print(f" baseURL + location (with /): {concat_result1}")
print(f" baseURL + location (no /): {concat_result2}")
print()
# 2. URL(URL base, String spec) constructor properly handles relative resolution
print("URL(base, spec) constructor:")
print(" Properly resolves relative URLs and handles base path correctly")
print(" Example: new URL(new URL('file:///templates'), 'main.hbs')")
print(" Result: file:///templates/main.hbs")
EOFRepository: 1CDevFlow/BSL-Doc
Length of output: 401
Fail fast on missing templates and resolve template URLs safely.
baseURL can be null if ClassLoader.getResource() doesn't find the templates resource, causing a NullPointerException at line 47. Additionally, string concatenation in new URL(baseURL + location) produces malformed URLs when baseURL lacks a trailing slash (e.g., "file:///templates" + "main.hbs" becomes "file:///templatesmain.hbs"). Add Objects.requireNonNull() to detect missing resources immediately, and use the URL(URL base, String spec) constructor to properly resolve relative template URLs.
🛠️ Proposed fix
-import java.util.HashMap;
-import java.util.Map;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
private RenderContext(TemplatesDefinition templatesDefinition) {
this.templatesDefinition = templatesDefinition;
- this.baseURL = getClass().getClassLoader().getResource(templatesDefinition.path());
+ this.baseURL = Objects.requireNonNull(
+ getClass().getClassLoader().getResource(templatesDefinition.path()),
+ "Templates path not found: " + templatesDefinition.path()
+ );
handlebars = new Handlebars()
.with(new URLTemplateLoader() {
`@Override`
protected URL getResource(String location) throws IOException {
- return new URL(baseURL + location);
+ return new URL(baseURL, location);
}🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.java`
around lines 40 - 48, The constructor RenderContext(TemplatesDefinition
templatesDefinition) can NPE when baseURL is missing and builds URLs incorrectly
by string concatenation; update the constructor to requireNonNull on the result
of getClass().getClassLoader().getResource(templatesDefinition.path())
(assigning to baseURL) to fail fast, and change the URL creation inside the
anonymous URLTemplateLoader.getResource to use the URL(URL base, String spec)
constructor (new URL(baseURL, location)) instead of concatenating strings so
relative template paths resolve correctly.
| @SneakyThrows | ||
| public void renderToFile(String templateName, Context context, Path output) { | ||
| var template = getTemplate(templateName); | ||
| try (var writer = new FileWriter(output.toFile())) { | ||
| template.apply(context, writer); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate and read the RenderContext.java file
find . -path "*src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.java" -type fRepository: 1CDevFlow/BSL-Doc
Length of output: 178
🏁 Script executed:
# Get line count to understand file size
wc -l src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 135
🏁 Script executed:
# Read the full file to understand context
cat -n src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 5881
Write files with an explicit charset.
FileWriter uses the platform default encoding, which can corrupt non‑ASCII docs. Use the Handlebars charset to keep output consistent.
🛠️ Proposed fix
@@
-import java.io.FileWriter;
+import java.nio.file.Files;
@@
- try (var writer = new FileWriter(output.toFile())) {
+ try (var writer = Files.newBufferedWriter(output, handlebars.getCharset())) {
template.apply(context, writer);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SneakyThrows | |
| public void renderToFile(String templateName, Context context, Path output) { | |
| var template = getTemplate(templateName); | |
| try (var writer = new FileWriter(output.toFile())) { | |
| template.apply(context, writer); | |
| } | |
| `@SneakyThrows` | |
| public void renderToFile(String templateName, Context context, Path output) { | |
| var template = getTemplate(templateName); | |
| try (var writer = Files.newBufferedWriter(output, handlebars.getCharset())) { | |
| template.apply(context, writer); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.java`
around lines 84 - 89, The renderToFile method currently opens the output with
FileWriter (in RenderContext.renderToFile), which uses the platform default
encoding; change it to create the writer with an explicit charset from the
Handlebars configuration (e.g., use the Handlebars charset or a Charset field on
RenderContext) and pass that writer to template.apply(context, writer) instead
of FileWriter so output encoding is consistent for non-ASCII documents.
| if (parts.find()) { | ||
| var result = parts.group(1) + content + parts.group(3); | ||
| return super.save(itemPath, result); |
There was a problem hiding this comment.
Bug: merge marker line is discarded, breaking subsequent merges.
The regex captures the generated_content marker in group(2), but the merge result only uses group(1) + content + group(3), discarding the marker. After one merge, the file loses the marker and future merges will fall through to the else branch.
Proposed fix to preserve the marker
if (parts.find()) {
- var result = parts.group(1) + content + parts.group(3);
+ var result = parts.group(1) + parts.group(2) + content + parts.group(3);
return super.save(itemPath, result);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (parts.find()) { | |
| var result = parts.group(1) + content + parts.group(3); | |
| return super.save(itemPath, result); | |
| if (parts.find()) { | |
| var result = parts.group(1) + parts.group(2) + content + parts.group(3); | |
| return super.save(itemPath, result); |
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/output/MergeStrategy.java` around
lines 25 - 27, The merge currently discards the merge marker captured as
parts.group(2) in MergeStrategy, causing the marker to be lost after the first
save; update the merge branch where parts.find() is true to include the marker
when composing the result (use parts.group(1), parts.group(2), content,
parts.group(3)) and then call super.save(itemPath, result) so future merges
still find the marker; locate this logic around the parts matcher usage in the
save method of MergeStrategy.
| @AllArgsConstructor | ||
| public class PathResolver { | ||
|
|
||
| private final Path destination; | ||
| private final OutputFormat format; | ||
| private final Stack<String> paths = new Stack<>(); | ||
| private final Lazy<Path> currentPath = new Lazy<>(this::computeCurrentPath); |
There was a problem hiding this comment.
@AllArgsConstructor generates incorrect constructor signature.
@AllArgsConstructor generates a constructor requiring all four fields: (Path, OutputFormat, Stack<String>, Lazy<Path>). However, paths and currentPath should be internally managed and not exposed to callers.
Proposed fix
-@AllArgsConstructor
public class PathResolver {
private final Path destination;
private final OutputFormat format;
private final Stack<String> paths = new Stack<>();
private final Lazy<Path> currentPath = new Lazy<>(this::computeCurrentPath);
+
+ public PathResolver(Path destination, OutputFormat format) {
+ this.destination = destination;
+ this.format = format;
+ }🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/PathResolver.java` around lines 10 -
16, Replace the incorrect `@AllArgsConstructor` on class PathResolver with a
custom constructor that accepts only the externally-provided fields destination
and format; initialize the internal mutable fields paths (new Stack<>()) and
currentPath (new Lazy<>(this::computeCurrentPath)) inside that constructor so
callers cannot supply or override them. Update any usages that relied on the
generated four-arg constructor to use the two-arg constructor and ensure
computeCurrentPath remains referenced for the Lazy initialization. Ensure
imports/annotations are adjusted (remove `@AllArgsConstructor`) and visibility of
fields remains the same.
|
@coderabbitai перепроверь реквест, все готово |
|
✅ Actions performedReview triggered.
|
theshadowco
changed the title
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@build.gradle.kts`:
- Line 68: The test dependency declares slf4j-reload4j with an alpha version
("slf4j-reload4j", "2.1.0-alpha1"); update this to the same stable version used
for the main slf4j-api dependency (replace "2.1.0-alpha1" with the project's
chosen stable slf4j version) so both main and test dependencies match and avoid
mixing alpha/stable artifacts.
- Line 55: Replace the alpha SLF4J dependency version with the stable release:
locate the dependency declaration implementation("org.slf4j", "slf4j-api",
"2.1.0-alpha1") in the build.gradle.kts and change the version string to
"2.0.17" so the project uses the stable slf4j-api 2.0.17 release.
- Line 63: The Handlebars dependency uses an insecure version string
implementation("com.github.jknack:handlebars:4.3.1"); update that dependency
declaration to use version 4.5.0
(implementation("com.github.jknack:handlebars:4.5.0")), then refresh/update
Gradle dependencies (re-run dependency resolution/refresh or update lockfiles)
and rebuild to ensure the new version is picked up and tests pass.
- Line 8: The project is using Spring Boot 2.7.10
(id("org.springframework.boot") version "2.7.10"), which is EOL; update the
plugin version to a Spring Boot 3.x release compatible with Java 17 and then
migrate code and dependencies to Jakarta EE namespaces (javax -> jakarta) and
any updated dependency coordinates; specifically change the Gradle plugin
version, update Spring-related dependencies (starter artifacts,
spring-boot-starter-*, spring-boot-gradle-plugin usages), run the Spring Boot 3
migration checklist (API changes, actuator, security, test libs), and adjust
imports/usages across classes (controllers, filters, config classes) to
jakarta.* to resolve compilation and runtime issues.
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/MDOHelper.java`:
- Around line 16-31: Add null-safety guards to both getPresent methods: check if
the MD parameter is null in getPresent(MD object) and return an empty string (or
other safe default) instead of calling object.getDescription(); similarly, check
if the ModuleType moduleType is null in getPresent(ModuleType moduleType) and
return "" (or a safe default) before the switch, otherwise proceed with the
existing switch logic and default case.
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/ModuleInfo.java`:
- Around line 32-34: getMethod currently calls getMethods().stream() and uses
name.equalsIgnoreCase(...), which can NPE if getMethods() returns null or if
name is null; guard both by treating a null methods list as empty and by
avoiding calling equalsIgnoreCase on a null name. Update getMethod to iterate
over Optional.ofNullable(getMethods()).orElse(Collections.emptyList()) (or check
getMethods()!=null) and filter with a null-safe comparison such as name != null
&& name.equalsIgnoreCase(it.getName()) or alternatively compare with
it.getName()!=null && it.getName().equalsIgnoreCase(name), returning
Optional.empty() if name is null.
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/ParameterDefinition.java`:
- Around line 32-34: isRequired() currently dereferences defaultValue and can
NPE if the builder sets it to null; update ParameterDefinition.isRequired() to
defensively handle a null defaultValue (e.g., treat null as "required") by
checking defaultValue for null before accessing defaultValue.type and return the
appropriate boolean based on ParameterType.EMPTY; reference
ParameterDefinition.isRequired(), defaultValue, and ParameterType.EMPTY when
making the change.
In `@src/main/java/ru/alkoleft/bsl/doc/model/ContentModelBuilder.java`:
- Around line 25-27: The catch block in ContentModelBuilder currently logs
"Error cleaning" which is a wrong copy-paste message; update the log.error call
inside the catch (IOException e) in class ContentModelBuilder to a descriptive
message such as "Error building content model" or "Error building ContentModel
in ContentModelBuilder" so the log correctly reflects the operation and include
the exception variable e as the second parameter.
In `@src/main/java/ru/alkoleft/bsl/doc/render/contexts/ContextFactory.java`:
- Around line 30-39: The call subsystem.getExplanation().get("ru") in
ContextFactory.create can return null when the "ru" key is missing; update
ContextFactory.create to safely obtain an explanation by checking
subsystem.getExplanation() for the "ru" entry and, if absent or null/blank, fall
back to the first available map value (e.g., stream map.values().findFirst())
before passing the value into SubsystemContext.builder().explanation(...),
ensuring you handle null/empty maps gracefully (use Optional or null checks) so
SubsystemContext always receives a non-null fallback when possible.
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/MdoPresent.java`:
- Around line 11-18: The apply method in MdoPresent calls context.toString()
without null-checking, causing a NullPointerException when Handlebars passes
null; update MdoPresent.apply to early-return a safe fallback (e.g., empty
string) if context is null, otherwise keep existing branches for MD (use
MDOHelper.getPresent(md)) and SubsystemContext (use
MDOHelper.getPresent(subsystemContext.getSubsystem())); ensure the null guard is
the very first check so MD and SubsystemContext instanceof checks are only
reached with non-null context.
In `@src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.java`:
- Around line 19-31: The test uses a hardcoded destination Path in
RenderCommand.builder() which should be replaced by a JUnit `@TempDir` Path
injected into the test method (replace the hardcoded
Path.of("/tmp/bsl-doc-fixture") with the temp dir.resolve(...) value) to avoid
platform/permission issues; also add concrete assertions after .run() (e.g.,
check that expected files exist under the destination, verify contents or
presence of key strings in generated files) to ensure the command produced the
correct outputs instead of merely not throwing exceptions—locate the test method
named run() in RenderCommandTest and update the builder invocation and
assertions accordingly.
♻️ Duplicate comments (4)
src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommandOptions.java (1)
29-33: JacksonPathdeserialization issue remains unresolved.This concern was raised in a previous review. The
ObjectMapperstill lacks custom deserialization forjava.nio.file.Pathfields (manualDocsat line 21 andPageBlock.pathat line 51), which will causeJsonMappingExceptionat runtime when loading JSON configs with path values.src/main/java/ru/alkoleft/bsl/doc/AutodocManager.java (1)
74-84: Deletion order issue remains - directories may fail to delete.This concern was raised in a previous review.
Files.walk()returns paths in depth-first order, but without sorting in reverse, parent directories may be processed before their children.PathUtils.delete()will fail on non-empty directories. Additionally,@SneakyThrowsis redundant sinceIOExceptionis already caught inside the try-catch.🐛 Proposed fix with reverse sorting
- `@SneakyThrows` public void clearOutput() { if (Files.exists(destination)) { try (var stream = Files.walk(destination)) { stream.filter(it -> !it.equals(destination)) + .sorted(java.util.Comparator.reverseOrder()) .forEach(this::deletePath); } catch (IOException e) { log.error("Error cleaning", e); } } }src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbol.java (1)
44-77: Prevent cyclic hyperlink recursion when resolving return types.If hyperlinks form cycles,
getReturnedValue()can recurse indefinitely. Add visited-link tracking to break cycles.🛠️ Possible fix
+import java.util.HashSet; +import java.util.Set; @@ - public TypeDescription getReturnedValue() { + public TypeDescription getReturnedValue() { + return getReturnedValue(new HashSet<>()); + } + + private TypeDescription getReturnedValue(Set<String> visited) { var result = fullDescription .map(MethodDescription::getReturnedValue) .filter(it -> !it.isEmpty()) .map(it -> it.get(0)) .orElse(null); - if (result != null) { - return createTypeDescription(result); - } else { - return null; - } + return result != null ? createTypeDescription(result, visited) : null; } - private TypeDescription createTypeDescription(TypeDescription baseDescription) { + private TypeDescription createTypeDescription(TypeDescription baseDescription, Set<String> visited) { TypeDescription result = null; HyperlinkTypeDescription hyperlinkTypeDescription = null; if (baseDescription instanceof HyperlinkTypeDescription typeDescription) { hyperlinkTypeDescription = typeDescription; } if (hyperlinkTypeDescription != null && !Strings.isNullOrEmpty(hyperlinkTypeDescription.hyperlink().link())) { - var methodInfo = BslContext.getCurrent().getMethodInfo(hyperlinkTypeDescription.hyperlink().link()); + var link = hyperlinkTypeDescription.hyperlink().link(); + if (!visited.add(link)) { + log.warn("Cyclic link: {}", link); + return baseDescription; + } + var methodInfo = BslContext.getCurrent().getMethodInfo(link); if (methodInfo != null && methodInfo.getMethod() != null) { - result = methodInfo.getMethod().getReturnedValue(); + result = methodInfo.getMethod().getReturnedValue(visited); } else { - log.warn("Bad link: {}", hyperlinkTypeDescription.hyperlink().link()); + log.warn("Bad link: {}", link); } } if (result == null) { result = baseDescription; } return result; }src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/MethodSymbolComputer.java (1)
112-124: Clear region state per compute and guard null ranges.
regionData/regionsare never reset, so repeatedcompute()calls can leak prior regions. Also guard against nullregion.rangeandmethod.getRange()to avoid NPEs in post-binding.🛠️ Possible fix
public List<MethodSymbol> compute(BSLTokenizer tokenizer) { this.tokenizer = tokenizer; methods.clear(); + regionData.clear(); + regions.clear(); visitFile(tokenizer.getAst()); // Используется постпривязка регионов, тк порядок обхода не соответствует порядку следования объект (в метод заходит раньше региона) for (var region : regionData) { + if (region.range == null) { + continue; + } for (var method : methods) { - if (SimpleRange.containsRange(region.range, method.getRange())) { + if (method.getRange() != null && SimpleRange.containsRange(region.range, method.getRange())) { method.setRegion(region.region); } } } return methods; }
🧹 Nitpick comments (8)
src/main/java/ru/alkoleft/bsl/doc/structure/MDObjectItem.java (1)
6-8: Consider adding null validation for theobjectparameter.If
nullis passed to this constructor, an NPE will occur onobject.getName(). Adding a null check or usingObjects.requireNonNull(object)would provide a clearer error message and fail-fast behavior.🛠️ Optional defensive improvement
+import java.util.Objects; + public class MDObjectItem extends Item { public MDObjectItem(MD object) { + Objects.requireNonNull(object, "MD object must not be null"); super(object, object.getName()); }src/main/java/ru/alkoleft/bsl/doc/model/ContentModelBuilder.java (1)
22-22: RedundantgetName(0)call on single-element path.
it.getFileName()already returns the filename as a single-elementPath. Calling.getName(0)on it is unnecessary.♻️ Suggested simplification
- .map(it -> new Page(it, it.getFileName().getName(0).toString(), PageType.MANUAL)) + .map(it -> new Page(it, it.getFileName().toString(), PageType.MANUAL))src/main/java/ru/alkoleft/bsl/doc/commands/RenderCommandOptions.java (1)
51-62: Consider handling the case where file read fails gracefully.
@SneakyThrowswill propagateIOExceptionunchecked ifFiles.readString(path)fails (e.g., file not found, permission denied). Depending on usage, you may want to log and returnnullor throw a domain-specific exception with context about which path failed.src/main/java/ru/alkoleft/bsl/doc/AutodocManager.java (1)
52-52: Result ofTitleProcessor.Factory.create()is not used.This call appears to be for side effects (likely initializing a singleton). Consider adding a brief comment to clarify the intent, or storing the result if it's needed later.
src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.java (2)
36-42: Fragile JSON config string construction.The JSON string with manual escaping for Windows paths (
replace("\\", "\\\\")) is error-prone. Consider using a JSON library or resource file for test configurations.
54-57:getResourcemay return null.If the resource is not found,
getClass().getClassLoader().getResource(name)returnsnull, andObjects.requireNonNullwill throwNullPointerExceptionwithout a descriptive message.♻️ Add descriptive message
`@SneakyThrows` private Path getResource(String name) { - return Path.of(Objects.requireNonNull(getClass().getClassLoader().getResource(name)).toURI()); + return Path.of(Objects.requireNonNull( + getClass().getClassLoader().getResource(name), + "Test resource not found: " + name).toURI()); }build.gradle.kts (2)
34-36: Review repository configuration for reproducibility.Two observations:
mavenLocal()can cause build reproducibility issues as it may contain different artifacts across machines.- The Sonatype snapshots repository is configured but no snapshot dependencies are present in the project. Consider removing it to reduce repository resolution overhead.
30-30: Update JUnit 5 to the latest version.JUnit 5.8.2 is from late 2021. The latest stable version is 5.13.4 (July 2025), which includes significant improvements: bug fixes, performance enhancements, and new features like
@TempDirenhancements (meta-annotation support, customizableTempDirFactorySPI, improved Windows cleanup, better lifecycle semantics, and enhanced exception diagnostics).
| id("com.gorylenko.gradle-git-properties") version "2.4.1" | ||
| id("io.freefair.lombok") version "8.4" | ||
| id("maven-publish") | ||
| id("org.springframework.boot") version "2.7.10" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Boot 2.7 end of life support date
💡 Result:
-
Community (open-source) end-of-life / end of community support for Spring Boot 2.7: November 24, 2023. [1][2]
-
Commercial (paid) support window was extended to August 2025 (previously scheduled to end February 2025). [3][4]
Sources:
[1] Spring Boot blog (2.6.14 post noting 2.7 OSS support extension to Nov 2023).
[2] Spring Boot GitHub / project notes confirming 2.7 OSS end-of-support date (Nov 24, 2023).
[3] Spring blog — "Spring Commercial Support Extension" (announces commercial support extension to August 2025).
[4] Spring blog — "Updates to Spring Support Policy" (policy / support timeline context).
Consider upgrading from Spring Boot 2.7.10, which reached end of community support in November 2023.
Spring Boot 2.7.x no longer receives security patches or bug fixes in the open-source track. Your project already has Java 17 configured, so you can upgrade to Spring Boot 3.x. Be aware that this requires updating imports to Jakarta EE namespaces and will need dependency adjustments, but the upgrade path is well-documented and you have the Java version prerequisite in place.
🤖 Prompt for AI Agents
In `@build.gradle.kts` at line 8, The project is using Spring Boot 2.7.10
(id("org.springframework.boot") version "2.7.10"), which is EOL; update the
plugin version to a Spring Boot 3.x release compatible with Java 17 and then
migrate code and dependencies to Jakarta EE namespaces (javax -> jakarta) and
any updated dependency coordinates; specifically change the Gradle plugin
version, update Spring-related dependencies (starter artifacts,
spring-boot-starter-*, spring-boot-gradle-plugin usages), run the Spring Boot 3
migration checklist (API changes, actuator, security, test libs), and adjust
imports/usages across classes (controllers, filters, config classes) to
jakarta.* to resolve compilation and runtime issues.
| implementation("io.github.1c-syntax", "bsl-common-library", "0.9.2") | ||
| implementation("io.github.1c-syntax", "bsl-parser", "0.30.0") | ||
| // logging | ||
| implementation("org.slf4j", "slf4j-api", "2.1.0-alpha1") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
slf4j latest stable version release 2025
💡 Result:
The latest stable SLF4J release is 2.0.17, published February 25, 2025. [1][2]
Sources:
- SLF4J download page (stable version 2.0.17). [1]
- SLF4J news: "Release of SLF4J 2.0.17" (2025-02-25). [2]
Use stable version of slf4j instead of alpha release.
slf4j-api:2.1.0-alpha1 is an alpha release and unsuitable for production. Use the latest stable version 2.0.17 instead.
🤖 Prompt for AI Agents
In `@build.gradle.kts` at line 55, Replace the alpha SLF4J dependency version with
the stable release: locate the dependency declaration
implementation("org.slf4j", "slf4j-api", "2.1.0-alpha1") in the build.gradle.kts
and change the version string to "2.0.17" so the project uses the stable
slf4j-api 2.0.17 release.
| implementation("com.fasterxml.jackson.core:jackson-annotations:$jacksonVersion") | ||
|
|
||
| // template engine | ||
| implementation("com.github.jknack:handlebars:4.3.1") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Maven Central repository info on jknack handlebars
curl -s "https://api.github.com/repos/jknack/handlebars.java/releases" | head -50Repository: 1CDevFlow/BSL-Doc
Length of output: 178
🌐 Web query:
Handlebars jknack 4.3.1 security vulnerabilities CVE
💡 Result:
Handlebars (jknack) 4.3.1 is reported by package trackers to be affected by multiple CVEs. Notable entries include CVE-2019-19919 (prototype‑pollution → RCE in Handlebars.js family) and more recent component CVEs reported against dependencies (e.g., CVE-2025-48924). For references and full lists see:
- Handlebars.java 4.3.1 package page (vulnerability summary). [1]
- NVD entry for CVE-2019-19919 (prototype pollution / RCE). [2]
- Advisory/details for CVE-2025-48924 (dependency issue cited against builds using 4.3.1). [3]
If you want, I can enumerate every CVE shown for 4.3.1 (with dates, severity and remediation for each) and suggest the minimum safe upgrade.
Sources:
[1] https://mvnrepository.com/artifact/com.github.jknack/handlebars.java/4.3.1
[2] https://nvd.nist.gov/vuln/detail/CVE-2019-19919
[3] https://www.tenable.com/cve/CVE-2025-48924
🌐 Web query:
handlebars.java latest version 2025 Maven Central
💡 Result:
Latest Maven Central version: com.github.jknack:handlebars.java 4.5.0 (released Aug 7, 2025). [1][2]
Maven dependency:
com.github.jknack
handlebars.java
4.5.0
Sources: [1] mvnrepository listing for handlebars.java 4.5.0. [2] GitHub release v4.5.0.
Upgrade Handlebars to 4.5.0 — security vulnerabilities in 4.3.1.
Version 4.3.1 has known security vulnerabilities, including CVE-2019-19919 (prototype-pollution/RCE) and CVE-2025-48924. Upgrade to 4.5.0 (latest available version as of August 2025).
🤖 Prompt for AI Agents
In `@build.gradle.kts` at line 63, The Handlebars dependency uses an insecure
version string implementation("com.github.jknack:handlebars:4.3.1"); update that
dependency declaration to use version 4.5.0
(implementation("com.github.jknack:handlebars:4.5.0")), then refresh/update
Gradle dependencies (re-run dependency resolution/refresh or update lockfiles)
and rebuild to ensure the new version is picked up and tests pass.
| implementation("commons-io", "commons-io", "2.18.0") | ||
|
|
||
| // tests | ||
| testImplementation("org.slf4j", "slf4j-reload4j", "2.1.0-alpha1") |
There was a problem hiding this comment.
Test dependency also uses alpha slf4j version.
slf4j-reload4j:2.1.0-alpha1 should match the stable version chosen for the main slf4j-api dependency to ensure consistency.
🤖 Prompt for AI Agents
In `@build.gradle.kts` at line 68, The test dependency declares slf4j-reload4j
with an alpha version ("slf4j-reload4j", "2.1.0-alpha1"); update this to the
same stable version used for the main slf4j-api dependency (replace
"2.1.0-alpha1" with the project's chosen stable slf4j version) so both main and
test dependencies match and avoid mixing alpha/stable artifacts.
| public String getPresent(MD object) { | ||
| return object.getDescription(); | ||
| } | ||
|
|
||
| public String getPresent(ModuleType moduleType) { | ||
| return switch (moduleType) { | ||
| case ManagerModule -> "Модуль менеджера"; | ||
| case BotModule, ObjectModule -> "Модуль объекта"; | ||
| case HTTPServiceModule -> "Модуль http-сервиса"; | ||
| case WEBServiceModule -> "Модуль web-сервиса"; | ||
| case CommonModule -> "Модуль"; | ||
| case CommandModule -> "Модуль команды"; | ||
| case RecordSetModule -> "Модуль набора записей"; | ||
| case ValueManagerModule -> "Модуль менеджера значений"; | ||
| default -> moduleType.name(); | ||
| }; |
There was a problem hiding this comment.
Add null-safety in getPresent(...) to avoid NPEs.
If a caller passes null (e.g., owner not resolved or missing module type), these methods will throw. Consider guarding and returning an empty string (or another safe default).
✅ Suggested guard
public String getPresent(MD object) {
- return object.getDescription();
+ if (object == null) {
+ return "";
+ }
+ return java.util.Objects.toString(object.getDescription(), "");
}
public String getPresent(ModuleType moduleType) {
+ if (moduleType == null) {
+ return "";
+ }
return switch (moduleType) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public String getPresent(MD object) { | |
| return object.getDescription(); | |
| } | |
| public String getPresent(ModuleType moduleType) { | |
| return switch (moduleType) { | |
| case ManagerModule -> "Модуль менеджера"; | |
| case BotModule, ObjectModule -> "Модуль объекта"; | |
| case HTTPServiceModule -> "Модуль http-сервиса"; | |
| case WEBServiceModule -> "Модуль web-сервиса"; | |
| case CommonModule -> "Модуль"; | |
| case CommandModule -> "Модуль команды"; | |
| case RecordSetModule -> "Модуль набора записей"; | |
| case ValueManagerModule -> "Модуль менеджера значений"; | |
| default -> moduleType.name(); | |
| }; | |
| public String getPresent(MD object) { | |
| if (object == null) { | |
| return ""; | |
| } | |
| return java.util.Objects.toString(object.getDescription(), ""); | |
| } | |
| public String getPresent(ModuleType moduleType) { | |
| if (moduleType == null) { | |
| return ""; | |
| } | |
| return switch (moduleType) { | |
| case ManagerModule -> "Модуль менеджера"; | |
| case BotModule, ObjectModule -> "Модуль объекта"; | |
| case HTTPServiceModule -> "Модуль http-сервиса"; | |
| case WEBServiceModule -> "Модуль web-сервиса"; | |
| case CommonModule -> "Модуль"; | |
| case CommandModule -> "Модуль команды"; | |
| case RecordSetModule -> "Модуль набора записей"; | |
| case ValueManagerModule -> "Модуль менеджера значений"; | |
| default -> moduleType.name(); | |
| }; | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/helpers/MDOHelper.java` around lines 16
- 31, Add null-safety guards to both getPresent methods: check if the MD
parameter is null in getPresent(MD object) and return an empty string (or other
safe default) instead of calling object.getDescription(); similarly, check if
the ModuleType moduleType is null in getPresent(ModuleType moduleType) and
return "" (or a safe default) before the switch, otherwise proceed with the
existing switch logic and default case.
| public boolean isRequired() { | ||
| return defaultValue.type == ParameterType.EMPTY; | ||
| } |
There was a problem hiding this comment.
Guard against null defaultValue in isRequired().
defaultValue is not annotated as non-null and the builder can pass null, which would throw. Consider a defensive check.
🔧 Suggested fix
- public boolean isRequired() {
- return defaultValue.type == ParameterType.EMPTY;
- }
+ public boolean isRequired() {
+ return defaultValue == null || defaultValue.type() == ParameterType.EMPTY;
+ }🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/bsl/symbols/ParameterDefinition.java`
around lines 32 - 34, isRequired() currently dereferences defaultValue and can
NPE if the builder sets it to null; update ParameterDefinition.isRequired() to
defensively handle a null defaultValue (e.g., treat null as "required") by
checking defaultValue for null before accessing defaultValue.type and return the
appropriate boolean based on ParameterType.EMPTY; reference
ParameterDefinition.isRequired(), defaultValue, and ParameterType.EMPTY when
making the change.
| } catch (IOException e) { | ||
| log.error("Error cleaning", e); | ||
| } |
There was a problem hiding this comment.
Incorrect error message - likely copy-paste error.
The log message says "Error cleaning" but this method is building a content model, not cleaning.
🐛 Proposed fix
} catch (IOException e) {
- log.error("Error cleaning", e);
+ log.error("Error building content model", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (IOException e) { | |
| log.error("Error cleaning", e); | |
| } | |
| } catch (IOException e) { | |
| log.error("Error building content model", e); | |
| } |
🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/model/ContentModelBuilder.java` around
lines 25 - 27, The catch block in ContentModelBuilder currently logs "Error
cleaning" which is a wrong copy-paste message; update the log.error call inside
the catch (IOException e) in class ContentModelBuilder to a descriptive message
such as "Error building content model" or "Error building ContentModel in
ContentModelBuilder" so the log correctly reflects the operation and include the
exception variable e as the second parameter.
| public SubsystemContext create(Subsystem subsystem, int index, int level) { | ||
| return SubsystemContext.builder() | ||
| .subsystem(subsystem) | ||
| .index(index) | ||
| .name(subsystem.getName()) | ||
| .present(MDOHelper.getPresent(subsystem)) | ||
| .description(subsystem.getComment()) | ||
| .explanation(subsystem.getExplanation().get("ru")) // Берем русскую локаль. Если ее не будет, то вернет любую | ||
| .level(level) | ||
| .build(); |
There was a problem hiding this comment.
getExplanation().get("ru") won’t fallback—may yield null.
Map#get returns null when the key is absent, so the comment is misleading. If you want a fallback locale, pick the first available value.
🛠️ Suggested fallback
public SubsystemContext create(Subsystem subsystem, int index, int level) {
+ var explanations = subsystem.getExplanation();
+ String explanation = null;
+ if (explanations != null) {
+ explanation = explanations.get("ru");
+ if (explanation == null && !explanations.isEmpty()) {
+ explanation = explanations.values().iterator().next();
+ }
+ }
return SubsystemContext.builder()
.subsystem(subsystem)
.index(index)
.name(subsystem.getName())
.present(MDOHelper.getPresent(subsystem))
.description(subsystem.getComment())
- .explanation(subsystem.getExplanation().get("ru")) // Берем русскую локаль. Если ее не будет, то вернет любую
+ .explanation(explanation) // Берем русскую локаль, иначе любую доступную
.level(level)
.build();
}🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/contexts/ContextFactory.java` around
lines 30 - 39, The call subsystem.getExplanation().get("ru") in
ContextFactory.create can return null when the "ru" key is missing; update
ContextFactory.create to safely obtain an explanation by checking
subsystem.getExplanation() for the "ru" entry and, if absent or null/blank, fall
back to the first available map value (e.g., stream map.values().findFirst())
before passing the value into SubsystemContext.builder().explanation(...),
ensuring you handle null/empty maps gracefully (use Optional or null checks) so
SubsystemContext always receives a non-null fallback when possible.
| public Object apply(Object context, Options options) { | ||
| if (context instanceof MD md) { | ||
| return MDOHelper.getPresent(md); | ||
| } else if (context instanceof SubsystemContext subsystemContext) { | ||
| return MDOHelper.getPresent(subsystemContext.getSubsystem()); | ||
| } else { | ||
| return context.toString(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "MdoPresent.java" | head -20Repository: 1CDevFlow/BSL-Doc
Length of output: 138
🏁 Script executed:
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/MdoPresent.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 891
🏁 Script executed:
find ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers -type f -name "*.java" | head -20Repository: 1CDevFlow/BSL-Doc
Length of output: 516
🏁 Script executed:
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/SingleLine.java
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/PageLink.java
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/Shifter.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 2375
🏁 Script executed:
find . -type f -name "*Test.java" | xargs grep -l "MdoPresent\|SingleLine\|Helper" | head -10Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🏁 Script executed:
find . -type f -name "*.java" -path "*/test/*" | head -20Repository: 1CDevFlow/BSL-Doc
Length of output: 128
🏁 Script executed:
cat -n ./src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 2302
🏁 Script executed:
rg "Helper<Object>" --type java -A 5 -B 2 | head -80Repository: 1CDevFlow/BSL-Doc
Length of output: 3449
🏁 Script executed:
rg "new.*Helper\|register.*Helper" --type java -A 3 -B 3 | head -100Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🏁 Script executed:
rg "Handlebars\|HandlebarsEngine\|registerHelper" --type java -B 3 -A 3 | head -150Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🏁 Script executed:
find . -type f -name "*.java" | xargs rg "MdoPresent\|SingleLine\|Shifter" | head -30Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🏁 Script executed:
rg "Helper" --type java -l | head -20Repository: 1CDevFlow/BSL-Doc
Length of output: 840
🏁 Script executed:
cat -n ./src/main/java/ru/alkoleft/bsl/doc/render/handlebars/RenderContext.javaRepository: 1CDevFlow/BSL-Doc
Length of output: 5735
🏁 Script executed:
find . -type f -name "pom.xml" | head -5Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🏁 Script executed:
find . -type f \( -name "build.gradle" -o -name "pom.xml" -o -name "gradle.properties" \) | head -10Repository: 1CDevFlow/BSL-Doc
Length of output: 43
🌐 Web query:
Handlebars Java library null context helper
💡 Result:
Yes — in Handlebars.java a helper can receive a null context (and you can also provide a global fallback via helperMissing). Two approaches:
- Simple null-check helper (register and use as block helper):
-
Java:
Handlebars handlebars = new Handlebars();
handlebars.registerHelper("isNull", new Helper() {
@Override
public CharSequence apply(Object context, Options options) throws IOException {
if (context == null) {
return options.fn(); // render {{#isNull...}}...{{/isNull}}
} else {
return options.inverse(); // render {{^isNull ...}}...{{/isNull}} (or else branch)
}
}
}); -
Template usage:
{{#isNullsomeValue}}someValue is null{{else}}someValue is not null{{/isNull}} - Register a helperMissing to handle unresolved variables or provide defaults:
- Java:
handlebars.registerHelperMissing(new Helper() {
@Override
public CharSequence apply(Object context, Options options) {
// return default text or options.fn() etc.
return options.hash("default") != null ? options.hash("default").toString() : "";
}
});Notes:
- The built-in {{
#if}} / {{unless}} treat null/undefined as falsy and can often avoid a custom helper. [1][2]
Sources:
- Handlebars.java GitHub docs (helpers, helperMissing). [1]
- IfHelper / built-in behaviour (null is falsy). [2]
Add null guard to prevent NullPointerException when Handlebars passes null context.
The
apply()method callscontext.toString()in the else branch without checking if context is null. Handlebars can pass null to helpers, which will cause a crash. Add a null check at the method start to return a safe fallback.Suggested fix
public Object apply(Object context, Options options) { + if (context == null) { + return ""; + } if (context instanceof MD md) { return MDOHelper.getPresent(md); } else if (context instanceof SubsystemContext subsystemContext) { return MDOHelper.getPresent(subsystemContext.getSubsystem()); } else { return context.toString(); } }🤖 Prompt for AI Agents
In `@src/main/java/ru/alkoleft/bsl/doc/render/handlebars/helpers/MdoPresent.java` around lines 11 - 18, The apply method in MdoPresent calls context.toString() without null-checking, causing a NullPointerException when Handlebars passes null; update MdoPresent.apply to early-return a safe fallback (e.g., empty string) if context is null, otherwise keep existing branches for MD (use MDOHelper.getPresent(md)) and SubsystemContext (use MDOHelper.getPresent(subsystemContext.getSubsystem())); ensure the null guard is the very first check so MD and SubsystemContext instanceof checks are only reached with non-null context. - The built-in {{
| void run() { | ||
| RenderCommand.builder() | ||
| .sources(getResource("configuration")) | ||
| .destination(Path.of("/tmp/bsl-doc-fixture")) | ||
| .format(OutputFormat.ConfluenceMarkdown) | ||
| .onlySubsystems(List.of("ППИ")) | ||
| .regions(List.of(RegionSymbol.PUBLIC_REGION_RU)) | ||
| .manualDocumentation(getResource("docs")) | ||
| .manualMergeStrategy(ManualMergeStrategy.MERGE) | ||
| .hierarchy(OutputHierarchy.SUBSYSTEM) | ||
| .build() | ||
| .run(); | ||
| } |
There was a problem hiding this comment.
Hardcoded /tmp path and missing assertions.
Two concerns with this test:
-
Line 22: Using a hardcoded
/tmp/bsl-doc-fixturepath instead of@TempDircan cause test pollution, platform incompatibility (Windows), and permission issues. -
No assertions: The test only verifies that no exception is thrown (smoke test). Consider adding assertions to verify expected output files are created or content is correct.
🐛 Proposed fix for hardcoded path
`@Test`
- void run() {
+ void run(`@TempDir` Path tempDir) {
RenderCommand.builder()
.sources(getResource("configuration"))
- .destination(Path.of("/tmp/bsl-doc-fixture"))
+ .destination(tempDir.resolve("bsl-doc-fixture"))
.format(OutputFormat.ConfluenceMarkdown)
// ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void run() { | |
| RenderCommand.builder() | |
| .sources(getResource("configuration")) | |
| .destination(Path.of("/tmp/bsl-doc-fixture")) | |
| .format(OutputFormat.ConfluenceMarkdown) | |
| .onlySubsystems(List.of("ППИ")) | |
| .regions(List.of(RegionSymbol.PUBLIC_REGION_RU)) | |
| .manualDocumentation(getResource("docs")) | |
| .manualMergeStrategy(ManualMergeStrategy.MERGE) | |
| .hierarchy(OutputHierarchy.SUBSYSTEM) | |
| .build() | |
| .run(); | |
| } | |
| void run(`@TempDir` Path tempDir) { | |
| RenderCommand.builder() | |
| .sources(getResource("configuration")) | |
| .destination(tempDir.resolve("bsl-doc-fixture")) | |
| .format(OutputFormat.ConfluenceMarkdown) | |
| .onlySubsystems(List.of("ППИ")) | |
| .regions(List.of(RegionSymbol.PUBLIC_REGION_RU)) | |
| .manualDocumentation(getResource("docs")) | |
| .manualMergeStrategy(ManualMergeStrategy.MERGE) | |
| .hierarchy(OutputHierarchy.SUBSYSTEM) | |
| .build() | |
| .run(); | |
| } |
🤖 Prompt for AI Agents
In `@src/test/java/ru/alkoleft/bsl/doc/commands/RenderCommandTest.java` around
lines 19 - 31, The test uses a hardcoded destination Path in
RenderCommand.builder() which should be replaced by a JUnit `@TempDir` Path
injected into the test method (replace the hardcoded
Path.of("/tmp/bsl-doc-fixture") with the temp dir.resolve(...) value) to avoid
platform/permission issues; also add concrete assertions after .run() (e.g.,
check that expected files exist under the destination, verify contents or
presence of key strings in generated files) to ensure the command produced the
correct outputs instead of merely not throwing exceptions—locate the test method
named run() in RenderCommandTest and update the builder invocation and
assertions accordingly.
1 participant
Собрал в одном реквесте фиксы с разных репозиториев
Summary by CodeRabbit
Release Notes
New Features
Build & Infrastructure
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.