Skip to content

fix(jsonrpc): preserve absolute parse error columns #4020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zakazaka95 wants to merge 3 commits into
base: main
Choose a base branch
from
+145 −17
Open

fix(jsonrpc): preserve absolute parse error columns #4020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ebcf73
fix(jsonrpc): preserve absolute parse error columns
zakazaka95 Sep 1, 2026
c285ded
fix(jsonrpc): address parse column review feedback
zakazaka95 Sep 3, 2026
b87a330
perf(jsonrpc): reduce parse context overhead
zakazaka95 Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 72 additions & 11 deletions jsonrpc/pretty_error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

import (
"bytes"
"encoding/binary"
"encoding/json"
"errors"
"fmt"
"io"
"math/bits"
"slices"
"strings"
"unicode/utf8"
Expand All @@ -19,28 +21,83 @@

// windowBuffer keeps the last maxWindowSize bytes read
type windowBuffer struct {
window []byte
consumedBytes int
newlinesSeen int
window []byte
consumedBytes int
newlinesSeen int
linePrefixRunes int
}

func (c *windowBuffer) Write(p []byte) (int, error) {
c.consumedBytes += len(p)
c.newlinesSeen += bytes.Count(p, []byte{'\n'})

if len(p) >= maxWindowSize {
c.window = append(c.window[:0], p[len(p)-maxWindowSize:]...)
start := nextRuneStartAt(p, len(p)-maxWindowSize)
c.advanceLinePrefix(c.window, p[:start])
c.window = append(c.window[:0], p[start:]...)
return len(p), nil
}

if overflow := len(c.window) + len(p) - maxWindowSize; overflow > 0 {
c.window = c.window[:copy(c.window, c.window[overflow:])]
start := nextRuneStartAt(c.window, overflow)
c.advanceLinePrefix(nil, c.window[:start])
c.window = c.window[:copy(c.window, c.window[start:])]
}
c.window = append(c.window, p...)

return len(p), nil
}

func nextRuneStartAt(p []byte, offset int) int {
for offset < len(p) && !utf8.RuneStart(p[offset]) {
offset++
}
return offset
}

func (c *windowBuffer) advanceLinePrefix(first, second []byte) {
if c.newlinesSeen > 0 {
if lastNewline := bytes.LastIndexByte(second, '\n'); lastNewline >= 0 {
c.linePrefixRunes = countRuneStarts(second[lastNewline+1:])
return
}
if lastNewline := bytes.LastIndexByte(first, '\n'); lastNewline >= 0 {
c.linePrefixRunes = countRuneStarts(first[lastNewline+1:]) + countRuneStarts(second)
return
}
}
c.linePrefixRunes += countRuneStarts(first) + countRuneStarts(second)
}

func countRuneStarts(p []byte) int {
count := 0
for len(p) >= 32 {
count += countRuneStartsInWord(binary.LittleEndian.Uint64(p))
count += countRuneStartsInWord(binary.LittleEndian.Uint64(p[8:]))
count += countRuneStartsInWord(binary.LittleEndian.Uint64(p[16:]))
count += countRuneStartsInWord(binary.LittleEndian.Uint64(p[24:]))
p = p[32:]
}
for len(p) >= 8 {
count += countRuneStartsInWord(binary.LittleEndian.Uint64(p))
p = p[8:]
}
for _, b := range p {
if utf8.RuneStart(b) {
count++
}
}
Comment thread
RafaelGranza marked this conversation as resolved.
return count
}

func countRuneStartsInWord(word uint64) int {
const highBits = uint64(0x8080808080808080)
// UTF-8 continuation bytes start with 10; isolate their high bits and
// subtract them from the eight possible rune starts in the word.
continuationBits := word & ^(word << 1) & highBits
return 8 - bits.OnesCount64(continuationBits)
}

func errorOffset(inputLength int, err error) (offset int, ok bool) {
var (
syntaxErr *json.SyntaxError
Expand All @@ -58,11 +115,15 @@
}
}

func lineAndColumn(c *windowBuffer, markerPos int) (line, col int) {
func lineAndColumn(c *windowBuffer, markerPos int) (line, relativeCol, absoluteCol int) {
line = c.newlinesSeen - bytes.Count(c.window[markerPos:], []byte{'\n'}) + 1
lineStart := bytes.LastIndexByte(c.window[:markerPos], '\n') + 1
col = utf8.RuneCount(c.window[lineStart:markerPos]) + 1
return line, col
relativeCol = countRuneStarts(c.window[lineStart:markerPos]) + 1
absoluteCol = relativeCol
if lineStart == 0 {
absoluteCol += c.linePrefixRunes
}
Comment thread
RafaelGranza marked this conversation as resolved.
return line, relativeCol, absoluteCol
}

func expectedToken(reason string) (string, bool) {
Expand Down Expand Up @@ -204,8 +265,8 @@
}

markerPos := absOffset - windowStart
line, col := lineAndColumn(c, markerPos)
msg := fmt.Sprintf("%s [line %d, position %d]", describeError(c.window, markerPos, err), line, col)
line, relativeCol, absoluteCol := lineAndColumn(c, markerPos)
msg := fmt.Sprintf("%s [line %d, position %d]", describeError(c.window, markerPos, err), line, absoluteCol)

Check failure on line 269 in jsonrpc/pretty_error.go

View workflow job for this annotation

GitHub Actions / lint 

The line is 108 characters long, which exceeds the maximum of 100 characters. (lll)

@RafaelGranza RafaelGranza Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our linter is pointing this line is too long, breaking it in two should fix it.


return drawMarker(c.window, windowStart, markerPos, col, msg)
return drawMarker(c.window, windowStart, markerPos, relativeCol, msg)
}
79 changes: 73 additions & 6 deletions jsonrpc/pretty_error_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package jsonrpc_test

import (
stdjson "encoding/json"
"io"
"strings"
"testing"
"unicode/utf8"

"github.com/NethermindEth/juno/jsonrpc"
"github.com/NethermindEth/juno/utils/log"
Expand All @@ -11,8 +14,11 @@ import (
)

var parseErrorTests = map[string]struct {
req string
res string
req string
res string
chunkSize int
position string
marker byte
}{

@RafaelGranza RafaelGranza Sep 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is not needed.
Just adding entries to parseErrorTests would cover the new changes. And please cover the case when there are broken runes (part of it is no longer in the window).

If you find it is not possible, for any reason, please let me know, and lets move back to having 2 different tests.

"invalid json": {
req: `{]`,
Expand Down Expand Up @@ -98,7 +104,7 @@ var parseErrorTests = map[string]struct {

"error exactly at the window start still draws the marker": {
req: `{"jsonrpc": 5, "method": "x", "params": [` + strings.Repeat(`"0x1", `, 66) + strings.Repeat(" ", 5) + `"0x2"], "id": 1}`,
res: `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"5, \"method\": \"x\", \"params\": [\"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x...\n^\nfield \"jsonrpc\" should be string, got number [line 1, position 1]"},"id":null}`,
res: `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"5, \"method\": \"x\", \"params\": [\"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x1\", \"0x...\n^\nfield \"jsonrpc\" should be string, got number [line 1, position 13]"},"id":null}`,
},

"long line is windowed": {
Expand Down Expand Up @@ -152,7 +158,28 @@ var parseErrorTests = map[string]struct {

"oversized single-line input keeps only the trailing window": {
req: `{"jsonrpc": "2.0", "method": "starknet_call", "params": [` + strings.Repeat(`"0x049d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7", `, 10) + `"0xbad" @]}`,
res: `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"...36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7\", \"0xbad\" @]}\n ^\nunexpected '@', expected ',' or ']' [line 1, position 510]"},"id":null}`,
res: `{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error","data":"...36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7\", \"0xbad\" @]}\n ^\nunexpected '@', expected ',' or ']' [line 1, position 766]"},"id":null}`,
Comment thread
RafaelGranza marked this conversation as resolved.
},

"oversized unicode line keeps absolute column and relative marker": {
req: `{"jsonrpc":"2.0","method":"x","padding":"` + strings.Repeat("👍", 160) + `","id":@` + strings.Repeat("x", 96) + `}`,
chunkSize: 127,
position: `[line 1, position 209]`,
marker: '@',
},

"oversized mixed-width line counts rune starts across words": {
req: `{"jsonrpc":"2.0","method":"x","padding":"` + strings.Repeat("aé€👍", 80) + `","id":@` + strings.Repeat("x", 96) + `}`,
chunkSize: 127,
position: `[line 1, position 369]`,
marker: '@',
},

"discarded newline resets the absolute column prefix": {
req: strings.Repeat(" ", 600) + "\n" + `{"padding":"` + strings.Repeat("👍", 160) + `","id":@` + strings.Repeat("x", 96) + `}`,
chunkSize: 127,
position: `[line 2, position 180]`,
marker: '@',

@RafaelGranza RafaelGranza Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we don't need this, we can use the same res field here.

The way we are doing with requireMarkerUnderByte is making the same marker position calculations as in prod and checking if they match. But this introduce a problem, how do we know they are not both equally wrong?

IMO, the right call would be not checking the internal behavior of pretty_error.go, just checking the error output is correct. How the computations are made internally don't matter that much.

If you are worried that the res field could get too large, just remember the printed message is capped by 512 bytes, so res is not that large.

@RafaelGranza RafaelGranza Sep 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you disagree with this approach, let me know.

But anyway, if you want to keep the chunkSize field, we should move to having two distinct tests, because TestHandleParseError increases a lot in complexity with these added fields.

},

"context is capped at three lines within the window": {
Expand Down Expand Up @@ -217,15 +244,55 @@ var parseErrorTests = map[string]struct {
},
}

type maxChunkReader struct {
io.Reader
size int
}

func (r *maxChunkReader) Read(p []byte) (int, error) {
return r.Reader.Read(p[:min(len(p), r.size)])
}

func requireMarkerUnderByte(t *testing.T, data string, marker byte) {
t.Helper()

lines := strings.Split(data, "\n")
require.GreaterOrEqual(t, len(lines), 3)
line, caret := lines[len(lines)-3], lines[len(lines)-2]
markerAt, caretAt := strings.IndexByte(line, marker), strings.IndexByte(caret, '^')
require.NotEqual(t, -1, markerAt)
require.NotEqual(t, -1, caretAt)
assert.Equal(t, utf8.RuneCountInString(line[:markerAt]), caretAt)
}

func TestHandleParseError(t *testing.T) {
server := jsonrpc.NewServer(1, log.NewNopZapLogger())

for desc, test := range parseErrorTests {
t.Run(desc, func(t *testing.T) {
res, httpHeader, err := server.HandleReader(t.Context(), strings.NewReader(test.req))
reader := io.Reader(strings.NewReader(test.req))
if test.chunkSize > 0 {
reader = &maxChunkReader{Reader: reader, size: test.chunkSize}
}

res, httpHeader, err := server.HandleReader(t.Context(), reader)
require.NoError(t, err)
assert.NotNil(t, httpHeader)
assert.JSONEq(t, test.res, string(res))
if test.res != "" {
assert.JSONEq(t, test.res, string(res))
}
if test.position != "" || test.marker != 0 {
var response struct {
Error struct {
Data string `json:"data"`
} `json:"error"`
}
require.NoError(t, stdjson.Unmarshal(res, &response))
assert.Contains(t, response.Error.Data, test.position)
if test.marker != 0 {
requireMarkerUnderByte(t, response.Error.Data, test.marker)
}
}
})
}
}
Loading