к #1532: ОткрытьПоток для ЗаписьJSON#1714
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesJSONWriter lifecycle and output handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant JSONTest
participant JSONWriter
participant MemoryStream
JSONTest->>JSONWriter: open on writable stream
JSONWriter->>MemoryStream: write JSON bytes
JSONTest->>JSONWriter: Close()
JSONWriter->>MemoryStream: flush and release writer
JSONTest->>MemoryStream: append bytes after JSON close
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/OneScript.StandardLibrary/Json/JSONWriter.cs (1)
494-504: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDispose closes the underlying writer after disposing its output — reverse the order.
_stringWriter?.Dispose()runs before_writer.Close()._writer.Close()flushes and auto-completes into the underlyingStringWriter, which has already been disposed. Close/flush_writerfirst, then dispose_stringWriter.♻️ Proposed reorder
protected virtual void Dispose(bool disposing) { if (disposing && _writer is not null) { - _stringWriter?.Dispose(); - _stringWriter = null; - - _writer.Close(); - _writer = null; + _writer.Close(); + _writer = null; + + _stringWriter?.Dispose(); + _stringWriter = null; } }🤖 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/JSONWriter.cs` around lines 494 - 504, Update JSONWriter.Dispose(bool disposing) to close _writer before disposing _stringWriter, so the writer can flush and complete its output into the underlying StringWriter; then clear and dispose the resources in the corrected order while preserving the existing null checks.
🤖 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/JSONWriter.cs`:
- Around line 468-484: Update SetString to close the currently open writer
before assigning a new StringWriter, then configure the new writer through
SetWriter. Preserve SetWriter’s existing cleanup behavior so reopening string
output retains the new _stringWriter and Close() returns its contents.
- Around line 267-274: The Close method reads _stringWriter before Dispose
closes the underlying writer, omitting auto-completed JSON delimiters. Update
Close to call Dispose before obtaining the string result, while preserving
String.Empty for non-string writers and returning the flushed content.
---
Nitpick comments:
In `@src/OneScript.StandardLibrary/Json/JSONWriter.cs`:
- Around line 494-504: Update JSONWriter.Dispose(bool disposing) to close
_writer before disposing _stringWriter, so the writer can flush and complete its
output into the underlying StringWriter; then clear and dispose the resources in
the corrected order while preserving the existing null checks.
🪄 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: 786882fc-ca23-49b8-a264-9b1439fb526c
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Json/JSONWriter.cs
| public string Close() | ||
| { | ||
| string res = ""; | ||
|
|
||
| if (IsOpenForString()) | ||
| { | ||
| res = _stringWriter.ToString(); | ||
| _stringWriter.Close(); | ||
| _stringWriter = null; | ||
| } | ||
|
|
||
| if (_writer != null) | ||
| { | ||
| _writer.Close(); | ||
| _writer = null; | ||
| } | ||
|
|
||
| return res; | ||
| var result = IsOpenForString() ? _stringWriter.ToString() : String.Empty; | ||
|
|
||
| Dispose(); | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Newtonsoft.Json JsonTextWriter AutoCompleteOnClose Flush behavior on Close
💡 Result:
In Newtonsoft.Json, the AutoCompleteOnClose property (which defaults to true) controls whether the JsonTextWriter automatically closes any open JSON objects or arrays when its Close method is called [1][2]. When Close is invoked, the behavior follows these steps: 1. Auto-completion: If AutoCompleteOnClose is true, the writer checks the current state of the JSON stack [2]. It will automatically write the necessary closing tokens (e.g., } for an object or ] for an array) for any nested structures that remain unclosed [2][3]. 2. Flushing and Closing: The JsonTextWriter then proceeds to close itself [4][5]. If the CloseOutput property is also set to true (which is its default behavior), the writer will additionally close the underlying TextWriter or stream that was passed to it [6][5]. It is important to note that while JsonTextWriter handles the completion of the JSON structure, it relies on the underlying stream or writer to handle physical data persistence [4]. If you want to ensure that all data is physically written to the destination without closing the underlying stream, you should explicitly call the Flush method before the writer is closed, and consider setting CloseOutput to false to prevent the JsonTextWriter from closing the underlying stream [6][7][8].
Citations:
- 1: https://www.newtonsoft.com/json/help/html/P_Newtonsoft_Json_JsonWriter_AutoCompleteOnClose.htm
- 2: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/JsonWriter.cs
- 3: https://stackoverflow.com/questions/47784416/json-net-how-to-fix-incomplete-json
- 4: https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/JsonTextWriter.cs
- 5: https://www.newtonsoft.com/json/help/html/M_Newtonsoft_Json_JsonTextWriter_Close.htm
- 6: https://stackoverflow.com/questions/43298527/newtonsoft-jsonwriter-close-when-using-response-stream
- 7: JsonTextWriter doesn't dispose TextWriter. JamesNK/Newtonsoft.Json#1812
- 8: https://www.newtonsoft.com/json/help/html/t_newtonsoft_json_jsontextwriter.htm
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/OneScript.StandardLibrary/Json/JSONWriter.cs"
echo "== file outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== relevant slice =="
sed -n '1,340p' "$FILE" | cat -n | sed -n '1,340p'Repository: EvilBeaver/OneScript
Length of output: 16803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/OneScript.StandardLibrary/Json/JSONWriter.cs"
echo "== dispose/setwriter slice =="
sed -n '460,540p' "$FILE" | cat -n
echo
echo "== json writer close semantics from package sources if present =="
rg -n "AutoCompleteOnClose|CloseOutput|class JsonTextWriter|override void Close|Dispose\\(" -g '*.cs' -g '*.csproj' -g '*.props' -g '*.targets' .Repository: EvilBeaver/OneScript
Length of output: 18866
Flush the JSON writer before reading the string. Close() captures _stringWriter.ToString() before _writer.Close() runs in Dispose(), so any auto-completed ]/} written on close won’t be included in the returned value. Move the read after Dispose() or close the writer first.
🤖 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/JSONWriter.cs` around lines 267 - 274, The
Close method reads _stringWriter before Dispose closes the underlying writer,
omitting auto-completed JSON delimiters. Update Close to call Dispose before
obtaining the string result, while preserving String.Empty for non-string
writers and returning the flushed content.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_writer.os`:
- Around line 218-226: Update the test flow around ЗаписьJSON.УстановитьСтроку
and ЗаписьJSON.ЗаписатьБезОбработки to keep JSON output directed to the opened
stream by removing УстановитьСтроку. Adjust the assertions so
ЗаписьJSON.Закрыть() is expected to return an empty string, while the final
Поток buffer assertion expects the interleaved JSON bytes alongside the existing
binary data.
🪄 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: 6c496d81-2a03-431c-9572-2465c9722dd2
📒 Files selected for processing (2)
src/OneScript.StandardLibrary/Json/JSONWriter.cstests/json/test-json_writer.os
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Json/JSONWriter.cs
1 participant
Есть отличие от платформы 1С: при открытии read-only потока исключение по невозможности записи выбрасывается сразу. 1С отрабатывает открытие без ошибки, а исключение возникает только при попытке записи.
Обоснование:
Summary by CodeRabbit
Close()behavior for in-memory/string outputs: it now flushes and returns generated JSON.