[WIP] fix #1679: добавлен метод ОткрытьПоток() объекта ЧтениеJSON#1680
[WIP] fix #1679: добавлен метод ОткрытьПоток() объекта ЧтениеJSON#1680Mr-Rm wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds JSONReader.OpenStream to initialize the internal reader from an IStreamWrapper (optional encoding, defaults to UTF-8) and a test that opens a memory stream with JSON {"ответ":42} and asserts parsed Ответ == 42; also reorders implemented interfaces in FileStreamContext declaration. ChangesJSON stream reading support
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}
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
|
Огонь огонь! |
| if (IsOpen()) | ||
| Close(); | ||
|
|
||
| var stream = streamContext switch |
There was a problem hiding this comment.
а просто IStreamWrapper не пойдет? Они все его реализуют
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
| public void OpenStream(IStreamWrapper streamContext, IValue encoding = null) | ||
| { | ||
| if (IsOpen()) | ||
| Close(); | ||
|
|
||
| var stream = streamContext.GetUnderlyingStream(); |
There was a problem hiding this comment.
Add null validation for the streamContext parameter.
The method does not validate that streamContext is non-null before calling GetUnderlyingStream() on line 285. While OneScript script calls may not pass null, direct C# calls to this method could, resulting in a NullReferenceException.
🛡️ Proposed fix to add defensive null check
public void OpenStream(IStreamWrapper streamContext, IValue encoding = null)
{
+ if (streamContext == null)
+ throw new ArgumentNullException(nameof(streamContext));
+
if (IsOpen())
Close();📝 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 void OpenStream(IStreamWrapper streamContext, IValue encoding = null) | |
| { | |
| if (IsOpen()) | |
| Close(); | |
| var stream = streamContext.GetUnderlyingStream(); | |
| public void OpenStream(IStreamWrapper streamContext, IValue encoding = null) | |
| { | |
| if (streamContext == null) | |
| throw new ArgumentNullException(nameof(streamContext)); | |
| if (IsOpen()) | |
| Close(); | |
| var stream = streamContext.GetUnderlyingStream(); |
🤖 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 - 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.
3 participants
Summary by CodeRabbit
New Features
Tests
Refactor