Add support for JSON encoding nested struct

[schmorrison]

Resolves #53

// Structs can be encoded as JSON by including the "json" option in that fields
// tag. The entire struct is then passed to encoding/json.Marshal where those
// tag signatures apply.
//
//  // Encoding a struct as JSON
//  Field A `url: "myName,json"`
//  // Result: myName={"someField": "cat", "numField": 1}
//

[codecov]

Codecov Report

Merging #54 (ff5bcf4) into master (f76b16e) will decrease coverage by 1.76%.
The diff coverage is 72.72%.

❗ Current head ff5bcf4 differs from pull request most recent head 48243d2. Consider uploading reports for the commit 48243d2 to get more accurate results

@@             Coverage Diff             @@
##            master      #54      +/-   ##
===========================================
- Coverage   100.00%   98.23%   -1.77%     
===========================================
  Files            1        1              
  Lines          162      170       +8     
===========================================
+ Hits           162      167       +5     
- Misses           0        2       +2     
- Partials         0        1       +1     

Impacted Files	Coverage Δ
query/encode.go	98.23% <72.72%> (-1.77%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f76b16e...48243d2. Read the comment docs.

[willnorris]

So this is a weird option to be sure. This seems pretty obscure, and so I'm a little hesitant to add it to the core library versus justing saying this user a custom encoder. That said, if we did include this, here's some thoughts...

does this need to be limited only to structs? If we're going to support a 'json' tag option, we should probably support it equally across types. So this logic can be moved higher up in the func... see willnorris@b9a5f66 for example. That seems like a reasonable place to put it, but I'm not 100% sure.

I'm not quite sure about the interaction with omitempty. If you have a field tag of url:",json,omitempty", and the result of json.Marshal is an empty string, shouldn't that be omitted? I don't think it would be in my link above, though I haven't tested it.

What is the interaction between a url tag and json tag on the same field? Would that be handled correctly? For example...

type T struct {
    field F `url:"f,json" json:"g,omitempty"`
}

What even should happen there? Fortunately, the encoding/json package doesn't define very many encoding options, so there's not too much to worry with. field names and omitempty are likely the main ones to test.

Speaking of tests, I think the test you have right now is actually more complex than it really needs to be, making it a little harder to understand. Or maybe it's just trying to test too much in each case that's making it hard to read? See TestValues_Slices for example, where test cases take advantage of anonymous structs... it is more verbose, but makes each test case more self-contained. And if we do expand this to be type agnostic, then we'd want to test a few different types, particularly things that have unique JSON encodings... maps, slices, and structs. Writing readable tests for this package has always been a challenge, so I can also continue to iterate on them later if needed.

[schmorrison]

So this is a weird option to be sure. This seems pretty obscure, and so I'm a little hesitant to add it to the core library versus justing saying this user a custom encoder. That said, if we did include this, here's some thoughts...

I've been working on an API wrapper for a SaaS called Zoho in my spare time. One of their services uses URL encoded forms, but some of the fields are encoded in JSON. In order to minimize work for the contributor of this endpoint I decided this was the simplest means. Admittedly a late-night weekend hackjob so I really appreciate the thoughtful response.

does this need to be limited only to structs? If we're going to support a 'json' tag option, we should probably support it equally across types. So this logic can be moved higher up in the func... see willnorris@b9a5f66 for example. That seems like a reasonable place to put it, but I'm not 100% sure.

I'm not quite sure about the interaction with omitempty. If you have a field tag of url:",json,omitempty", and the result of json.Marshal is an empty string, shouldn't that be omitted? I don't think it would be in my link above, though I haven't tested it.

No, omitempty wouldn't be observed currently. A totally empty JSON should still be {} at least I think. Testing the struct for emptiness would require some extra reflection I think, something like reflect.New(sv.Elem()).Interface() == sv.Interface().

Testing the returned string/[]byte isn't feasible if the provided struct contains additional nested structs, see below.

What is the interaction between a url tag and json tag on the same field? Would that be handled correctly? For example...

type T struct {
    field F `url:"f,json" json:"g,omitempty"`
}

What even should happen there? Fortunately, the encoding/json package doesn't define very many encoding options, so there's not too much to worry with. field names and omitempty are likely the main ones to test.

Not sure what should happen. When providing a struct to encoding/json it will not observe omitempty on a nested struct it seems. So in the snippet above, passing T to json.Marshal will always return {"g":{.....}} and this package would return f={.....}. Check https://play.golang.org/p/nBS3YrgJhuz.

I can see the concern if a user were to provide a tag like: url:"f,json,omitempty" and received f={"x":{}} as the response. But they would/should have that expectation from encoding/json already, hopefully.

Speaking of tests, I think the test you have right now is actually more complex than it really needs to be, making it a little harder to understand. Or maybe it's just trying to test too much in each case that's making it hard to read? See TestValues_Slices for example, where test cases take advantage of anonymous structs... it is more verbose, but makes each test case more self-contained. And if we do expand this to be type agnostic, then we'd want to test a few different types, particularly things that have unique JSON encodings... maps, slices, and structs. Writing readable tests for this package has always been a challenge, so I can also continue to iterate on them later if needed.

I just copy-pasted some other tests to get something that passed. I'll do something nicer if we decide to publish.

I'm happy to work it out anyway you want, or we can abandon this. My use case is rather thin, I've made a fork, and I really hope Zoho updates the endpoints in question some day (there aren't many services like this AFAIK).

If you have a preferred direction I can give it another go, otherwise feel free to close or sit on this PR.

Thanks and best regards,

Schmorrison