fix #1679: добавлен метод ОткрытьПоток() объекта ЧтениеJSON#1680
Open
Mr-Rm wants to merge 4 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds JSONReader.OpenStream to initialize the internal JSON reader from an IStreamWrapper (optional encoding, default UTF-8), makes OpenFile/SetString always call Close() before opening, reorders FileStreamContext interfaces, and adds tests that read JSON {"ответ":42} from a memory stream (including repeated open and reopen-after-close scenarios). ChangesJSON stream reading support
Sequence Diagram(s)sequenceDiagram
participant Caller
participant JSONReader
participant IStreamWrapper
participant Stream as StreamReader
participant JsonReaderInternal
Caller->>JSONReader: OpenStream(streamContext, encoding?)
JSONReader->>IStreamWrapper: Get underlying Stream
IStreamWrapper-->>JSONReader: Stream (not null)
JSONReader->>Stream: new StreamReader(Stream, encoding, leaveOpen:true)
JSONReader->>JsonReaderInternal: new JsonReaderInternal(StreamReader) (SupportMultipleContent = true)
JSONReader-->>Caller: ready to Read/Parse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/json/test-json_reader.os (1)
225-233: ⚖️ Poor tradeoffConsider expanding test coverage.
The test correctly validates the basic happy path with
ПотокВПамяти(MemoryStreamContext). To improve robustness, consider adding tests for:
- The optional
encodingparameter with non-UTF-8 encodings- Error cases: invalid stream context type, null argument
- Other stream context types (
FileStreamContextorGenericStream)- Stream state after reading (verify
Close()behavior)These additions would help ensure the implementation handles edge cases correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/json/test-json_reader.os` around lines 225 - 233, Add additional test cases alongside Тест_Должен_ПроверитьОткрытиеПотока to cover edge cases: create a test that passes a ПотокВПамяти with a non-UTF8 encoding and assert correct parsing using ЧтениеJSON and ПрочитатьJSON; add tests that call Чтение.ОткрытьПоток with an invalid stream context type and with null and assert that the right exceptions are raised; add a test using FileStreamContext or GenericStream to ensure file-backed streams parse the same way; and add a test that reads then checks stream state/Close() behavior after ПрочитатьJSON to verify the stream is closed or left open as intended.src/OneScript.StandardLibrary/Json/JSONReader.cs (1)
280-300: ⚡ Quick winAdd exception handling for consistency with
OpenFile.The
OpenFilemethod (lines 252-262) wraps all file and encoding operations in a try-catch block and converts exceptions toRuntimeException. For API consistency,OpenStreamshould follow the same pattern, sinceGetUnderlyingStream(),TextEncodingEnum.GetEncoding(), and theStreamReaderconstructor can all throw exceptions.🔄 Proposed exception handling
public void OpenStream(IValue streamContext, IValue encoding = null) { if (IsOpen()) Close(); + try + { var stream = streamContext switch { GenericStream s => s.GetUnderlyingStream(), FileStreamContext s => s.GetUnderlyingStream(), MemoryStreamContext s => s.GetUnderlyingStream(), _ => throw RuntimeException.InvalidNthArgumentType(1) }; var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8; _reader = new JsonReaderInternal(new StreamReader(stream, enc)) { SupportMultipleContent = true }; + } + catch (Exception e) + { + throw new RuntimeException(e.Message, e); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Json/JSONReader.cs` around lines 280 - 300, OpenStream lacks the try-catch used in OpenFile, so exceptions from GetUnderlyingStream, TextEncodingEnum.GetEncoding or StreamReader/JsonReaderInternal are not converted to RuntimeException; wrap the body of OpenStream in a try-catch that mirrors OpenFile: catch any Exception and rethrow as RuntimeException (use the same message/format used in OpenFile), while keeping the same behavior of closing an already open reader via IsOpen()/Close() and preserving SupportMultipleContent on JsonReaderInternal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs`:
- Around line 280-300: OpenStream lacks the try-catch used in OpenFile, so
exceptions from GetUnderlyingStream, TextEncodingEnum.GetEncoding or
StreamReader/JsonReaderInternal are not converted to RuntimeException; wrap the
body of OpenStream in a try-catch that mirrors OpenFile: catch any Exception and
rethrow as RuntimeException (use the same message/format used in OpenFile),
while keeping the same behavior of closing an already open reader via
IsOpen()/Close() and preserving SupportMultipleContent on JsonReaderInternal.
In `@tests/json/test-json_reader.os`:
- Around line 225-233: Add additional test cases alongside
Тест_Должен_ПроверитьОткрытиеПотока to cover edge cases: create a test that
passes a ПотокВПамяти with a non-UTF8 encoding and assert correct parsing using
ЧтениеJSON and ПрочитатьJSON; add tests that call Чтение.ОткрытьПоток with an
invalid stream context type and with null and assert that the right exceptions
are raised; add a test using FileStreamContext or GenericStream to ensure
file-backed streams parse the same way; and add a test that reads then checks
stream state/Close() behavior after ПрочитатьJSON to verify the stream is closed
or left open as intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a309e50b-9baa-46fc-8be3-1f4f577b6437
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Json/JSONReader.cstests/json/test-json_reader.os
Collaborator
|
Огонь огонь! |
EvilBeaver
reviewed
May 13, 2026
| if (IsOpen()) | ||
| Close(); | ||
|
|
||
| var stream = streamContext switch |
Owner
There was a problem hiding this comment.
а просто IStreamWrapper не пойдет? Они все его реализуют
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/OneScript.StandardLibrary/Json/JSONReader.cs (2)
285-291: ⚡ Quick winConsider whether to add error handling to
OpenStreamfor consistency withOpenFile.The
OpenFilemethod (lines 252-262) wraps stream operations in a try-catch block to convert exceptions toRuntimeException. However, theOpenStreammethod (lines 280-292) lacks similar error handling for operations likeGetUnderlyingStream(),TextEncodingEnum.GetEncoding(), or theStreamReaderconstructor, creating inconsistency between the two public API methods.Note: Other similar
OpenStreammethods in the codebase (e.g.,TextReadImpl,TextWriteImpl) do not include try-catch blocks around stream operations, suggesting this may be an intentional design choice. Adding error handling would improve intra-class consistency but would diverge from the broader codebase pattern forOpenStreammethods.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Json/JSONReader.cs` around lines 285 - 291, OpenStream currently calls streamContext.GetUnderlyingStream(), TextEncodingEnum.GetEncoding(encoding) and new StreamReader(...) to create JsonReaderInternal without guarding exceptions; make it consistent with OpenFile by wrapping those operations in a try-catch that catches Exception and rethrows a RuntimeException with a clear message (include the original exception as inner/innerException) so callers get the same RuntimeException behavior as OpenFile; update the block that constructs _reader (the JsonReaderInternal(new StreamReader(...)) assignment) to be inside this try-catch and reference the same symbols: OpenStream, streamContext.GetUnderlyingStream(), TextEncodingEnum.GetEncoding, StreamReader, JsonReaderInternal and RuntimeException.
270-292: ⚖️ Poor tradeoffDocument that OpenStream takes ownership of the provided stream and will dispose it.
When
Close()is called (line 230), it calls_reader.Close()on theJsonTextReader, which closes its underlyingStreamReader(created at line 288). This disposes the stream obtained fromIStreamWrapper.GetUnderlyingStream(), preventing the caller from reusing it.While this behavior mirrors
OpenFile, which also closes its underlying stream, the disposal semantics differ:OpenFileowns resources it creates, whereasOpenStreamdisposes a caller-provided resource. Add a remark to the XML documentation clarifying that the stream is taken over and will be disposed when the reader closes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OneScript.StandardLibrary/Json/JSONReader.cs` around lines 270 - 292, OpenStream currently takes the stream returned by IStreamWrapper.GetUnderlyingStream(), wraps it in a StreamReader for JsonReaderInternal stored in _reader, and Close() calls on this reader will dispose that underlying stream; update the XML documentation for OpenStream (method OpenStream) to explicitly state that the provided stream is taken ownership of by the JSONReader and will be disposed when the reader is closed (e.g., when Close() is called), referencing that _reader (JsonReaderInternal/JsonTextReader) closes the underlying StreamReader and stream so callers should not expect the stream to remain usable after calling Close().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs`:
- Around line 280-285: The OpenStream method lacks a null check for its
streamContext parameter; before calling streamContext.GetUnderlyingStream() add
a defensive validation that throws an ArgumentNullException (or returns an
appropriate error) when streamContext is null. Locate OpenStream in
JSONReader.cs and insert the null check at the top (after the IsOpen/Close
handling if desired) so the call to GetUnderlyingStream() is only made on a
non-null streamContext. Ensure the exception message names the parameter
("streamContext") to aid callers.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs`:
- Around line 285-291: OpenStream currently calls
streamContext.GetUnderlyingStream(), TextEncodingEnum.GetEncoding(encoding) and
new StreamReader(...) to create JsonReaderInternal without guarding exceptions;
make it consistent with OpenFile by wrapping those operations in a try-catch
that catches Exception and rethrows a RuntimeException with a clear message
(include the original exception as inner/innerException) so callers get the same
RuntimeException behavior as OpenFile; update the block that constructs _reader
(the JsonReaderInternal(new StreamReader(...)) assignment) to be inside this
try-catch and reference the same symbols: OpenStream,
streamContext.GetUnderlyingStream(), TextEncodingEnum.GetEncoding, StreamReader,
JsonReaderInternal and RuntimeException.
- Around line 270-292: OpenStream currently takes the stream returned by
IStreamWrapper.GetUnderlyingStream(), wraps it in a StreamReader for
JsonReaderInternal stored in _reader, and Close() calls on this reader will
dispose that underlying stream; update the XML documentation for OpenStream
(method OpenStream) to explicitly state that the provided stream is taken
ownership of by the JSONReader and will be disposed when the reader is closed
(e.g., when Close() is called), referencing that _reader
(JsonReaderInternal/JsonTextReader) closes the underlying StreamReader and
stream so callers should not expect the stream to remain usable after calling
Close().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38b2e98e-4bdd-4294-a3fe-90a0effac490
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Binary/FileStreamContext.cssrc/OneScript.StandardLibrary/Json/JSONReader.cs
✅ Files skipped from review due to trivial changes (1)
- src/OneScript.StandardLibrary/Binary/FileStreamContext.cs
Owner
|
@Mr-Rm вижу WIP, еще нельзя вливать? |
Collaborator
Author
|
Пока WIP, есть проблемы с закрытием-открытием потока |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/json/test-json_reader.os (1)
227-235: 💤 Low valueConsider fixing indentation consistency.
The first line of the procedure body (line 228) appears to use spaces for indentation instead of tabs, which is inconsistent with the rest of the file's indentation style.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/json/test-json_reader.os` around lines 227 - 235, The procedure Тест_Должен_ПроверитьОткрытиеПотока() has inconsistent indentation: the first line of the body uses spaces instead of tabs; update the leading whitespace for the body lines (e.g., the line starting with БДД = ПолучитьБуферДвоичныхДанныхИзСтроки(...)) to use the same tab indentation style as the rest of the file so all lines inside Тест_Должен_ПроверитьОткрытиеПотока and its subsequent statements (Поток, Чтение, Чтение.ОткрытьПоток, Json, юТест.ПроверитьРавенство) are consistently indented with tabs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/json/test-json_reader.os`:
- Around line 248-257: Rename and rewrite the
Тест_Должен_ПроверитьСохранениеПотокаПослеЗакрытияЧтения test so it actually
verifies the stream remains open after closing the reader: instantiate Поток =
Новый ПотокВПамяти(...), call Чтение.ОткрытьПоток(Поток) (optionally read via
ПрочитатьJSON to advance), then call Чтение.Закрыть(), assert the Поток is still
usable by setting Поток.Позиция = 0 (or reading again) and reopening/using the
reader (ОткрытьПоток/ПрочитатьJSON) to confirm data can still be read; ensure
you reference the existing methods ОткрытьПоток, Закрыть, ПрочитатьJSON and
class ПотокВПамяти, and fix indentation to match file style.
---
Nitpick comments:
In `@tests/json/test-json_reader.os`:
- Around line 227-235: The procedure Тест_Должен_ПроверитьОткрытиеПотока() has
inconsistent indentation: the first line of the body uses spaces instead of
tabs; update the leading whitespace for the body lines (e.g., the line starting
with БДД = ПолучитьБуферДвоичныхДанныхИзСтроки(...)) to use the same tab
indentation style as the rest of the file so all lines inside
Тест_Должен_ПроверитьОткрытиеПотока and its subsequent statements (Поток,
Чтение, Чтение.ОткрытьПоток, Json, юТест.ПроверитьРавенство) are consistently
indented with tabs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0af6ca48-f64e-423d-ac76-be975ad2cfe4
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Json/JSONReader.cstests/json/test-json_reader.os
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Json/JSONReader.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs`:
- Around line 214-216: OpenFile and SetString (and the other public methods
around lines 294-297 that accept concrete string parameters) currently assume
non-null inputs; add explicit null checks at the start of each such public
method to guard against direct C# callers passing null by either throwing
ArgumentNullException(nameof(param)) or otherwise handling null
deterministically (e.g., treat as empty string) — locate the methods by name
(OpenFile, SetString and the similarly patterned public string-accepting
methods) and add a clear null-guard for each string parameter with the parameter
name in the exception to provide immediate, unambiguous diagnostics.
- Around line 253-258: OpenStream currently allows .NET exceptions from
TextEncodingEnum.GetEncoding and new JsonReaderInternal to leak; mirror OpenFile
by wrapping the encoding resolution and _reader initialization in a try/catch
and throw a RuntimeException instead. Specifically, surround the calls to
TextEncodingEnum.GetEncoding(encoding) and the JsonReaderInternal/StreamReader
creation that assigns _reader with a try block, catch Exception ex, and rethrow
a new RuntimeException (include a clear message and set ex as the inner
exception) so OpenStream behavior matches OpenFile.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7bd23c7-d51c-4fc4-99a8-fce6699a4bd0
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Json/JSONReader.cstests/json/test-json_reader.os
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/json/test-json_reader.os
Comment on lines
214
to
+216
| public void OpenFile(string JSONFileName, IValue encoding = null) | ||
| { | ||
| if (IsOpen()) | ||
| Close(); | ||
| Close(); |
Contributor
There was a problem hiding this comment.
Add explicit null guards for concrete string inputs.
OpenFile and SetString accept string parameters but don’t explicitly handle null. Direct C# calls can still pass null, and failures then happen deeper with less clear diagnostics.
Proposed fix
public void OpenFile(string JSONFileName, IValue encoding = null)
{
+ if (JSONFileName == null)
+ throw new ArgumentNullException(nameof(JSONFileName));
+
Close();
StreamReader _fileReader; public void SetString(string JSONString)
{
+ if (JSONString == null)
+ throw new ArgumentNullException(nameof(JSONString));
+
Close();
_reader = new JsonReaderInternal(new StringReader(JSONString))Based on learnings: when script-side calls omit concrete string arguments they may become "", while direct calls may pass null, so public methods should explicitly handle null.
Also applies to: 294-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs` around lines 214 - 216,
OpenFile and SetString (and the other public methods around lines 294-297 that
accept concrete string parameters) currently assume non-null inputs; add
explicit null checks at the start of each such public method to guard against
direct C# callers passing null by either throwing
ArgumentNullException(nameof(param)) or otherwise handling null
deterministically (e.g., treat as empty string) — locate the methods by name
(OpenFile, SetString and the similarly patterned public string-accepting
methods) and add a clear null-guard for each string parameter with the parameter
name in the exception to provide immediate, unambiguous diagnostics.
Comment on lines
+253
to
+258
| var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8; | ||
|
|
||
| _reader = new JsonReaderInternal(new StreamReader(stream, enc, leaveOpen:true)) | ||
| { | ||
| SupportMultipleContent = true | ||
| }; |
Contributor
There was a problem hiding this comment.
Align OpenStream open/init exception behavior with OpenFile.
OpenFile wraps initialization failures into RuntimeException, while OpenStream currently leaks raw .NET exceptions from encoding resolution/reader creation. Keeping this consistent improves script-facing behavior.
Proposed fix
- var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8;
-
- _reader = new JsonReaderInternal(new StreamReader(stream, enc, leaveOpen:true))
- {
- SupportMultipleContent = true
- };
+ try
+ {
+ var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8;
+
+ _reader = new JsonReaderInternal(new StreamReader(stream, enc, leaveOpen: true))
+ {
+ SupportMultipleContent = true
+ };
+ }
+ catch (Exception e)
+ {
+ throw new RuntimeException(e.Message, 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.
Suggested change
| var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8; | |
| _reader = new JsonReaderInternal(new StreamReader(stream, enc, leaveOpen:true)) | |
| { | |
| SupportMultipleContent = true | |
| }; | |
| try | |
| { | |
| var enc = encoding != null ? TextEncodingEnum.GetEncoding(encoding) : System.Text.Encoding.UTF8; | |
| _reader = new JsonReaderInternal(new StreamReader(stream, enc, leaveOpen: true)) | |
| { | |
| SupportMultipleContent = true | |
| }; | |
| } | |
| catch (Exception e) | |
| { | |
| throw new RuntimeException(e.Message, e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/OneScript.StandardLibrary/Json/JSONReader.cs` around lines 253 - 258,
OpenStream currently allows .NET exceptions from TextEncodingEnum.GetEncoding
and new JsonReaderInternal to leak; mirror OpenFile by wrapping the encoding
resolution and _reader initialization in a try/catch and throw a
RuntimeException instead. Specifically, surround the calls to
TextEncodingEnum.GetEncoding(encoding) and the JsonReaderInternal/StreamReader
creation that assigns _reader with a try block, catch Exception ex, and rethrow
a new RuntimeException (include a clear message and set ex as the inner
exception) so OpenStream behavior matches OpenFile.
Mr-Rm
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Refactor