From 8c90d320d84fbfb1b9396800269f555f2bd2c512 Mon Sep 17 00:00:00 2001 From: Joe Tsai Date: Sun, 24 Nov 2024 16:06:40 -0800 Subject: [PATCH] Always update stack pointer Previously, we used to populate the stack pointer only under the semantics of AllowDuplicateNames(false). The argument made at the time was that the stack pointer becomes imprecise if there are duplicate names and that someone might want to disable duplicate name checks for the sake of performance in which case the logic to maintain the name stack becomes an unnecessary burden if the user does not care about ever retrieving the stack. However, preserving the stack pointer is important for providing good errors and v1 has functionally always operated under AllowDuplicateNames(false) semantics. Thus, in order to preserve good errors in v1, always maintain the name stack needed to produce a JSON pointer. --- arshal_default.go | 4 +- arshal_test.go | 2 +- jsontext/coder_test.go | 112 ++++++++++++++++++++--------------------- jsontext/decode.go | 38 +++++++------- jsontext/encode.go | 68 +++++++++++++------------ 5 files changed, 114 insertions(+), 110 deletions(-) diff --git a/arshal_default.go b/arshal_default.go index 819bd2b..dadc87a 100644 --- a/arshal_default.go +++ b/arshal_default.go @@ -1037,9 +1037,7 @@ func makeStructArshaler(t reflect.Type) *arshaler { b, _ = jsonwire.AppendQuote(b, f.name, &xe.Flags) } xe.Buf = b - if !xe.Flags.Get(jsonflags.AllowDuplicateNames) { - xe.Names.ReplaceLastQuotedOffset(n0) - } + xe.Names.ReplaceLastQuotedOffset(n0) xe.Tokens.Last.Increment() } else { if err := enc.WriteToken(jsontext.String(f.name)); err != nil { diff --git a/arshal_test.go b/arshal_test.go index 054d76e..f4d1c1b 100644 --- a/arshal_test.go +++ b/arshal_test.go @@ -1011,7 +1011,7 @@ func TestMarshal(t *testing.T) { opts: []Options{ Deterministic(true), WithMarshalers(MarshalFuncV2(func(enc *jsontext.Encoder, v string, opts Options) error { - if p := enc.StackPointer(); p != "/0" { + if p := enc.StackPointer(); p != "/X" { return fmt.Errorf("invalid stack pointer: got %s, want /0", p) } switch v { diff --git a/jsontext/coder_test.go b/jsontext/coder_test.go index 21f65c7..a5cd3c6 100644 --- a/jsontext/coder_test.go +++ b/jsontext/coder_test.go @@ -493,70 +493,66 @@ func testCoderInterleaved(t *testing.T, where jsontest.CasePos, modeName string, func TestCoderStackPointer(t *testing.T) { tests := []struct { - token Token - wantWithRejectDuplicateNames Pointer - wantWithAllowDuplicateNames Pointer + token Token + want Pointer }{ - {Null, "", ""}, - - {ArrayStart, "", ""}, - {ArrayEnd, "", ""}, - - {ArrayStart, "", ""}, - {Bool(true), "/0", "/0"}, - {ArrayEnd, "", ""}, - - {ArrayStart, "", ""}, - {String("hello"), "/0", "/0"}, - {String("goodbye"), "/1", "/1"}, - {ArrayEnd, "", ""}, - - {ObjectStart, "", ""}, - {ObjectEnd, "", ""}, - - {ObjectStart, "", ""}, - {String("hello"), "/hello", "/0"}, - {String("goodbye"), "/hello", "/0"}, - {ObjectEnd, "", ""}, - - {ObjectStart, "", ""}, - {String(""), "/", "/0"}, - {Null, "/", "/0"}, - {String("0"), "/0", "/1"}, - {Null, "/0", "/1"}, - {String("~"), "/~0", "/2"}, - {Null, "/~0", "/2"}, - {String("/"), "/~1", "/3"}, - {Null, "/~1", "/3"}, - {String("a//b~/c/~d~~e"), "/a~1~1b~0~1c~1~0d~0~0e", "/4"}, - {Null, "/a~1~1b~0~1c~1~0d~0~0e", "/4"}, - {String(" \r\n\t"), "/ \r\n\t", "/5"}, - {Null, "/ \r\n\t", "/5"}, - {ObjectEnd, "", ""}, - - {ArrayStart, "", ""}, - {ObjectStart, "/0", "/0"}, - {String(""), "/0/", "/0/0"}, - {ArrayStart, "/0/", "/0/0"}, - {ObjectStart, "/0//0", "/0/0/0"}, - {String("#"), "/0//0/#", "/0/0/0/0"}, - {Null, "/0//0/#", "/0/0/0/0"}, - {ObjectEnd, "/0//0", "/0/0/0"}, - {ArrayEnd, "/0/", "/0/0"}, - {ObjectEnd, "/0", "/0"}, - {ArrayEnd, "", ""}, + {Null, ""}, + + {ArrayStart, ""}, + {ArrayEnd, ""}, + + {ArrayStart, ""}, + {Bool(true), "/0"}, + {ArrayEnd, ""}, + + {ArrayStart, ""}, + {String("hello"), "/0"}, + {String("goodbye"), "/1"}, + {ArrayEnd, ""}, + + {ObjectStart, ""}, + {ObjectEnd, ""}, + + {ObjectStart, ""}, + {String("hello"), "/hello"}, + {String("goodbye"), "/hello"}, + {ObjectEnd, ""}, + + {ObjectStart, ""}, + {String(""), "/"}, + {Null, "/"}, + {String("0"), "/0"}, + {Null, "/0"}, + {String("~"), "/~0"}, + {Null, "/~0"}, + {String("/"), "/~1"}, + {Null, "/~1"}, + {String("a//b~/c/~d~~e"), "/a~1~1b~0~1c~1~0d~0~0e"}, + {Null, "/a~1~1b~0~1c~1~0d~0~0e"}, + {String(" \r\n\t"), "/ \r\n\t"}, + {Null, "/ \r\n\t"}, + {ObjectEnd, ""}, + + {ArrayStart, ""}, + {ObjectStart, "/0"}, + {String(""), "/0/"}, + {ArrayStart, "/0/"}, + {ObjectStart, "/0//0"}, + {String("#"), "/0//0/#"}, + {Null, "/0//0/#"}, + {ObjectEnd, "/0//0"}, + {ArrayEnd, "/0/"}, + {ObjectEnd, "/0"}, + {ArrayEnd, ""}, } for _, allowDupes := range []bool{false, true} { var name string - var want func(i int) Pointer switch allowDupes { case false: name = "RejectDuplicateNames" - want = func(i int) Pointer { return tests[i].wantWithRejectDuplicateNames } case true: name = "AllowDuplicateNames" - want = func(i int) Pointer { return tests[i].wantWithAllowDuplicateNames } } t.Run(name, func(t *testing.T) { @@ -567,8 +563,8 @@ func TestCoderStackPointer(t *testing.T) { if err := enc.WriteToken(tt.token); err != nil { t.Fatalf("%d: Encoder.WriteToken error: %v", i, err) } - if got := enc.StackPointer(); got != want(i) { - t.Fatalf("%d: Encoder.StackPointer = %v, want %v", i, got, want(i)) + if got := enc.StackPointer(); got != tests[i].want { + t.Fatalf("%d: Encoder.StackPointer = %v, want %v", i, got, tests[i].want) } } @@ -577,8 +573,8 @@ func TestCoderStackPointer(t *testing.T) { if _, err := dec.ReadToken(); err != nil { t.Fatalf("%d: Decoder.ReadToken error: %v", i, err) } - if got := dec.StackPointer(); got != want(i) { - t.Fatalf("%d: Decoder.StackPointer = %v, want %v", i, got, want(i)) + if got := dec.StackPointer(); got != tests[i].want { + t.Fatalf("%d: Decoder.StackPointer = %v, want %v", i, got, tests[i].want) } } }) diff --git a/jsontext/decode.go b/jsontext/decode.go index 44cdb9d..2ac2fec 100644 --- a/jsontext/decode.go +++ b/jsontext/decode.go @@ -491,13 +491,15 @@ func (d *decoderState) ReadToken() (Token, error) { } else { pos += n } - if !d.Flags.Get(jsonflags.AllowDuplicateNames) && d.Tokens.Last.NeedObjectName() { - if !d.Tokens.Last.isValidNamespace() { - return Token{}, errInvalidNamespace - } - if d.Tokens.Last.isActiveNamespace() && !d.Namespaces.Last().insertQuoted(d.buf[pos-n:pos], flags.IsVerbatim()) { - err = newDuplicateNameError(d.buf[pos-n : pos]) - return Token{}, d.injectSyntacticErrorWithPosition(err, pos-n) // report position at start of string + if d.Tokens.Last.NeedObjectName() { + if !d.Flags.Get(jsonflags.AllowDuplicateNames) { + if !d.Tokens.Last.isValidNamespace() { + return Token{}, errInvalidNamespace + } + if d.Tokens.Last.isActiveNamespace() && !d.Namespaces.Last().insertQuoted(d.buf[pos-n:pos], flags.IsVerbatim()) { + err = newDuplicateNameError(d.buf[pos-n : pos]) + return Token{}, d.injectSyntacticErrorWithPosition(err, pos-n) // report position at start of string + } } d.Names.ReplaceLastQuotedOffset(pos - n) // only replace if insertQuoted succeeds } @@ -531,8 +533,8 @@ func (d *decoderState) ReadToken() (Token, error) { if err = d.Tokens.pushObject(); err != nil { return Token{}, d.injectSyntacticErrorWithPosition(err, pos) } + d.Names.push() if !d.Flags.Get(jsonflags.AllowDuplicateNames) { - d.Names.push() d.Namespaces.push() } pos += 1 @@ -543,8 +545,8 @@ func (d *decoderState) ReadToken() (Token, error) { if err = d.Tokens.popObject(); err != nil { return Token{}, d.injectSyntacticErrorWithPosition(err, pos) } + d.Names.pop() if !d.Flags.Get(jsonflags.AllowDuplicateNames) { - d.Names.pop() d.Namespaces.pop() } pos += 1 @@ -646,14 +648,16 @@ func (d *decoderState) ReadValue(flags *jsonwire.ValueFlags) (Value, error) { case 'n', 't', 'f': err = d.Tokens.appendLiteral() case '"': - if !d.Flags.Get(jsonflags.AllowDuplicateNames) && d.Tokens.Last.NeedObjectName() { - if !d.Tokens.Last.isValidNamespace() { - err = errInvalidNamespace - break - } - if d.Tokens.Last.isActiveNamespace() && !d.Namespaces.Last().insertQuoted(d.buf[pos-n:pos], flags.IsVerbatim()) { - err = newDuplicateNameError(d.buf[pos-n : pos]) - break + if d.Tokens.Last.NeedObjectName() { + if !d.Flags.Get(jsonflags.AllowDuplicateNames) { + if !d.Tokens.Last.isValidNamespace() { + err = errInvalidNamespace + break + } + if d.Tokens.Last.isActiveNamespace() && !d.Namespaces.Last().insertQuoted(d.buf[pos-n:pos], flags.IsVerbatim()) { + err = newDuplicateNameError(d.buf[pos-n : pos]) + break + } } d.Names.ReplaceLastQuotedOffset(pos - n) // only replace if insertQuoted succeeds } diff --git a/jsontext/encode.go b/jsontext/encode.go index 1035aa9..c9c5a5c 100644 --- a/jsontext/encode.go +++ b/jsontext/encode.go @@ -295,11 +295,11 @@ func (e *encoderState) UnwriteEmptyObjectMember(prevName *string) bool { if e.Tokens.Last.isActiveNamespace() { e.Namespaces.Last().removeLast() } - e.Names.clearLast() - if prevName != nil { - e.Names.copyQuotedBuffer(e.Buf) // required by objectNameStack.replaceLastUnquotedName - e.Names.replaceLastUnquotedName(*prevName) - } + } + e.Names.clearLast() + if prevName != nil { + e.Names.copyQuotedBuffer(e.Buf) // required by objectNameStack.replaceLastUnquotedName + e.Names.replaceLastUnquotedName(*prevName) } return true } @@ -323,8 +323,8 @@ func (e *encoderState) UnwriteOnlyObjectMemberName() string { if e.Tokens.Last.isActiveNamespace() { e.Namespaces.Last().removeLast() } - e.Names.clearLast() } + e.Names.clearLast() return name } @@ -367,14 +367,16 @@ func (e *encoderState) WriteToken(t Token) error { if b, err = t.appendString(b, &e.Flags); err != nil { break } - if !e.Flags.Get(jsonflags.AllowDuplicateNames) && e.Tokens.Last.NeedObjectName() { - if !e.Tokens.Last.isValidNamespace() { - err = errInvalidNamespace - break - } - if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], false) { - err = newDuplicateNameError(b[pos:]) - break + if e.Tokens.Last.NeedObjectName() { + if !e.Flags.Get(jsonflags.AllowDuplicateNames) { + if !e.Tokens.Last.isValidNamespace() { + err = errInvalidNamespace + break + } + if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], false) { + err = newDuplicateNameError(b[pos:]) + break + } } e.Names.ReplaceLastQuotedOffset(pos) // only replace if insertQuoted succeeds } @@ -389,8 +391,8 @@ func (e *encoderState) WriteToken(t Token) error { if err = e.Tokens.pushObject(); err != nil { break } + e.Names.push() if !e.Flags.Get(jsonflags.AllowDuplicateNames) { - e.Names.push() e.Namespaces.push() } case '}': @@ -398,8 +400,8 @@ func (e *encoderState) WriteToken(t Token) error { if err = e.Tokens.popObject(); err != nil { break } + e.Names.pop() if !e.Flags.Get(jsonflags.AllowDuplicateNames) { - e.Names.pop() e.Namespaces.pop() } case '[': @@ -467,13 +469,15 @@ func (e *encoderState) AppendRaw(k Kind, safeASCII bool, appendFn func([]byte) ( } // Update the state machine. - if !e.Flags.Get(jsonflags.AllowDuplicateNames) && e.Tokens.Last.NeedObjectName() { - if !e.Tokens.Last.isValidNamespace() { - return errInvalidNamespace - } - if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], isVerbatim) { - err := newDuplicateNameError(b[pos:]) - return e.injectSyntacticErrorWithPosition(err, pos) + if e.Tokens.Last.NeedObjectName() { + if !e.Flags.Get(jsonflags.AllowDuplicateNames) { + if !e.Tokens.Last.isValidNamespace() { + return errInvalidNamespace + } + if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], isVerbatim) { + err := newDuplicateNameError(b[pos:]) + return e.injectSyntacticErrorWithPosition(err, pos) + } } e.Names.ReplaceLastQuotedOffset(pos) // only replace if insertQuoted succeeds } @@ -546,14 +550,16 @@ func (e *encoderState) WriteValue(v Value) error { case 'n', 'f', 't': err = e.Tokens.appendLiteral() case '"': - if !e.Flags.Get(jsonflags.AllowDuplicateNames) && e.Tokens.Last.NeedObjectName() { - if !e.Tokens.Last.isValidNamespace() { - err = errInvalidNamespace - break - } - if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], false) { - err = newDuplicateNameError(b[pos:]) - break + if e.Tokens.Last.NeedObjectName() { + if !e.Flags.Get(jsonflags.AllowDuplicateNames) { + if !e.Tokens.Last.isValidNamespace() { + err = errInvalidNamespace + break + } + if e.Tokens.Last.isActiveNamespace() && !e.Namespaces.Last().insertQuoted(b[pos:], false) { + err = newDuplicateNameError(b[pos:]) + break + } } e.Names.ReplaceLastQuotedOffset(pos) // only replace if insertQuoted succeeds }