Skip to content

Commit

Permalink
Always update stack pointer
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dsnet
dsnet committed Nov 25, 2024
1 parent 7696f99 commit 8c90d32
Show file tree
Hide file tree
Showing 5 changed files with 114 additions and 110 deletions.
4 changes: 1 addition & 3 deletions arshal_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion arshal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
112 changes: 54 additions & 58 deletions jsontext/coder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}
})
Expand Down
38 changes: 21 additions & 17 deletions jsontext/decode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
68 changes: 37 additions & 31 deletions jsontext/encode.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}

Expand Down Expand Up @@ -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
}
Expand All @@ -389,17 +391,17 @@ 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 '}':
b = append(b, '}')
if err = e.Tokens.popObject(); err != nil {
break
}
e.Names.pop()
if !e.Flags.Get(jsonflags.AllowDuplicateNames) {
e.Names.pop()
e.Namespaces.pop()
}
case '[':
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit 8c90d32

Please sign in to comment.