FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go


Description:
encoding/json: A JSON tag can be any valid JSON string.

Fixes issue 3887.

Please review this at https://codereview.appspot.com/6997045/

Affected files:
M src/pkg/encoding/json/encode.go
M src/pkg/encoding/json/encode_test.go
M src/pkg/encoding/json/tagkey_test.go


Index: src/pkg/encoding/json/encode.go
===================================================================
--- a/src/pkg/encoding/json/encode.go
+++ b/src/pkg/encoding/json/encode.go
@@ -17,9 +17,7 @@
"runtime"
"sort"
"strconv"
- "strings"
"sync"
- "unicode"
"unicode/utf8"
)

@@ -431,23 +429,39 @@
return
}

+func isValidString(s string) bool {
+ escape := false // have we read a '\' character?
+
+ for i := 0; i < len(s); {
+ if b := s[i]; b < utf8.RuneSelf {
+ switch {
+ case b == '\\' && escape == false:
+ escape = true
+ i++
+ continue
+ case b == '"' && escape == false:
+ case b < 0x20:
+ return false
+ }
+
+ escape = false
+ i++
+ continue
+ }
+ c, size := utf8.DecodeRuneInString(s[i:])
+ if c == utf8.RuneError && size == 1 {
+ return false
+ }
+ i += size
+ }
+ return escape == false
+}
+
func isValidTag(s string) bool {
if s == "" {
return false
}
- for _, c := range s {
- switch {
- case strings.ContainsRune("!#$%&()*+-./:<=>?@[]^_{|}~", c):
- // Backslash and quote chars are reserved, but
- // otherwise any punctuation chars are allowed
- // in a tag name.
- default:
- if !unicode.IsLetter(c) && !unicode.IsDigit(c) {
- return false
- }
- }
- }
- return true
+ return isValidString(s)
}

func fieldByIndex(v reflect.Value, index []int) reflect.Value {
Index: src/pkg/encoding/json/encode_test.go
===================================================================
--- a/src/pkg/encoding/json/encode_test.go
+++ b/src/pkg/encoding/json/encode_test.go
@@ -84,6 +84,28 @@
}
}

+type ValidTag struct {
+ Space bool `json:"with space"`
+ Unicode bool `json:"Ελλάδα"`
+}
+
+var validTagExpected = `{
+ "with space": false,
+ "Ελλάδα": false
+}`
+
+func TestValidTag(t *testing.T) {
+ var s ValidTag
+ got, err := MarshalIndent(&s, "", " ")
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if got := string(got); got != validTagExpected {
+ t.Fatalf(" got: %s\nwant: %s\n", got, validTagExpected)
+ }
+}
+
// byte slices are special even if they're renamed types.
type renamedByte byte
type renamedByteSlice []byte
Index: src/pkg/encoding/json/tagkey_test.go
===================================================================
--- a/src/pkg/encoding/json/tagkey_test.go
+++ b/src/pkg/encoding/json/tagkey_test.go
@@ -56,8 +56,12 @@
Y string `:"BadFormat"`
}

