fix(output): render JsonNode and JsonElement results as JSON data in human output - #111
carldebilly wants to merge 5 commits into
Conversation
A handler returning System.Text.Json's JsonObject — or rows of JsonObject through IReplPageSource<T> — got its CLR members in human output: Options, Parent, Root, Count, or ValueKind for a JsonElement. A JsonObject enumerates as key/value pairs, so it went down the collection path and its values down reflection. The Spectre renderer did worse: its inline path recursed through JsonNode's Root and Parent, which point back at each other, and overflowed the stack, taking the process with it. JsonHumanShape (Repl.Core, shared by both renderers) recognizes JsonNode and JsonElement and writes compact JSON literals. Each renderer checks for it before its enumerable and reflection branches: - an object becomes key: value lines; - rows of objects become a table whose columns are the union of their keys in first-seen order, with a missing key leaving an empty cell; - an array of scalars becomes one literal per line, JSON nulls included; - any JSON value nested in an ordinary result, or passed as a result's details, shows as a compact literal. The literal is written with Utf8JsonWriter and no serializer options, so a JsonValue wrapping a CLR object keeps its own type info. Options without a type resolver made that throw, where --json succeeds. The relaxed encoder keeps non-ASCII readable and escapes every control character, and format characters (bidirectional overrides, isolates, zero-width marks), which it lets through, are escaped afterwards, so a payload can neither drive the terminal nor disguise what it shows. The JSON row scan only runs once a collection's first item is JSON, so ordinary collections do not pay for it. docs/output-system.md gains a "JSON results" section. TDD: the human tests were red first showing the CLR members, and the Spectre ones crashed with a stack overflow. Review then found four more gaps, each with a test red before its fix: a JsonValue wrapping a CLR object threw, an all-null array rendered empty "- " lines, JSON details showed CLR members, and bidi characters passed raw. Each fix was then falsified with a compiling substitution that turned its own test red. Refs #92
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51b9d430b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (includeHeader) | ||
| { | ||
| tableRows.Add([.. columns.Select(JsonHumanShape.Label)]); | ||
| } |
There was a problem hiding this comment.
For an IReplPageSource<JsonObject> whose later page inserts keys in a different order or introduces a new key, RenderJsonItems recomputes the columns for that page but omits its header in continuation mode. For example, an initial {"id":1,"name":"a"} page followed by {"name":"b","id":2} displays "b" 2 beneath the original id name headings, mislabeling both values; the Spectre path has the same behavior. Keep the initial schema for continuations or retain a changed continuation header.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac5cf10. A JSON table takes its columns from its own rows' keys, so it now always renders its header, continuation pages included. That alone was not enough: PagerPayloadParser dropped every header it detected on a continuation page, so the header never reached the plain human More pager or the Spectre ANSI pager. The parser now keeps a continuation header unless it repeats the pinned one. The comparison is word by word, so the same header padded to other widths is still dropped. A kept header keeps its separator line. Covered at pager level by When_ThePagerAppendsAPageWithOtherKeys_Then_ItsRowsKeepTheirOwnHeader (human and Spectre), plus parser tests for kept headers, repeated headers at other widths, and kept separators.
| columns = [.. ordered]; | ||
| rows = converted; | ||
| return true; |
There was a problem hiding this comment.
When an array or page contains only empty objects such as [{}], anyRow is true but ordered remains empty, so this reports a successful table shape with zero columns. The human formatter consequently returns an empty string and drops every row, while the Spectre renderer is handed a zero-column table; treat the zero-column case as literals or otherwise render each {} explicitly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac5cf10: TryGetObjectRows now fails when no row has a key, so rows that are all {} render as one {} literal per line (When_EveryRowIsAnEmptyObject_Then_EachReadsAsAnEmptyObject, human and Spectre).
| /// <summary>The cell for <paramref name="column"/>: empty when the row does not have the key.</summary> | ||
| public static string Cell(JsonObject? row, string column) => | ||
| row is not null && row.TryGetPropertyValue(column, out var value) ? Literal(value) : string.Empty; |
There was a problem hiding this comment.
When rows use JsonObjects created with JsonNodeOptions.PropertyNameCaseInsensitive = true and their keys differ only by case, such as one row containing Name and another containing name, the ordinal column union creates two distinct columns but TryGetPropertyValue resolves both names in every row. Each value is consequently duplicated into both columns instead of leaving the differently cased key's cell empty. Either merge columns with the node's comparer or perform an ordinal property-name lookup when populating cells.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac5cf10: JsonHumanShape.Cell looks the key up with IndexOf/GetAt and requires an ordinal match on the stored key, the same way the columns are collected. A case-insensitive row no longer fills the other-case column (When_ARowIsCaseInsensitive_Then_ItsCellsStillMatchKeysOrdinally).
| // Only once the first item is JSON: ordinary collections must not pay for the JSON row scan. | ||
| if (JsonHumanShape.IsJson(firstNonNull)) | ||
| { | ||
| text = RenderJsonItems(values, settings, includeHeader); | ||
| return true; |
There was a problem hiding this comment.
When an IReplPageSource<JsonNode?> returns a nonempty page whose items are all JSON nulls, firstNonNull is null, so this JSON-specific branch is never reached. The human renderer emits blank bullet rows and the Spectre renderer reports No results., even though the page contains rows that should each display null; the explicit all-null handling for a top-level JsonArray does not cover pages. Use IReplPage.ItemType (including nullable element types) to select JSON rendering when runtime values cannot identify the declared row type.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac5cf10: both transformers' RenderPage now route a page whose declared ItemType is JSON (JsonNode and its subtypes, JsonElement, Nullable<JsonElement>) straight to the JSON item renderer, so a page that is all JSON nulls renders null per item. Tests cover JsonNode? pages on both transformers and JsonElement? pages on the human one. A related gap is fixed too: a JsonElement of kind Null in a page of objects is now an empty row instead of turning the table into literals.
…al cells and null pages Addresses the review of #111. A JSON table takes its columns from its rows' keys, not from a type, so a continuation page can have other keys, or the same keys in another order, than the first page. Rendered without a header, its values sat under the first page's headings. JSON tables now always carry their own header, and the pager keeps a continuation header unless it repeats the pinned one. Before, it dropped every header it detected on a continuation page, so the transformer's header never reached the plain human More pager or the Spectre ANSI pager. The repeat check compares words, so the same header padded to other column widths is still dropped. A kept header keeps its separator line, even one identical to the pinned separator. CLR tables are unaffected: their continuation pages render no header. Also: - Rows that are all empty objects produced a table with no columns, which printed nothing. They now read {}. - Cells are looked up ordinally (IndexOf/GetAt plus an ordinal key check), the way the columns are collected. A JsonObject built with case-insensitive property names no longer fills a "name" column from its "Name" key. - A page declared with a JSON item type (IReplPage.ItemType, Nullable<JsonElement> included) renders as JSON even when every item is a JSON null. Before, it rendered blank "- " rows or "No results.". - A JsonElement of kind Null in a page of objects is an empty row, like a CLR null, instead of turning the whole table into literals. Repl.Core grants InternalsVisibleTo to Repl.SpectreTests, so its tests can render continuation pages and drive PagerSession. TDD: each new test was red first, for its predicted reason. The pager-level tests drive the transformer output through PagerSession, which is where the user sees it. Each fix was falsified with a compiling substitution that turned its own test red. Falsifying the word comparison also broke the existing duplicate-header pager tests. Refs #92
The two Spectre pager tests used the default transformer, which has no OutputOptions and so leaves the ANSI decision to Spectre's own console detection. Locally that console emitted the bold header that the pager detects and pins. On CI it did not: nothing was pinned, both headers stayed inline, and When_ThePagerAppendsAPageWithTheSameKeys_Then_OnlyItsRowIsAdded failed on all three OSes. Both tests now force AnsiMode.Always, the ANSI pager case the review raised. Checked both ways: with AnsiMode.Never the same-keys test fails as it did on CI, and with ANSI forced, neutralising the pager's keep-a-differing-header fix turns the other-keys test red. Refs #92
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b61b4ac0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private static bool RepeatsHeader(PagerHeader pinned, PagerHeader candidate) => | ||
| pinned.Lines.Count > 0 | ||
| && candidate.Lines.Count > 0 | ||
| && HeaderWords(pinned.Lines[0]).SequenceEqual(HeaderWords(candidate.Lines[0]), StringComparer.Ordinal); |
There was a problem hiding this comment.
When JSON keys contain spaces, comparing only whitespace-delimited words can mistake different schemas for the same header. For example, columns first / last name and first last / name both reduce to first, last, name; the continuation header is then dropped and its values appear beneath the wrong pinned columns. Compare column structure rather than only the flattened word sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c43687: RepeatsHeader now compares column labels, not flattened words. In a plain table whose header is aligned on its separator, each label is the text over one run of dashes. This matters because TextTableFormatter leaves a single space between columns below 100 columns, where no gap rule can tell first / last name from first last / name. In a styled header, each label is its own run between escape sequences, which is how Spectre writes a bold header cell, spaces included. Otherwise, labels are what double-space gaps leave. Covered by plain, narrow, styled, and real-Spectre pager tests with regrouped words, alongside the existing duplicate-header tests.
| if (JsonHumanShape.TryGetNode(result.Details, out var jsonDetails)) | ||
| { | ||
| return $"{message}{Environment.NewLine}{RenderJson(jsonDetails, settings)}"; |
There was a problem hiding this comment.
When an IReplResult carries a JSON object or array in Details, this invokes the top-level JSON renderer, so the human formatter expands an object into labeled lines or an array into rows. The Spectre formatter instead emits a compact literal, matching the documented behavior in docs/output-system.md:42-43; therefore the same result changes shape depending on the selected human formatter. Use the literal path here, as is already done for JSON-valued properties.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that the two transformers must agree. I aligned them the other way: Spectre now renders JSON details like a JSON result, as the human transformer already did (2c43687). Both transformers expand object details rather than printing them as one literal, and a JSON object carried as details is the same kind of payload. The compact literal stays for a JSON value nested in a result object's property. docs/output-system.md now says this. The Spectre path composes the JSON renderable into the layout directly: wrapping the pre-rendered string in Text made Spectre count its escape sequences as columns and re-wrap wide lines. When_AResultCarriesWideJsonDetails_Then_TheyRenderAsTheResultWould covers that case.
…e JSON details like a JSON result Addresses the second review of #111. The pager compared a continuation header with the pinned one word by word. So JSON keys containing spaces, such as 'first' / 'last name' against 'first last' / 'name', looked like the same header, the continuation header was dropped, and its rows sat under the wrong columns. The comparison is now by column label, read three ways: - In a plain table whose header is aligned on its separator line, each label is the text over one run of dashes. This matters because TextTableFormatter leaves a single space between columns below 100 columns wide. - In a styled header, each label is its own run between escape sequences, which is how Spectre 0.55 writes a bold header cell, spaces included. Each run is also split at double-space gaps, so a header styled as a single run with its padding inside still matches at other widths. - Otherwise, labels are what gaps of two or more spaces leave. This covers a header that is not aligned on its separator, as in the existing hash-header fixture. AnsiTextMetrics gains SplitAtControlSequences for the styled case. A JSON value passed as an IReplResult's details rendered as a compact literal in Spectre, but expanded in the human transformer. Spectre now renders it like a JSON result, as both transformers already do for object details. The JSON renderable is composed into the result's layout directly. Wrapping the already-rendered string in Text, as the page-details path still does, made Spectre count its escape sequences as columns, so wide lines wrapped a second time. The docs line is updated. TDD: the regroup tests (plain, narrow, styled), the whole-line-bold repeat, the Spectre details test and the wide-details test were red first. Each strategy was falsified with a compiling substitution that turned its own test red: - no separator spans: the narrow test; - no styled runs: the styled repeat; - no alignment guard: the existing hash-header test; - styled runs split on single spaces: both regroup tests, one of them driven through real Spectre output. Refs #92
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c43687962
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var memberValue = member.Property.GetValue(value); | ||
| if (JsonHumanShape.TryGetNode(memberValue, out var jsonValue)) |
There was a problem hiding this comment.
When an ordinary result object has a property declared as JsonNode? or JsonElement? whose value is JSON null, this runtime-only check cannot recognize it because the reflected value is C# null; the code falls through to RenderScalar and emits an empty cell/value rather than the documented null. The Spectre paths have the same ordering, checking value is null before JSON handling. Inspect the member's declared property type (while respecting an explicit NullDisplayText) so null JSON properties render consistently with nulls in JSON objects and pages.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf40d08. Both transformers now default a member's NullDisplayText to null when its declared type is JSON (JsonNode and subtypes, JsonElement, Nullable<JsonElement>). The default is decided once per member when the display members are built, so every null path (fields, table cells, Spectre inline values) reads null, as --json writes it. An explicit DisplayFormat.NullDisplayText still wins. Tests: When_AJsonTypedPropertyIsNull_Then_ItReadsNull (human and Spectre, field and table cell) and When_AJsonTypedPropertyHasANullDisplayText_Then_ItIsUsed.
| return string.Join( | ||
| Environment.NewLine, | ||
| items.Select(item => JsonHumanShape.TryLiteral(item, out var literal) | ||
| ? literal | ||
| : RenderScalar(item, member: null, depth: 0, compactCollection: true, settings.Width, settings))); |
There was a problem hiding this comment.
When an IReplPageSource<JsonNode?> starts with object rows but a continuation page contains scalars, JSON nulls, or only empty objects, this fallback emits bare literal lines without a header. PagerPayloadParser consequently finds no candidate header and keeps displaying the first page's pinned object header, so values such as null or "x" appear beneath unrelated id/name columns. The fresh case not covered by the earlier continuation-header fix is precisely a continuation that no longer forms an object table; emit a shape boundary/header or otherwise prevent the pinned table header from labeling these rows. The Spectre fallback has the same behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed that this is a real gap. I am not fixing it in this PR, because it is not specific to JSON. Any heterogeneous page source has it: an IReplPageSource<object> whose continuation page holds another CLR type also renders without a header under the first page's pinned columns. So does a continuation that is all nulls. Neither transformer can unpin a header. The fix needs a pager-level notion of a shape boundary, or better, transformers that declare the header lines they emitted instead of having the pager infer them from styling. That also removes the styling heuristics behind the other pager threads here. I will propose it as a follow-up issue, including the all-null case, where the literal null lines read like rows with null values.
| if (header is not null && !RepeatsHeader(header, payloadHeader)) | ||
| { | ||
| content.AddRange(payloadHeader.Lines); |
There was a problem hiding this comment.
With ANSI enabled in the built-in human formatter, the default table header starts with ESC[1;38;5;221m, but DetectHeader recognizes only the exact substring ESC[1m. The initial JSON header is therefore stored as content rather than as the pinned header, and because JSON continuation pages now always emit headers, identical headings are repeated on every fetched page instead of reaching this deduplication path. Unlike the earlier schema-change case, this occurs even when every page has the same columns; recognize bold within a combined SGR sequence (and the light-theme equivalent) when detecting the header.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in bf40d08. A line whose leading escape sequences set bold, before any text, now counts as a header, so the human palette's ESC[1;38;5;221m header (and the light theme's) is pinned. SGR parameters are parsed, so a 1 that is an extended colour's argument (38;5;1) is not taken for bold. Recognizing more headers widens the dedupe, so two guards bound it. A header whose text is empty once normalized, such as a lone style opener, is no header. And lines equal to the header are only dropped from continuation pages: inside one payload, such as a long string result, a repeat of the first line is data. Tests cover the combined sequence, the colour index, the ANSI pager with the same keys, and the three guards. A custom palette whose TableHeaderStyle is not bold still is not pinned, so its JSON continuation headers repeat. That is a duplicate line, not a mislabel, and the header-contract follow-up above would remove it.
…d properties as null Addresses the third review of #111. The pager recognized a header line only by the exact ESC[1m. The human palette styles its table header as a combined sequence (ESC[1;38;5;221m, or ESC[1;38;5;52m in the light theme), so in the interactive human ANSI pager the first page's header was never pinned. Since JSON continuation pages now carry their own header, that header repeated on every page fetched. A line whose leading escape sequences set bold, before any text, now counts as a header. The SGR parameters are parsed, so a 1 that is an extended colour's argument (38;5;1, 48;2;r;g;b) is not taken for bold. The existing ESC[1m rule is unchanged. Recognizing more headers widens what the pager treats as one, so two guards bound it: - A header whose text is empty after normalization, such as a line holding only a style opener, is no header. Otherwise every blank line after it would be dropped as a repeat. - Lines equal to the header are dropped only from continuation pages. Within one payload, such as a long string result, a line identical to its first line is data and is kept. A property declared as JSON (JsonNode and subtypes, JsonElement, Nullable<JsonElement>) that holds null rendered as an empty value, while --json writes null. Both transformers now default such a member's NullDisplayText to "null", decided once per member from the declared type. An explicit DisplayFormat.NullDisplayText still wins. TDD: the combined-bold header, the ANSI pager with the same keys, the null JSON-typed property (human and Spectre), the repeated first line, the escape-only first line and the bold-after-text line were red first. The colour-index and NullDisplayText tests are guards. Each fix was falsified on its own with a compiling substitution that turned its test red. Refs #92
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf40d08ccc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private string RenderJsonItems(IReadOnlyList<object?> items) => | ||
| JsonHumanShape.TryGetObjectRows(items, out var columns, out var rows) | ||
| ? RenderToString(BuildJsonTable(columns, rows)) |
There was a problem hiding this comment.
When Spectre runs with ANSI disabled (for example under NO_COLOR, TERM=dumb, or AnsiMode.Never), this produces a borderless plain-text header with no separator. PagerPayloadParser.DetectHeader recognizes plain headers only via a separator or the # convention, so the first header is never pinned and every same-schema continuation header remains in the content. Multi-page JSON output therefore repeats headings on every page and consumes pager buffer rows despite the documented promise that repeated headers are dropped; emit a detectable plain header or otherwise pass its structure to the pager.
Useful? React with 👍 / 👎.
| return line.Contains('\u001b', StringComparison.Ordinal) | ||
| // Each run is split at its own gaps too: a header can also be styled as one run, padding included. | ||
| ? AnsiTextMetrics.SplitAtControlSequences(line).SelectMany(static run => SplitAtGaps(run)) |
There was a problem hiding this comment.
For an ANSI-enabled human table whose first page has keys a b / c and continuation has a / b c, each real column is independently styled, but SelectMany(SplitAtGaps) splits the two spaces inside each key and reduces both schemas to the same a, b, c sequence. RepeatsHeader then drops the changed continuation header and displays its values beneath the wrong pinned labels. Fresh evidence beyond the earlier single-space fix is that repeated whitespace is valid inside a JSON key; independently styled runs need to remain single labels rather than being split at their internal gaps.
Useful? React with 👍 / 👎.
| for (var i = payloadHeader.Lines.Count; i < lines.Count; i++) | ||
| { | ||
| var normalized = NormalizeLine(lines[i]); | ||
| if (resolvedHeader.NormalizedLines.Contains(normalized) | ||
| // Only a continuation repeats a header; within one payload, a line like its first one is data. | ||
| if ((header is not null && resolvedHeader.NormalizedLines.Contains(normalized)) |
There was a problem hiding this comment.
When a continuation JSON table has a single column named 1 and its row contains the numeric value 1 (similarly, a null column containing JSON null), the rendered data line has exactly the same normalized text as the pinned header. payloadHeader.Lines.Count has already skipped the continuation's actual header before this loop, but this condition also removes every later matching line, so the real result row silently disappears. Restrict deduplication to the detected header rather than filtering matching text throughout the page content.
Useful? React with 👍 / 👎.
| public static string Cell(JsonObject? row, string column) | ||
| { | ||
| if (row is null || row.IndexOf(column) is not (>= 0 and var index)) | ||
| { | ||
| return string.Empty; |
There was a problem hiding this comment.
When a JSON object page has one column and its final row is a CLR null or null/default JsonElement, such as rows {"id":1} followed by null, that row is retained in rows but this returns an empty sole cell. The human table consequently ends with an empty line, which PagerPayloadParser.SplitLines removes as a presumed payload terminator, so an interactively paged source silently loses its final result row; Spectre's TrimEnd can erase the same row before parsing. Render null object rows with a visible marker such as null, or otherwise preserve terminal empty rows through paging.
Useful? React with 👍 / 👎.
1 participant
Closes #92
Problem
A handler returning
System.Text.Json'sJsonObject, or rows ofJsonObjectthroughIReplPageSource<T>, showed the node's CLR members inhumanoutput (Options,Parent,Root,Count, andValueKindfor aJsonElement) instead of its data.spectredid worse: its inline renderer recursed throughJsonNode.Root/Parent, which point back at each other, and overflowed the stack. That crashes the process onmaintoday.Fix
JsonHumanShape(internal, inRepl.Core) recognizesJsonNodeandJsonElement, and both human transformers use it before their enumerable and reflection branches:key: valuelines;IReplResult's details, renders as a compact literal.Literals are written with
Utf8JsonWriterand no serializer options, so aJsonValuethat wraps a CLR object keeps its own type info. Serializing through options with no type resolver makes that case throw, even though--jsonsucceeds. The relaxed encoder keeps non-ASCII readable and escapes control characters. It lets Unicode format characters through (bidi overrides and isolates, zero-width marks), so those are escaped afterwards. That way a payload can neither drive the terminal nor disguise what it shows.The JSON row scan only runs once a collection's first item is JSON, so ordinary collections pay nothing for it.
docs/output-system.mdgains a "JSON results" section.Tests
Repl.Tests/Given_HumanOutputJson.cs(13 tests) andRepl.SpectreTests/Given_SpectreHumanOutputJson.cs(10 tests).JsonValuethrew;-lines;Out of scope
markdowntransformer does not crash on aJsonObject(it renders a Key | Value table) and is left unchanged.stringvalues in results are still written raw, with no control-character sanitising.