Skip to content

cmd/vet: diagnose xml/json tag with space after comma #19520

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

Jump to bottom
Closed
searKing opened this issue Mar 12, 2017 · 7 comments
Closed

cmd/vet: diagnose xml/json tag with space after comma #19520

searKing opened this issue Mar 12, 2017 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker
Milestone
Go1.10

Comments

@searKing
Copy link

searKing commented Mar 12, 2017 •
edited
Loading

What version of Go are you using (go version)?

go version go1.8 windows/amd64

What did you do?

I have tried to use a struct tag value json:"Name, string" to cause primitive fields to be quoted by double quotes inside JSON strings, but it doesn't affect because an space is written between a comma[,] and string， I don't know the reason until I read the code of json.

tags right:      `json:"Name,  string"`
tags written:  `json:"Name, string"`

What did you expect to see?

I propose that any spaces can be ignored between commas[,], just as the Go grammar did，to help us avoid mistakes such as one unobvious space after a comma.

Here is the example for the difference with/without a space after the comma

package main

import (
	"encoding/json"
	"fmt"
)

type Animal struct {
	Name  string `json:"AnimalName,string"`
	// SEE ME HERE: a space is after the comma[,]
	Order string `json:"AndimalOrder, string"`
}

func main() {
	a := Animal{
		Name:  "dog",
		Order: "first",
	}
	aJson, err := json.Marshal(&a)
	if err != nil {
		fmt.Println("json marshal error:", err)
	}
	fmt.Printf("marshaled:\n%s\n", aJson)
}

output:

marshaled:
{"AnimalName":"\"dog\"","AndimalOrder":"first"}

expected:

marshaled:
{"AnimalName":"\"dog\"","AndimalOrder":"\"first\""}

Propose

Here is one way to ignore spaces between comma,

// C:\Go\src\encoding\json\tags.go line:38
// Contains reports whether a comma-separated list of options
// contains a particular substr flag. substr must be surrounded by a
// string boundary or commas.
func (o tagOptions) Contains(optionName string) bool {
	if len(o) == 0 {
		return false
	}
	s := string(o)
	for s != "" {
		var next string
		i := strings.Index(s, ",")
		if i >= 0 {
			s, next = s[:i], s[i+1:]
		}
		// SEE ME HERE: trim space here,so any space can be found between comma ,
		s = strings.TrimSpace(s)
		if s == optionName {
			return true
		}
		s = next
	}
	return false
}

output of last example:

marshaled:
{"AnimalName":"\"dog\"","AndimalOrder":"\"first\""}
@searKing searKing changed the title Proposal: encoding/json: tag support any white spaces between struct tag Proposal: encoding/json: tag support any white spaces can be ignored between struct tag values Mar 12, 2017
@ALTree ALTree added this to the Proposal milestone Mar 13, 2017
@rsc rsc changed the title Proposal: encoding/json: tag support any white spaces can be ignored between struct tag values proposal: encoding/json: allow space after comma in tag Mar 13, 2017
@kevinburke
Copy link
Contributor

kevinburke commented Mar 15, 2017 •
edited
Loading

I'd rather there just be one way to do this. If we were going to do this we'd want to do it for everything that parses struct tags (xml, etc), which would introduce a difference between older versions and go1.9+ in how output data gets generated.

If anything, if it's feasible I'd rather go vet return an error if you have spaces in a struct tag in the wrong place, to indicate that some of the data in the tag might be ignored.

@rsc
Copy link
Contributor

rsc commented Mar 27, 2017

We already vet for struct with duplicate json tags (on different fields). We could easily also check for unexpected spaces. That's probably better than introducing new spellings, especially if we start running vet automatically during tests.

@rsc
Copy link
Contributor

rsc commented Apr 3, 2017

OK, let's just have vet do this.

@rsc rsc changed the title proposal: encoding/json: allow space after comma in tag cmd/vet: diagnose xml/json tag with space after comma Apr 3, 2017
@rsc rsc modified the milestones: Go1.9, Proposal Apr 3, 2017
@rsc
Copy link
Contributor

rsc commented Apr 3, 2017

Contributions welcome. Note that vet already looks at json/xml tags.

@stengaard
Copy link

This seems like a low-hanging fruit, I can give it a stab.

To be clear, go vet should error out on the first struct tag, but not the second here?

type T struct {
   A int `x:"n, n"`
   B int `x:"n n"`
}

Or is this more narrowly aimed at the json/xml annotations?

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/43295 mentions this issue.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jul 22, 2017
@bradfitz bradfitz added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Jul 22, 2017
@fatih
Copy link
Member

fatih commented Aug 24, 2017

Did we also changed reflect.Structtag with this new check? If not we should sync the code with reflect.Structtag as well: https://golang.org/src/reflect/type.go?s=30875:30896#L1158

Another issue is that option is not part of the spec defined in reflect.Structtag. There it says:

By convention, tag strings are a concatenation of optionally space-separated key:"value" pairs. ... Each value is quoted using U+0022 '"' characters and Go string literal syntax.

Here the notion of option (such as omitempty) is not defined and it was always a special syntax that was used by packages such as encoding/json. Now that vet is vetting this special condition we should do the followings:

  1. Update reflect.Structtag code as well so it's up to date
  2. Update doc comment of reflect.Structtag so it reflects that option is now part of the spec

Any thoughts here?

@golang golang locked and limited conversation to collaborators Aug 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted release-blocker
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
8 participants
@bradfitz @rsc @kevinburke @fatih @stengaard @ALTree @searKing @gopherbot