-type badCodeTag struct {
- Z string `json:" !\"#&'()*+,."`
+type spaceTag struct {
+ Z string `json:"With space"`
+}
+
+type unicodeTag struct {
+ Q string `json:"Ελλάδα"`
}

var structTagObjectKeyTests = []struct {
@@ -75,9 +79,10 @@
{emptyTag{"Pour Moi"}, "Pour Moi", "W"},
{misnamedTag{"Animal Kingdom"}, "Animal Kingdom", "X"},
{badFormatTag{"Orfevre"}, "Orfevre", "Y"},
- {badCodeTag{"Reliable Man"}, "Reliable Man", "Z"},
{percentSlashTag{"brut"}, "brut", "text/html%"},
{punctuationTag{"Union Rags"}, "Union Rags", "!#$%&()*+-./:<=>?@[]^_{|
}~"},
+ {spaceTag{"Perreddu"}, "Perreddu", "With space"},
+ {unicodeTag{"Loukanikos"}, "Loukanikos", "Ελλάδα"},
}

func TestStructTagObjectKey(t *testing.T) {

Search Discussions

  • Daniel Morsing at Dec 22, 2012 at 9:45 am

    On 2012/12/22 01:32:06, stephane.travostino wrote:
    Please discard the modification to encode_test.go.
    I can't really figure out a way to discard that file.
    hg change 6997045 , remove encode_test.go from the file and then hg
    upload 6997045

    https://codereview.appspot.com/6997045/
  • Remyoudompheng at Dec 22, 2012 at 10:14 am
    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go
    File src/pkg/encoding/json/encode.go (right):

    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go#newcode432
    src/pkg/encoding/json/encode.go:432: func isValidString(s string) bool {
    I don't understand what this function is doing nor the relation with the
    CL description. Can you explain?

    https://codereview.appspot.com/6997045/
  • Daniel Morsing at Dec 22, 2012 at 10:22 am
    This CL doesn't seem to handle the case where someone wants a , in their
    json field name.

    Also, because of the go field tag syntax, getting \" in the string from
    a tag means the tag looks like `json:"\\\""`. Hardly intuitive. I think
    the better option is just to have the tag parser handle that case.

    https://codereview.appspot.com/6997045/
  • Rsc at Dec 22, 2012 at 3:40 pm
    Thank you for working on this. I don't believe that we need to go quite
    this far. We've been careful about limiting what we allow as JSON tags,
    and I haven't seen any argument or even discussion about removing those
    limitations. The issue is about allowing spaces in the tag names, so it
    should suffice to add space to the allowed character set, a 1-line
    change.



    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go
    File src/pkg/encoding/json/encode.go (left):

    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go#oldcode440
    src/pkg/encoding/json/encode.go:440: case
    strings.ContainsRune("!#$%&()*+-./:<=>?@[]^_{|}~", c):
    It seems to me that you can just add space here.

    https://codereview.appspot.com/6997045/
  • Rsc at Dec 22, 2012 at 6:36 pm
    LGTM

    Thanks. The backwards compatibility promise means we have to be very
    conservative about what we allow, because we're stuck with it once we
    do.


    https://codereview.appspot.com/6997045/
  • Rsc at Dec 22, 2012 at 6:37 pm
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=bd8499d19a94 ***

    encoding/json: A JSON tag can be any valid JSON string.

    Fixes issue 3887.

    R=golang-dev, daniel.morsing, remyoudompheng, rsc
    CC=golang-dev
    https://codereview.appspot.com/6997045

    Committer: Russ Cox <rsc@golang.org>


    https://codereview.appspot.com/6997045/
  • Stephane Travostino at Dec 22, 2012 at 6:44 pm
  • Stephane Travostino at Dec 22, 2012 at 6:44 pm
    On 2012/12/22 10:14:06, remyoudompheng wrote:

    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go
    File src/pkg/encoding/json/encode.go (right):

    https://codereview.appspot.com/6997045/diff/5001/src/pkg/encoding/json/encode.go#newcode432
    src/pkg/encoding/json/encode.go:432: func isValidString(s string) bool {
    I don't understand what this function is doing nor the relation with the CL
    description. Can you explain?
    The spec at json.org specifies that the member key in an object can be
    any valid string. This is what this function is doing: checking that the
    custom key passed by the user in the struct tag is a valid JSON string.

    https://codereview.appspot.com/6997045/
  • Russ Cox at Dec 22, 2012 at 4:19 pm
    The json keys we allow in struct tags are a subset of those allowed by full
    json. That's intentional, and fine. For example we do not allow comma,
    because we use comma to introduce attributes controlling the encoding. This
    bug is about allowing spaces, not about allowing everything. In the worst
    case clients can always use a map[string]interface{} to get access to keys
    with arbitrary syntax.
  • Stephane Travostino at Dec 22, 2012 at 6:44 pm
    @rsc, @DMorsing: I know that corner cases such as "," being a valid JSON
    string character are correctly unhandled, but this is due to a
    limitation of the how the JSON tag in the struct is coded and parsed.

    I'd leave those particular cases out, but the change modifies how the
    key is validated after being parsed from the `json:etc" tag. Once it is
    parsed correctly, just make sure it is a valid JSON string.
    I don't think this breaks anything, and gives more freedom to the user
    that wants to put weird characters in their JSON, as long as it is valid
    JSON.

    https://codereview.appspot.com/6997045/
  • Russ Cox at Dec 22, 2012 at 4:32 pm
    We are intentionally reserving some syntax for future expansion. We will
    gladly consider specific characters but we're not just going to accept
    anything at all. Spaces are okay.

    Russ

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 22, '12 at 8:56a
activeDec 22, '12 at 6:44p
posts12
users4
websitegolang.org

People

Translate

site design / logo © 2021 Grokbase